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

joshuarli · 2018-05-31T22:36:50Z

For some context, this was causing issues described in getsentry/sentry#8623.

nvm_is_version_installed should not be checking for the presence of an installed node version by simply checking the presence of a directory in versions/node. It just isn't a strict enough check. One step would be to check for the presence of an executable bin/node, which is what this PR implements.

Even more strict would be to actually try and invoke bin/node itself afterwards, and comparing [ "$(node -v)" = "v${1}" ]. Please let me know if that sounds good, or if you have any questions.

…le instead of root dir

ljharb

Thanks, I think this is a great change.

As for making it even stricter, I don't think invoking node is worth the slowdown.

ljharb · 2018-06-02T06:51:54Z

nvm.sh

@@ -130,7 +130,7 @@ nvm_has_system_iojs() {
 }

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


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

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.

ljharb · 2018-06-02T18:39:57Z

oh also, could you add a test that would have failed without this change? Thanks!

joshuarli · 2018-06-05T01:51:01Z

Added a test in test/installation_node, let me know what you think. I also saw nvm_is_version_installed invoked in 2 files in test/installation_iojs, but have no idea what that is.

ljharb · 2018-06-05T04:30:46Z

test/installation_node/install hook

@@ -19,6 +19,13 @@ 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
+mkdir -p "${VERSION_PATH}"
+if [ -z "$(ls -A ${VERSION_PATH})" ]; then


when would the dir be non-empty here, and wouldn't we want to fail the test if this is the case?

Are you saying that the -z test on ls -A for empty dir is unnecessary? I guess in other words, am I right to assume that the $VERSION is uninstalled after line 20 finishes?

If uninstallation fails, line 20 would exit.

You could rm -rf "${VERSION_PATH} && mkdir -p "${VERSION_PATH}" instead, though, just in case?

Right, so assuming uninstallation is successful, $VERSION_PATH shouldn't even exist after line 20. I did initially consider a rm -rf but opted for a sanity check instead - that's what the if was for.

If we're 100% confident about the correctness of the non-zero exit code from nvm uninstall, then we don't need to do anything just in case, and can remove the if statement altogether. What do you think?

With the if statement, with a non-empty folder, it silently skips this test. That's not good.
With nothing, with a non-empty folder, it silently tests the wrong thing. That's not good either.

In other words, if the precondition fails, either the whole test should blow up, or, it should be silently cleaned up so that the next test is actually testing something.

Good points! I can't really think of a better approach at the moment. 5b8da24

joshuarli · 2018-06-05T05:32:14Z

@ljharb The fast test suites have been failing across all my commits in this PR - does it look like a problem to you?

ljharb · 2018-06-05T05:35:12Z

@joshuarli hmm, yes it does :-/ it's possible that your change to make the check more strict, means that the mocks in tests need to fake node more thoroughly

joshuarli · 2018-06-12T21:25:45Z

@ljharb Hey, sorry for the delay.

So that seems to be correct - only the fast tests are failing since they mock node installations by just creating the dir (hence, fast).

Conveniently, make_fake_node in common.sh does exactly what we're looking for (creates an executable node file at $(nvm_version_path ${VERSION})/bin/node) and interestingly enough, the function isn't used anywhere! According to the git blame, you added it a while back in 186eb88 and it hasn't changed or been used since.

I started substituting a lot of the mkdir -p "${NVM_DIR}/versions/node/v0.12.1" and similar lines in the fast tests, but came across stuff like this:

mkdir -p "${NVM_DIR}/versions/node/v0.12.1"
mkdir "${NVM_DIR}/v0.1.3"

nvm ls 0.12 | grep v0.12.1 || die '"nvm ls" did not list a version in the versions/ directory'
nvm ls 0.1 | grep v0.1.3 || die '"nvm ls" did not list a version not in the versions/ directory'

Can you clarify what that is, and do you think it would be okay to just substitute the mkdir calls with .../versions/node/... in the path like so:

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

make_fake_node v0.12.1
mkdir "${NVM_DIR}/v0.1.3"

For reference, this is test/fast/Listing\ versions/Running\ \"nvm\ ls\"\ should\ list\ versions\ in\ the\ \"versions\"\ directory.

ljharb · 2018-06-12T21:56:07Z

lol, i must have added it in preparation for other branches and never used it. Good catch!

In that case, I think it should be two make_fake_node calls.

joshuarli · 2018-06-12T22:06:43Z

But mkdir "${NVM_DIR}/v0.1.3" is done as preparation for this test: nvm ls 0.1 | grep v0.1.3 || die '"nvm ls" did not list a version not in the versions/ directory'

make_fake_node is the appropriate substitute for mkdir -p "${NVM_DIR}/versions/node/v0.12.1", but not mkdir "${NVM_DIR}/v0.1.3". What's the difference between these two old mocks? I thought all nvm-installed nodes were placed in .../versions/?

ljharb · 2018-06-12T22:17:07Z

All versions < 0.12 are placed in the top-level dir.

joshuarli · 2018-06-12T22:27:48Z

So would this be correct?

make_fake_node v0.12.1
mkdir "${NVM_DIR}/v0.1.3"

ljharb · 2018-06-12T23:06:59Z

I would expect:

make_fake_node v0.12.1
make_fake_node v0.1.3

since 186eb88#diff-daaa154e22be44798ffcddf12a70e03eR33 will handle the right version path

joshuarli · 2018-06-12T23:46:44Z

Thanks, just wanted to make sure. You know nvm and test internals way, way better than I do, I'm just a passerby. Will be making changes soon.

ljharb · 2018-06-13T18:46:56Z

I'm going to peel out the make_fake_node commit directly into master, and will rebase this PR once i do. Thanks!

ljharb

(I'll fix this comment in the commit I land on master prior to rebasing this PR)

ljharb · 2018-06-13T20:38:26Z

test/fast/Listing paths/Running "nvm which 0.0.2" should display only version 0.0.2.

@@ -1,10 +1,11 @@
 #!/bin/sh

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


there's a number of places where this path is incorrect; common.sh is not a sibling of nvm.sh, it's inside test

Oops! Sorry I missed that, staring at all those dots can get confusing sometimes 😆

ljharb · 2018-06-13T22:18:56Z

@joshuarli the make_fake_node commit is now in master (thanks!), and this PR is rebased. Tests are still failing, however.

joshuarli · 2018-06-13T23:09:23Z

Right, so there are some stray tests that looked out of the ordinary, so I didn't modify them with the previous commit (hence [Tests] update most mkdirs ...). For example:

#!/bin/sh

set -ex

cd ../..
mkdir v0.0.1
mkdir src/node-v0.0.1

. ./nvm.sh
nvm uninstall v0.0.1

[ ! -d 'v0.0.1' ] && [ ! -d 'src/node-v0.0.1/files' ]

What's the src/node-v0.0.1/...? This test, Running\ \"nvm\ uninstall\"\ should\ remove\ the\ appropriate\ directory., appears to have been written long ago. Can it be replaced by something like this?

#!/bin/sh

set -ex

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

make_fake_node v0.0.1

nvm uninstall v0.0.1

[ ! -d 'v0.0.1' ]

ljharb · 2018-06-13T23:17:32Z

In that case, it's the extracted source files for a to-be-compiled version of node v0.0.1 - it's not the same as make_fake_node, it's more like make_fake_source_cache, but I'm not sure if we need that.

joshuarli · 2018-06-14T17:41:53Z

Alright, down from 11 to 6 to 3 tests failed. Do you know what's going on here? I can't figure this out. This is line 594+ in the SHELL=sh TEST_SUITE=fast matrix job.

+mkdir src alias
    Aliases
    ✓ "nvm alias" should not accept aliases with slashes
    ✓ "nvm unalias" should not accept aliases with slashes
    ✗ Running "nvm alias <aliasname> <target>" again should change the target
! WARNING: Version '0.0.2' does not exist.
test-stable-1 -> 0.0.2 (-> N/A)
nvm alias test-stable-1 0.0.2 did not set test-stable-1 to 0.0.2: got 'test-stable-10 -> 0.0.10 (-> N/A)
test-stable-1 -> 0.0.2 (-> N/A)'
    ✓ Running "nvm alias <aliasname>" should list but one alias.
    ✓ Running "nvm alias" lists implicit aliases when they do not exist
    ✓ Running "nvm alias" lists manual aliases instead of implicit aliases when present
    ✗ Running "nvm alias" should list all aliases.
did not find test-stable-1 alias; got 'test-iojs-10 -> 0.2.10 (-> N/A)

ljharb · 2018-06-14T21:06:39Z

The first test is doing these steps:

make an alias called "test-stable-1" pointing to "0.0.2"
retrieve the alias value for "test-stable-1", assert it's "0.0.2" (this is failing)
make an alias called "test-stable-2" pointing to "0.0.1"
retrieve the alias value for "test-stable-2", assert it's "0.0.1"

I think the setup file in that directory needs to convert some mkdirs into make_fake_nodes?

joshuarli · 2018-06-15T04:46:54Z

@ljharb Thanks, that helped. Everything's green now :)

ljharb

Thanks!

joshuarli · 2018-06-15T05:17:42Z

No problem, thanks for being a big help.

[Fix] Improve nvm_is_version_installed to check for a node executab…

0cdc184

…le instead of root dir

ljharb requested changes Jun 2, 2018

View reviewed changes

ljharb added the needs followup We need some info or action from whoever filed this issue/PR. label Jun 2, 2018

ljharb reviewed Jun 5, 2018

View reviewed changes

ljharb reviewed Jun 13, 2018

View reviewed changes

ljharb approved these changes Jun 15, 2018

View reviewed changes

ljharb removed the needs followup We need some info or action from whoever filed this issue/PR. label Jun 15, 2018

ljharb merged commit 0cdc184 into nvm-sh:master Jun 15, 2018

joshuarli deleted the improve-nvm-is-version-installed branch June 15, 2018 17:35

joshuarli mentioned this pull request Jun 15, 2018

build(travis): revert travis cache and nvm install hotfix getsentry/sentry#8755

Closed

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

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

Conversation

joshuarli commented May 31, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarli Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

ljharb commented Jun 2, 2018

joshuarli commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarli commented Jun 5, 2018

ljharb commented Jun 5, 2018

joshuarli commented Jun 12, 2018 • edited Loading

ljharb commented Jun 12, 2018

joshuarli commented Jun 12, 2018

ljharb commented Jun 12, 2018

joshuarli commented Jun 12, 2018

ljharb commented Jun 12, 2018

joshuarli commented Jun 12, 2018

ljharb commented Jun 13, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jun 13, 2018

joshuarli commented Jun 13, 2018 • edited Loading

ljharb commented Jun 13, 2018

joshuarli commented Jun 14, 2018 • edited Loading

ljharb commented Jun 14, 2018

joshuarli commented Jun 15, 2018

ljharb left a comment

Choose a reason for hiding this comment

joshuarli commented Jun 15, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joshuarli Jun 5, 2018 •

edited

Loading

joshuarli commented Jun 12, 2018 •

edited

Loading

joshuarli commented Jun 13, 2018 •

edited

Loading

joshuarli commented Jun 14, 2018 •

edited

Loading