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

Environment collection should not be applied on hidden terminals (non-windows) #197187

Closed
Tracked by #22879
karrtikr opened this issue Nov 1, 2023 · 21 comments · Fixed by #206543
Closed
Tracked by #22879

Environment collection should not be applied on hidden terminals (non-windows) #197187

karrtikr opened this issue Nov 1, 2023 · 21 comments · Fixed by #206543
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders remote Remote system operations issues terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. verified Verification succeeded
Milestone

Comments

@karrtikr
Copy link
Contributor

karrtikr commented Nov 1, 2023

Does this issue occur when all extensions are disabled?: Yes/No

Version: 1.85.0-insider
OS: Linux (codespaces)

WORKS FINE ON WINDOWS

Steps to Reproduce:

(.venv) in the terminal prompt is contributed via env collection by Python extension.

Normal terminal:
image

Hovering says that collection is applied.

Hidden terminal:
image

Although it says that no environment collection is contributed upon hovering, collection is still applied as can be seen in the terminal prompt.

@karrtikr karrtikr changed the title Environment collection should not be applied on hidden terminals (linux) Environment collection should not be applied on hidden terminals (non-windows) Nov 1, 2023
@meganrogge meganrogge added the terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. label Nov 1, 2023
@Tyriar Tyriar added this to the November 2023 milestone Nov 6, 2023
@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label Nov 6, 2023
@Tyriar
Copy link
Member

Tyriar commented Nov 6, 2023

@karrtikr it seems like this is actually a bug where the initially hidden terminal doesn't show the applied environments but should? Making a terminal hidden was just meant to be about visibility, not environment?

@karrtikr
Copy link
Contributor Author

karrtikr commented Nov 6, 2023

I'm fine if we change hidden terminals to also apply environment collection, as long as we get support for #194819.

Python extension decided not to send activate commands to hidden terminals: microsoft/vscode-remote-release#1760, as hidden terminals are considered private terminals belonging to a specific extension. One could probably argue similarly for not applying environment collection to such terminals.

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Nov 7, 2023
@karrtikr
Copy link
Contributor Author

karrtikr commented Nov 8, 2023

Based on offline discussion we're continue to go with NOT applying environment collection to hidden terminals, due to it affecting remote scenarios.

@karrtikr
Copy link
Contributor Author

We investigated that env collection from shell integration in unix is still being applied on hidden terminals, i.e applyAtShellIntegration: true is still being applied when it should not. Any further updates? @Tyriar

@Tyriar
Copy link
Member

Tyriar commented Feb 29, 2024

Code to disable for hideFromUser terminals:

if (!this._isDisposed && !shellLaunchConfig.strictEnv && !shellLaunchConfig.hideFromUser) {

if (!shellLaunchConfig.strictEnv && !shellLaunchConfig.hideFromUser) {

Where VSCODE_ENV_ vars are set:

if (mutator.options?.applyAtShellIntegration ?? false) {
const key = `VSCODE_ENV_${mutatorTypeToLabelMap.get(mutator.type)!}`;
env[key] = (env[key] ? env[key] + ':' : '') + variable + '=' + this._encodeColons(value);
}

@Tyriar
Copy link
Member

Tyriar commented Feb 29, 2024

I cannot reproduce this on Windows or macOS. Steps:

  1. Open Code - OSS
  2. Open a terminal
  3. Run env and verify the environment does not include FOO
  4. Install terminal-sample from vscode-extension-samples via symlink
  5. Reload
  6. Open a terminal
  7. Run env and verify the environment does FOO
  8. Run Terminal API: Create Terminal (hideFromUser)
  9. Run Terminal API: Show Terminal and select the new terminal
  10. Run env and verify the environment does not include FOO

Maybe you had the variable set in the parent environment (main/server?) which is why it showed up?

@Tyriar Tyriar closed this as completed Feb 29, 2024
@Tyriar Tyriar added the *not-reproducible Issue cannot be reproduced by VS Code Team member as described label Feb 29, 2024
@karrtikr
Copy link
Contributor Author

karrtikr commented Feb 29, 2024

Please try it on Linux (codespaces), stated in the original report, this issue doesn't exist on Windows. And I don't have a macOS to verify.

@karrtikr karrtikr reopened this Feb 29, 2024
@karrtikr karrtikr removed the *not-reproducible Issue cannot be reproduced by VS Code Team member as described label Feb 29, 2024
Tyriar added a commit that referenced this issue Feb 29, 2024
Checked all causes before applyToProcessEnvironment was used.

Fixes #197187
@Tyriar
Copy link
Member

Tyriar commented Feb 29, 2024

Thanks, missed the remote bit

@Tyriar Tyriar added the remote Remote system operations issues label Feb 29, 2024
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Feb 29, 2024
Tyriar added a commit that referenced this issue Mar 18, 2024
As I understand it, hidden terminals were always exempty from shell
integration because hidden terminals were thought of as private to an
extension in a way, and also since they probably weren't going to be
interacted with. Especially since the introduction of the environment
variable collection API I think we should change this decision, not
doing so will lead to weirdness for extensions where hidden ones (which
may be shown later) aren't fully featured and get inconsistent envs.

Fixes #199611
Part of #197187
@karrtikr
Copy link
Contributor Author

Seems like this behavior has now changed with: #208036. So we should either reopen this one or #194819, to accomplish #194819 (comment).

@Tyriar
Copy link
Member

Tyriar commented Mar 18, 2024

@karrtikr you can accomplish #194819 by using strictEnv

@karrtikr
Copy link
Contributor Author

I see, suppose if strictEnv is true, but no env is passed explicitly, I assume system environment variables and shell initialization scripts are still applied?

When this is false (default), the environment will be based on the window's environment and also apply configured platform settings like terminal.integrated.env.windows on top.

What does window's environment mean here and will it be applied if strictEnv is true, but no env is passed.

@karrtikr
Copy link
Contributor Author

karrtikr commented Mar 18, 2024

If you don't want env collections you must pass strictEnv, which means you need to provide the entire environment.

We don't have any environment to provide, all we want to do is skip applying env collection while the rest of the environment stays as-is, can you guide us on how to exactly use strictEnv to achieve that? Also, we would expect shell integration to be active.

@Tyriar
Copy link
Member

Tyriar commented Mar 18, 2024

@karrtikr basing the environment on { ...process.env } should do the trick

@karrtikr
Copy link
Contributor Author

Gotcha, will try that. I assume shell initialization and integration scripts are still run?

@Tyriar
Copy link
Member

Tyriar commented Mar 19, 2024

@karrtikr looking at the code, I think shell integration does activate even when strictEnv is set

@rzhao271
Copy link
Contributor

rzhao271 commented Mar 26, 2024

What are some verification steps for this issue?
For example, how do I get Python deactivate to show up?

@Tyriar
Copy link
Member

Tyriar commented Mar 26, 2024

@rzhao271 steps at #197187 (comment), remote only

@rzhao271 rzhao271 added the verification-steps-needed Steps to verify are needed for verification label Mar 26, 2024
@Tyriar Tyriar removed the verification-steps-needed Steps to verify are needed for verification label Mar 26, 2024
@karrtikr
Copy link
Contributor Author

@Tyriar Actually we reverted the change made in this issue with #197187 (comment), so even hidden terminals have environment collection applied by default now.

@Tyriar
Copy link
Member

Tyriar commented Mar 27, 2024

@karrtikr 👍

So the steps are:

  1. Open a terminal
  2. Run env and verify the environment does not include FOO
  3. Install terminal-sample from vscode-extension-samples via symlink
  4. Reload
  5. Open a terminal
  6. Run env and verify the environment does FOO
  7. Run Terminal API: Create Terminal (hideFromUser)
  8. Run Terminal API: Show Terminal and select the new terminal
  9. Run env and verify the environment does include FOO

@rzhao271
Copy link
Contributor

@Tyriar How do I do step 3? Am I opening the terminal-sample workspace after cloning and creating a symlink to it?

@Tyriar
Copy link
Member

Tyriar commented Mar 27, 2024

@rzhao271 you can build the extension and do it in the insiders extension development host

@rzhao271 rzhao271 added the verified Verification succeeded label Mar 27, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders remote Remote system operations issues terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants