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

Cannot pass environment variable that is already defined in shell's rc file. #4779

Closed
saml opened this issue Mar 29, 2016 · 3 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug *duplicate Issue identified as a duplicate of another issue(s)
Milestone

Comments

@saml
Copy link

saml commented Mar 29, 2016

  • VSCode Version: 0.10.11
  • OS Version: Ubuntu 14.04

Steps to Reproduce:

  1. Export a variable in your default shell (~/.zlogin for zsh; ~/.bash_login for bash, for example. You can use .bashrc as well).

    export FOOBAR="1"
  2. Pass different value of the same variable to VSCode.

    FOOBAR=2 code .
  3. VSCode uses value defined for login shell, not current session:

Help > Toggle Developer Tools. And in Console, process.env.FOOBAR gives "1" when I expect "2".

It looks like VSCode executes default shell as login shell again before launching the program UI.

Being able to pass different values for existing environment variables greatly helps writing/testing extensions.

Related discussion: microsoft/vscode-go#220 (comment)

strace example:
$ strace -E GOPATH=1 -E FOOBAR=2 -s 1000000 -e verbose=execve -e abbrev=none ./.build/electron/electron  .
execve("./.build/electron/electron", ["./.build/electron/electron", "."], [... "GOPATH=1", ...]) # What I expected
...
read(62, "...\nGOPATH=/home/saml/go\n....")  # What is this?
...
@saml
Copy link
Author

saml commented Mar 30, 2016

Looks like this is the place that resets envvars:

https://github.com/Microsoft/vscode/blob/25ce394231a63cad030c804460bd957a1102d14e/src/vs/workbench/electron-main/main.ts#L252-L256

// On some platforms we need to manually read from the global environment variables
// and assign them to the process environment (e.g. when doubleclick app on Mac)
getUserEnvironment()
    .then(userEnv => {
        assign(process.env, userEnv);

https://github.com/Microsoft/vscode/blob/25ce394231a63cad030c804460bd957a1102d14e/src/vs/base/node/env.ts#L22

         let child = cp.spawn(process.env.SHELL, ['-ilc', 'env'], {

Do we really need to execute $SHELL -c env there? Can we assume or check if env command exists and execute it?

# intuitive behavior: variable GOPATH can be overwritten
$ GOPATH=1 env |grep GOPATH
GOPATH=1

# variable cannot be overwritten. 
$ GOPATH=1 $SHELL -c env |grep GOPATH
GOPATH=/home/saml/go

Or, getUserEnvironment() in main.ts should only be called if it's not invoked from terminal:

import tty = require('tty');
if (!tty.isatty(process.stdout)) {
  getUserEnvirionment() .....
}

saml added a commit to saml/vscode that referenced this issue Mar 30, 2016
…tached

fixes microsoft#4779

Sometimes users want to launch `code` from terminal after
modifying environment variables.
When `code` is launched from terminal, it respects those modifications.
@Tyriar Tyriar self-assigned this Mar 30, 2016
@Tyriar Tyriar added the bug Issue identified by VS Code Team member as probable bug label Mar 30, 2016
@Tyriar
Copy link
Member

Tyriar commented Mar 30, 2016

@bpasero @joaomoreno any comment on why this is used over process.env? I just fixed an issue with it recently because multi-line variables weren't working.

@joaomoreno
Copy link
Member

Duplicate of #1033, fixed already.

@bpasero bpasero added the *duplicate Issue identified as a duplicate of another issue(s) label Jul 1, 2016
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
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 *duplicate Issue identified as a duplicate of another issue(s)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants