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

Normalize paths in `nvm_tree_contains_path` before comparing #2045

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@stevebob
Copy link

commented May 11, 2019

It's possible that tree and node_path both refer to the same
directory, but are lexically different. This converts both paths
into a canonical form so that any lexical-only differences are
removed.

This solves a problem where setting NVM_DIR to /home/user//.nvm
instead of /home/user/.nvm results in the infamous:
nvm is not compatible with the npm config "prefix" option

This adds a test that the above case no longer results in an error. An
existing test stopped failing as expected as a result, as the test set
the npm prefix to a local-directory-relative path, which resolved to
a directory inside the NVM_DIR when running the tests from within the
NVM_DIR. This test is updated to use an absolute path.

Normalize paths in `nvm_tree_contains_path` before comparing
It's possible that `tree` and `node_path` both refer to the same
directory, but are lexically different. This converts both paths
into a canonical form so that any lexical-only differences are
removed.

This solves a problem where setting `NVM_DIR` to /home/user//.nvm
instead of /home/user/.nvm results in the infamous:
nvm is not compatible with the npm config "prefix" option

This adds a test that the above case no longer results in an error. An
existing test stopped failing as expected as a result, as the test set
the npm prefix to a local-directory-relative path, which resolved to
a directory inside the `NVM_DIR` when running the tests from within the
`NVM_DIR`. This test is updated to use an absolute path.
@stevebob

This comment has been minimized.

Copy link
Author

commented May 11, 2019

This will not work on macos because macos doesn't come with realpath (it's part of coreutils which can be installed with brew). I'm opening this PR to get people's thoughts.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

The lack of realpath in POSIX (and thus on Macs) is why this hasn't been added before, which is why #617 remains a problem. I'd like a solution, but there has to be a fallback for when realpath is not available, or else it won't be viable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.