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

With 0.29.0, nvm install 4 fails when -e and -o pipefail shell options are set #865

Closed
jfirebaugh opened this issue Oct 9, 2015 · 6 comments
Labels
installing node Issues with installing node/io.js versions.

Comments

@jfirebaugh
Copy link
Contributor

The following script will fail with exit code 1:

#!/usr/bin/env bash

set -e
set -o pipefail

curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.29.0/install.sh | bash
source ~/.nvm/nvm.sh
nvm install 4

If you replace 4 with 0.12 or 0.10, it works. If the -e and -o pipefail options are not set, it works. And finally, if you replace the nvm version v0.29.0 with v0.28.0, it works.

We found this because our CI script does the above. Here's a reduced test case.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2015

Thanks for the report! These options are not compatible with nvm - it uses nonzero exit codes as a normal part of its operation.

I'd advise against using those options, and rely on explicit failure detections in your tests instead of simply failing on any nonzero exit code.

The fact that it's different between 4 and 0.12 is an artifact of it having two different code paths, but set -e is always something that is likely to break other code in the same shell.

@ljharb ljharb closed this as completed Oct 9, 2015
@jfirebaugh
Copy link
Contributor Author

@ljharb Okay, I'm happy to add failure detection to my script. Can you point me to the documentation for nvms exits codes, so I can see how to distinguish failures and successes?

@ljharb
Copy link
Member

ljharb commented Oct 9, 2015

The final exit codes will be 0. The issue you're seeing is that because nvm is a sourced function, it will have nonzero exit codes inside its own code, which isn't something you will be capable of handling.

I'm suggesting adding explicit failure detection to your tests, not nvm, rather than just blindly failing with set -e whenever something doesn't exit with a zero - because, like in nvm's case, it might be intentional.

@jfirebaugh
Copy link
Contributor Author

In that case, I firmly disagree with your stance on set -e. Using set -e is a widely accepted good practice for robust shell scripting. I would especially expect tooling such as nvm that is expected to integrate into a wide variety of environments to be compatible with it.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2015

Since nvm doesn't run in a subshell, nor is executed, it's sourced in the current environment. I have to constantly work around all sorts of builtin shell aliases people override, or zsh options they set, and it's a huge hassle. set -e is just a nonstarter - I'd have to make nvm significantly slower and with messier code in order to avoid the very common and normal case of having a nonzero exit code and using that for shell code control flow.

You could try setting set -e after nvm install has ran, since I believe it can be set at any time, just not unset - that may be a workaround.

jfirebaugh added a commit to jfirebaugh/nvm that referenced this issue Oct 9, 2015
@jfirebaugh
Copy link
Contributor Author

Thank you for the explanation, I appreciate it. I'll send a PR to add set -e to the "Compatibility Issues" section of the README, and I think the workaround you suggest will be fine.

ljharb added a commit that referenced this issue Oct 9, 2015
[Docs] Note compatibility issue with `set -e` (#866, #865, #721)
@ljharb ljharb added the installing node Issues with installing node/io.js versions. label Oct 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installing node Issues with installing node/io.js versions.
Projects
None yet
Development

No branches or pull requests

2 participants