Reload environment variables by default; add setting to disable #14999

ianjoneill · 2023-03-16T08:55:04Z

Summary of the Pull Request

Adds a global setting compatibility.reloadEnvironmentVariables with a default value of true. When set, during connection creation a new environment block will be generated to ensure it has the latest environment variables.

Validation Steps Performed

Manually tested - see video in the ~~first~~ second comment.

PR Checklist

Closes Feature Request: Terminal should listen for the WM_SETTINGCHANGE for environment variable updates #1125
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

ianjoneill · 2023-03-16T09:04:43Z

ReloadEnvironmentVariables.mp4

zadjii-msft

wow you're right that was easy.

My tiny nit is that we've been discussing internally on re-aligning some of the experimental settings we have as compatibility flags instead. That's not something you'd have possibly known, and I'm pretty sure my initial writeup called for experimental. too.

Honestly this is gonna be such goodness that I want to just hit ✅ anyways

src/cascadia/TerminalSettingsModel/MTSMSettings.h

doc/cascadia/profiles.schema.json

src/cascadia/TerminalControl/IControlSettings.idl

ianjoneill · 2023-03-16T13:54:12Z

Umm why am I getting static analysis failures on code I haven't touched?

Edit: I see #15002 has been opened to address this.

ianjoneill · 2023-03-16T19:11:02Z

Now the build's failing due to #14581.

DHowett · 2023-03-16T23:40:10Z

src/cascadia/TerminalApp/TerminalPage.cpp

+                {
+                    envMap.Insert(key, value);
+                }
+            }
            envMap.Insert(L"WT_PROFILE_ID", guidWString);


I love the simplicity of this, but I am a little bit concerned about what happens after we hand this environment block off.

We produce a new env block here, painstakingly inserting it into envMap... and then ConptyConnection uses EnvironmentVariableMapW to generate a new map based on the current process environment and inserts the contents of envMap from here into it.

In brief... what we will get is a hybrid of Terminal's spawning environment and the current environment. As well, we will probably end up parsing the entire env twice.

Should we move this down to ConptyConnection (worm the boolean down in through the valueset for all I mind, ha) and make sure we do not call UpdateEnvironmentMapW?

zadjii-msft · 2023-03-17T14:12:14Z

Huh, moving that down to connection made audit run on env.h, which it does not like. If you don't want to fix them now, I'm okay with that, there's a way to disable audit mode from yelling about an include. You could add that around the include and then we could just fix that in post.

Probably like this, with a push/pop

// Some of the interactivity classes pulled in by ServiceLocator.hpp are not
// yet audit-safe, so for now we'll need to disable a bunch of warnings.
#pragma warning(disable : 26432)
#pragma warning(disable : 26440)
#pragma warning(disable : 26455)

// We end up including ApiMessage.h somehow, which uses nameless unions
#pragma warning(disable : 4201)

Or you could fix the warnings now :P

    35>C:\a\_work\1\s\src\inc\til\env.h(346): error C26490: Don't use reinterpret_cast (type.1). 
    35>C:\a\_work\1\s\src\inc\til\env.h(352): error C26490: Don't use reinterpret_cast (type.1). 
    35>C:\a\_work\1\s\src\inc\til\env.h(489): error C26440: Function 'til::env::is_path_var' can be declared 'noexcept' (f.6). 
    35>C:\a\_work\1\s\src\inc\til\env.h(521): error C26481: Don't use pointer arithmetic. Use span instead (bounds.1). 
    35>C:\a\_work\1\s\src\inc\til\env.h(506): error C26481: Don't use pointer arithmetic. Use span instead (bounds.1). 
    35>C:\a\_work\1\s\src\inc\til\env.h(526): error C26455: Default constructor should not throw. Declare it 'noexcept' (f.6). 
    35>C:\a\_work\1\s\src\inc\til\env.h(574): error C26440: Function 'til::env::as_map' can be declared 'noexcept' (f.6). 
    35>C:\a\_work\1\s\src\cascadia\TerminalConnection\ConptyConnection.cpp(106): error C26478: Don't use std::move on constant variables. (es.56).

DHowett

beautiful. thanks for the quick patch!

DHowett · 2023-03-17T17:06:09Z

Hey @lhecker, would you be willing to fix the audit failures on-branch so we can quick merge Ian's work?

lhecker · 2023-03-17T18:07:24Z

I've fixed the audit mode issues, as well as some others. I've also made some further improvements to til::env along the way.

However now that I understand how til::env was intended to be used, I have to confess that I'd be happier with a solution like ConEmu's. Theirs is clearly superior as it's simpler and more performant. It only reloads on WM_SETTINGCHANGE, just like the feature request asked. Because of this I'm not sure if the exact feature request of #1125 is solved with this PR just yet. Alternatively we should create a followup issue to address this, as the code that til::env runs is rather heavy and there's almost never any reason to run it. (After all, how often do environment variables change anyways?)

zadjii-msft · 2023-03-17T18:10:16Z

In the name of incremental progress - merge now, and fast-follow with a PR for "cache a static 'base' env block and only reload it on WM_SETTINGCHANGE"?

ianjoneill · 2023-03-17T18:31:54Z

I had actually fixed some of the audit issues this morning - rather than just ignoring them :)

DHowett · 2023-03-17T18:48:47Z

Ah, sorry to override/duplicate your effort @ianjoneill!

Ares9323 · 2023-09-07T23:54:35Z

ReloadEnvironmentVariables.mp4

I've tried to look for "experimental.reloadEnviromentVariables": true, or similar names both in the regular windows terminal and in the preview version but I wasn't able to find anything, has it been implemented differently or elsewhere?

zadjii-msft · 2023-09-08T11:24:25Z

What are you trying to accomplish exactly? The setting added as compatibility.reloadEnvironmentVariables, in the globals, but it's also enabled by default in 1.18.

Ares9323 · 2023-09-08T11:44:31Z

What are you trying to accomplish exactly? The setting added as compatibility.reloadEnvironmentVariables, in the globals, but it's also enabled by default in 1.18.

Thank you for your answer, I tried looking for both experimental.reloadEnviromentVariables and compatibility.reloadEnvironmentVariables in the settings.json but nothing was showing up in the autocompletion, so I wrongly assumed that the option was missing (Screenshot from WT preview 1.18.14):

The default terminal was obviously not working because it hasn't been updated to 1.18 yet:

Add a setting to reload environment variables

7bc0e9e

microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Mar 16, 2023

This comment has been minimized.

Sign in to view

Fix typo

cdcd73d

zadjii-msft reviewed Mar 16, 2023

View reviewed changes

src/cascadia/TerminalSettingsModel/MTSMSettings.h Outdated Show resolved Hide resolved

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

src/cascadia/TerminalControl/IControlSettings.idl Outdated Show resolved Hide resolved

ianjoneill added 3 commits March 16, 2023 13:36

Make the diff smaller

12c71ef

experimental -> compatibility

7a9c922

Format

010b7b1

Merge branch 'main' into f-reload-environment-variables-v2

0b52fb6

DHowett requested changes Mar 16, 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 Mar 16, 2023

Move the work down into ConptyConnection

dba5220

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 Mar 17, 2023

zadjii-msft approved these changes Mar 17, 2023

View reviewed changes

DHowett approved these changes Mar 17, 2023

View reviewed changes

lhecker added 3 commits March 17, 2023 18:59

Fix AuditMode failures

e37bf5e

Fix clang-tidy warnings

db7be93

Reduce memory allocations in til::env

31676b4

This comment has been minimized.

Sign in to view

Fix spelling issues

055226a

DHowett approved these changes Mar 17, 2023

View reviewed changes

DHowett changed the title ~~Add a setting to reload environment variables~~ Reload environment variables by default; add setting to disable Mar 17, 2023

DHowett merged commit 5166148 into microsoft:main Mar 17, 2023

chawyehsu mentioned this pull request Mar 22, 2023

Cannot change 'Path' in registry ScoopInstaller/Install#48

Closed

zadjii-msft mentioned this pull request Mar 24, 2023

The environment variables of "Open in Windows Terminal" and "Manually Start Windows Terminal" are inconsistent. #12602

Closed

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

The environment variables of "Open in Windows Terminal" and "Manually Start Windows Terminal" are inconsistent. #12602

Closed

WHYBBE mentioned this pull request Apr 1, 2023

fix(env): Apply env immediately by SendMessageTimeout ScoopInstaller/Scoop#5452

Merged

dunhor mentioned this pull request Apr 3, 2023

[Doc] Add missing info to building/testing section of README microsoft/wil#307

Merged

lhecker mentioned this pull request Apr 4, 2023

Reload environment variables only on WM_SETTINGCHANGE #15102

Open

DHowett mentioned this pull request Apr 7, 2023

SESSIONNAME not set during RDP operations #15134

Closed

This was referenced Apr 16, 2023

Open alias "wt" have unexpected behavior. #12545

Closed

Environment variable references in environment variables are not resolved with wt.exe #15191

Closed

eZioPan mentioned this pull request May 25, 2023

wt ignore env-vars set in the script launching it, due to new "auto reload system env vars with new window / tab" #15430

Closed

lhecker mentioned this pull request Jun 27, 2023

Windows Terminal does not check new PATH variables #12214

Closed

citelao mentioned this pull request Aug 28, 2023

[Install] winget installation instructions don't add az to path MicrosoftDocs/azure-docs-cli#3568

Closed

3 tasks

Jaswir mentioned this pull request Sep 13, 2023

Add a key binding to set or toggle acrylic #2531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reload environment variables by default; add setting to disable #14999

Reload environment variables by default; add setting to disable #14999

ianjoneill commented Mar 16, 2023 •

edited

Loading

This comment has been minimized.

ianjoneill commented Mar 16, 2023

zadjii-msft left a comment

ianjoneill commented Mar 16, 2023 •

edited

Loading

ianjoneill commented Mar 16, 2023

DHowett Mar 16, 2023

zadjii-msft commented Mar 17, 2023

DHowett left a comment

DHowett commented Mar 17, 2023

This comment has been minimized.

lhecker commented Mar 17, 2023

zadjii-msft commented Mar 17, 2023

ianjoneill commented Mar 17, 2023

DHowett commented Mar 17, 2023

Ares9323 commented Sep 7, 2023

zadjii-msft commented Sep 8, 2023

Ares9323 commented Sep 8, 2023

Reload environment variables by default; add setting to disable #14999

Reload environment variables by default; add setting to disable #14999

Conversation

ianjoneill commented Mar 16, 2023 • edited Loading

Summary of the Pull Request

Validation Steps Performed

PR Checklist

This comment has been minimized.

ianjoneill commented Mar 16, 2023

zadjii-msft left a comment

Choose a reason for hiding this comment

ianjoneill commented Mar 16, 2023 • edited Loading

ianjoneill commented Mar 16, 2023

DHowett Mar 16, 2023

Choose a reason for hiding this comment

zadjii-msft commented Mar 17, 2023

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Mar 17, 2023

This comment has been minimized.

lhecker commented Mar 17, 2023

zadjii-msft commented Mar 17, 2023

ianjoneill commented Mar 17, 2023

DHowett commented Mar 17, 2023

Ares9323 commented Sep 7, 2023

zadjii-msft commented Sep 8, 2023

Ares9323 commented Sep 8, 2023

ianjoneill commented Mar 16, 2023 •

edited

Loading

ianjoneill commented Mar 16, 2023 •

edited

Loading