-
Notifications
You must be signed in to change notification settings - Fork 7.6k
UseNewEnvironment restores environment variables for child process #10745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
get => _environmentVariables; | ||
} | ||
|
||
private static IDictionary _environmentVariables; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: usually a field definition appears before it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// Use environment variables from saved at startup dictionary | ||
var envVariables = startInfo.EnvironmentVariables; | ||
envVariables.Clear(); | ||
IDictionaryEnumerator e = System.Management.Automation.Runspaces.EarlyStartup.InitialEnvironmentVariables.GetEnumerator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why did you use an enumerator object explicitly rather than:
foreach (DictionaryEntry entry in EarlyStartup.InitialEnvironmentVariables)
{
...
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the code from CoreFX.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But committee will have to decide whether to take this breaking change.
/cc @SteveL-MSFT for PowerShell Committee review. |
Will talk about this with @PowerShell/powershell-committee today, but I think this is a worthwhile change given it's what's the norm across Unix systems. New processes should be expected to set environment state they don't want to be handed by parent processes. Would love to understand how this works on Windows today, though, as it was my expectation that things already worked this way: PowerShell 6.2.1
Copyright (c) Microsoft Corporation. All rights reserved.
https://aka.ms/pscore6-docs
Type 'help' to get help.
Loading personal and system profiles took 883ms.
C:\Users\jaiello> $env:foo = "bar"
C:\Users\jaiello> powershell
Windows PowerShell
Copyright (C) Microsoft Corporation. All rights reserved.
Try the new cross-platform PowerShell https://aka.ms/pscore6
PS C:\Users\jaiello> $env:foo
bar This also is true when I use |
I misunderstood the original intent of this PR, apologies (reading too fast). @PowerShell/powershell-committee discussion on this was interesting. First, it looks like Additionally, this caching behavior can be somewhat non-deterministic when I'm executed from multiple shells. For instance, I could:
I'm now getting the modified state post-step 4 instead of the original login shell environment post-step 1 (as I'd expect to get on Windows today). Therefore, we think the behavior that folks would expect from this is to get the login shell behavior with If you disagree, can you help us understand a scenario where you'd actually want the exact environment state from two shells up your stack? |
The determinism I was going for was:
That is indeed not the same as "provide the same environment a new shell process would see if created directly", not via another shell instance, but, pragmatically speaking:
As for "provide the same environment a new shell process would see if created directly" functionality: If it can be done, consistently across all platforms, I agree that it is the better choice.
|
P.S.: Forgot to mention that # Opens a new cmd.exe window that DOES see $env:FOO,
# because it was present in the startup environment in the intermediate cmd.exe instance,
# but not $env:FOO2
$env:FOO='bar'; cmd /c "set FOO2=no & start /I cmd /k echo %FOO%-%FOO2%" |
Replaced by #10830 |
Thinking about this some more:
In short: There's no existing PowerShell functionality that we're really beholden to, on either platform. Based on the pragmatic arguments made above:
my vote is therefore to implement the "give me a copy of this process' original environment, without any in-session changes" - unless someone knows how to - robustly, in a conceptually clean manner - implement the pristine-environment feature on macOS and Linux. I think that is preferable to having the pristine-environment feature, but on Windows only. |
@mklement0 Thanks for great comment! As for Unix-s my current thought is to use Robert's workaround for login shell. |
A login shell is only appropriate for macOS, not Linux. In case you're thinking of the That will (a) require you to start with a cleared environment, as with $ env -i /bin/sh -l -c printenv
PATH=/usr/local/bin:/usr/bin:/bin:...
SHLVL=1
_=/usr/bin/printenv As you can see, vital environment variables such as |
Where do these variables come from? |
It all relates to the unspoken premise of the "give me a pristine but functional environment" feature: A pristine environment obtained how? In terms of effect, you'd want the same kind of environment you get:
My point is that we - certainly I - don't know how to do that programmatically or even whether it's feasible on all Unix platforms (and at this point I'm also not convinced that the Windows-only #10830 is fully functional). So, instead of going down that rabbit hole, we could implement the "give me a copy of this process' original environment, without any in-session changes" approach, document it, and be done. I suspect no one will complain, because:
|
PR Summary
Formally it is a breaking change in grey area.
I hope we do not load any additional dlls with the change at startup and slowdown the startup scenario will be minimal.
PR Context
Fix #4671
Fix #3545
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.