Reload environment variables by default; add setting to disable

[ianjoneill]

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]

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

[ianjoneill]

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]

Now the build's failing due to #14581.

[DHowett]

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]

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]

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

[lhecker]

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]

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]

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

[DHowett]

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

[Ares9323]

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]

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]

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: