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

Concurrently 3.0.0 runs the wrong version of node #61

Closed
aaronjensen opened this issue Sep 27, 2016 · 14 comments
Closed

Concurrently 3.0.0 runs the wrong version of node #61

aaronjensen opened this issue Sep 27, 2016 · 14 comments

Comments

@aaronjensen
Copy link

I use asdf to manage my node versions. The new shell execution method in concurrently 3 appears to ignore my asdf setup and my previous environment so it runs w/ an older, system node. This makes everything fail.

@kimmobrunfeldt
Copy link
Contributor

Sounds like a problem with PATH. Could you run

npm i spawn-default-shell && node -e "console.log(require('spawn-default-shell/src/get-shell')())"

and paste the last line of the output here? That will tell which shell concurrently is spawning to get forward with this. It may be that the spawned shell doesn't read configuration from .bash_profile or similar env files.

@aaronjensen
Copy link
Author

{ shell: '/bin/zsh', executeFlag: '-c' }

@kimmobrunfeldt
Copy link
Contributor

Ok I have the same shell, and I can correctly use the node version but I'm using nvm. Could you run concurrently 'which node && node -v' and concurrently 'echo $PATH'. You could then try to resolve why the spawned shell is using the wrong node binary.

@kimmobrunfeldt
Copy link
Contributor

For me theses commands return:

➜  ~ concurrently 'which node && node -v'
[0] /Users/kbru/.nvm/versions/node/v6.3.1/bin/node
[0] v6.3.1
[0] which node && node -v exited with code 0
➜  ~ concurrently 'echo $PATH'
[0] /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Library/Frameworks/Mono.framework/Versions/Current/Commands:/Users/kbru/.gem/bin:/Applications/Postgres.app/Contents/Versions/latest/bin:/Users/kbru/.nvm/versions/node/v6.3.1/bin:/Users/kbru/.rvm/bin
[0] echo $PATH exited with code 0

@aaronjensen
Copy link
Author

$ concurrently 'which node && node -v'
[0] node is /usr/local/bin/node
[0] node is /usr/local/bin/node
[0] v0.10.35
[0] which node && node -v exited with code 0
$ concurrently 'echo $PATH'
[0] /usr/local/heroku/bin:/Users/aaronjensen/Library/Haskell/bin:/usr/local/bin:/usr/local/sbin:/usr/local/mysql/bin:/opt/local/bin:/opt/local/sbin:/usr/local/share/npm/bin:/Users/aaronjensen/.bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin
[0] echo $PATH exited with code 0

My setup for asdf is in my zprofile

@kimmobrunfeldt
Copy link
Contributor

Ok thanks for reporting this bug. It seems that when we are running commands with /bin/zsh -c "...", profile files are not read. If anyone knows a flag to enable the profile file reading, I'm interested to know.

@kimmobrunfeldt
Copy link
Contributor

Ok found it from man bash, we need to invoke the shells as a login shell (use -l flag). Read more from https://github.com/kimmobrunfeldt/spawn-default-shell README.

@kimmobrunfeldt
Copy link
Contributor

@aaronjensen Ok this is now fixed in spawn-default-shell, it will by default use -l flag when running shell. E.g. with bash: /bin/bash -l -c "...". This will invoke bash as a login shell and it will execute ~/.bash_profile before running the command. With zsh the same flag will read ~/.zprofile correctly.

To my mind this is the expected behaviour of concurrently, but could there be some unwanted side-effects? Should I make the -l flag default in concurrently, or should it be an optional flag(e.g. --invoke-login-shell) for concurrently?

@aaronjensen
Copy link
Author

@kimmobrunfeldt now that I've thought about this some more, I think that this method won't actually work, even with -l.

What concurrently should do is use a shell that shares the same exact environment as the shell running it.

Here's why:

Let's say you are using concurrently in continuous integration. You also have some secrets that you keep in environment variables associated with your build. Your CI server injects those environment variables via a script it runs to execute your build. This enables anything run within that script to have access to them. It does not update your profile or anything along those lines. This would mean that any shell started to run things that require those environment variables must share that environment. Starting a totally fresh shell would be bad, and making it a login shell would not bring those environment variables in. As a matter of fact, even if you inherited the environment and made it a login shell your login shell scripts may be overriding things that were exported in the parent environment.

In short, concurrently should behave as running bash -c ... or zsh -c ... from an existing shell. It should not have its environment reset at all.

@kimmobrunfeldt
Copy link
Contributor

@aaronjensen I'm not sure if I understand what you mean. Concurrently does already share the environment as well as possible. Running concurrently "x" in your current shell is from environment variable perspective same as running bash -c "x". I think PATH is an exception to this and running bash -c "x" some how resets the PATH variable, but e.g. the env variables set by your CI are available at that new shell.

I don't know any better way to "use a shell that shares the same exact environment as the shell running it." than the current solution. If you have an idea please share.

In short, concurrently should behave as running bash -c ... or zsh -c ... from an existing shell. It should not have its environment reset at all.

There is no special "reset" done by concurrently or spawn-default-shell. So it is as close to running these commands from an existing shell as possible. It's just the way bash -c ... and others work. E.g. see this output:

~ bash -c "nvm"
bash: nvm: command not found
➜  ~ nvm

Node Version Manager
...

nvm is available in my "normal" shell, but not when using bash -c "nvm". The only way I know to fix this is to use bash -l -c "nvm", which correctly reads my .bashrc and makes nvm available.

@aaronjensen
Copy link
Author

You're right, bash/zsh will exec the rc files regardless, which will usually override PATH.

If you want to maintain all env vars, I think bash -fc "..." will work, but that won't copy over functions or aliases. Since it will maintain path it may be exactly what concurrently needs.

I guess one thing I don't understand is why does concurrently need to run each thing in its own shell? I don't understand it well enough to know that.

The other option would be to do something like bash -lc "PATH=$PATH ..." which would copy your current path over to any command you ran, but that's fragile since a command could have && in it or the like. Maybe there's another way, I'm not sure.

gkalpak referenced this issue in gkalpak/angular Feb 9, 2017
Previously, the `integration/` tests were failing, because `concurrently "foo"`
does not inherit the `PATH` env var ([more info][1]).

This commit fixes it, by setting the `PATH` env var explicitly:
`concurrently "PATH=$PATH foo"`.

This commit also includes some minor refactoring of the `integration/` tests scripts:

- Move build-related operations to `ci-lite/build.sh` (for consistency).
- Use `yarn run ...` instead of `npm run ...` inside package.json scripts.
- Use global `yarn` (since we are already using it for `aio/`).
- Fix some `travis_fold` statements.

[1]: https://github.com/kimmobrunfeldt/concurrently/issues/61#issuecomment-252081610
mhevery referenced this issue in angular/angular Feb 9, 2017
Previously, the `integration/` tests were failing, because `concurrently "foo"`
does not inherit the `PATH` env var ([more info][1]).

This commit fixes it, by setting the `PATH` env var explicitly:
`concurrently "PATH=$PATH foo"`.

This commit also includes some minor refactoring of the `integration/` tests scripts:

- Move build-related operations to `ci-lite/build.sh` (for consistency).
- Use `yarn run ...` instead of `npm run ...` inside package.json scripts.
- Use global `yarn` (since we are already using it for `aio/`).
- Fix some `travis_fold` statements.

[1]: https://github.com/kimmobrunfeldt/concurrently/issues/61#issuecomment-252081610
@kentcdodds
Copy link
Contributor

kentcdodds commented Feb 10, 2017

Anyone here want to try out my fork and comment back if it worked? I think that'll fix the issue.

npm install https://github.com/kentcdodds/concurrently.git#pr/use-spawn-command

@gustavohenke
Copy link
Member

Published v3.3.0.
Could you try it, @aaronjensen?

@aaronjensen
Copy link
Author

This works. Thank you @kentcdodds and @gustavohenke!

asnowwolf referenced this issue in asnowwolf/angular Aug 11, 2017
Previously, the `integration/` tests were failing, because `concurrently "foo"`
does not inherit the `PATH` env var ([more info][1]).

This commit fixes it, by setting the `PATH` env var explicitly:
`concurrently "PATH=$PATH foo"`.

This commit also includes some minor refactoring of the `integration/` tests scripts:

- Move build-related operations to `ci-lite/build.sh` (for consistency).
- Use `yarn run ...` instead of `npm run ...` inside package.json scripts.
- Use global `yarn` (since we are already using it for `aio/`).
- Fix some `travis_fold` statements.

[1]: https://github.com/kimmobrunfeldt/concurrently/issues/61#issuecomment-252081610
juleskremer referenced this issue in juleskremer/angular Aug 28, 2017
Previously, the `integration/` tests were failing, because `concurrently "foo"`
does not inherit the `PATH` env var ([more info][1]).

This commit fixes it, by setting the `PATH` env var explicitly:
`concurrently "PATH=$PATH foo"`.

This commit also includes some minor refactoring of the `integration/` tests scripts:

- Move build-related operations to `ci-lite/build.sh` (for consistency).
- Use `yarn run ...` instead of `npm run ...` inside package.json scripts.
- Use global `yarn` (since we are already using it for `aio/`).
- Fix some `travis_fold` statements.

[1]: https://github.com/kimmobrunfeldt/concurrently/issues/61#issuecomment-252081610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants