microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.18k stars 28.84k forks source link

Support using a "clean" environment for the terminal #70248

Closed praenubilus closed 5 years ago

praenubilus commented 5 years ago

Steps to Reproduce:

  1. From OS terminal, type echo $PATH, you may see values similar to following:
    /Users/xxx/anaconda3/bin:/Users/xxx/anaconda3/condabin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
  2. Open a vscode instance, in the integrated terminal type echo $PATH, now the value may look like following:
    /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/xxx/anaconda3/bin:/Users/xxx/anaconda3/condabin

    The CONTENT in two $PATH values are the same, BUT in different order. Problems it may cause: some system configuration depends on the first executable value they can find in the $PATH. Different $PATH value order will have different default program being used. In previous example, the value in step 1 will use python3 and pip3 from anaconda. But in vscode integrated terminal, the $PATH value in step 2 will use the python2 and pip2 in /usr/local/bin. Even you activate a virtualenv in integrated terminal, it will still use the wrong virtual env. More details and a proposed workaround can be seen in vscode-python #4434.

This has been observed on both MacOS and Ubuntu. I don't have windows computer, but I guess that may also have the same issue. Some related issues can be found in this issue vscode-python #4668.

Now I'm trying to fix this issue with a pull request. But I need some help to indicate me to the correct location in the code base. I also addressed this question on StackOverflow:

Through my own observation, I found the root cause may be: after some steps, somewhere during the create terminal process, the actual environment variable values(specific to PATH) has been reordered/changed. Maybe it is being processed for remove duplication.

I download the vscode codebase(1.33) and follow the debugging until method createProcess in source file src/vs/workbench/contrib/terminal/electron-browser/terminalProcessManager.ts. What I have found is: even after the terminal instance has been created, the env.PATH in the main process has NOT been changed. For ptyProcess, the constructor takes options(which contains env) as input, but I cannot find any env related property in created ptyProcess. And after the terminal instance created, the debugging ends, integrated terminal appears in the workbench. I get lost after terminal instance created, can someone from the core team or used to working on integrated terminal module indicate me to the right place in the codebase so I can try to solve the issue? There is not enough documents on internet or comments in the source code can help me find/locate the environments changing during/after the terminal instance creation.

praenubilus commented 5 years ago

Also, if I want to debug vscode platform source code, where should I expect a User Settings or Workspace Settings so I can add some customized stuff in it?

DonJayamanne commented 5 years ago

Current workaround is:

"terminal.integrated.env.osx": {
        "PATH": ""
}
Tyriar commented 5 years ago

The reason this happens is because on mac and Linux there are two ways to launch VS Code:

This sets up the main process environment which is inherited by each VS Code window, its tasks, debug sessions and terminals. Below explain this in detail:

Ubuntu launching from terminal

Ubuntu launching from dash

macOS launching from terminal

macOS launching from dock

* needs ~/.bashrc already sourced ** needs ~/.bash_profile already sourced

The underlying problem is:

When a terminal is launched, it needs to run its launch script again; .bashrc on Linux and .bash_profile on macOS (because terminal.integrated.osx.shellArgs defaults to [ "-l" ]). This will shuffle the environment based on what the user's scripts does, and can mess things up if their script is not idempotent.

So as for fixing this issue once and for all this will be a little tricky, unfortunately it's not just as easy as holding onto the original environment and using that for the terminal instead of the one generated by getShellEnvironment, that will only work when launching VS Code from the dock/dash but not when run through the terminal.

I propose we fetch the environment of the parent process of the code executable which will probably be systemd on Linux or the root process on macOS, we can do this on Linux by running and parsing cat /proc/<pid>/environ on Linux, and ps eww <pid> on macOS.

It's worth noting that this could break some people's set up and potentially cause some other unexpected issues as before it was an assumption that the terminal process uses the environment of VS Code's window. Maybe this should even be an option in settings, something like "terminal.integrated.useWindowEnvironment": false?

Tyriar commented 5 years ago

@praenubilus if you wanted to put together a PR for this I would stick the fetching of the environment code insider electron-browser/terminalService and grab that inside TerminalProcessManager instead of basing it on process.env here:

https://github.com/Microsoft/vscode/blob/d3443fd68d625c980325e1571066850e943dfbef/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts#L128

I think for Windows we always want the old behavior.

praenubilus commented 5 years ago

@Tyriar Thanks for the information. Can you answer my another question: if I want to debug with customized settings, (for example "terminal.integrated.env.osx": { "PATH": ""} ) where should I expect a User Settings or Workspace Settings file I can play with?

Tyriar commented 5 years ago

@praenubilus I haven't touched any debugging stuff with extensions, this might be relevant?

https://github.com/Microsoft/vscode/blob/1615a36eb45182de5e34c53cfdd5b3bce8cddbf3/src/vs/vscode.d.ts#L8484-L8500

auchenberg commented 5 years ago

Ping @DonJayamanne

DonJayamanne commented 5 years ago

Yes, this might help, however the downside of ALWAYS using this approach is the fact that we'll end up printing a tonne of environment variables (EVERYTHING) to the console in the terminal window. Will check if we can try to perform a diff, though that too might not work due to the current process (VSC - Env Variables) vs default shell Process (Env variables)

Will double check this and get back.

bandrei commented 5 years ago

Not sure if this helps with your assessment @Tyriar however, my bash_profile sources my bashrc file. If VSCode is to load bash_profile in either scenarios (dock vs terminal), why is the VSCODE_CLI variable not being set affect the execution order of the sourcing?

Tyriar commented 5 years ago

@DonJayamanne can you elaborate a little? Why don't you do that now? Do you expect users to setup the venv in a terminal and launch code from the terminal or something?

@bandrei the issues typically arise because the scripts are run multiple times, or at least a different number of times to what a standalone terminal does.

praenubilus commented 5 years ago

@DonJayamanne can you elaborate a little? Why don't you do that now? Do you expect users to setup the venv in a terminal and launch code from the terminal or something?

@bandrei the issues typically arise because the scripts are run multiple times, or at least a different number of times to what a standalone terminal does.

@Tyriar , I get what @bandrei is talking about. In my example at the beginning of the issue, my zshrc or bashrc only have the following command to operate on the $PATH environment variable.

export PATH="/home/xxx/anaconda3/bin:$PATH"

For this statement, even it has been run multiple times, /home/xxx/anaconda3/bin should always appear at the beginning of the $PATH in integrated terminal.

And the line/place you indicate me,

vscode/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts

Line 128 in d3

I have debugged tons of times and quite sure at that line the environment variable value has not been changed. And actually the environment variable is always in correct/expected order even after the createProcess function invocation. My own observation for the environment variable works in vscode terminal is: Main UI->Terminal Tab->Terminal Process->ptyProcess. Then I got lost after createProcess function invocation. Since after that, everything has set, I don't know where to monitor the change of environment variables other than the env in main process(but I guess it will not being used by terminal anymore). However, the final PATH environment variable value order has been changed when the UI appears. So I have two questions(they are the same as what I want to ask in my original question):

  1. WHERE is the final environment variables we use in the integrated terminal stored? Can you indicate me to the exact class/variable?
  2. After terminal tab created, is there any possible places in the code will re-order the environment variable?
DonJayamanne commented 5 years ago

@Tyriar apologies for the previous comment, please ignore that. I was replying to another comment if yours that wasn't for me.

I propose we fetch the environment of the parent process of the code executable which will probably be systemd on Linux or the root process on macOS, we can do this on Linux by running and parsing cat /proc//environ on Linux, and ps eww on macOS.

Sounds great

It's worth noting that this could break some people's set up and potentially cause some other unexpected issues as before it was an assumption that the terminal process uses the environment of VS Code's window. Maybe this should even be an option in settings, something like "terminal.integrated.useWindowEnvironment": false?

I like this approach.

Tyriar commented 5 years ago

Then I got lost after createProcess function invocation

@praenubilus createProcess creates the process and the shell scripts launch. To gleam some insights into what's happening after, you can pepper some echo calls throughout your .bashrc/.bash_profile/.profile/etc.

Tyriar commented 5 years ago

This could also solve https://github.com/Microsoft/vscode/issues/47816 by reloading the environment, we could also support doing this on Windows by fetching a clean environment block.

ladyrick commented 5 years ago

The underlying problem is:

When a terminal is launched, it needs to run its launch script again; .bashrc on Linux and .bash_profile on macOS (because terminal.integrated.osx.shellArgs defaults to [ "-l" ]). This will shuffle the environment based on what the user's scripts does, and can mess things up if their script is not idempotent.

So as for fixing this issue once and for all this will be a little tricky, unfortunately it's not just as easy as holding onto the original environment and using that for the terminal instead of the one generated by getShellEnvironment, that will only work when launching VS Code from the dock/dash but not when run through the terminal.

I propose we fetch the environment of the parent process of the code executable which will probably be systemd on Linux or the root process on macOS, we can do this on Linux by running and parsing cat /proc/<pid>/environ on Linux, and ps eww <pid> on macOS.

@Tyriar Hi, I noticed something new. See this video: https://ladyrick.github.io/2/index.html. According to this video, I think maybe this problem is nothing to do with whether the .zshrc is idempotent or not and its father process. Probably something is wrong while starting vscode and parsing .zshrc.

And this problem also exists when no extensions are enabled.

Tyriar commented 5 years ago

@ladyrick the difference could depend on whether the external terminal launches as a login shell or not and what terminal.integrated.shellArgs.osx is set to.

Probably something is wrong while starting vscode and parsing .zshrc.

VS Code itself doesn't touch .zshrc, it simply launches zsh with a set of args (which is in an environment that has already had some script sourced as described above).

DonJayamanne commented 5 years ago

@Tyriar First, thanks for the taking time to look into this and helping us get to the bottom of it (https://github.com/Microsoft/vscode/issues/70248#issuecomment-472582012).

Looking at the suggestion here https://github.com/Microsoft/vscode/issues/70248#issuecomment-472631999, all you're suggesting is we get the env variables from the parent process and use that instead of the current process. Is that correct?

It's worth noting that this could break some people's set up and potentially cause some other unexpected issues as before it was an assumption that the terminal process uses the environment of

Agreed. My suggestion is, we make some changes to the Python extension and based on some analysis we can prompt the user to add the setting "terminal.integrated.env.<platform key>": {"PATH": ""}. Its not a fix, but merely a work around for Python users. This way, when the ~/.bash_profile is sourced again in the terminal, its basically doing this on a fresh PATH env variable. What do you think?

Tyriar commented 5 years ago

Looking at the suggestion here #70248 (comment), all you're suggesting is we get the env variables from the parent process and use that instead of the current process. Is that correct?

The parent process of the executable, as per the bottom of https://github.com/Microsoft/vscode/issues/70248#issuecomment-472582012:

I propose we fetch the environment of the parent process of the code executable which will probably be systemd on Linux or the root process on macOS, we can do this on Linux by running and parsing cat /proc//environ on Linux, and ps eww on macOS.


Agreed. My suggestion is, we make some changes to the Python extension and based on some analysis we can prompt the user to add the setting "terminal.integrated.env.": {"PATH": ""}. Its not a fix, but merely a work around for Python users. This way, when the ~/.bash_profile is sourced again in the terminal, its basically doing this on a fresh PATH env variable. What do you think?

That might work for most cases in the Python ext, but it's still just a workaround. The proposal above would fix this issue for Python as well as unexpected behavior around PATH being out of order for a lot of user's set ups.

DonJayamanne commented 5 years ago

@Tyriar FYI, using ps eww <pid> is not very reliable. E.g. I just created an env variable as follows: export WOW="THAT ONE=123 1234 SOME=1234" Looking at the output of ps eww it now looks as though we have three variables, WOW, ONE, and SOME. Personally I'd prefer to find a better approach of identifying the environment variables, than using an approach that's flaky.

Will keep looking.

Tyriar commented 5 years ago

@DonJayamanne bummer, I only found that one after a few minutes of looking. Hopefully there's alternative methods

Tyriar commented 5 years ago

@DonJayamanne just exposed "terminal.integrated.inheritEnv" which currently only affects mac and Linux (Windows deferred to https://github.com/microsoft/vscode/issues/47816). It currently defaults to true, let me know if setting it to false works, seems like it should do the trick. If all is good we should probably switch the default over in a later version, after warning in the release notes of the upcoming change.

One caveat with this is that on macOS I couldn't find a reliable way of pulling the environment, because of this I just use the root environment which always seems to be {}. In other words, if you launch a terminal, export some env in there, the terminal won't pick that up.

Tyriar commented 5 years ago

Actually we do need to detect for mac, looking into it but this is ready to test on Linux.

Tyriar commented 5 years ago

Couldn't figure it out for mac, doesn't seem possible to get the environment before the launch CLI has run (and tampered with the environment) and /system/launchd's environment always says it's nothing with ps e -o command. I did a workaround to cherry pick a small set of environment variables off the current process.

hologerry commented 5 years ago

After set

"terminal.integrated.env.osx": {
        "PATH": ""
}

I can not build cpp file:

Screen Shot 2019-06-15 at 00 29 29
Tyriar commented 5 years ago

@hologerry try setting terminal.integrated.inheritEnv to false using the Insiders build and let me know if that works.

hologerry commented 5 years ago

@Tyriar With the Insiders build and setting terminal.integrated.inheritEnv to false. It works! Thanks!

Tyriar commented 5 years ago

While inheritEnv false on macOS is better than the current workaround (setting PATH to ""), I suspect that it probably isn't good enough of a fix to switch the default over as the initial PATH will be completely empty, so things like sh won't be available (unless the login shell adds them).

Tyriar commented 5 years ago

Actually scratch that, I think that's how it's meant to be and it's the reason why the default shell always has the absolute path (/bin/sh) instead of the shell name (sh). path_helper pulls the $PATH in from /etc/paths via /etc/profile.

lwabish commented 5 years ago

At first I used bash+vscode+conda,this problem appears and I fixed it by setting "terminal.integrated.env.osx": {"PATH": ""} in vscode and it works. But this time I just change my main shell to zsh(also in vscode),this method has failed : vscode prompt something like "zsh -l can't start". @Tyriar I tried using vscode insider edition and set "terminal.integrated.inheritEnv": false,,this works

ieipi commented 5 years ago

using vscode on mac and set "terminal.integrated.inheritEnv": false,,this works;

ladyrick commented 5 years ago

using vscode on mac and set "terminal.integrated.inheritEnv": false,,this works;

But sometimes I really need to inheritEnv. inheritEnv: false will make something goes wrong.

DonJayamanne commented 5 years ago

inheritEnv: false will make something goes wrong.

Please could you provide more details on what this something is.

ladyrick commented 5 years ago

Please could you provide more details on what this something is.

In remote development, "inheritEnv: false" will make "$PATH" wrong. Don't know why. Maybe it's a bug of remote dev?

Tyriar commented 5 years ago

@ladyrick what's your OS/version?

praenubilus commented 5 years ago

Please could you provide more details on what this something is.

In remote development, "inheritEnv: false" will make "$PATH" wrong. Don't know why. Maybe it's a bug of remote dev?

Can you add the following line at the end of your zshrc or bashrc?

echo $PATH

with this line, every time integrated terminal will display PATH value if the script being sourced/run, if the terminal has been printed more than once, and it may help you to locate the problem.