Support environment variables in the settings by ianjoneill · Pull Request #15082 · microsoft/terminal

ianjoneill · 2023-04-01T18:43:00Z

Existing environment variables can be referenced by enclosing the name in percent characters (e.g. %PATH%).

Tests added and manually tested.

Closes #2785
Closes #9233

Co-authored-by: Chris Tapley [email protected]

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

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

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

* Use INHERITABLE_SETTING macro

zadjii-msft · 2023-04-01T19:18:28Z

holy shit the madman actually did it

ianjoneill · 2023-04-01T21:11:54Z

Test failures are CI flakes

zadjii-msft · 2023-04-03T14:45:01Z

idle thought before reading the code: the original PR might predate layering. Lets see how this works with layering. Can the defaults say (for example) TERM=windows-terminal, and then a profile say TERM=, to unset it? Or does setting it in a profile entirely replace the lower layer? If it's a per-variable layering, we could use that to vaguely do #11057.

ianjoneill · 2023-04-03T15:49:28Z

Currently the concrete profile's environment completely replaces the one specified in defaults.

So if you've got:

"defaults": {
  "environment": {
    "Foo": "Bar"
  }
},
"list": [
  {
    "name": "PowerShell",
    "environment": {
      "Baz": "Qux"
    }
  }
]

Your PowerShell profile will get Baz=Qux and your cmd profile will get Foo=Bar.

ianjoneill

Some thoughts before I open Visual Studio

ianjoneill · 2023-04-04T19:55:24Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+        til::env environment;
        auto zeroEnvMap = wil::scope_exit([&]() noexcept {
            // Can't zero the keys, but at least we can zero the values.
-            for (auto& [name, value] : environment)
+            for (auto& [name, value] : environment.as_map())
            {
                ::SecureZeroMemory(value.data(), value.size() * sizeof(decltype(value.begin())::value_type));
            }

-            environment.clear();
+            environment.as_map().clear();
        });


Unless I'm missing something, if we did this as part of regenerate() the map would be cleared on exit of the method which would mean the map would be populated and immediately cleared.

It would probably make sense to extract this for loop into a clear() method on til::env, so that the caller doesn't need to worry about the internal representation.

With regards to whether it's necessary, I can't answer that - I don't know why the code originally did this. It's possible the environment could contain sensitive information (e.g. temporary credentials), which you may want to clear out.

src/cascadia/TerminalConnection/ConptyConnection.cpp

ianjoneill · 2023-04-04T19:59:36Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

            }
            _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);


Ah - it seemed silly both this and TerminalPage doing the same dance fiddling with WSLENV though.

ianjoneill · 2023-04-06T20:44:31Z

Just FYI I should be able to take a look at this tomorrow (Friday).

DHowett · 2023-04-07T15:30:26Z

This is very subtle (and I am only testing as of >5 hours ago, so please ignore if changed), but you can resurrect inherited environment variables depending on sort order.

I don't know if I think this is an issue worth fixing, but it is fun!

"environment": {
    "Z_NEW_PATH": "%PATH%", // gets new path!
    "PATH": "A",
    "0_OLD_PATH": "%PATH%", // gets old path!
}

ianjoneill · 2023-04-07T18:15:51Z

This is seemingly how Windows resolves environment variables - though the order as far as I can tell is the order they're inserted into the registry.

Starting off with a system environment variable FOO=Foo and setting up the registry with REG_EXPAND_SZ user variables 0_FOO=%FOO%, FOO=NewFoo and Z_FOO=%FOO%, as below:

Leads to them being resolved as 0_FOO=Foo, FOO=NewFoo and Z_FOO=NewFoo in a conhost window, as below:

Worth pointing out though that if you create the FOO user variable in the system properties window, it's set up as a REG_SZ variable - so it does get resolved first, leading to 0_FOO and Z_FOO both resolving to NewFoo.

I guess we could put in a pre-processing step that inserts variables with values not containing percent characters into the environment map first. It would require iterating over the values in the map twice though and would complicate the logic somewhat - your call.

DHowett · 2023-04-07T18:47:19Z

Oh, no, I'm totally happy with how it works today! I legitimately found it interesting 😄

DHowett

I love it. Thanks @ianjoneill and @christapley :)

eggbean · 2024-10-10T17:25:22Z

So glad to find that it's possible to use variables in settings.json but it doesn't seem to work with boolean settings which expect a true/false value.

😞

DHowett · 2024-10-10T18:05:28Z

So glad to find that it's possible to use variables in settings.json

Just to clarify: as of this PR, it is possible to set environment variables in settings.json for the spawned process to inherit. It is not generally possible to use environment variables in settings.json :)

eggbean · 2024-10-10T18:20:41Z

@DHowett Oh, that sucks. Just when I thought that I had finally found a way to easily manage my settings.json in git. Thanks for the info.

christapley and others added 18 commits February 25, 2021 21:18

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

Code review feedback

b4efbb8

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

Code review update (KalleOlaviNiemitalo)

5d68c94

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

Fix spelling mistake

ea56d64

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

9ec2218

fix build (microsoft#2785)

f1c1cc4

* Use INHERITABLE_SETTING macro

fix code formatting (microsoft#2785)

0b6f411

Run unix2dos on TerminalSettings.h and Profile.h

46a9b35

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

ad11e67

It compiles

95cacbc

It works

3d2e3fd

Add warning about environment variables with different cases

e4c9cbb

Tests

fb7ea2d

Remove refactored methods

12c1939

Format

b8165ac

Tidy

92d2351

Schema

c60e2e5

ianjoneill mentioned this pull request Apr 1, 2023

implement env vars in settings (#2785) #9287

Closed

6 tasks

Don't clobber the WSLENV environment variable

3c8554f

ianjoneill mentioned this pull request Apr 1, 2023

Order of environment variables different in WT #9233

Closed

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Apr 3, 2023

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 Apr 4, 2023

ianjoneill commented Apr 4, 2023

View reviewed changes

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 Apr 4, 2023

zadjii-msft added this to the Terminal v1.18 milestone Apr 4, 2023

zadjii-msft self-assigned this Apr 6, 2023

DHowett added a commit that referenced this pull request Apr 6, 2023

PRE-MERGE #15082 Support environment variables in the settings

5af2721

ianjoneill added 7 commits April 7, 2023 11:07

Move til::details::wstring_case_insensitive_compare

f1a538c

Make clear() a function of til::env

460be47

Helpfully update WSLENV

722f34e

Address JSON comments

e7e3f59

Switch to IMap

de5b246

Fix tests

7231f0c

Audit mode

0bfe19e

DHowett added a commit that referenced this pull request Apr 11, 2023

PRE-MERGE #15082 Support environment variables in the settings

90122f8

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Apr 11, 2023

zadjii-msft removed their assignment Apr 11, 2023

DHowett approved these changes Apr 11, 2023

View reviewed changes

DHowett merged commit 56d451d into microsoft:main Apr 11, 2023

ianjoneill mentioned this pull request Apr 12, 2023

Settings error/warning dialog seems to self-dismiss on startup #15170

Closed

zadjii-msft mentioned this pull request Sep 20, 2023

wt.exe should accept env vars on the commandline #16008

Open

2 tasks

This was referenced Jun 6, 2024

We forgot to add profile.environment to the docs MicrosoftDocs/terminal#781

Open

Support custom environment. #17379

Closed

mostpinkest mentioned this pull request Jan 12, 2026

WT should set COLORTERM #11057

Open

Conversation

ianjoneill commented Apr 1, 2023 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Apr 1, 2023

Uh oh!

ianjoneill commented Apr 1, 2023

Uh oh!

zadjii-msft commented Apr 3, 2023

Uh oh!

ianjoneill commented Apr 3, 2023

Uh oh!

ianjoneill left a comment

Choose a reason for hiding this comment

Uh oh!

ianjoneill Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ianjoneill Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

ianjoneill commented Apr 6, 2023

Uh oh!

DHowett commented Apr 7, 2023

Uh oh!

ianjoneill commented Apr 7, 2023

Uh oh!

DHowett commented Apr 7, 2023

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

eggbean commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eggbean commented Oct 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ianjoneill commented Apr 1, 2023 •

edited by DHowett

Loading

eggbean commented Oct 10, 2024 •

edited

Loading

DHowett commented Oct 10, 2024 •

edited

Loading