-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Add "current" symlink to NVM_DIR #447
Conversation
@@ -526,6 +526,7 @@ nvm() { | |||
export NODE_PATH | |||
export NVM_PATH="$NVM_DIR/$VERSION/lib/node" | |||
export NVM_BIN="$NVM_DIR/$VERSION/bin" | |||
rm -rf "$NVM_DIR/current" && ln -sf "$NVM_DIR/$VERSION" "$NVM_DIR/current" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using -f
on ln
, why rm
first? Alternatively, if you've just rm
d it, why -f
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -f
doesn't actually force a new link (I tried). The -f
on both rm
and ln
make sure there's no noise or warnings if they fail. I can change it to non-silent fail if that's preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that makes sense. This is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-r
is not needed here though, and -f
can be left out for ln
if you remove the link first
Could you try to add a test? You should be able to write a fast test, that does something like |
Sure |
OK, added test. But test seems to fail. Although test seems to fail on main master as well, so not sure what to do about that. |
This looks great, but we need the test to fail on master and pass on your branch in order for it to be valid. |
That makes no sense. The test passes when I run it locally. A bunch of other tests fail locally however. |
You should unload nvm ( |
I have a use case where I install NVM to The change in this PR breaks NVM for my regular users, as they are now unable to select a Node.JS version for their current session. Users get the following error:
Any ideas? |
|
The failing |
Sorry, you're correct in that it isn't breaking anything in NVM. It does cause the script to exit with a non-zero exit code, which causes some of my other scripts to fail (e.g. Vagrant, Ansible, etc). Node.JS's presence in distribution package managers is now much better than it was when I first resorted to using NVM, so I've switched to that where I can. In the remaining areas (e.g. Node.JS on ARM), I'll transition to per-user |
This is what I'm seeing. Are you sure this is the cause of the non-zero exit code? $ nvm use 0.10
rm: /Users/daniel/tmp/nvm/current: Permission denied
Now using node v0.10.29
[~/tmp/nvm]$ echo $?
0 |
Adresses #430