Check installation dir permissions before uninstalling #847

lukechilds · 2015-09-23T20:48:57Z

Fixes #840

ljharb · 2015-09-23T20:50:15Z

This LGTM at first glance - I'll want to play with it a bit first.

Can you also add an automated test?

lukechilds · 2015-09-23T20:53:02Z

Yeah I'll add a test in.

This is the new output btw:

$ nvm uninstall 4.0
Cannot uninstall, incorrect permissions on installation folder.
This is usually caused by running 'npm install -g' as root. Run the following command as root to fix the permissions and then try again.

  chown -R lukechilds /Users/lukechilds/Dev/oss/nvm/versions/node/v4.0.0

$ sudo chown -R lukechilds /Users/lukechilds/Dev/oss/nvm/versions/node/v4.0.0
Password:

$ nvm uninstall 4.0
Uninstalled node v4.0.0

$ nvm ls
         v4.1.1
node -> stable (-> v4.1.1) (default)
stable -> 4.1 (-> v4.1.1) (default)
iojs -> N/A (default)

ljharb · 2015-09-23T20:59:15Z

Looks great. You could even simplify it a bit by using $NVM_DIR in the provided command :-) but it's fine as-is too (as I do in nvm debug)

lukechilds · 2015-09-23T21:37:04Z

I'll leave it simply because I've already got $VERSION_PATH for the checks.

Builds are failing, but I have a hunch it may be the existing fast uninstall test that's wrong, not the new uninstall code. Meaning technically the permissions are wrong on the failing test.

Would it be ok to remove the fast uninstall test and add a slow one that actually tests installing and then uninstalling node rather than faking it with empty folders?

ljharb · 2015-09-23T21:37:51Z

Let's start by adding the slow one; if that passes and the fast one fails, I'd love to look at both and see if we can save the fast test.

lukechilds · 2015-09-23T23:26:55Z

I'm cloning into a random folder and working in there but when I run tests it's messing with my main ~/.nvm installation. Is that expected behavior?

ljharb · 2015-09-23T23:27:28Z

Yes, that's the sad state of dev/testing - ideally it doesn't clobber existing installs when possible.

lukechilds · 2015-09-23T23:35:38Z

I don't understand why sourcing the dev nvm.sh doesn't fix it though. Surely that should replace all the ENVs.

lukechilds · 2015-09-23T23:42:09Z

Think I've just implemented a fix that will make tests use the local nvm dir rather than the globally sourced one. I'll do some more testing and create a seperate pr for it tomorrow.

If I can get that merged in and rebase this from it, it'll make writing these tests a hell of a lot easier

lukechilds · 2016-04-16T23:33:28Z

Do you still want this PR?

Got some time to have a look into that failing test this weekend if you do.

ljharb · 2016-04-17T01:58:40Z

@lukechilds yes! sorry if it's fallen through the cracks. What do you think about adding a test that verifies that it fails when permissions are wrong?

lukechilds · 2016-04-17T02:09:58Z

No probs, I've been super busy.

Got the fast test that was previously failing working. It was globbing doing weird stuff when files didn't exist (because the fast test creates an empty dir), using find works. I can't find anything conclusive but it seems find is pretty portable, is it ok to use it?

Yeah adding a test to check the fail message gets triggered would be good, I'll get on it.

ljharb · 2016-04-17T02:50:22Z

find should be just fine - check existing usage of find for definitely-portable options.

lukechilds · 2016-04-17T11:09:03Z

Ok, added a test to check for the error message.

I'm also checking file permission with find -exec now as most shells wont handle file names correctly if they contain whitespace/newline chars. I've tested it and this will.

I still can't find anything saying how portable find is but the method I'm using is recommended here (good read) and I've found a lot of Stack Overflow answers regarding portability and POSIX compliancy recommending find.

If you're happy this should be all sorted.

ljharb · 2016-04-17T16:55:18Z

nvm.sh

+      # Check version dir permissions
+      local INVALID_FILES="$(find "$VERSION_PATH" -exec [ ! -w "{}" ] \; -exec echo "{}" \;)"
+      if [ "$INVALID_FILES" ]; then
+        echo "Cannot uninstall, incorrect permissions on installation folder."


these should probably all echo to stderr - >&2 echo blah

ljharb · 2016-04-17T16:56:45Z

@lukechilds thanks! Some minor comments, and it'd be nice to rebase these down to fewer conceptually atomic commits, but otherwise, good to go!

lukechilds · 2016-04-17T21:44:50Z

Made those tweaks and squished everything down to three commits :)

ljharb · 2016-04-18T05:04:42Z

I tried this out locally, installed a version, and chmod -R a-w on the version dir, and then tried to nvm uninstall it. It then seemed to infinite loop.

lukechilds · 2016-04-18T07:05:13Z

Hmmn, I'll take a look when I get in from work.

lukechilds · 2016-04-18T12:40:23Z

Works fine for me, just tested on a fresh ubuntu vm:

vagrant@vagrant-ubuntu-trusty-64:~$ nvm install 5.10.0
Downloading https://nodejs.org/dist/v5.10.0/node-v5.10.0-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v5.10.0 (npm v3.8.3)
vagrant@vagrant-ubuntu-trusty-64:~$ nvm install 5.10.1
Downloading https://nodejs.org/dist/v5.10.1/node-v5.10.1-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v5.10.1 (npm v3.8.3)
vagrant@vagrant-ubuntu-trusty-64:~$ chmod -R a-w .nvm/versions/node/v5.10.0/
vagrant@vagrant-ubuntu-trusty-64:~$ nvm uninstall 5.10.0
Cannot uninstall, incorrect permissions on installation folder.
This is usually caused by running `npm install -g` as root. Run the following command as root to fix the permissions and then try again.

  chown -R vagrant /home/vagrant/.nvm/versions/node/v5.10.0

I can't see any way it could cause an infinite loop, it's just running find and checking the output. There was a slight lag before the error message though. I suppose if you had a lot of global modules installed find may run for a long time.

Did you have many global modules installed on the version you were uninstalling? Can you try on a freshly installed version?

ljharb · 2016-04-18T15:54:53Z

I was testing this on a fresh install with no additional global modules, on Mac OS.

ljharb · 2016-04-18T15:55:14Z

nvm.sh

+
+      # Check version dir permissions
+      local INVALID_FILES="$(find "$VERSION_PATH" -exec [ ! -w "{}" ] \; -exec echo "{}" \;)"
+      if [ "$INVALID_FILES" ]; then


what are you intending to test here? perhaps you need -z or -n?

$INVALID_FILES contains a list of any files that we don't have write permissions for, so it's just a truthy test. If everything's good it should be an empty variable (falsey).

Works fine as is with sh, bash, zsh and dash but if you think it would help readability I could add in -z.

Maybe, if it's readability you're concerned about, it would be better to rename the var to $FILES_WITHOUT_WRITE_PERMISIONS. Then we could have:

if [ "$FILES_WITHOUT_WRITE_PERMISIONS" ]; then # Error message fi

which I think makes it obvious what's going on. Or is that too verbose?

does it also work with ksh?

I'm just surprised to see a truthiness test in shell scripting :-) won't -z test the same thing, more explicitly?

Yeah it works in ksh:

% ksh $ var="" $ [ $var ] && echo yep || echo nope nope

I also just checked and -z won't actually work, it returns true on an empty string which isn't what I want.

bash-3.2$ var="" bash-3.2$ [ $var ] && echo yep || echo nope nope bash-3.2$ [ -z $var ] && echo yep || echo nope yep

Personally I reckon this is ok but open to suggestions if there's a better way.

ljharb · 2016-04-18T15:55:50Z

nvm.sh

+        >&2 echo 'Cannot uninstall, incorrect permissions on installation folder.'
+        >&2 echo 'This is usually caused by running `npm install -g` as root. Run the following command as root to fix the permissions and then try again.'
+        >&2 echo
+        >&2 echo "  chown -R $(whoami) $VERSION_PATH"


nit: the VERSION_PATH should probably be in quotes, and the command surrounded by backticks, in case it has spaces.

lukechilds · 2016-04-18T16:47:22Z

Just tested it on OS X 10.11.1 and it worked for me.

$ nvm install 4.2.0
Downloading https://nodejs.org/dist/v4.2.0/node-v4.2.0-darwin-x64.tar.gz...
######################################################################## 100.0%
Now using node v4.2.0 (npm v2.14.7)

$ nvm use default
Now using node v5.1.0 (npm v3.3.12)

$ chmod -R a-w ~/.nvm/versions/node/v4.2.0

$ nvm uninstall 4.2.0
Cannot uninstall, incorrect permissions on installation folder.
This is usually caused by running `npm install -g` as root. Run the following command as root to fix the permissions and then try again.

  chown -R lukechilds /Users/lukechilds/.nvm/versions/node/v4.2.0

It did hang for maybe a second or two while it did the find.

It's working on travis with all the different shells so it seems it's an anomaly on your system. Any ideas what it could be? What node version did you test with?

lukechilds · 2016-04-18T21:38:26Z

Ok, I've wrapped the file path in the command output in quotes and I've renamed the file permissions var to make it super clear what's going on.

Also managed to get a proper slow test working that installs a package globally as root and checks for the error message on uninstall. It's working correctly on all shells on Travis.

ljharb · 2016-04-18T23:56:16Z

ok, got it working locally - it hung there for awhile, but worked.
on master:

$ time nvm uninstall 4.2.1
Uninstalled node v4.2.1

real    0m0.255s
user    0m0.051s
sys     0m0.207s

$ time nvm uninstall 4.2.0
Uninstalled node v4.2.0

real    0m0.156s
user    0m0.056s
sys     0m0.103s

(note the silent fast failure on the latter case)

on your branch:

$ time nvm uninstall 4.2.1
Uninstalled node v4.2.1

real    0m3.838s
user    0m1.453s
sys     0m2.255s

$ time nvm uninstall 4.2.0
Cannot uninstall, incorrect permissions on installation folder.
This is usually caused by running `npm install -g` as root. Run the following command as root to fix the permissions and then try again.

  chown -R `whoami` "$NVM_DIR/versions/node/v4.2.0"

real    0m6.965s
user    0m2.801s
sys     0m3.936s

This is a significant slowdown. I'm wondering if it would be better to just try removing, and grep nvm ls for the version number afterwards to determine success?

lukechilds · 2016-04-19T00:29:13Z

I see your point, the lag is less than ideal. Checking after is an option, the only issue is if the user ignores it, or misses it because it's in a script or whatever, nvm ls will still list the version and you can still nvm use it. Then when you try and use node you'll get a missing binary error. That's how I found the bug in the first place, it was pretty confusing. I don't think this scenario should ever be able to happen.

I think the reason it's so slow is because find --exec is spawning a new shell process for every file. I've had a look and I think there is a way I could do it purely in the shell and I think it would be portable. It's not gonna be pretty though.

It would involve replacing:

local FILES_WITHOUT_WRITE_PERMISIONS="$(find "$VERSION_PATH" -exec [ ! -w "{}" ] \; -exec echo "{}" \;)"

with something like:

local PERMISSIONS_OK=true
check_file_permissions() {
  for FILE in $1/* $1/.[!.]* $1/..?* ; do
      if [ -d "$FILE" ]; then
        check_file_permissions "$FILE"
      elif [ -e "$FILE" ]; then
        [ ! -w "$FILE" ] && PERMISSIONS_OK=false
      fi
  done
}
check_file_permissions "$VERSION_PATH"

Worth a shot I suppose, what do you reckon?

Might even be slower 😭🔫

ljharb · 2016-04-19T00:30:00Z

I'd say it's worth doing it in a separate commit, and then we can time it :-)

ljharb · 2016-04-19T00:52:40Z

$ time nvm uninstall 4.2.1
Uninstalled node v4.2.1

real    0m0.514s
user    0m0.275s
sys     0m0.244s

$ time nvm uninstall 4.2.0
Cannot uninstall, incorrect permissions on installation folder.
This is usually caused by running `npm install -g` as root. Run the following command as root to fix the permissions and then try again.

  chown -R `whoami` "$NVM_DIR/versions/node/v4.2.0"

real    0m0.413s
user    0m0.279s
sys     0m0.135s

i think we have a winner. (Let's keep the commit separate, so both this discussion and the relevant code remain in the history)

lukechilds · 2016-04-19T00:53:39Z

Woah, it actually works!

It wasn't quite as slow for me but it's gone from 1.4s to 0.15s:

$ node_dir=/home/vagrant/.nvm/versions/node/v5.10.0/

$ time find "$node_dir" -exec [ ! -w "{}" ] \; -exec echo "{}" \;

real    0m1.420s
user    0m0.090s
sys 0m1.306s

$ time check_file_permissions "$node_dir"

real    0m0.148s
user    0m0.131s
sys 0m0.014s

What do you get?

lukechilds · 2016-04-19T00:55:44Z

Oops, just seen your reply.

Glad this is sorted, only took 7 months!

ljharb · 2016-04-19T00:56:27Z

:-D thanks for the work!

…n a closure. Also, make it return early, to be a bit more efficient. (from #847)

v0.31.1 Changelog New Stuff: - `nvm uninstall`: Check installation dir permissions before uninstalling; display "fix" commands (nvm-sh#847) - `nvm alias`: colorize output to match `nvm ls` - `nvm alias`: colorize output when creating aliases - `nvm ls`/`nvm alias`/`nvm ls-remote`: only colorize when colors are supported Fixes: - don’t use bash `==` in conditionals - `nvm run`: pass through `--silent` on bare `nvm run` - `nvm exec`: show “io.js” for io.js versions - `set -e`: ensure `nvm_version` returning 3, and `nvm_alias` returning 2, doesn’t terminate the process - `nvm alias`: explicitly forbid user aliases in subdirs - `read` exits 1 when `.nvmrc` lacks a trailing newline; avoid that - `set -x`: avoid an unbound variable - `deactivate`: unset `$NVM_BIN` and `$NVM_PATH` (nvm-sh#1033) Performance: - `nvm alias`: slightly speed up alias resolution - Use `awk` to improve version comparison performance Robustness: - add a missing `command` to a `sed` call Misc: - Various README tweaks - Various testing improvements - Prefer `nvm --help` over `nvm help`

lukechilds mentioned this pull request Sep 24, 2015

Clear environment variables when resourcing nvm #849

Closed

ljharb added the needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. label Oct 6, 2015

ljharb added installing node Issues with installing node/io.js versions. needs followup We need some info or action from whoever filed this issue/PR. and removed needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. labels Apr 17, 2016

ljharb reviewed Apr 17, 2016
View reviewed changes

ljharb removed the needs followup We need some info or action from whoever filed this issue/PR. label Apr 18, 2016

ljharb reviewed Apr 18, 2016
View reviewed changes

lukechilds added 4 commits April 18, 2016 21:16

Check dir permissions before attempting to nvm uninstall

005e29a

Add slow test to check nvm uninstall

6fb528f

Add fast test to check nvm uninstall with file permission error

dff4f80

Add slow test to check nvm uninstall with file permission error

5970a06

Check permissions in shell rather than using find -exec

3a2d35f

ljharb merged commit 86c8b11 into nvm-sh:master Apr 19, 2016

ljharb added a commit that referenced this pull request Apr 19, 2016

[Refactor] make check_file_permissions be pure instead of relying o…

755c392

…n a closure. Also, make it return early, to be a bit more efficient. (from #847)

Uh oh!

Check installation dir permissions before uninstalling #847

Check installation dir permissions before uninstalling #847

Uh oh!

Conversation

lukechilds commented Sep 23, 2015

Uh oh!

ljharb commented Sep 23, 2015

Uh oh!

lukechilds commented Sep 23, 2015 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Sep 23, 2015

Uh oh!

lukechilds commented Sep 23, 2015

Uh oh!

ljharb commented Sep 23, 2015

Uh oh!

lukechilds commented Sep 23, 2015

Uh oh!

ljharb commented Sep 23, 2015

Uh oh!

lukechilds commented Sep 23, 2015

Uh oh!

lukechilds commented Sep 23, 2015

Uh oh!

lukechilds commented Apr 16, 2016

Uh oh!

ljharb commented Apr 17, 2016

Uh oh!

lukechilds commented Apr 17, 2016

Uh oh!

ljharb commented Apr 17, 2016

Uh oh!

lukechilds commented Apr 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb Apr 17, 2016

Choose a reason for hiding this comment

Uh oh!

ljharb commented Apr 17, 2016

Uh oh!

lukechilds commented Apr 17, 2016

Uh oh!

ljharb commented Apr 18, 2016

Uh oh!

lukechilds commented Apr 18, 2016

Uh oh!

lukechilds commented Apr 18, 2016

Uh oh!

ljharb commented Apr 18, 2016

Uh oh!

ljharb Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

lukechilds Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

ljharb Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

lukechilds Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

ljharb Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

lukechilds Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

lukechilds commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukechilds commented Apr 18, 2016

Uh oh!

ljharb commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukechilds commented Apr 19, 2016

Uh oh!

lukechilds commented Sep 23, 2015 •

edited

Loading

lukechilds commented Apr 17, 2016 •

edited

Loading

lukechilds commented Apr 18, 2016 •

edited

Loading

ljharb commented Apr 18, 2016 •

edited

Loading