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.

Sign up for GitHub

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

Jump to bottom

[Fix] Install failed should return correct exit status, fix #1347 #1348

Merged
merged 2 commits into from
Mar 18, 2017
Merged

[Fix] Install failed should return correct exit status, fix #1347 #1348

merged 2 commits into from
Mar 18, 2017

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Dec 9, 2016 •
edited
Loading

Fix #1347

PeterDaveHello
PeterDaveHello commented Dec 9, 2016
View reviewed changes
nvm.sh Outdated
fi

fi

if [ "$NVM_INSTALL_SUCCESS" = true ] && nvm use "$VERSION"; then
if [ "$NVM_INSTALL_EXIT_STATUS" = 0 ] && nvm use "$VERSION"; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if can make the exit status volatile so I try to save the exit status.

@Turbo87 Turbo87 mentioned this pull request Dec 12, 2016
Closed
ljharb
ljharb approved these changes Dec 14, 2016
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just a few changes and this is good to go.

nvm.sh Outdated
fi
if [ "$NVM_INSTALL_SUCCESS" != true ]; then
if [ "$NVM_INSTALL_EXIT_CODE" != 0 ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should use -ne 0

nvm.sh Outdated
fi

fi

if [ "$NVM_INSTALL_SUCCESS" = true ] && nvm use "$VERSION"; then
if [ "$NVM_INSTALL_EXIT_CODE" = 0 ] && nvm use "$VERSION"; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and -eq 0

nvm.sh Outdated
fi
fi
return $?
return "${NVM_INSTALL_EXIT_CODE}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the quotes or curlies here, it will error if it's not a number

@ljharb ljharb added bugs Oh no, something's broken :-( installing node Issues with installing node/io.js versions. labels Dec 14, 2016
@ljharb ljharb self-assigned this Dec 14, 2016
@ljharb
Copy link
Member

ljharb commented Dec 14, 2016

The test failure looks legit, btw.

@PeterDaveHello
Copy link
Collaborator Author

hmmm ... didn't realize the reason it failed yet ... will update the updates I can do first

ljharb
ljharb requested changes Dec 27, 2016
View reviewed changes
nvm.sh Outdated
@@ -2425,7 +2425,7 @@ nvm() {
nvm_err '*** Third-party $NVM_INSTALL_THIRD_PARTY_HOOK env var claimed to succeed, but failed to install! ***'
return 33
fi
NVM_INSTALL_SUCCESS=true
NVM_INSTALL_EXIT_CODE=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should set it to "${EXIT_CODE}" since that's the exit code from the hook.

nvm.sh Outdated
fi
fi
return $?
return ${NVM_INSTALL_EXIT_CODE}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the braces here only work inside a string - so they should be removed.

@PeterDaveHello
Copy link
Collaborator Author

Fixed!

ljharb
ljharb requested changes Dec 27, 2016
View reviewed changes
nvm.sh
fi
if [ "$NVM_INSTALL_SUCCESS" != true ]; then
if [ "$EXIT_CODE" -ne 0 ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're going to do this check, we should set it to -1 initially, or else set -u might complain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! https://github.com/creationix/nvm/pull/1348/files#diff-e1e8e036966ad5aaf7709712cd5d8ee3R2419

ljharb
ljharb requested changes Dec 27, 2016
View reviewed changes
nvm.sh Outdated
@@ -2417,6 +2416,7 @@ nvm() {
local VERSION_PATH
VERSION_PATH="$(nvm_version_path "${VERSION}")"
local EXIT_CODE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line should be removed, or else the local will be run twice, which creates shell output in zsh

nvm.sh Outdated
@@ -2417,6 +2416,7 @@ nvm() {
local VERSION_PATH
VERSION_PATH="$(nvm_version_path "${VERSION}")"
local EXIT_CODE
EXIT_CODE=-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, this should be initialized at the first definition of the var, after line 2408.

@PeterDaveHello
Copy link
Collaborator Author

Hope this time we are good to go, thanks for your time @ljharb

@ljharb
Copy link
Member

ljharb commented Dec 27, 2016

Looks like https://travis-ci.org/creationix/nvm/jobs/186911975 is still a legit test failure.

@PeterDaveHello
Copy link
Collaborator Author

The error message looks ... so unrelated ... lol ... still trying to find out the problem

@getify
Copy link

getify commented Mar 17, 2017

Any update on this? Downstream it's still affecting travis builds -- over the last couple of days, there was another outage where node downloads were failing, but travis builds didn't stop, making triage more confusing.

@PeterDaveHello
Copy link
Collaborator Author

We didn't figure out the reason why test failed yet.

@ljharb
Copy link
Member

ljharb commented Mar 17, 2017

@PeterDaveHello I'm wondering if configure behaves differently in zsh such that --without-snapshot isn't present? that's the only thing i can think of.

Would you mind rebasing this to see if it's still failing?

@PeterDaveHello
Copy link
Collaborator Author

Okay, will need to fix the conflicts first.

@PeterDaveHello
Copy link
Collaborator Author

Rebased

@PeterDaveHello
Copy link
Collaborator Author

Still failed here, lol ...
https://travis-ci.org/creationix/nvm/jobs/212337387

@ljharb
[Fix] nvm install -s: ensure additional params lack a trailing space.
9b26293 
Note: I believe this does not fix the underlying issue in zsh, which is
that it does not split up `$ADDITIONAL_PARAMETERS` and instead passes
the contents as one single argument.
@ljharb
Copy link
Member

ljharb commented Mar 18, 2017

@PeterDaveHello k, i think i fixed the issue for this PR - but I did not fix the underlying issue with zsh and $ADDITIONAL_PARAMETERS.

If you can fix that, or if you have a better way than mine, please feel free to amend my commit.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb I think it looks good enough for me right now.

@ljharb ljharb merged commit 9b26293 into nvm-sh:master Mar 18, 2017
@PeterDaveHello PeterDaveHello deleted the fix-install-failed-exit branch March 18, 2017 07:58
@PeterDaveHello PeterDaveHello mentioned this pull request Apr 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees

@ljharb ljharb

Labels
bugs Oh no, something's broken :-( installing node Issues with installing node/io.js versions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PeterDaveHello @ljharb @getify