Allow inheriting env vars from wt again and other env var changes t…

…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
microsoft · Sep 22, 2023 · f2426c9 · f2426c9 · github-actions · Sep 22, 2023
1 parent 9206575
commit f2426c9
Show file tree

Hide file tree

Showing 43 changed files with 381 additions and 78 deletions.
diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json
@@ -2171,11 +2171,6 @@
           "description": "When set to true, the background image for the currently focused profile is expanded to encompass the entire window, beneath other panes.",
           "type": "boolean"
         },
-        "compatibility.reloadEnvironmentVariables": {
-          "default": true,
-          "description": "When set to true, when opening a new tab or pane it will get reloaded environment variables.",
-          "type": "boolean"
-        },
         "initialCols": {
           "default": 120,
           "description": "The number of columns displayed in the window upon first load. If \"launchMode\" is set to \"maximized\" (or \"maximizedFocus\"), this property is ignored.",
@@ -2424,6 +2419,11 @@
             "null"
           ]
         },
+        "compatibility.reloadEnvironmentVariables": {
+          "default": true,
+          "description": "When set to true, when opening a new tab or pane it will get reloaded environment variables.",
+          "type": "boolean"
+        },
         "unfocusedAppearance": {
           "$ref": "#/$defs/AppearanceConfig",
           "description": "Sets the appearance of the terminal when it is unfocused.",

diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
@@ -74,6 +74,8 @@ namespace SettingsModelLocalTests
         TEST_METHOD(TestInheritedCommand);
         TEST_METHOD(LoadFragmentsWithMultipleUpdates);
 
+        TEST_METHOD(MigrateReloadEnvVars);
+
     private:
         static winrt::com_ptr<implementation::CascadiaSettings> createSettings(const std::string_view& userJSON)
         {
@@ -2020,4 +2022,40 @@ namespace SettingsModelLocalTests
         // GH#12520: Fragments should be able to override the name of builtin profiles.
         VERIFY_ARE_EQUAL(L"NewName", loader.userSettings.profiles[0]->Name());
     }
+
+    void DeserializationTests::MigrateReloadEnvVars()
+    {
+        static constexpr std::string_view settings1Json{ R"(
+        {
+            "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+            "compatibility.reloadEnvironmentVariables": false,
+            "profiles": [
+                {
+                    "name": "profile0",
+                    "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1,
+                    "commandline": "cmd.exe"
+                }
+            ],
+            "actions": [
+                {
+                    "name": "foo",
+                    "command": "closePane",
+                    "keys": "ctrl+shift+w"
+                }
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ settings1Json, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.FinalizeLayering();
+
+        VERIFY_IS_TRUE(loader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+
+        Log::Comment(L"Ensure that the profile defaults have the new setting added");
+        VERIFY_IS_TRUE(settings->ProfileDefaults().HasReloadEnvironmentVariables());
+        VERIFY_IS_FALSE(settings->ProfileDefaults().ReloadEnvironmentVariables());
+    }
 }
diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
@@ -42,6 +42,9 @@ namespace SettingsModelLocalTests
         TEST_METHOD(CascadiaSettings);
         TEST_METHOD(LegacyFontSettings);
 
+        TEST_METHOD(RoundtripReloadEnvVars);
+        TEST_METHOD(DontRoundtripNoReloadEnvVars);
+
     private:
         // Method Description:
         // - deserializes and reserializes a json string representing a settings object model of type T
@@ -513,4 +516,114 @@ namespace SettingsModelLocalTests
 
         VERIFY_ARE_EQUAL(toString(jsonOutput), toString(result));
     }
+
+    void SerializationTests::RoundtripReloadEnvVars()
+    {
+        static constexpr std::string_view oldSettingsJson{ R"(
+        {
+            "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+            "compatibility.reloadEnvironmentVariables": false,
+            "profiles": [
+                {
+                    "name": "profile0",
+                    "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1,
+                    "commandline": "cmd.exe"
+                }
+            ],
+            "actions": [
+                {
+                    "name": "foo",
+                    "command": "closePane",
+                    "keys": "ctrl+shift+w"
+                }
+            ]
+        })" };
+
+        static constexpr std::string_view newSettingsJson{ R"(
+        {
+            "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+            "profiles":
+            {
+                "defaults":
+                {
+                    "compatibility.reloadEnvironmentVariables": false
+                },
+                "list":
+                [
+                    {
+                        "name": "profile0",
+                        "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                        "historySize": 1,
+                        "commandline": "cmd.exe"
+                    }
+                ]
+            },
+            "actions": [
+                {
+                    "name": "foo",
+                    "command": "closePane",
+                    "keys": "ctrl+shift+w"
+                }
+            ]
+        })" };
+
+        implementation::SettingsLoader oldLoader{ oldSettingsJson, DefaultJson };
+        oldLoader.MergeInboxIntoUserSettings();
+        oldLoader.FinalizeLayering();
+        VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk");
+        const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
+        const auto oldResult{ oldSettings->ToJson() };
+
+        implementation::SettingsLoader newLoader{ newSettingsJson, DefaultJson };
+        newLoader.MergeInboxIntoUserSettings();
+        newLoader.FinalizeLayering();
+        newLoader.FixupUserSettings();
+        const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
+        const auto newResult{ newSettings->ToJson() };
+
+        VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult));
+    }
+
+    void SerializationTests::DontRoundtripNoReloadEnvVars()
+    {
+        // Kinda like the above test, but confirming that _nothing_ happens if
+        // we don't have a setting to migrate.
+
+        static constexpr std::string_view oldSettingsJson{ R"(
+        {
+            "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name": "profile0",
+                    "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1,
+                    "commandline": "cmd.exe"
+                }
+            ],
+            "actions": [
+                {
+                    "name": "foo",
+                    "command": "closePane",
+                    "keys": "ctrl+shift+w"
+                }
+            ]
+        })" };
+
+        implementation::SettingsLoader oldLoader{ oldSettingsJson, DefaultJson };
+        oldLoader.MergeInboxIntoUserSettings();
+        oldLoader.FinalizeLayering();
+        oldLoader.FixupUserSettings();
+        const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(oldLoader));
+        const auto oldResult{ oldSettings->ToJson() };
+
+        Log::Comment(L"Now, create a _new_ settings object from the re-serialization of the first");
+        implementation::SettingsLoader newLoader{ toString(oldResult), DefaultJson };
+        newLoader.MergeInboxIntoUserSettings();
+        newLoader.FinalizeLayering();
+        newLoader.FixupUserSettings();
+        const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
+        VERIFY_IS_FALSE(newSettings->ProfileDefaults().HasReloadEnvironmentVariables(),
+                        L"Ensure that the new settings object didn't find a reloadEnvironmentVariables");
+    }
 }
diff --git a/src/cascadia/LocalTests_SettingsModel/pch.h b/src/cascadia/LocalTests_SettingsModel/pch.h
@@ -64,6 +64,7 @@ Author(s):
 
 // Manually include til after we include Windows.Foundation to give it winrt superpowers
 #include "til.h"
+#include <til/winrt.h>
 
 // Common includes for most tests:
 #include "../../inc/conattrs.hpp"

diff --git a/src/cascadia/Remoting/CommandlineArgs.h b/src/cascadia/Remoting/CommandlineArgs.h
@@ -15,10 +15,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
 
         CommandlineArgs(const winrt::array_view<const winrt::hstring>& args,
                         winrt::hstring currentDirectory,
-                        const uint32_t showWindowCommand) :
+                        const uint32_t showWindowCommand,
+                        winrt::hstring envString) :
             _args{ args.begin(), args.end() },
             _cwd{ currentDirectory },
-            _ShowWindowCommand{ showWindowCommand }
+            _ShowWindowCommand{ showWindowCommand },
+            CurrentEnvironment{ envString }
         {
         }
 
@@ -27,6 +29,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         void Commandline(const winrt::array_view<const winrt::hstring>& value);
         winrt::com_array<winrt::hstring> Commandline();
 
+        til::property<winrt::hstring> CurrentEnvironment;
+
         WINRT_PROPERTY(uint32_t, ShowWindowCommand, SW_NORMAL); // SW_NORMAL is 1, 0 is SW_HIDE
 
     private:

diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h
@@ -47,7 +47,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
             _WindowName{ windowInfo.WindowName() },
             _args{ command.Commandline() },
             _CurrentDirectory{ command.CurrentDirectory() },
-            _ShowWindowCommand{ command.ShowWindowCommand() } {};
+            _ShowWindowCommand{ command.ShowWindowCommand() },
+            _CurrentEnvironment{ command.CurrentEnvironment() } {};
 
         WindowRequestedArgs(const winrt::hstring& window, const winrt::hstring& content, const Windows::Foundation::IReference<Windows::Foundation::Rect>& bounds) :
             _Id{ 0u },
@@ -65,6 +66,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         WINRT_PROPERTY(winrt::hstring, CurrentDirectory);
         WINRT_PROPERTY(winrt::hstring, Content);
         WINRT_PROPERTY(uint32_t, ShowWindowCommand, SW_NORMAL);
+        WINRT_PROPERTY(winrt::hstring, CurrentEnvironment);
         WINRT_PROPERTY(Windows::Foundation::IReference<Windows::Foundation::Rect>, InitialBounds);
 
     private:

diff --git a/src/cascadia/Remoting/Monarch.idl b/src/cascadia/Remoting/Monarch.idl
@@ -27,6 +27,7 @@ namespace Microsoft.Terminal.Remoting
         String[] Commandline { get; };
         String CurrentDirectory { get; };
         UInt32 ShowWindowCommand { get; };
+        String CurrentEnvironment { get; };
 
         String Content { get; };
         Windows.Foundation.IReference<Windows.Foundation.Rect> InitialBounds { get; };

diff --git a/src/cascadia/Remoting/Peasant.idl b/src/cascadia/Remoting/Peasant.idl
@@ -7,11 +7,12 @@ namespace Microsoft.Terminal.Remoting
     runtimeclass CommandlineArgs
     {
         CommandlineArgs();
-        CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand);
+        CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand, String env);
 
         String[] Commandline { get; set; };
         String CurrentDirectory { get; };
         UInt32 ShowWindowCommand { get; };
+        String CurrentEnvironment { get; };
     };
 
     runtimeclass RenameRequestArgs

diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp
@@ -349,7 +349,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         // If the name wasn't specified, this will be an empty string.
         p->WindowName(args.WindowName());
 
-        p->ExecuteCommandline(*winrt::make_self<CommandlineArgs>(args.Commandline(), args.CurrentDirectory(), args.ShowWindowCommand()));
+        p->ExecuteCommandline(*winrt::make_self<CommandlineArgs>(args.Commandline(),
+                                                                 args.CurrentDirectory(),
+                                                                 args.ShowWindowCommand(),
+                                                                 args.CurrentEnvironment()));
 
         _monarch.AddPeasant(*p);
 

diff --git a/src/cascadia/Remoting/pch.h b/src/cascadia/Remoting/pch.h
@@ -52,3 +52,4 @@ TRACELOGGING_DECLARE_PROVIDER(g_hRemotingProvider);
 #include "til.h"
 
 #include <cppwinrt_utils.h>
+#include <til/winrt.h>
diff --git a/src/cascadia/TerminalApp/AppCommandlineArgs.cpp b/src/cascadia/TerminalApp/AppCommandlineArgs.cpp
@@ -568,6 +568,11 @@ void AppCommandlineArgs::_addNewTerminalArgs(AppCommandlineArgs::NewTerminalSubc
     subcommand.colorSchemeOption = subcommand.subcommand->add_option("--colorScheme",
                                                                      _startingColorScheme,
                                                                      RS_A(L"CmdColorSchemeArgDesc"));
+    subcommand.inheritEnvOption = subcommand.subcommand->add_flag(
+        "--inheritEnvironment,!--reloadEnvironment",
+        _inheritEnvironment,
+        RS_A(L"CmdInheritEnvDesc"));
+
     // Using positionals_at_end allows us to support "wt new-tab -d wsl -d Ubuntu"
     // without CLI11 thinking that we've specified -d twice.
     // There's an alternate construction where we make all subcommands "prefix commands",
@@ -589,7 +594,8 @@ NewTerminalArgs AppCommandlineArgs::_getNewTerminalArgs(AppCommandlineArgs::NewT
 {
     NewTerminalArgs args{};
 
-    if (!_commandline.empty())
+    const auto hasCommandline{ !_commandline.empty() };
+    if (hasCommandline)
     {
         std::ostringstream cmdlineBuffer;
 
@@ -655,6 +661,13 @@ NewTerminalArgs AppCommandlineArgs::_getNewTerminalArgs(AppCommandlineArgs::NewT
         args.ColorScheme(winrt::to_hstring(_startingColorScheme));
     }
 
+    bool inheritEnv = hasCommandline;
+    if (*subcommand.inheritEnvOption)
+    {
+        inheritEnv = _inheritEnvironment;
+    }
+    args.ReloadEnvironmentVariables(!inheritEnv);
+
     return args;
 }
 

diff --git a/src/cascadia/TerminalApp/AppCommandlineArgs.h b/src/cascadia/TerminalApp/AppCommandlineArgs.h
@@ -67,6 +67,7 @@ class TerminalApp::AppCommandlineArgs final
         CLI::Option* tabColorOption;
         CLI::Option* suppressApplicationTitleOption;
         CLI::Option* colorSchemeOption;
+        CLI::Option* inheritEnvOption;
     };
 
     struct NewPaneSubcommand : public NewTerminalSubcommand
@@ -99,6 +100,7 @@ class TerminalApp::AppCommandlineArgs final
     std::string _startingTabColor;
     std::string _startingColorScheme;
     bool _suppressApplicationTitle{ false };
+    bool _inheritEnvironment{ false };
 
     winrt::Microsoft::Terminal::Settings::Model::FocusDirection _moveFocusDirection{ winrt::Microsoft::Terminal::Settings::Model::FocusDirection::None };
     winrt::Microsoft::Terminal::Settings::Model::FocusDirection _swapPaneDirection{ winrt::Microsoft::Terminal::Settings::Model::FocusDirection::None };

diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw
@@ -368,6 +368,10 @@
     <value>Open the tab with tabTitle overriding default title and suppressing title change messages from the application</value>
     <comment>{Locked="\"tabTitle\""}</comment>
   </data>
+  <data name="CmdInheritEnvDesc" xml:space="preserve">
+    <value>Inherit the terminal's own environment variables when creating the new tab or pane, rather than creating a fresh environment block. This defaults to set when a "command" is passed. </value>
+    <comment>{Locked="\"command\""}</comment>
+  </data>
   <data name="CmdColorSchemeArgDesc" xml:space="preserve">
     <value>Open the tab with the specified color scheme, instead of the profile's set "colorScheme"</value>
     <comment>{Locked="\"colorScheme\""}</comment>