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

Jump to bottom

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b2a4377
[Fix] `install`: detect user shell and try shellrc file first
steelcowboy Jul 18, 2020
d01b62b
[Fix] `install`: detect user shell and try shellrc file first
steelcowboy Jul 18, 2020
78933af
Merge branch 'install-script-fix-zsh' of github.com:steelcowboy/nvm i…
steelcowboy Jul 21, 2020
3cdedc9
condense logic
steelcowboy Jul 21, 2020
9ffd330
Better shell detection, and better test handling
steelcowboy Aug 9, 2020
2743261
unneccessary check for empty string
steelcowboy Aug 9, 2020
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
54 changes: 46 additions & 8 deletions install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,16 +233,54 @@ nvm_detect_profile() {
local DETECTED_PROFILE
DETECTED_PROFILE=''

if [ -n "${BASH_VERSION-}" ]; then
if [ -f "$HOME/.bashrc" ]; then
DETECTED_PROFILE="$HOME/.bashrc"
elif [ -f "$HOME/.bash_profile" ]; then
DETECTED_PROFILE="$HOME/.bash_profile"
fi
elif [ -n "${ZSH_VERSION-}" ]; then
DETECTED_PROFILE="$HOME/.zshrc"
# Detect the user's login shell
local USER_SHELL
local USER_SHELL_NAME
USER_SHELL=''

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

Choose a reason for hiding this comment

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

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

USER_SHELL=$(getent passwd $(whoami) | cut -d: -f7)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. Make all supported shells work on Linux
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

elif [ -n "$SHELL" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

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

USER_SHELL="$SHELL"
fi

USER_SHELL_NAME="${USER_SHELL##*/}"

# First try to find the config file based on the shell name
if [ -n "${USER_SHELL_NAME}" ]; then
case "${USER_SHELL_NAME}" in
bash)
if [ -f "$HOME/.bashrc" ]; then
DETECTED_PROFILE="$HOME/.bashrc"
elif [ -f "$HOME/.bash_profile" ]; then
DETECTED_PROFILE="$HOME/.bash_profile"
fi
;;
zsh)
if [ -f "$HOME/.zshrc" ]; then
DETECTED_PROFILE="$HOME/.zshrc"
elif [ -f "$HOME/.zprofile" ]; then
DETECTED_PROFILE="$HOME/.zprofile"
fi
;;
ksh*)
if [ -f "$HOME/.kshrc" ]; then
DETECTED_PROFILE="$HOME/.kshrc"
fi
;;
mksh)
if [ -f "$HOME/.mkshrc" ]; then
DETECTED_PROFILE="$HOME/.mkshrc"
fi
;;
*)
;;
esac
fi

# Now brute force
if [ -z "$DETECTED_PROFILE" ]; then
for EACH_PROFILE in ".profile" ".bashrc" ".bash_profile" ".zshrc"
do
Expand Down
22 changes: 19 additions & 3 deletions test/install_script/nvm_detect_profile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

setup () {
HOME="."
SHELL='/bin/bash'
NVM_ENV=testing \. ../../install.sh
NVM_TESTING='yes'
touch ".bashrc"
touch ".bash_profile"
touch ".zshrc"
Expand All @@ -12,6 +14,7 @@ setup () {

cleanup () {
unset HOME
unset SHELL
unset NVM_ENV
unset NVM_DETECT_PROFILE
unset BASH_VERSION
Expand All @@ -34,6 +37,12 @@ if [ -n "$NVM_DETECT_PROFILE" ]; then
die "nvm_detect_profile still detected a profile even though PROFILE=/dev/null"
fi

# .bashrc should be detected if the shell is bash
NVM_DETECT_PROFILE="$(unset PROFILE; nvm_detect_profile)"
if [ "$NVM_DETECT_PROFILE" != "$HOME/.bashrc" ]; then
die "nvm_detect_profile didn't pick \$HOME/.bashrc for bash"
fi

# .bashrc should be detected for bash
NVM_DETECT_PROFILE="$(BASH_VERSION="1"; unset PROFILE; nvm_detect_profile)"
if [ "$NVM_DETECT_PROFILE" != "$HOME/.bashrc" ]; then
Expand All @@ -46,8 +55,14 @@ if [ "$NVM_DETECT_PROFILE" != "test_profile" ]; then
die "nvm_detect_profile ignored \$PROFILE"
fi

# .zshrc should be detected if the shell is zsh
NVM_DETECT_PROFILE="$(SHELL="/bin/zsh"; unset PROFILE; unset BASH_VERSION; nvm_detect_profile)"
if [ "$NVM_DETECT_PROFILE" != "$HOME/.zshrc" ]; then
die "nvm_detect_profile didn't pick \$HOME/.zshrc for zsh"
fi

# .zshrc should be detected for zsh
NVM_DETECT_PROFILE="$(ZSH_VERSION="1"; unset PROFILE; unset BASH_VERSION; nvm_detect_profile)"
NVM_DETECT_PROFILE="$(SHELL="/bin/zsh"; ZSH_VERSION="1"; unset PROFILE; unset BASH_VERSION; nvm_detect_profile)"
if [ "$NVM_DETECT_PROFILE" != "$HOME/.zshrc" ]; then
die "nvm_detect_profile didn't pick \$HOME/.zshrc for zsh"
fi
Expand Down Expand Up @@ -82,7 +97,8 @@ fi
# return an empty value if everything fails
#

# It should favor .profile if file exists
# It should favor .profile if other detection methods fail and file exists and
SHELL="nonsense"
NVM_DETECT_PROFILE="$(unset BASH_VERSION; unset ZSH_VERSION; nvm_detect_profile)"
if [ "$NVM_DETECT_PROFILE" != "$HOME/.profile" ]; then
die "nvm_detect_profile should have selected .profile"
Expand All @@ -104,7 +120,7 @@ fi

# Otherwise, it should favor .zshrc if file exists
rm ".bash_profile"
NVM_DETECT_PROFILE="$(unset BASH_VERSION; unset ZSH_VERSION; nvm_detect_profile)"
NVM_DETECT_PROFILE="$(unset BASH_VERSION; unset ZSH_VERSION; unset SHELL; nvm_detect_profile)"
if [ "$NVM_DETECT_PROFILE" != "$HOME/.zshrc" ]; then
die "nvm_detect_profile should have selected .zshrc"
fi
Expand Down
2 changes: 1 addition & 1 deletion test/install_script/nvm_install_with_aliased_dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
steelcowboy marked this conversation as resolved.
Show resolved Hide resolved
#!/bin/bash

setup () {
shopt -s expand_aliases
Expand Down