Create til::env to help with refreshing environment variables by ianjoneill · Pull Request #14839 · microsoft/terminal

ianjoneill · 2023-02-13T13:26:50Z

Adds a helper that replicates how the RegenerateUserEnvironment()
method in shell32.dll behaves.

Raises Create til::env to help with refreshing environment variables #12516 from the dead.
Half of Feature Request: Terminal should listen for the WM_SETTINGCHANGE for environment variable updates #1125

Co-authored-by: Michael Niksa [email protected]

src/inc/til/env.h

ianjoneill · 2023-02-13T18:42:27Z

Hmm, the environment blocks produced by RegenerateUserEnvironment() and the new method aren't the same during the test run on Azure. The expected temp directory has the DOS short name for the username, whereas the environment class produces a path containing the full username. On my local machine the test passes - as my username is Ian. I'm guessing Michael's username was miniksa so he didn't see this either?

2023-02-13T14:09:08.3435350Z Error: Verify: AreEqual(expectedValue, actual.as_map()[expectedKey]) - Values (C:\Users\CLOUDT~1\AppData\Local\Temp, C:\Users\cloudtest\AppData\Local\Temp) [File: C:\a\_work\1\s\src\til\ut_til\EnvTests.cpp, Function: EnvTests::Generate, Line: 44]

ianjoneill · 2023-02-13T22:28:39Z

So on my work PC, which has %USERPROFILE% as C:\Users\ian.oneill, RegenerateUserEnvironment() returns the following:

TEMP=C:\Users\IAN~1.ONE\AppData\Local\Temp
TEMPTEST1=C:\WINDOWS\TEMP\foo
TEMPTEST2=C:\WINDOWS\TEMP
TEMPTEST3=C:\Users\IAN~1.ONE\AppData\Local\Temp\foo
TEMPTEST4=C:\Users\IAN~1.ONE\AppData\Local\Temp
TMP=C:\Users\IAN~1.ONE\AppData\Local\Temp

With HKCU\Environment set to:

This suggests check_for_temp() needs a bit of tweaking.

src/inc/til/env.h

DHowett · 2023-02-16T00:34:43Z

src/inc/til/env.h

+                                    }
+                                    else
+                                    {
+                                        set_user_environment_var(valueName, std::wstring{ data });


this will expand it even during pass 1 for a REG_SZ; is that definitely what we want? I suspect it is, because the environment is strange

src/inc/til/env.h

DHowett · 2023-02-16T00:36:40Z

src/inc/til/env.h

+        static constexpr std::wstring_view os2LibPath{ L"Os2LibPath" };
+        bool is_path_var(std::wstring_view input)
+        {
+            return !_wcsicmp(input.data(), path.data()) || !_wcsicmp(input.data(), libPath.data()) || !_wcsicmp(input.data(), os2LibPath.data());


@lhecker may be able to recommend a cleaner way to do this

src/inc/til/env.h

ianjoneill · 2023-02-17T22:02:24Z

All comments should now hopefully be addressed

ianjoneill · 2023-02-18T19:08:23Z

Test failures are flaky feature tests.

lhecker

I'm not too happy about the amount of string copies, but I'd be happy to merge this anyways. I'd love to see some or all of my comments to be addressed though.

src/inc/til/env.h

lhecker · 2023-03-01T16:27:22Z

src/inc/til/env.h

+                                                                                                  // length will receive the number of bytes including trailing null byte. Convert to a number of wchar_t's.
+                                                                                                  // AdaptFixedSizeToAllocatedResult will then resize buffer to valueLengthNeededWithNull.
+                                                                                                  // We're rounding up to prevent infinite loops if the data isn't a REG_SZ and length isn't divisible by 2.
+                                                                                                  *valueLengthNeededWithNul = (length + sizeof(wchar_t) - 1) / sizeof(wchar_t);


This function is a little bit cheeky since RegQueryValueExW returns binary data, but this assumes the caller is trying to get a wide string. I think it's kind of fine though...

zadjii-msft

Assuming that the underlying algorithm that @miniksa wrote for regenerating the environment is correct (I have no reason to believe it isn't), then the rest of this looks good to me.

I'd love if @miniksa could give this another once over, but the Terminal ain't his day job anymore 😥

Sorry it took us so long to get around to this! You caught us at a particularly busy couple months 😅 Looking forward to shipping this (and the follow up 😉)

zadjii-msft · 2023-02-22T15:30:15Z

src/inc/til/env.h

+            inline HRESULT GetComputerNameW(string_type& result) WI_NOEXCEPT
+            {
+                return wil::AdaptFixedSizeToAllocatedResult<string_type, initialBufferLength>(result,
+                                                                                              [&](_Out_writes_(valueLength) PWSTR value, size_t valueLength, _Out_ size_t* valueLengthNeededWithNul) -> HRESULT {


holy indenting batman. This is totally a nit, but I'd pull that lambda out to a local, just for the sake of some sensible indenting

ianjoneill · 2023-03-02T15:24:01Z

Looking forward to shipping this (and the follow up 😉)

That is waiting in the wings and is a grand total of 22 additional lines.

zadjii-msft · 2023-03-03T18:44:59Z

@msftbot merge this at 7pm

(to give @miniksa and @DHowett a last chance to block)

DHowett

block to review

DHowett

LET'S GO! YEAH!

miniksa and others added 8 commits January 14, 2022 15:15

attempt to replicate algorithm

6cac096

IT WORKS!

cd1c4b6

make a note

3fb8a8b

Migrate spelling-0.0.21 changes from main

65ada35

Merge branch 'main' into dev/miniksa/env

5eb08ff

Merge branch 'main' into dev/miniksa/env

6236898

Don't inherit from std::map

e409d15

Spelling

fd59513

This comment has been minimized.

Sign in to view

ianjoneill commented Feb 13, 2023

View reviewed changes

src/inc/til/env.h Outdated Show resolved Hide resolved

Maybe this is the one the bot is complaining about

d5346be

DHowett reviewed Feb 16, 2023

View reviewed changes

ianjoneill added 4 commits February 17, 2023 21:37

Tests should now pass

bc92eba

CompareStringOrdinal

1dd65b3

std::move

b3434dc

Code format

1f7dee4

ianjoneill added 4 commits February 18, 2023 14:36

Add some logging to debug tests in CI

b733a6c

Strip off trailing nulls

f41d31d

Format

d66acfe

Use true and false over TRUE and FALSE

24044c6

lhecker approved these changes Mar 1, 2023

View reviewed changes

ianjoneill added 5 commits March 1, 2023 19:11

Address some of the comments

fe4a48f

Address the other comment

c39ee85

Don't use underscores in variable names

7503c86

Finally get it right

6bc9050

This is actually the final thing

d45841f

zadjii-msft approved these changes Mar 2, 2023

View reviewed changes

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

DHowett mentioned this pull request Mar 3, 2023

New tabs should re-read environment variable list. #14948

Closed

DHowett approved these changes Mar 13, 2023

View reviewed changes

DHowett merged commit eb871bf into microsoft:main Mar 13, 2023

zadjii-msft mentioned this pull request Mar 21, 2023

REG_EXPAND_SZ environment variables not properly expanded for shells #9741

Closed

ianjoneill mentioned this pull request Mar 24, 2023

implement env vars in settings (#2785) #9287

Closed

6 tasks

Conversation

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

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

ianjoneill commented Feb 13, 2023

Uh oh!

ianjoneill commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ianjoneill commented Feb 17, 2023

Uh oh!

ianjoneill commented Feb 18, 2023

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhecker Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

ianjoneill commented Mar 2, 2023

Uh oh!

zadjii-msft commented Mar 3, 2023

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ianjoneill commented Feb 13, 2023 •

edited by DHowett

Loading