Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Use symlinks for aliases #358

Closed
wants to merge 1 commit into from

3 participants

@drkibitz

This is something I've seen mentioned in another issue, and something that would greatly simplify my own CI setup, and allow me to use nvm in more versatile ways.

I didn't try for backwards compatibility, so yes these are indeed breaking changes. If this is rejected I won't bother, but if you are interested in merging, but want backwards compatibility, then I have another idea of using symlinks in addition to files.

One thing to note is why I am removing the link if it exists before creating it instead of just using ln -f. This is because (on OS X at least) if you ln -sf path/to/dir existing/link/to/old/dir then it creates a link inside of existing/link/to/old/dir. This essentially gives you existing/link/to/old/dir/dir, which points to nothing if path/to/dir is relative.


This is my first contribution to nvm so wanted to note a few of issues I came across. These are not related to this pull request:

  • nvm deactivate fails in OS X Mavericks, and fails test of course (might be if PATH is set by launchd).
  • Not sure what is the preferred shell compatibility nvm is going for. It looks to be bash, but there's many inconsistencies between using bash only things versus sh compatible things.
  • There are quite a few variables not declared as local (ALIAS being one of them).

If you are interested in further pull requests for these, please let me know.

@drkibitz

Ah ok, I just saw this #345

I added a couple bashisms here, so if that PR is merged I'll fix this one if this is interesting to anyone.

@ljharb
Collaborator

nvm ideally works in sh, bash, and zsh, at the least. There's been interest in reducing this down to just sh so it will work on as many platforms as possible.

In addition, variables that are not local but should be - that's a great fix.

This is kind of large, and has conficts - could you split this up into as many small PRs as possible? That way the majority of it can get in more easily.

@drkibitz

@ljharb Did you mean this particular pull request is large or the notes about the other issues? I can definitely make separate pull requests for other issues, but not sure how to split this one up in particular?

About the shell compatibility and this pull request, I can fix it to not add additional bashisms.

@ljharb
Collaborator

This one's not large but not small. I'd love each change to be in a PR with an explanation, so it's easier to test on multiple shells.

@drkibitz

These are the minimum changes required to change from using file aliases to symlink aliases. Using readlink in place of cat, and ln in place of echo >. If this pull request is broken up, no one change would work without the rest.

@ljharb
Collaborator

Fair enough :-) Please rebase it on top of the latest master, and I'll test it locally before merging.

@drkibitz

Will do, and those additional bashisms will be removed.

@drkibitz

Rebased, and bashisms (that were specific to this pull request) are removed.

@drkibitz

Rebased on to master.

@ljharb ljharb referenced this pull request
Closed

Make aliases actual symlinks #89

@ljharb
Collaborator

Relates to #467

@mzgol

Aliases can be dynamic, how would that work?

nvm install 0.10 # installs v0.10.28
nvm alias default 0.10
# some time later...
nvm install 0.10 # installs v0.10.29
# `nvm use default` should activate 0.10.29 now
nvm uninstall 0.10.28
# `nvm use default` should still activate 0.10.29
@drkibitz

What do you think this should do exactly?

nvm alias default 0.10
@mzgol

I don't know what it should do exactly in the symlink approach, that would probably require rewriting symlinks during nvm install to point to the proper version.

That said, whatever the implementation, the scenario from my last post should continue to work. Otherwise it'll be a huge step backwards.

EDIT: one solution would be to have nvm alias default 0.10 create two aliases: default -> 0.10 and 0.10 -> 0.10.28. Then, when nvm install 0.10 installs 0.10.29, the symlink should change: 0.10 -> 0.10.29.

This may not cover all the cases but sth like that will have to be done if we want to use symlinks.

EDIT 2: another approach would be to continue using files for aliases and just have a separate directory for symlinks. Every nvm install would then go over all aliases from files and rewrite related symlinks.

@drkibitz

Wasn't asking what it should do specifically in the symlink approach was asking in general.

@mzgol
@ljharb
Collaborator

Taking another look at this one - I think I'm missing why using symlinks for aliases would be better than the current setup? Using files, as @mzgol points out, allows us to make an alias to an alias - not just aliasing to 0.10, but aliasing to other aliases (like nvm alias foobar 0.10 && nvm alias default foobar still works if i then do nvm alias foobar 0.11). Using symlinks would make that more complex, such that we'd need to rewrite symlinks every nvm install, nvm uninstall, and nvm alias - and there's probably more that I'm not thinking of.

@mzgol

I can see the appeal of aliases as then it's easy to set PATH for GUI apps or specific ones so that they point to Node binaries of a specific alias. That said, it's definitely more difficult to make it right this way than what we currently have. Current dynamic nature is done via nvm in runtime which is easy; aliases need to do it by themselves and installing/uninstalling Node versions can change where aliases point to.

I think all that's needed to make it work is to re-compute all aliases on nvm install or nvm uninstall. Am I missing any other cases where symlink targets can change except those two and nvm alias?

@ljharb
Collaborator

nvm install, nvm uninstall, nvm alias, and nvm unalias would need to recompute all symlinked aliases. However, currently, you can edit an alias file any time you want, and nvm picks up on it automatically - with symlinks, if the user chooses to rename a symlink alias, nvm should pick up on that too. So, I'd think that everything that touches aliases, including nvm run, nvm use, nvm ls, and nvm exec would need to recompute also.

@ljharb
Collaborator

I'm going to close this out for the time being - to sum up the issues:

  • to use symlinks for aliases is tricky to get right. install/uninstall/alias/unalias need to recompute all symlinked aliases, and run/use/ls/exec/which/etc would also need to recompute them. This will both be complex, and not very performant.
  • The only value here seems to be to make using an IDE easier - however, nvm is not meant to be used as anything except a node version manager for specific shells. Thus, I think this use case is outside the scope of nvm at this time, especially considering that via nvm which you'll now be able to dynamically locate a node binary by alias or version.

I'm happy to be convinced otherwise and this topic is by no means closed permanently, but I want to keep the issue/PR list clean :-)

@ljharb ljharb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 12, 2014
  1. @drkibitz

    Use symlinks for aliases

    drkibitz authored
This page is out of date. Refresh to see the latest.
View
25 nvm.sh
@@ -78,8 +78,8 @@ nvm_ls() {
return
fi
- if [ -f "$NVM_DIR/alias/$PATTERN" ]; then
- nvm_version `cat $NVM_DIR/alias/$PATTERN`
+ if [ -h "$NVM_DIR/alias/$PATTERN" ]; then
+ nvm_version $(basename $(readlink $NVM_DIR/alias/$PATTERN))
return
fi
# If it looks like an explicit version, don't do anything funny
@@ -373,9 +373,11 @@ nvm() {
echo "Uninstalled node $VERSION"
# Rm any aliases that point to uninstalled version.
- for ALIAS in `\grep -l $VERSION $NVM_DIR/alias/* 2>/dev/null`
+ for ALIAS in $NVM_DIR/alias/*
do
- nvm unalias `basename $ALIAS`
+ if [ $(expr $(readlink $ALIAS) : ".*${VERSION}.*") -ne 0 ]; then
+ nvm unalias $(basename $ALIAS)
+ fi
done
;;
@@ -491,7 +493,7 @@ nvm() {
local DEST
for ALIAS in $NVM_DIR/alias/$2*; do
if [ -e "$ALIAS" ]; then
- DEST=`cat $ALIAS`
+ DEST=$(basename $(readlink $ALIAS))
VERSION=`nvm_version $DEST`
if [ "$DEST" = "$VERSION" ]; then
echo "$(basename $ALIAS) -> $DEST"
@@ -512,17 +514,24 @@ nvm() {
if [ $? -ne 0 ]; then
echo "! WARNING: Version '$3' does not exist." >&2
fi
- echo $3 > "$NVM_DIR/alias/$2"
+ # We are linking directories, so explicitly remove it rather than forcing with ln -f
+ if [ -h "$NVM_DIR/alias/$2" ]; then
+ rm -f "$NVM_DIR/alias/$2"
+ fi
if [ ! "$3" = "$VERSION" ]; then
- echo "$2 -> $3 (-> $VERSION)"
+ # link to other alias
+ ln -s "./$3" "$NVM_DIR/alias/$2"
+ echo "$2 -> $3 (-> $VERSION)"
else
+ # link to node installation
+ ln -s "../$3" "$NVM_DIR/alias/$2"
echo "$2 -> $3"
fi
;;
"unalias" )
mkdir -p $NVM_DIR/alias
[ $# -ne 2 ] && nvm help && return
- [ ! -f $NVM_DIR/alias/$2 ] && echo "Alias $2 doesn't exist!" && return
+ [ ! -h $NVM_DIR/alias/$2 ] && echo "Alias $2 doesn't exist!" && return
rm -f $NVM_DIR/alias/$2
echo "Deleted alias $2"
;;
View
2  test/fast/Running "nvm alias" should create a file in the alias directory.
@@ -2,4 +2,4 @@
. ../../nvm.sh
nvm alias test v0.1.2
-[ $(cat ../../alias/test) = 'v0.1.2' ]
+[ -h ../../alias/test ] && [ $(readlink ../../alias/test) = '../v0.1.2' ]
View
4 test/fast/Running "nvm unalias" should remove the alias file.
@@ -1,6 +1,6 @@
#!/bin/sh
-echo v0.1.2 > ../../alias/test
+ln -s ../v0.1.2 ../../alias/test
. ../../nvm.sh
nvm unalias test
-! [ -e ../../alias/test ]
+! [ -h ../../alias/test ]
Something went wrong with that request. Please try again.