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.

Sign up for GitHub

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

Improve nvm_is_version_installed to check for a node executable instead of root dir #1824

Merged
merged 1 commit into from
Jun 15, 2018
+27 −19
Merged

Improve nvm_is_version_installed to check for a node executable instead of root dir #1824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nvm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ nvm_has_system_iojs() {
}

nvm_is_version_installed() {
[ -n "${1-}" ] && [ -d "$(nvm_version_path "${1-}" 2> /dev/null)" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to keep the [ -n "${1-}" ] test, since nvm_version_path will error on empty input.

Copy link
Contributor Author

@joshuarli joshuarli Jun 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That check was actually unnecessary before as without it, nvm_version_path will print "version is required" to stderr which is eaten by /dev/null, and so [ -d "" ] will always exit with 1 as expected.

But with the new change, the check is actually required since we don't want the test argument to evaluate to /bin/node if $1 isn't present. Good catch!

See e5be6b8.

[ -n "${1-}" ] && [ -x "$(nvm_version_path "$1" 2> /dev/null)"/bin/node ]
}

nvm_print_npm_version() {
Expand Down
9 changes: 6 additions & 3 deletions test/fast/Aliases/setup
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
#!/bin/sh

\. ../../../nvm.sh
\. ../../common.sh

for i in $(seq 1 10)
do
echo 0.0.$i > ../../../alias/test-stable-$i
mkdir -p ../../../v0.0.$i
make_fake_node v0.0.$i
echo 0.1.$i > ../../../alias/test-unstable-$i
mkdir -p ../../../v0.1.$i
make_fake_node v0.1.$i
echo 0.2.$i > ../../../alias/test-iojs-$i
mkdir -p ../../../versions/io.js/v0.2.$i
make_fake_iojs v0.2.$i
done
9 changes: 5 additions & 4 deletions test/fast/Running "nvm deactivate" should unset the nvm environment variables.
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

set -ex

mkdir -p ../../v0.2.3

die () { echo "$@" ; exit 1; }

[ `expr $PATH : ".*v0.2.3/.*/bin.*"` = 0 ] || echo "WARNING: Unexpectedly found v0.2.3 already active" >&2

\. ../../nvm.sh
\. ../common.sh

make_fake_node v0.2.3

[ `expr $PATH : ".*v0.2.3/.*/bin.*"` = 0 ] || echo "WARNING: Unexpectedly found v0.2.3 already active" >&2

nvm use --delete-prefix v0.2.3 || die "Failed to activate v0.2.3"
[ `expr "$PATH" : ".*v0.2.3/.*/bin.*"` != 0 ] || die "PATH not set up properly"
Expand Down
10 changes: 5 additions & 5 deletions test/fast/Running "nvm uninstall" should remove the appropriate directory.
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

set -ex

cd ../..
mkdir v0.0.1
mkdir src/node-v0.0.1
\. ../../nvm.sh
\. ../common.sh

make_fake_node v0.0.1

. ./nvm.sh
nvm uninstall v0.0.1

[ ! -d 'v0.0.1' ] && [ ! -d 'src/node-v0.0.1/files' ]
[ ! -d 'v0.0.1' ]
10 changes: 4 additions & 6 deletions test/fast/Running "nvm uninstall" with incorrect file permissions fails nicely
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@

set -ex

cd ../..
mkdir v0.0.1
mkdir src/node-v0.0.1
\. ../../nvm.sh
\. ../common.sh

sudo touch v0.0.1/sudo

. ./nvm.sh
make_fake_node v0.0.1
sudo touch ""$(nvm_version_path v0.0.1)"/sudo"

RETURN_MESSAGE="$(nvm uninstall v0.0.1 2>&1 || echo)"
CHECK_FOR="Cannot uninstall, incorrect permissions on installation folder"
Expand Down
6 changes: 6 additions & 0 deletions test/installation_node/install hook
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ fail() {

! nvm_is_version_installed "${VERSION}" || nvm uninstall "${VERSION}" || die 'uninstall failed'

# an existing but empty VERSION_PATH directory should not be enough to satisfy nvm_is_version_installed
rm -rf "${VERSION_PATH}"
mkdir -p "${VERSION_PATH}"
nvm_is_version_installed "${VERSION}" && die 'nvm_is_version_installed check not strict enough'
rmdir "${VERSION_PATH}"

OUTPUT="$(NVM_INSTALL_THIRD_PARTY_HOOK=succeed nvm install "${VERSION}")"
USE_OUTPUT="$(nvm use "${VERSION}")"
EXPECTED_OUTPUT="${VERSION} node std binary ${VERSION_PATH}
Expand Down