10000 UseNewEnvironment restores environment variables for child process by iSazonov · Pull Request #10745 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

iSazonov
Copy link
Collaborator
@iSazonov iSazonov commented Oct 9, 2019

PR Summary

  • Save environment variables at startup
  • Restore these saved environment variables for child process is UseNewEnvironment parameter is used in Start-Process cmdlet

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

@iSazonov iSazonov added Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Oct 9, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.5 milestone Oct 9, 2019
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Oct 9, 2019
get => _environmentVariables;
}

private static IDictionary _environmentVariables;
Copy link
Contributor

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.

Copy link
Collaborator Author

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();
Copy link
Contributor

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)
{
    ...
}

?

Copy link
Collaborator Author

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.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 10, 2019
Copy link
Contributor
@PaulHigin PaulHigin left a 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.

@iSazonov iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 11, 2019
@iSazonov
Copy link
Collaborator Author

/cc @SteveL-MSFT for PowerShell Committee review.

@joeyaiello
Copy link
Contributor

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 Start-Process powershell and get a new terminal.

@joeyaiello
Copy link
Contributor

I misunderstood the original intent of this PR, apologies (reading too fast).

@PowerShell/powershell-committee discussion on this was interesting.

First, it looks like Start-Process -UseNewEnvironment on Windows gives the equivalent of a fresh login (i.e. Machine and User variables as they're stored in the registry).

Additionally, this caching behavior can be somewhat non-deterministic when I'm executed from multiple shells. For instance, I could:

  1. start my login shell as Bash
  2. modify my environment
  3. start pwsh
  4. modify my environment
  5. start pwsh
  6. modify my environment
  7. Start-Process -UseNewEnvironment pwsh

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 -UseNewEnvironment. On Linux, this would match @rjmholt 's behavior for pwsh -l where we use sh -l on Linux and zsh -l on macOS each in conjunction with -c export to retrieve explicitly and only the exported environment variables from the login shell invocation.

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?

@mklement0
Copy link
Contributor
mklement0 commented Oct 17, 2019

@joeyaiello:

The determinism I was going for was:

  • For the child process about to be created, provide the same environment that the current process saw when it started.

  • In other words: Ignore all in-session modifications made by the current process.

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:

  • It may be good enough (you don't typically nest shells, and the need to ignore in-session changes strikes me as the typical use case)

  • It is easy to implement consistently, in a platform-neutral manner (and easy to explain and document).


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.

on Windows, the current code clears the env block, then fills it from the system profile (hence username is system) and then the user profile overwriting any that exist in the system. Hence Path is incomplete. It seems the fix here is to merge Path (append user PATH to end of machine PATH). USERNAME should be special cased to the user.

  • Unix-like platforms:

    • macOS: A login-shell like environment seems appropriate - but the CLI's -l workaround (launching via a hidden sh instance) is not really an option here; not sure how to get the right environment directly.

    • Linux: ? (No personal knowledge - no need for a login shell)

8000

@mklement0
Copy link
Contributor

P.S.: Forgot to mention that cmd.exe's start /I feature - which is presumably what Start-Process -UseNewEnvironment was inspired by - indeed uses the use-the-shell's-own-startup-environment approach, as originally suggested:

# 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%"

@iSazonov
Copy link
Collaborator Author

Replaced by #10830

@iSazonov iSazonov closed this Oct 18, 2019
@mklement0
Copy link
Contributor

Thinking about this some more:

  • Unix platforms have no native programmatic mechanism that I know of for "give me a pristine but functional environment":

    • Utility env with -i radically clears the environment, which makes for a non-functional environment that bash itself happens to compensate for by having hard-coded defaults.

    • Someone who truly needs to start with a pristine environment can use env -i on Unix and manually reconstruct the essential system variables, if really needed - and using env -i is also an option from PowerShell.

    • I'm not sure anyone would miss that ability on Windows, since it has never been there.

  • On Windows, the original feature similar to "give me a pristine but functional environment" was actually "give me a copy of this process' original environment, without any in-session changes" in the form of cmd.exe's start /I


In short: There's no existing PowerShell functionality that we're really beholden to, on either platform.

Based on the pragmatic arguments made above:

  • "give me a copy of this process' original environment, before in-session changes" is typically the same as the pristine-environment feature.

  • the former is easy to explain, easy to document, easy to implement across platforms

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.

@iSazonov
Copy link
Collaborator Author
iSazonov commented Oct 19, 2019

@mklement0 Thanks for great comment!

As for Unix-s my current thought is to use Robert's workaround for login shell.

@mklement0
Copy link
Contributor

@iSazonov:

A login shell is only appropriate for macOS, not Linux.

In case you're thinking of the /bin/sh -c 'exec pwsh ... workaround (please clarify):

That will (a) require you to start with a cleared environment, as with env -i, which (b) will not give you a functional environment, even with -l:

$ 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 USER, SHELL, LANG, ... are missing.

@iSazonov
Copy link
Collaborator Author

As you can see, vital environment variables such as USER, SHELL, LANG, ... are missing.

Where do these variables come from?

@mklement0
Copy link
Contributor

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:

  • on Windows: when you open a new shell window via the Start Menu or the Run dialog, for instance.

  • on Unix: when you use the default terminal program to open a new shell window.

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:

  • it's the only functioning approach currently available on Windows, via cmd.exe's start /I

  • neither feature currently exists on Unix (only the clear-all feature does, which continues to be available via env -i)

  • in the typical use case it will be the same as "give me a pristine but functional environment"

@iSazonov iSazonov deleted the update-usenewenvironment branch October 21, 2019 04:20
@daxian-dbw daxian-dbw removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Documentation Needed in this repo Documentation is needed in this repo
Projects
None yet
5 participants
0