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

adjust for symbolic links #2143

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

adjust for symbolic links #2143

wants to merge 2 commits into from

Conversation

aiqui
Copy link

@aiqui aiqui commented Jan 8, 2020

Currently the nvm.sh script does not handle directories that are symbolic links. This is a common problem on servers that use symbolic links to persistent drives. For example, an admin might want to install nvm into /opt/local/node, but the real location (without the symlinks) is /files2/opt-local/node/nvm. This results in:

export NVM_DIR=/opt/local/node/nvm && . "$NVM_DIR/nvm.sh"
nvm is not compatible with the npm config "prefix" option: currently set to "/files2/opt-local/node/nvm/versions/node/v8.11.3"
Run `npm config delete prefix` or `nvm use --delete-prefix v8.11.3 --silent` to unset it.

Assuming the admin knows this issue and uses the real directory, there is no problem:

export NVM_DIR=/files2/opt-local/node/nvm && . "$NVM_DIR/nvm.sh"

The small change I am providing will get the real directory to avoid the problem with symbolic links.

I don't know if cd -P is available on all Linux systems, but I have tested in MacOS, RedHat, AWS AMI and Debian.

Co-Authored-By: Charlie Hileman <aiquicorp@gmail.com>
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
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.

Turns out cd -P is indeed in POSIX! https://pubs.opengroup.org/onlinepubs/9699919799/utilities/cd.html \( ゚◡゚)/

This will need tests.

nvm.sh Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jan 8, 2020

Does this perhaps fix #617 and then also close #1385, and then perhaps address some of the issues in #855?

nvm.sh Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jan 12, 2020

The code looks good, but this is breaking existing unit tests for nvm_tree_contains_path.

@ljharb
Copy link
Member

ljharb commented Sep 17, 2020

@aiqui any interest in completing this PR?

@olivermg
Copy link

I've started some more work on this in https://github.com/olivermg/nvm .

However, the current state does not yet fix the complaint of nvm whenever $HOME is being set to a path that contains a symlink. I'll see if I can find out some more and improve the situation.

@dylanarmstrong
Copy link
Contributor

Hey @olivermg, want to check on latest master branch, my PR from the other day adjusted the prefix check to handle symlinks, so it never hits the nvm_tree_contains_path code anymore.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2020

(See also, #2146 and #2045)

@ljharb
Copy link
Member

ljharb commented Dec 5, 2023

@aiqui @olivermg i'd love to finish this PR and get it landed; would either of you be interested in doing that? @olivermg, you can comment a link to your branch and I'll pull in the changes.

@ljharb ljharb marked this pull request as draft December 5, 2023 04:52
@ljharb ljharb force-pushed the master branch 2 times, most recently from c6cfc3a to c20db2a Compare June 10, 2024 18:13
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.

4 participants