Skip to content
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

Terminal preview does not accept environment variables from parent process #15496

Closed
Tracked by #4632
ChrisKnue-MSFT opened this issue Jun 1, 2023 · 10 comments · Fixed by #15897
Closed
Tracked by #4632

Terminal preview does not accept environment variables from parent process #15496

ChrisKnue-MSFT opened this issue Jun 1, 2023 · 10 comments · Fixed by #15897
Labels
Area-Commandline wt.exe's commandline arguments Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@ChrisKnue-MSFT
Copy link

Windows Terminal version

1.18.1421.0

Windows build number

10.0.22631.0

Other Software

No response

Steps to reproduce

Create a batch file "test.bat" with contents

echo %TEST%
pause

open cmd terminal profile
run SET TEST=wow
run wt test.bat
run ./test.bat

Expected Behavior

New terminal window should open up with output (this is the behavior in 1.16)

>echo wow
wow
>pause
press any key to continue . . .

Then outputs in parent window

>echo wow
wow
>pause
press any key to continue . . .

Actual Behavior

New terminal window is opened with output

>echo
ECHO is on.
>pause
Press any key to continue . . .

Then outputs in parent window

>echo wow
wow
>pause
press any key to continue . . .
@ChrisKnue-MSFT ChrisKnue-MSFT added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 1, 2023
@DHowett
Copy link
Member

DHowett commented Jun 1, 2023

Ah, indeed!

With WT transitioning to a long-lived single process model like Explorer, we're evaluating a stance that all tabs and panes receive a completely new environment block on creation. That's the only way--apart from a risky three-way merge--to ensure that applications get up-to-date environment variables without requiring everything run through Terminal to actively reconstruct and merge its own envrionment. In short, inheritance is in tension with #1125 and #7239.

The move to that single-process model has made it somewhat difficult to maintain a chain of custody for the env. block in any way other than to construct a new one! There is probably something we could do with the wt remote control to send over environment variables that are "important," but that will require some doing.

@DHowett
Copy link
Member

DHowett commented Jun 1, 2023

This was already a bit "wacky" for window grouping, even in Preview 1.16 (and 1.15, 1.14, ...):

SET TEST=wow
wt -w 0 cmd

has different environment block behavior than...

SET TEST=wow
wt -w -1 cmd

Between the two, they are now at the very least consistent as of 1.18 😄

@carlos-zamora
Copy link
Member

@ChrisKnue-MSFT Thanks for filing! Going to close this based on Dustin's responses.

@carlos-zamora carlos-zamora closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2023
@timmydo
Copy link

timmydo commented Jun 7, 2023

We have hundreds of developers relying on windows terminal for running about 6 tabs of applications to run our platform. Not propagating environment variables is a pretty large breaking change--I hope you will provide a workaround for passing environment variables to new tabs or at least give us some time to rearchitect things before the preview becomes the standard version. @DHowett

@DHowett
Copy link
Member

DHowett commented Jun 15, 2023

(I'm gonna reopen this and mark it for discussion among the team.)

There's roughly one quarter of a year until this ships broadly, which is the big win we get from having a preview channel 😄

@DHowett DHowett reopened this Jun 15, 2023
@DHowett DHowett added the Needs-Discussion Something that requires a team discussion before we can proceed label Jun 15, 2023
@zadjii-msft
Copy link
Member

quick notes

  • collect the env when wt is run. Package that up with the CommandlineArgs to send to the monarch
  • add a --flag to tell NewTab/SplitPane to use those env vars instead of the regenerated ones
  • don't just keep using those vars for future tabs/panes

@DHowett
Copy link
Member

DHowett commented Jul 11, 2023

Maybe the --flag should be to disable inheritance

@PankajBhojwani PankajBhojwani added Product-Terminal The new Windows Terminal. Area-Commandline wt.exe's commandline arguments labels Jul 12, 2023
@PankajBhojwani PankajBhojwani added this to the Terminal v1.19 milestone Jul 12, 2023
@PankajBhojwani PankajBhojwani removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Discussion Something that requires a team discussion before we can proceed labels Jul 12, 2023
@zadjii-msft
Copy link
Member

Argument for --flag disabling inheritance

  • this is how the wt CLI has always operated since inception in 0.9

Argument against:

  • Can we tell if wt was invoked for the commandline, vs just started by explorer.exe? There have been myriad issues in the past that our env vars are just plain wacky. I'd rather that a fresh "click the icon" launch not try to inherit, but idk if we can differentiate

@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Aug 17, 2023
@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 24, 2023

Notes from discussion:

We're gonna approach this as many bits:

  • wt -E foo=bar to manually pass env vars to the NewTerminalArgs. This'll let a caller manually pass env vars on the commandline. Great!
  • wt --inheritEnvironment
    • This one's more complicated.
    • On wt launch, bundle the entire environment that wt was spawned with, and put it into the Remoting.CommandlineArgs, and give them to the monarch (and ultimately, down to TerminalPage with the AppCommandlineArgs). DO THIS ALWAYS.
    • IF there was no overridden commandline, anywhere, in any subcommand, then we'll default to what we're doing in 1.18 Preview - don't inherit. Terminal act's like file explorer, and just starts up the profile with a fresh env block1.
    • IF THERE WAS AN OVERRIDDEN commandline, then default this to "inherit from the caller of WT".
  • part the third: Convert the compatibility.reloadEnvironmentVariables setting to a per-profile one.

At the end of this:

  • wt, wt -p foo will still use a fresh env block, instead of the inherited one. Yay!
  • wt -- cmd, wt foo.bat, wt --inheritEnvironment will all use the inherited one. Just like in 1.17, yay.
  • set TEST=failure && wt -E TEST=wow cmd.exe /k echo such %TEST% will set TEST in the spawned cmd to wow, and that'll be a good boy.

working notes


image
image
image


Related:

Footnotes

  1. Plus the environment setting, of course.

zadjii-msft added a commit that referenced this issue Sep 19, 2023
…oo (#15897)

This PR is a few things:

* part the first: Convert the `compatibility.reloadEnvironmentVariables`
setting to a per-profile one.
* The settings should migrate it from the user's old global place to the
new one.
  * We also added it to "Profile>Advanced" while I was here.
* Adds a new pair of commandline flags to `new-tab` and `split-pane`:
`--inheritEnvironment` / `--reloadEnvironment`
* On `wt` launch, bundle the entire environment that `wt` was spawned
with, and put it into the `Remoting.CommandlineArgs`, and give them to
the monarch (and ultimately, down to `TerminalPage` with the
`AppCommandlineArgs`). DO THIS ALWAYS.
* As a part of this, we’ll default to _reloading_ if there’s no explicit
commandline set, and _inheriting_ if there is.
* For example, `wt -- cmd` would inherit, and `wt -p “Command Prompt”`
would reload.[^1]
* This is a little wacky, but we’re trying to separate out the
intentions here:
* `wt -- cmd` feels like “I want to run cmd.exe (in a terminal tab)”.
That feels like the user would _like_ environment variables from the
calling process. They’re doing something more manual, so they get more
refined control over it.
* `wt` (or `wt -p “Command Prompt”`) is more like, “I want to run the
Terminal (or, my Command Prompt profile) using whatever the Terminal
would normally do”. So that feels more like a situation where it should
just reload by default. (Of course, this will respect their settings
here)

## References and Relevant Issues

#15496 (comment)
has more notes.

## Detailed Description of the Pull Request / Additional comments
This is so VERY much plumbing. I'll try to leave comments in the
interesting parts.

## PR Checklist
- [x] This is not _all_ of #15496. We're also going to do a `-E foo=bar`
arg on top of this.
- [x] Tests added/passed
- [x] Schema updated

[^1]: In both these cases, plus the `environment` setting, of course.
@zadjii-msft zadjii-msft removed the Severity-Blocking We won't ship a release like this! No-siree. label Sep 20, 2023
@zadjii-msft zadjii-msft removed their assignment Sep 20, 2023
@zadjii-msft
Copy link
Member

Actually, y'know what. I'm gonna mark this as closed by #15897, and then fork off the -E request to it's own issue.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Sep 20, 2023
@zadjii-msft zadjii-msft added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Needs-Tag-Fix Doesn't match tag requirements labels Sep 20, 2023
DHowett pushed a commit that referenced this issue Sep 22, 2023
…oo (#15897)

This PR is a few things:

* part the first: Convert the `compatibility.reloadEnvironmentVariables`
setting to a per-profile one.
* The settings should migrate it from the user's old global place to the
new one.
  * We also added it to "Profile>Advanced" while I was here.
* Adds a new pair of commandline flags to `new-tab` and `split-pane`:
`--inheritEnvironment` / `--reloadEnvironment`
* On `wt` launch, bundle the entire environment that `wt` was spawned
with, and put it into the `Remoting.CommandlineArgs`, and give them to
the monarch (and ultimately, down to `TerminalPage` with the
`AppCommandlineArgs`). DO THIS ALWAYS.
* As a part of this, we’ll default to _reloading_ if there’s no explicit
commandline set, and _inheriting_ if there is.
* For example, `wt -- cmd` would inherit, and `wt -p “Command Prompt”`
would reload.[^1]
* This is a little wacky, but we’re trying to separate out the
intentions here:
* `wt -- cmd` feels like “I want to run cmd.exe (in a terminal tab)”.
That feels like the user would _like_ environment variables from the
calling process. They’re doing something more manual, so they get more
refined control over it.
* `wt` (or `wt -p “Command Prompt”`) is more like, “I want to run the
Terminal (or, my Command Prompt profile) using whatever the Terminal
would normally do”. So that feels more like a situation where it should
just reload by default. (Of course, this will respect their settings
here)

#15496 (comment)
has more notes.

This is so VERY much plumbing. I'll try to leave comments in the
interesting parts.

- [x] This is not _all_ of #15496. We're also going to do a `-E foo=bar`
arg on top of this.
- [x] Tests added/passed
- [x] Schema updated

[^1]: In both these cases, plus the `environment` setting, of course.

(cherry picked from commit 59248de)
Service-Card-Id: 90383402
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Commandline wt.exe's commandline arguments Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants