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

try to use realpath to find nvm's dir #1371

Closed
wants to merge 2 commits into from
Closed

Conversation

jchip
Copy link

@jchip jchip commented Dec 29, 2016

From #1369

@jchip jchip mentioned this pull request Dec 29, 2016
4 tasks
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.

Thanks! This PR needs tests before it could be accepted.

Can you elaborate on what this is fixing - ie, in what scenario does nvm-exec fail, that your change makes work?

Can this change also be made in nvm.sh so that nvm itself can also benefit from it?

@jchip
Copy link
Author

jchip commented Dec 29, 2016

realpath is more reliable. If I symlink /usr/local/bin/nvm-exec to nvm-exec, then only realpath can find the real dir.

I am not familiar with the way the tests are setup. Where is the existing test I can update?

@ljharb
Copy link
Member

ljharb commented Dec 29, 2016

What's the usecase for symlinking it, as opposed to adding its directory to the PATH, or using nvm exec?

Each test has its own file - https://github.com/creationix/nvm/tree/f7763c8ba96e61825e34ebaf979811a1bde2a997/test/slow/nvm%20exec are all of the nvm exec tests atm, and https://github.com/creationix/nvm/blob/f7763c8ba96e61825e34ebaf979811a1bde2a997/test/install_script/nvm_source is the test for the install script being able to locate nvm-exec, which may be relevant here.

@ljharb
Copy link
Member

ljharb commented Dec 29, 2016

This also might be related to #617.

@jchip
Copy link
Author

jchip commented Dec 29, 2016

the main use of nvm-exec is for when installing nvm some where under /usr where only root has access. symlinking so no need to mess with PATH. In this case, you can't do nvm install without sudo. sudo can't execute nvm because it's a shell function. so only way is to execute nvm with nvm-exec, which is to be added later, or in the first PR I submitted.

I don't see any test for the file `nvm-exec.

@ljharb
Copy link
Member

ljharb commented Dec 29, 2016

nvm should be installed per-user - one separate copy of NVM_DIR for each user that wants to access it. Installing nvm to share across user accounts isn't a supported use case.

All the tests are for nvm exec, which invokes nvm-exec under the hood - nvm exec is the important thing not to break, using nvm-exec directly is also not something that's officially encouraged.

@jchip
Copy link
Author

jchip commented Dec 29, 2016

Installing nvm to share across user accounts isn't a supported use case.

You don't want to make that a supported use case?

All existing nvm exec tests passed.

@ljharb
Copy link
Member

ljharb commented Dec 29, 2016

Not particularly, you're supposed to always have an entire nvm installation for each user account that wants to use nvm and node.

I don't mind improving nvm-exec and nvm.sh in concert to work better with symlinks, though.

@jchip
Copy link
Author

jchip commented Dec 29, 2016

work better with symlinks

This PR changes to use realpath if it's available is more reliable.

To support shared usage, you just need to make nvm-exec be able to invoke nvm itself. There was a question somewhere (stackoverflow?) in which someone suggested sudo nvm-exec, but that doesn't work w/o being able to invoke nvm using sudo somehow.

shared usage is helpful on shared machines.

@jchip
Copy link
Author

jchip commented Dec 29, 2016

I've been using this for more than a year to support shared usage. I just found some time now to see if there interest for this so I figured I PR it in.

@jchip
Copy link
Author

jchip commented Dec 29, 2016

that doesn't work w/o being able to invoke nvm using sudo somehow.

unless you login as root directly.

@jchip jchip closed this Jan 13, 2017
@jchip jchip deleted the realpath branch January 13, 2017 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants