implement env vars in settings (#2785) by christapley · Pull Request #9287 · microsoft/terminal

christapley · 2021-02-25T13:35:55Z

Summary of the Pull Request

Implement environment variables with validation
use StringMap as a container
recursively resolve ${env:NAME} vars in values in the StringMap or in
the process' environment
implement some tests

References

PR Checklist

Closes (Feature Request) Terminal Environment setting (like in VScode) #2785
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Added tests and manual validation

* use StringMap as a container * recursively resolve ${env:NAME} vars in values in the StringMap or in the process' environment * implement some tests

zadjii-msft

I'm really happy with the tests for this - they make me feel quite a bit more confident in this.

I'm just gonna be a stickler about the comments on ResolveEnvironmentVariableValue though. I want to make sure the next person who comes through and has to touch that has a bit of an easier job 😄

That's really my only major concern. I think the rest of my comments basically count as nits.

src/cascadia/TerminalControl/IControlSettings.idl

src/cascadia/TerminalSettingsModel/Profile.h

zadjii-msft · 2021-03-04T16:33:04Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+
+void Profile::ValidateEvaluatedEnvironmentVariables() const
+{
+    const auto envVars = _EvaluatedEnvironmentVariables();


Idea: could we cache the result of _EvaluatedEnvironmentVariables inside the Profile, the first time it's called, and then just return that value out of EvaluatedEnvironmentVariables? That way we wouldn't need to evaluate the envvars every time a new tab/pane for this profile is created

Though, I suppose this helps re-evaluate envvars. If the environment changed between the last time the settings have loaded, and when a new tab is created, then the way it is would use the updated values.

Oof okay now my head hurts. @DHowett help me understand how this interacts with #7239 and #1125

src/cascadia/TerminalSettingsModel/JsonUtils.h

zadjii-msft · 2021-03-04T17:01:27Z

src/cascadia/TerminalSettingsModel/JsonUtils.h

+    std::decay_t<T> GetValue(const Json::Value& json);
+
+    template<>
+    struct ConversionTrait<winrt::Windows::Foundation::Collections::StringMap>


If we move this ConversionTrait near the end of the file, would we need the fwddecls for nit: SetValueForKey and GetValue above?

I believe this is a bit of a chicken and egg story. I can't put it below SetValueForKey or GetValue as they both use the ConversionTrait template. So while, yes, it could go lower I don't believe it can go low enough to not require the fwd declarations and then it wouldn't be near the other ConversionTrait template specialisations.

The other alternative is that I don't put this ConversionTrait specialisation in JsonUtil.h. It could go in Profile.cpp if you think this is better?

src/cascadia/TerminalSettingsModel/Profile.cpp

zadjii-msft · 2021-03-04T17:39:31Z

@msftbot make sure @DHowett signs off on this.

I think he had some thoughts on three-way merging the variables or something? He mentioned something in team sync a couple weeks ago, but I forget what it was exactly. As a heads up, he's OOF currently, so it might be a few days before he's able to get back to this.

ghost · 2021-03-04T17:39:35Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @DHowett

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

* add comments to ResolveEnvironmentVariableValue * use wil helper * remove unused code

KalleOlaviNiemitalo · 2021-03-07T08:34:09Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+                if (regexMatch.size() != 2)
+                {
+                    std::wstringstream error;
+                    error << L"Unexpected regex match count '" << regexMatch.size()
+                          << L"' (expected '2') when matching '" << std::wstring(textIter, value.rawValue.cend()) << L"'";
+                    throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::hstring(error.str()));


Perhaps that should be a fail-fast exception if it can only happen when parameterRegex is incorrectly defined.

@zadjii-msft, what do you think about this? Should we RaiseFailFastException and terminate under this (and other) unexpected error conditions?

I always lean towards not fail-fasting if at all possible. This seems like a perfectly fine way to gracefully handle the error for now.

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

KalleOlaviNiemitalo · 2021-03-07T08:40:13Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+    //             that each layer in the recursion only has keys seen leading up to it
+    // Return Value:
+    // - The resolved value for key
+    std::wstring ResolveEnvironmentVariableValue(const std::wstring& key, std::map<std::wstring, VariablePair>& envMap, std::list<std::wstring> seenKeys = {})


Everything in this method compares the names of environment variables case-sensitively. Is that by design?

It was intended that way, however, I am struggling to find a use case where it needs case sensitivity. @zadjii-msft, should I make the environment variables case insensitive?

Even with a profile like git bash (which supports case sensitive env vars when running, they can't be applied through terminal:

{ "acrylicOpacity": 0.75, "background": "#000000", "closeOnExit": true, "colorScheme": "Campbell", "commandline": "\"%PROGRAMFILES%\\git\\bin\\bash.exe\" --login -i -l", "cursorColor": "#FFFFFF", "cursorShape": "bar", "fontFace": "Consolas", "fontSize": 10, "guid": "{00000000-0000-0000-0000-000000012345}", "historySize": 9001, "icon": "%PROGRAMFILES%\\git\\mingw64\\share\\git\\git-for-windows.ico", "name": "Git Bash", "padding": "0, 0, 0, 0", "snapOnInput": true, "useAcrylic": true, "environment": { "key": "a", "KEY": "b" } }

Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ echo $KEY Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ echo $key b Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ export MY_KEY=a Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ export my_key=b Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ echo $MY_KEY a Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $ echo $my_key b Chris@DESKTOP-R8BPF5L MINGW64 /c/WINDOWS/SysWOW64 $

Yikes, this one I'm not sure about. I always assumed that env vars were case insensitive on Windows. So maybe the resolution here should be as well. That seems to make sense to me, but I don't think this is work blocking over tbh. @DHowett might have other thoughts though.

Alas, yes.. I believe Windows' environment block expects to never find multiple case-differing copies of a single environment variable. We may want to keep Windows' tenets in the profile loader rather than Linux's.

Are the case-insensitive string comparisons in GetEnvironmentVariable, ExpandEnvironmentStrings, etc. culture-dependent? I hope not.

You can look in PT Run's environment helper class. I copied the entire behavior of windows. The helper behaves exactly the same as Windows does.

Are global profile environment variables planned?

@htcfreek your insight might be appreciated over in #1125, where @miniksa is working on https://github.com/microsoft/terminal/blob/dev/miniksa/env/src/inc/til/env.h

I can help with how windows behave. But I can't help eith coding. I don't know much about C++ language.

KalleOlaviNiemitalo · 2021-03-07T08:44:12Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+            if (!it->second.resolvedValue.empty())
+            {
+                // key has already been resolved, so just return it
+                return it->second.resolvedValue;
+            }


This won't notice if the environment variable has been previously resolved with an empty value. But okay, it's only a shortcut.

I could make this more explicit and store something so that we know it has been resolved if you think it is worth the complexity?

I think the complexity is not worth it until this becomes so slow that users notice.

src/cascadia/TerminalSettingsModel/Profile.cpp

* allow appending or prepending process environment variables * don't handle all exceptions on validation failure

christapley · 2021-03-07T23:34:24Z

@zadjii-msft, should I resolve the merge conflicts by rebasing off your main and force pushing? I know this is not ideal for people reviewing this PR but I don't have any other way of resolving the conflicts from my end.

zadjii-msft · 2021-03-08T13:27:31Z

@christapley I think github let me resolve the conflict for you? I really hope that worked - I haven't had a ton of success with the web conflict resolver in the past.

* Use INHERITABLE_SETTING macro

zadjii-msft

Okay thanks for the extra comments!

zadjii-msft · 2021-03-08T16:47:19Z

src/cascadia/TerminalSettingsModel/Profile.h

-{
-    BASIC_FACTORY(Profile);
-}
+/*++


ho boy, I bet I blew up the line endings when I merged this on github 😢

zadjii-msft · 2021-03-08T16:48:38Z

src/cascadia/TerminalApp/TerminalSettings.h

-{
-    BASIC_FACTORY(TerminalSettings);
-}
+/*++


uhg same here, this is my fault 😢

christapley · 2021-03-15T11:12:59Z

@DHowett, let me know if there's anything you don't like or want changed. Thanks!

DHowett · 2021-03-17T19:25:24Z

@christapley sorry I'm so backlogged. I have ~ ~ thoughts ~ ~ about environment variables and want to make sure I have them in order before bossing you around on a code review. 😄

Thanks for the patience!

christapley · 2021-10-12T12:22:04Z

Hey @DHowett, any feedback on this pr?

zadjii-msft · 2021-10-19T16:15:57Z

@christapley FWIW @DHowett is stuck doing manager-y things for at least the rest of the week, so you might have a bit of a delay⁽¹⁾ getting some response on this one

(1): more than the 6 months it's already taken, sorry 😢

DHowett · 2021-10-19T16:51:50Z

Good lord. I am so sorry to leave you hanging like that. It's my responsibility to uphold my end of the bargain when it comes to community management and PR review.

DHowett

Okay, this is the review I've waited months to wrap my head around. I completely understand if you've lost interest in pursuing this change.

I'm mostly concerned with how this interacts with environment expansion/environment refresh ala #7239.

Here's my thoughts.

The environment might change outside of Terminal, and Terminal will need to re-evaluate everything in Profile's environment map.
- If we don't, a profile could specify ${env:PATH} and resurrect the PATH from before the system environment changed.
- Q: Should we defer expansion until the connection is about to start? This makes error reporting difficult.
  - Sub-Q: If error reporting is difficult, can we just not translate bad entries? I suspect Windows does this when the environment block is self-referencing or recursively-self-referencing¹.
Not every connection type will support an environment or environment variables. Only ConptyConnection, which deals in native processes, really knows/cares.

If we defer expansion until the connection is about to start, we might also want to simply tackle all environment block re-evaluation. That's an immense ask, I know. The biggest issue plaguing Terminal Preview today is that we generate a new environment block for every connection, and it loses environment variables WT inherited from its parent. We'd need a 3-way merge (this has been discussed in other threads, but I am barely hanging on to a working browser state as it is and I don't want to lose this comment-in-progress.) The follow-on effects from this include, like, powershell x86 failing to find its modules, because it relies on inheriting the x64 powershell's PSModulePath from its parent. Yeah.

I love where this is going, or has gone while I was not looking at it, and I think it might be a great stone with which we can knock out a bunch of birds at the same time.

What do you think?

¹ Aha!:

DHowett · 2021-11-09T23:35:25Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+    //             that each layer in the recursion only has keys seen leading up to it
+    // Return Value:
+    // - The resolved value for key
+    std::wstring ResolveEnvironmentVariableValue(const std::wstring& key, std::map<std::wstring, VariablePair>& envMap, std::list<std::wstring> seenKeys = {})


Alas, yes.. I believe Windows' environment block expects to never find multiple case-differing copies of a single environment variable. We may want to keep Windows' tenets in the profile loader rather than Linux's.

zadjii-msft · 2021-11-18T15:34:23Z

(I'm tagging this for discussion but it almost needs it's own dedicated meeting / spec review at this point)

### ⇒ [doc link](https://github.com/microsoft/terminal/blob/dev/migrie%2Fs%2F642-logging/doc/specs/drafts/%23642%20-%20Buffer%20Exporting%20and%20Logging/%23642%20-%20Buffer%20Exporting%20and%20Logging.md) ⇐ ## Summary of the Pull Request This is an intentionally brief spec to address the full scope of #642. The intention of this spec is to quickly build consensus around all the features we want for logging, and prepare an implementation plan. ### Abstract > A common user need is the ability to export the history of a terminal session to > a file, for later inspection or validation. This is something that could be > triggered manually. Many terminal emulators provide the ability to automatically > log the output of a session to a file, so the history is always captured. This > spec will address improvements to the Windows Terminal to enable these kinds of > exporting and logging scenarios. ## PR Checklist * [x] Specs: #642 * [x] References: #5000, #9287, #11045, #11062 * [x] I work here ## Detailed Description of the Pull Request / Additional comments _\*\*\* read the spec \*\*\*_ ## Open Discussion * [ ] What formatting string syntax and variables do we want to use? * [ ] How exactly do we want to handle "log printable output"? Do we include backspaces? Do we only log on newlines? * [ ] > maybe consider even simpler options like just `${date}` and `${time}`, and allow for future variants with something like `${date:yyyy-mm-dd}` or `${time:hhmm}`

zadjii-msft · 2023-03-21T16:45:45Z

Hey now that #14999 has merged, I think we can run at this hill again. Admittedly, this PR is MANY months out of date - the changes might need to just be rebased onto a new branch. However, this might be a good time to try again. @ianjoneill this might be something you're interested in doing, if @christapley is cool with that. (don't feel obligated to though)

Sorry that this ended up stagnating like this! Our environment variable story was singularly unpleasant for a long time, hopefully we can finally get past all that ☺️

ianjoneill · 2023-03-24T22:11:06Z

I've had a look over this - and I think I'd agree with @DHowett's comment above - the resolving of the variables should happen when the connection is started (where we refresh the environment block).

Following #14839, we'd get the set_user_environment_var() method in til::env to do the resolving for us - which will deal with the self-referencing etc. (which will allow adding stuff to %PATH%) and variables that reference one another - assuming we iterate over them in a well-defined order - say alphabetical (ignoring case).

In fact if we change the format from ${env:NAME} to use the Windows standard %NAME% format, this will come for free - assuming you're happy with changing that.

I can take a look about merging and updating this as per the above over the next couple of weekends if you agree.

zadjii-msft · 2023-03-27T18:48:25Z

@ianjoneill I'm cool with that. I'm cool with the %var% syntax too. I kinda liked the other one stylistically, but consistency with the rest of the OS is probably smarter anyways.

ianjoneill · 2023-04-01T13:11:01Z

That's a lot of conflicts:

$ git merge main --no-ff
Auto-merging src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
CONFLICT (content): Merge conflict in src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp
Auto-merging src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
CONFLICT (content): Merge conflict in src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
Auto-merging src/cascadia/TerminalApp/AppLogic.cpp
CONFLICT (content): Merge conflict in src/cascadia/TerminalApp/AppLogic.cpp
Auto-merging src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Auto-merging src/cascadia/TerminalApp/TerminalPage.cpp
CONFLICT (modify/delete): src/cascadia/TerminalApp/TerminalSettings.cpp deleted in main and modified in HEAD.  Version HEAD of src/cascadia/TerminalApp/TerminalSettings.cpp left in tree.
CONFLICT (modify/delete): src/cascadia/TerminalApp/TerminalSettings.h deleted in main and modified in HEAD.  Version HEAD of src/cascadia/TerminalApp/TerminalSettings.h left in tree.
CONFLICT (modify/delete): src/cascadia/TerminalApp/TerminalSettings.idl deleted in main and modified in HEAD.  Version HEAD of src/cascadia/TerminalApp/TerminalSettings.idl left in tree.
Auto-merging src/cascadia/TerminalControl/IControlSettings.idl
CONFLICT (content): Merge conflict in src/cascadia/TerminalControl/IControlSettings.idl
CONFLICT (modify/delete): src/cascadia/TerminalSettingsEditor/Profiles.h deleted in main and modified in HEAD.  Version HEAD of src/cascadia/TerminalSettingsEditor/Profiles.h left in tree.
CONFLICT (modify/delete): src/cascadia/TerminalSettingsEditor/Profiles.idl deleted in main and modified in HEAD.  Version HEAD of src/cascadia/TerminalSettingsEditor/Profiles.idl left in tree.
Auto-merging src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Auto-merging src/cascadia/TerminalSettingsModel/CascadiaSettings.h
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/CascadiaSettings.h
Auto-merging src/cascadia/TerminalSettingsModel/JsonUtils.h
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/JsonUtils.h
Auto-merging src/cascadia/TerminalSettingsModel/Profile.cpp
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/Profile.cpp
Auto-merging src/cascadia/TerminalSettingsModel/Profile.h
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/Profile.h
Auto-merging src/cascadia/TerminalSettingsModel/Profile.idl
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/Profile.idl
Auto-merging src/cascadia/TerminalSettingsModel/TerminalWarnings.idl
CONFLICT (content): Merge conflict in src/cascadia/TerminalSettingsModel/TerminalWarnings.idl
Automatic merge failed; fix conflicts and then commit the result.

ianjoneill · 2023-04-01T18:44:31Z

Follow-up: #15082

zadjii-msft · 2023-04-06T20:05:37Z

Closed in favor of #15082

@christapley

Existing environment variables can be referenced by enclosing the name in percent characters (e.g. `%PATH%`). Resurrects #9287 by @christapley. Tests added and manually tested. Closes #2785 Closes #9233 Co-authored-by: Chris Tapley <[email protected]>

implement env vars in settings (microsoft#2785)

b0c426b

* use StringMap as a container * recursively resolve ${env:NAME} vars in values in the StringMap or in the process' environment * implement some tests

Invoke-CodeFormat

5036cb0

zadjii-msft self-assigned this Mar 3, 2021

zadjii-msft requested changes Mar 4, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 4, 2021

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 4, 2021

zadjii-msft assigned DHowett and christapley and unassigned zadjii-msft Mar 4, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 6, 2021

Code review feedback

b4efbb8

* add comments to ResolveEnvironmentVariableValue * use wil helper * remove unused code

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 6, 2021

KalleOlaviNiemitalo reviewed Mar 7, 2021

View reviewed changes

Code review update (KalleOlaviNiemitalo)

5d68c94

* allow appending or prepending process environment variables * don't handle all exceptions on validation failure

This comment has been minimized.

Sign in to view

Fix spelling mistake

ea56d64

Merge branch 'main' into feature/2785-profile-env-vars-with-string-map

9ec2218

zadjii-msft self-assigned this Mar 8, 2021

christapley added 2 commits March 8, 2021 22:31

fix build (microsoft#2785)

f1c1cc4

* Use INHERITABLE_SETTING macro

fix code formatting (microsoft#2785)

0b6f411

zadjii-msft approved these changes Mar 8, 2021

View reviewed changes

zadjii-msft unassigned christapley Mar 8, 2021

zadjii-msft removed their assignment Mar 8, 2021

rashil2000 mentioned this pull request May 9, 2021

Color transitions are not smooth #10062

Closed

floh96 mentioned this pull request Jul 10, 2021

add environment documentation (#2785) MicrosoftDocs/terminal#250

Closed

zadjii-msft mentioned this pull request Aug 31, 2021

DRAFT Spec for Buffer Exporting and Logging #11090

Merged

6 tasks

DHowett reviewed Nov 9, 2021

View reviewed changes

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Nov 18, 2021

zadjii-msft mentioned this pull request Jan 4, 2022

Feature Request: Terminal should listen for the WM_SETTINGCHANGE for environment variable updates #1125

Closed

zadjii-msft added this to the 22H1 milestone Jan 4, 2022

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Jan 7, 2022

raghavthind2005 approved these changes Apr 30, 2022

View reviewed changes

WanderLanz mentioned this pull request Jul 3, 2022

custom environment variables in settings.json map #13406

Closed

zadjii-msft modified the milestones: 22H1, Terminal v1.18 Nov 28, 2022

ianjoneill mentioned this pull request Apr 1, 2023

Support environment variables in the settings #15082

Merged

zadjii-msft closed this Apr 6, 2023

Conversation

christapley commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft commented Mar 4, 2021

Uh oh!

ghost commented Mar 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

christapley commented Mar 7, 2021

Uh oh!

zadjii-msft commented Mar 8, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christapley commented Mar 15, 2021

Uh oh!

DHowett commented Mar 17, 2021

Uh oh!

christapley commented Oct 12, 2021

Uh oh!

zadjii-msft commented Oct 19, 2021

christapley commented Feb 25, 2021 •

edited

Loading

DHowett commented Oct 19, 2021 •

edited

Loading