Allow inheriting env vars from `wt` again and other env var changes too #15897

zadjii-msft · 2023-08-29T17:52:30Z

This PR is a few things:

part the first: Convert the compatibility.reloadEnvironmentVariables setting to a per-profile one.
- The settings should migrate it from the user's old global place to the new one.
- We also added it to "Profile>Advanced" while I was here.
Adds a new pair of commandline flags to new-tab and split-pane: --inheritEnvironment / --reloadEnvironment
- On wt launch, bundle the entire environment that wt was spawned with, and put it into the Remoting.CommandlineArgs, and give them to the monarch (and ultimately, down to TerminalPage with the AppCommandlineArgs). DO THIS ALWAYS.
- As a part of this, we’ll default to reloading if there’s no explicit commandline set, and inheriting if there is.
  - For example, wt -- cmd would inherit, and wt -p “Command Prompt” would reload.¹
- This is a little wacky, but we’re trying to separate out the intentions here:
  - wt -- cmd feels like “I want to run cmd.exe (in a terminal tab)”. That feels like the user would like environment variables from the calling process. They’re doing something more manual, so they get more refined control over it.
  - wt (or wt -p “Command Prompt”) is more like, “I want to run the Terminal (or, my Command Prompt profile) using whatever the Terminal would normally do”. So that feels more like a situation where it should just reload by default. (Of course, this will respect their settings here)

References and Relevant Issues

#15496 (comment) has more notes.

Detailed Description of the Pull Request / Additional comments

This is so VERY much plumbing. I'll try to leave comments in the interesting parts.

PR Checklist

This is not all of Terminal preview does not accept environment variables from parent process #15496. We're also going to do a -E foo=bar arg on top of this.
Tests added/passed
Schema updated

In both these cases, plus the environment setting, of course. ↩

It doesn't actually use it though, unless `reloadEnvVars` isn't set. But now we can _potentially_ use the env vars of the calling WT rather than our own.

zadjii-msft · 2023-08-29T17:53:26Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

@@ -662,6 +668,13 @@ NewTerminalArgs AppCommandlineArgs::_getNewTerminalArgs(AppCommandlineArgs::NewT
        args.AppendCommandLine(_appendCommandLineOption);
    }

+    bool inheritEnv = hasCommandline;


of interest: default to inherit if we do have a commandline. If the user passed the arg in manually, then use the arg.

zadjii-msft · 2023-08-29T17:53:57Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -576,6 +577,12 @@ namespace winrt::TerminalApp::implementation
            _WindowProperties.VirtualWorkingDirectory(originalVirtualCwd);
        });

+        // Literally the same thing with env vars too


All this is exactly the same way VirtualWorkingDirectory (above this) works

zadjii-msft · 2023-08-29T17:54:31Z

src/cascadia/TerminalApp/TerminalPage.cpp

                                                                                 environment,
                                                                                 settings.InitialRows(),
                                                                                 settings.InitialCols(),
                                                                                 winrt::guid(),
                                                                                 profile.Guid());

            valueSet.Insert(L"passthroughMode", Windows::Foundation::PropertyValue::CreateBoolean(settings.VtPassthrough()));
-            valueSet.Insert(L"reloadEnvironmentVariables",


Promoted this to be a proper param to CreateSettings

zadjii-msft · 2023-08-29T17:55:57Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -286,6 +293,19 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
            _reloadEnvironmentVariables = winrt::unbox_value_or<bool>(settings.TryLookup(L"reloadEnvironmentVariables").try_as<Windows::Foundation::IPropertyValue>(),
                                                                      _reloadEnvironmentVariables);
            _profileGuid = winrt::unbox_value_or<winrt::guid>(settings.TryLookup(L"profileGuid").try_as<Windows::Foundation::IPropertyValue>(), _profileGuid);
+
+            const auto& initialEnvironment{ winrt::unbox_value_or<winrt::hstring>(settings.TryLookup(L"initialEnvironment").try_as<Windows::Foundation::IPropertyValue>(), L"") };


interesting: get the serialized env block here, and use it to make a til::env that this connection will use as it's basis.

zadjii-msft · 2023-08-29T17:56:25Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -450,6 +450,13 @@ bool SettingsLoader::FixupUserSettings()
        }
    }

+    if (!userSettings.globals->LegacyReloadEnvironmentVariables())


migration logic is here. This seemed like the right place?

SGTM. @lhecker?

But wait, won't this set Reload to false when in globals it was set to true??

for example, it's set to true in the constructor. doesn't that mean that everybody's base layer profile will get it set to false???

Yeah I think we should put all our migration code into this place (if possible).
I think it should be if (globals->HasLegacyReloadEnvironmentVariables()) and then set it to base->ReloadEnvironmentVariables(globals->LegacyReloadEnvironmentVariables()) right?

Hmm see, I was thinking that the old setting defaulted to true, so the only thing we'd want to migrate is a false. Similarly, the _legacyReloadEnvironmentVariables defaults to true, so the only time we write a false in is if we found a false.

I added a SerializationTests::DontRoundtripNoReloadEnvVars for a test case 😜

oh I see. since it's 2-state you're just checking the set value instead of Has. It's an unexpected technique, but it'll do. Thanks.

I missed the ! i think

zadjii-msft · 2023-08-29T17:57:28Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

@@ -105,7 +106,9 @@ bool WindowEmperor::HandleCommandlineArgs()
    GetStartupInfoW(&si);
    const uint32_t showWindow = WI_IsFlagSet(si.dwFlags, STARTF_USESHOWWINDOW) ? si.wShowWindow : SW_SHOW;

-    Remoting::CommandlineArgs eventArgs{ { args }, { cwd }, showWindow };
+    const auto currentEnv{ til::env::from_current_environment() };


Yep, it's this easy to package up the calling wt's env vars into a string that we can move around easily.

this takes care of the embedded null bytes? wow

…env var changes too

DHowett · 2023-08-29T22:44:52Z

How did this compile in the selfhost build but not in the CI build?

…t-env-vars

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/TerminalPage.idl

src/cascadia/TerminalConnection/ConptyConnection.cpp

DHowett · 2023-09-06T00:18:58Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -246,20 +247,26 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
        vs.Insert(L"commandline", Windows::Foundation::PropertyValue::CreateString(cmdline));
        vs.Insert(L"startingDirectory", Windows::Foundation::PropertyValue::CreateString(startingDirectory));
        vs.Insert(L"startingTitle", Windows::Foundation::PropertyValue::CreateString(startingTitle));
+        vs.Insert(L"reloadEnvironmentVariables", Windows::Foundation::PropertyValue::CreateBoolean(reloadEnvironmentVariables));


I can't figure out where this guy is used. The ValueSet is an internal contract between ConptyConnection and ConptyConnection, so I expected to see it in ConptyConnection.

It's in ConptyConnection::Initialize. That isn't part of the diff because it was already there, I'm just moving the setting into ConptyConnection. Setting it outside, in TerminalPage, is wack.

good call thx

src/cascadia/TerminalSettingsEditor/Profiles_Advanced.xaml

DHowett · 2023-09-06T00:24:54Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -450,6 +450,13 @@ bool SettingsLoader::FixupUserSettings()
        }
    }

+    if (!userSettings.globals->LegacyReloadEnvironmentVariables())


SGTM. @lhecker?

DHowett · 2023-09-06T00:25:26Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -450,6 +450,13 @@ bool SettingsLoader::FixupUserSettings()
        }
    }

+    if (!userSettings.globals->LegacyReloadEnvironmentVariables())


But wait, won't this set Reload to false when in globals it was set to true??

DHowett · 2023-09-06T00:26:56Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -450,6 +450,13 @@ bool SettingsLoader::FixupUserSettings()
        }
    }

+    if (!userSettings.globals->LegacyReloadEnvironmentVariables())


for example, it's set to true in the constructor. doesn't that mean that everybody's base layer profile will get it set to false???

DHowett · 2023-09-06T00:27:49Z

src/cascadia/TerminalSettingsModel/TerminalSettings.h

@@ -166,6 +166,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        INHERITABLE_SETTING(Model::TerminalSettings, bool, RightClickContextMenu, false);
        INHERITABLE_SETTING(Model::TerminalSettings, bool, RepositionCursorWithMouse, false);

+        INHERITABLE_SETTING(Model::TerminalSettings, bool, ReloadEnvironmentVariables, true);


qq: why does the control need to know about this?

Control doesn't, but the page does. This is in the same weird case as Elevate. It's not a Control setting, but it is a per-control setting.

DHowett · 2023-09-06T00:28:42Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

@@ -105,7 +106,9 @@ bool WindowEmperor::HandleCommandlineArgs()
    GetStartupInfoW(&si);
    const uint32_t showWindow = WI_IsFlagSet(si.dwFlags, STARTF_USESHOWWINDOW) ? si.wShowWindow : SW_SHOW;

-    Remoting::CommandlineArgs eventArgs{ { args }, { cwd }, showWindow };
+    const auto currentEnv{ til::env::from_current_environment() };


this takes care of the embedded null bytes? wow

…t-env-vars

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalConnection/ConptyConnection.cpp

…t-env-vars

lhecker · 2023-09-19T19:46:45Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+    T unbox_prop_or(const Windows::Foundation::Collections::ValueSet& blob, std::wstring_view key, T defaultValue)
+    {
+        return winrt::unbox_value_or<T>(blob.TryLookup(key).try_as<Windows::Foundation::IPropertyValue>(), defaultValue);
+    }


…oo (#15897) This PR is a few things: * part the first: Convert the `compatibility.reloadEnvironmentVariables` setting to a per-profile one. * The settings should migrate it from the user's old global place to the new one. * We also added it to "Profile>Advanced" while I was here. * Adds a new pair of commandline flags to `new-tab` and `split-pane`: `--inheritEnvironment` / `--reloadEnvironment` * On `wt` launch, bundle the entire environment that `wt` was spawned with, and put it into the `Remoting.CommandlineArgs`, and give them to the monarch (and ultimately, down to `TerminalPage` with the `AppCommandlineArgs`). DO THIS ALWAYS. * As a part of this, we’ll default to _reloading_ if there’s no explicit commandline set, and _inheriting_ if there is. * For example, `wt -- cmd` would inherit, and `wt -p “Command Prompt”` would reload.[^1] * This is a little wacky, but we’re trying to separate out the intentions here: * `wt -- cmd` feels like “I want to run cmd.exe (in a terminal tab)”. That feels like the user would _like_ environment variables from the calling process. They’re doing something more manual, so they get more refined control over it. * `wt` (or `wt -p “Command Prompt”`) is more like, “I want to run the Terminal (or, my Command Prompt profile) using whatever the Terminal would normally do”. So that feels more like a situation where it should just reload by default. (Of course, this will respect their settings here) #15496 (comment) has more notes. This is so VERY much plumbing. I'll try to leave comments in the interesting parts. - [x] This is not _all_ of #15496. We're also going to do a `-E foo=bar` arg on top of this. - [x] Tests added/passed - [x] Schema updated [^1]: In both these cases, plus the `environment` setting, of course. (cherry picked from commit 59248de) Service-Card-Id: 90383402 Service-Version: 1.18

zadjii-msft added 7 commits August 28, 2023 15:28

start plumbing from the top down

434fcdb

start plumbing from the bottom up

54209f7

set FOO=fooz & wtd -w -1 will pass %FOO% to the new tab...

8415db1

It doesn't actually use it though, unless `reloadEnvVars` isn't set. But now we can _potentially_ use the env vars of the calling WT rather than our own.

well dang that's the whole thing

619615f

migrate the setting, too

c592d97

This should really be in the SUI, too

a68395f

spel

26639d3

zadjii-msft commented Aug 29, 2023

View reviewed changes

shema too

c6d3b0d

zadjii-msft added the zBugBash-Consider label Aug 29, 2023

zadjii-msft added a commit that referenced this pull request Aug 29, 2023

PRE-MERGE #15897 Allow inheriting env vars from wt again and other …

b182abe

…env var changes too

zadjii-msft added 2 commits September 5, 2023 05:05

Merge remote-tracking branch 'origin/main' into dev/migrie/f/--inheri…

e4ef76a

…t-env-vars

it's important to build and run the tests too

e4b0aee

zadjii-msft assigned DHowett Sep 5, 2023

DHowett requested changes Sep 6, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 6, 2023

Merge remote-tracking branch 'origin/main' into dev/migrie/f/--inheri…

bd9076f

…t-env-vars

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 7, 2023

nits from review

629d5c4

This comment has been minimized.

Sign in to view

zadjii-msft added 2 commits September 7, 2023 10:27

YOu know, actually make it a real test

c4349d0

happends

2efdd72

zadjii-msft requested a review from DHowett September 7, 2023 15:29

This comment has been minimized.

Sign in to view

DHowett approved these changes Sep 8, 2023

View reviewed changes

zadjii-msft assigned lhecker and unassigned DHowett Sep 18, 2023

lhecker reviewed Sep 19, 2023

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

lhecker reviewed Sep 19, 2023

View reviewed changes

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved

zadjii-msft added 3 commits September 19, 2023 09:52

Merge remote-tracking branch 'origin/main' into dev/migrie/f/--inheri…

ed380ac

…t-env-vars

hey we don't need this member

f8130ff

I'm sick and tired of copypasting that block

7811c22

zadjii-msft requested a review from lhecker September 19, 2023 16:43

lhecker approved these changes Sep 19, 2023

View reviewed changes

zadjii-msft merged commit 59248de into main Sep 19, 2023
17 checks passed

zadjii-msft deleted the dev/migrie/f/--inherit-env-vars branch September 19, 2023 20:03

zadjii-msft mentioned this pull request Sep 20, 2023

Terminal preview does not accept environment variables from parent process #15496

Closed

zadjii-msft linked an issue Sep 20, 2023 that may be closed by this pull request

Terminal preview does not accept environment variables from parent process #15496

Closed

zadjii-msft mentioned this pull request Oct 25, 2023

Inherited environment variables don't work (for the very first tab) #16226

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow inheriting env vars from `wt` again and other env var changes too #15897

Allow inheriting env vars from `wt` again and other env var changes too #15897

zadjii-msft commented Aug 29, 2023 •

edited by DHowett

Loading

zadjii-msft Aug 29, 2023

zadjii-msft Aug 29, 2023

zadjii-msft Aug 29, 2023

zadjii-msft Aug 29, 2023

zadjii-msft Aug 29, 2023

DHowett Sep 6, 2023

DHowett Sep 6, 2023

DHowett Sep 6, 2023

lhecker Sep 7, 2023 •

edited

Loading

zadjii-msft Sep 7, 2023

zadjii-msft Sep 7, 2023

DHowett Sep 7, 2023

DHowett Sep 7, 2023

zadjii-msft Aug 29, 2023

DHowett Sep 6, 2023

DHowett commented Aug 29, 2023

DHowett Sep 6, 2023

zadjii-msft Sep 7, 2023

DHowett Sep 8, 2023

DHowett Sep 6, 2023

DHowett Sep 6, 2023

DHowett Sep 6, 2023

DHowett Sep 6, 2023

zadjii-msft Sep 7, 2023

DHowett Sep 6, 2023

This comment has been minimized.

This comment has been minimized.

lhecker Sep 19, 2023

Allow inheriting env vars from wt again and other env var changes too #15897

Allow inheriting env vars from wt again and other env var changes too #15897

Conversation

zadjii-msft commented Aug 29, 2023 • edited by DHowett Loading

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

PR Checklist

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Allow inheriting env vars from `wt` again and other env var changes too #15897

Allow inheriting env vars from `wt` again and other env var changes too #15897

zadjii-msft commented Aug 29, 2023 •

edited by DHowett

Loading

lhecker Sep 7, 2023 •

edited

Loading