[Fix] `install`: detect user shell and try shellrc file first #2260

steelcowboy · 2020-07-18T19:29:35Z

Situation

Currently, the nvm install script has a specific order in which it looks for files:

.bashrc
.bash_profile
.zshrc

However, this doesn't take into account any kind of user preference. I suggest we first try and see if we can find the user's shell and determine a good RC file to use based on that. If not, we fall back to the current behavior.

Tests

> [email protected] test/install_script /home/steelcowboy/github/nvm
> shell=$(basename -- $(ps -o comm= $(ps -o ppid= -p $PPID)) | sed 's/^-//'); make TEST_SUITE=install_script test-$shell


Running tests in zsh
Running tests at 2020-08-08T18:49:44
  test/install_script
  ✓ nvm_check_global_modules
  ✓ nvm_detect_profile
  ✓ nvm_do_install
  ✓ nvm_install_dir
  ✓ nvm_install_with_aliased_dot
  ✓ nvm_install_with_node_version
  ✓ nvm_profile_is_bash_or_zsh
  ✓ nvm_reset
  ✓ nvm_source

Done, took 11 seconds.
9 tests passed.
0 tests failed.

I had to change the shell on nvm_install_with_aliased_dot to bash in order to use shopt, open to a different approach though!

Current Behavior

steelcowboy@asclepius:~/bin|⇒  echo $SHELL
/bin/zsh
steelcowboy@asclepius:~/bin|⇒  bash -c 'echo $SHELL'
/bin/zsh
steelcowboy@asclepius:~/bin|⇒  ./nvm-install.sh 
=> Downloading nvm from git to '/home/steelcowboy/.nvm'
=> Cloning into '/home/steelcowboy/.nvm'...
...
Resolving deltas: 100% (35/35), done.
=> Compressing and cleaning up git repository

=> Appending nvm source string to /home/steelcowboy/.bashrc
=> Appending bash_completion source string to /home/steelcowboy/.bashrc
=> Close and reopen your terminal to start using nvm or run the following to use it now:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion

New Behavior

steelcowboy@asclepius:~/github/nvm|install-script-fix-zsh ⇒  curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.3/install.sh | bash
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 13527  100 13527    0     0  53678      0 --:--:-- --:--:-- --:--:-- 53678
=> Downloading nvm from git to '/home/steelcowboy/.nvm'
=> Cloning into '/home/steelcowboy/.nvm'...
...
Resolving deltas: 100% (35/35), done.
=> Compressing and cleaning up git repository

=> Appending nvm source string to /home/steelcowboy/.zshrc
=> Appending bash_completion source string to /home/steelcowboy/.zshrc
=> Close and reopen your terminal to start using nvm or run the following to use it now:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion

Docker Testing

bash

https://gist.github.com/steelcowboy/451d45ae6a1728dba7678ce2e68dbcc4

zsh

https://gist.github.com/steelcowboy/1d7ac8aed8e7b6a275aa1d35ac43cd1a

dash

https://gist.github.com/steelcowboy/1af21c23cac3208b9dc77676475e65d3

ksh

https://gist.github.com/steelcowboy/5b62b7ac44c7e425c504a86ba511d1dc
Note: It doesn't like the local in nvm.sh, but this is a known issue: #574

mksh

https://gist.github.com/steelcowboy/5b62b7ac44c7e425c504a86ba511d1dc
Interestingly unlike AT&T ksh, mksh is perfectly happy with the local variables :)

MacOS Issue

Resolves #592, but #2148 still remains because Catalina doesn't create any default .zshrc it would seem. If we wanted to take the spirit of this change further, we could simply touch $SHELLRC because if your shell is set I'd imagine there's a high likelihood you want an rc file for it :)

I think if we do want to go that direction, the best approach is to do it in another PR to keep the changes well-scoped.

install.sh

test/install_script/nvm_install_with_aliased_dot

…nto install-script-fix-zsh Rebasing on remote

steelcowboy · 2020-07-26T23:41:50Z

Hey @ljharb, I accepted a few of your suggestions and added some context for the sh-bash change. What more do we need to move this forward?

ljharb · 2020-07-30T06:24:23Z

@steelcowboy sorry for the delay; replied to your comment.

steelcowboy · 2020-08-09T01:33:36Z

Hey @ljharb, I just made a few changes:

I updated shell detection to use getent passwd and fall back to the $SHELL variable -- otherwise we fall back to brute forcing it as before
I added a testing variable to make it always use $SHELL for testing, as that's easier to mock than getent
I tested against 5 different shells with Docker containers, using a minimal Ubuntu image as my base

Let me know what you think!

ljharb · 2020-08-09T21:08:25Z

What is getent? Is it in POSIX and thus on all supported systems? My Mac doesn't have it.

ljharb

I try to make "testing" variables only determine whether a command runs or not, kind of like a "dry run" - this one actually follows a different code path which seems less than ideal.

ljharb · 2020-08-09T21:08:44Z

install.sh

+
+  # If we're not testing, try to get shell from passwd
+  # Otherwise, try the SHELL variable
+  if [ "$NVM_TESTING" != 'yes' ]; then


Suggested change

if [ "$NVM_TESTING" != 'yes' ]; then

if [ "${NVM_TESTING-}" != 'yes' ]; then

ljharb · 2020-08-09T21:09:24Z

install.sh

+  # Otherwise, try the SHELL variable
+  if [ "$NVM_TESTING" != 'yes' ]; then
+    USER_SHELL=$(getent passwd $(whoami) | cut -d: -f7)
+  elif [ -n "$SHELL" ]; then


Suggested change

elif [ -n "$SHELL" ]; then

elif [ -n "${SHELL-}" ]; then

ljharb · 2020-08-09T21:10:11Z

install.sh

+  # If we're not testing, try to get shell from passwd
+  # Otherwise, try the SHELL variable
+  if [ "$NVM_TESTING" != 'yes' ]; then
+    USER_SHELL=$(getent passwd $(whoami) | cut -d: -f7)


When I run $(getent passwd $(whoami) | cut -d: -f7) on my Mac, I get -bash: getent: command not found on stderr, which doesn't seem like something we want. What is getent?

Shoot yeah I meant to see if getent exists on Mac, apparently it doesn't :/ Back to the drawing board!

So looking at it, there doesn't seem to be a standard POSIX way to do this -- and the current behavior of checking various shell versions certainly isn't POSIX, as there is no $DASH_VERSION for example.

As such, I propose we do this: getent is the most reliable way on Linux machines, and it's quite standard. We could go one step further and just grep $(whoami) on /etc/passwd if we want to avoid getent.

On MacOS, I think the best we can do is hope the $SHELL variable is accurate because Apple. MacOS apparently does its own thing for storing user info, so we could either write in logic to use getent on Linux and whatever MacOS-equivalent on MacOS, or we can see if getent exists, use it if possible, fall back to $SHELL, and finally fall back to brute forcing. Either way, I'm confident that doing that will:

Make all supported shells work on Linux

Support at LEAST bash and zsh on MacOS, except there's still the remaining issue of MacOS not creating zshrc :) But again, that's a hurdle for a different PR

What do you think?

I'm fine with trying a bunch of different things in the install script, as long as they fail gracefully on systems where they don't work.

Got it! So getent obviously works reliably on Linux, but I'm exploring a more cross-platform solution of simply seeing what the parent process of the script is :) I think it's quite reasonable, because it's most likely that people would be installing nvm from their default shell.

Furthermore, if you decide to start a new bash session when your default shell is zsh simply to install nvm, I think it's fair to say it's the user's responsibility at that point since my impression is that opening a different type of shell just to install something is pretty non-standard. We can document it to say that running the script will "install nvm for the currently running shell" too so the behavior doesn't surprise anyone.

steelcowboy · 2020-08-10T05:48:03Z

That makes sense on testing variables! The issue here we need to resolve is that we're trying to learn about system state in a testing environment, which makes sense if we can just pass a variable (e,.g. SHELL) but it's a lot harder if we want to mock, for example, /etc/passwd. So if we want to trust some system file over $SHELL, I think for the unit tests we'll have to just ignore that code path or overengineer the test case.

Do you see a better way forward?

ljharb · 2020-08-10T05:51:50Z

I'd focus on what makes the production code correct, and I'm happy to help figure out how to mock it properly, rather than thinking about tests when writing the real code :-)

tg90nor · 2021-08-12T12:55:03Z

Can't you just look at the $SHELL variable @steelcowboy ?

[~] ❯ bash -c 'echo $BASH_VERSION'
3.2.57(1)-release
[~] ❯ bash -c 'echo $ZSH_VERSION'

[~] ❯ echo $ZSH_VERSION
5.8
[~] ❯ bash -c 'echo $SHELL'
/usr/local/bin/zsh

This is on MacOS, but I expect it would work the same on Linux

Edit: Didn't see how old this PR was. Guess this is abandoned and I should make a new PR.
Edit2: Made a new PR with the more naive $SHELL detection approach. #2556

[Fix] install: detect user shell and try shellrc file first

b2a4377

ljharb requested changes Jul 19, 2020

View reviewed changes

install.sh Outdated Show resolved Hide resolved

install.sh Outdated Show resolved Hide resolved

test/install_script/nvm_install_with_aliased_dot Show resolved Hide resolved

ljharb added the installing nvm: profile detection Issues with nvm's own install script, related to figuring out the right profile file. label Jul 19, 2020

steelcowboy added 3 commits July 20, 2020 18:46

[Fix] install: detect user shell and try shellrc file first

d01b62b

Merge branch 'install-script-fix-zsh' of github.com:steelcowboy/nvm i…

78933af

…nto install-script-fix-zsh Rebasing on remote

condense logic

3cdedc9

Better shell detection, and better test handling

9ffd330

unneccessary check for empty string

2743261

ljharb requested changes Aug 9, 2020

View reviewed changes

ljharb marked this pull request as draft August 10, 2020 05:48

ljharb mentioned this pull request Sep 12, 2020

mac big sur uses zsh #2300

Closed

ljharb force-pushed the master branch from a09e064 to 49e806c Compare February 1, 2021 22:36

ljharb force-pushed the master branch from d3710f8 to 07253ec Compare April 16, 2021 05:45

ljharb force-pushed the master branch from 7846e33 to 1f970cc Compare November 1, 2023 22:11

ljharb force-pushed the master branch 2 times, most recently from c6cfc3a to c20db2a Compare June 10, 2024 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] `install`: detect user shell and try shellrc file first #2260

[Fix] `install`: detect user shell and try shellrc file first #2260

steelcowboy commented Jul 18, 2020 •

edited

Loading

steelcowboy commented Jul 26, 2020

ljharb commented Jul 30, 2020

steelcowboy commented Aug 9, 2020

ljharb commented Aug 9, 2020

ljharb left a comment

ljharb Aug 9, 2020

ljharb Aug 9, 2020

ljharb Aug 9, 2020

steelcowboy Aug 10, 2020

steelcowboy Aug 10, 2020

ljharb Aug 10, 2020

steelcowboy Aug 15, 2020

steelcowboy commented Aug 10, 2020

ljharb commented Aug 10, 2020

tg90nor commented Aug 12, 2021 •

edited

Loading

	if [ "$NVM_TESTING" != 'yes' ]; then
	if [ "${NVM_TESTING-}" != 'yes' ]; then

[Fix] install: detect user shell and try shellrc file first #2260

Are you sure you want to change the base?

[Fix] install: detect user shell and try shellrc file first #2260

Conversation

steelcowboy commented Jul 18, 2020 • edited Loading

Situation

Tests

Current Behavior

New Behavior

Docker Testing

bash

zsh

dash

ksh

mksh

MacOS Issue

steelcowboy commented Jul 26, 2020

ljharb commented Jul 30, 2020

steelcowboy commented Aug 9, 2020

ljharb commented Aug 9, 2020

ljharb left a comment

Choose a reason for hiding this comment

ljharb Aug 9, 2020

Choose a reason for hiding this comment

ljharb Aug 9, 2020

Choose a reason for hiding this comment

ljharb Aug 9, 2020

Choose a reason for hiding this comment

steelcowboy Aug 10, 2020

Choose a reason for hiding this comment

steelcowboy Aug 10, 2020

Choose a reason for hiding this comment

ljharb Aug 10, 2020

Choose a reason for hiding this comment

steelcowboy Aug 15, 2020

Choose a reason for hiding this comment

steelcowboy commented Aug 10, 2020

ljharb commented Aug 10, 2020

tg90nor commented Aug 12, 2021 • edited Loading

[Fix] `install`: detect user shell and try shellrc file first #2260

[Fix] `install`: detect user shell and try shellrc file first #2260

steelcowboy commented Jul 18, 2020 •

edited

Loading

tg90nor commented Aug 12, 2021 •

edited

Loading