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

[steelcowboy]

Situation

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

1. .bashrc
2. .bash_profile
3. .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.

[steelcowboy]

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]

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

[steelcowboy]

Hey @ljharb, I just made a few changes:

1. 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
2. I added a testing variable to make it always use $SHELL for testing, as that's easier to mock than getent
3. I tested against 5 different shells with Docker containers, using a minimal Ubuntu image as my base

Let me know what you think!

[ljharb]

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

[steelcowboy]

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]

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]

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