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.

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

Do not fail .bashrc if nvm is not installed #1805

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@lifeofguenter
Copy link

commented May 4, 2018

Currently installing nvm might add something like this into your .bashrc:

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

This will unfortunately cause bash to spawn with a return-code of 1.

My fix will solve this issue and make sure that it will work if those files exist, and that it won't throw an error should they not exist:

    [ ! -s ~/.bashrc ] || echo yes; echo $? # file exists, outputs "yes", rc=0
yes
0
    [ ! -s ~/.bashrrc ] || echo yes; echo $? # file does not exist, outputs nothing, rc=0
0
    [ -s ~/.bashrc ] && echo yes; echo $? # file exists, outputs "yes", rc=0
yes
0
    [ -s ~/.bashrrc ] && echo yes; echo $? # file does not exist, outputs nothing, rc=1
1
@ljharb

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2018

Wouldn't that only happen if you were using set -e?

I'm very hesitant to change one of the most core parts of using nvm - something that's worked for years - without a very good reason, and using set -e in your shell profile files wouldn't be one of them.

@lifeofguenter

This comment has been minimized.

Copy link
Author

commented May 4, 2018

@ljharb nope I do not explicitly do set -e in my shell profile, and unfortunately this also happens with set +e

I am running a pretty recent bash version on macOS, so maybe something changed:

~  echo "${BASH_VERSION}"
4.4.18(1)-release

Script:

#!/usr/bin/env bash

#set +e (uncommenting this gives same result)
[ -s ~/.bbashrc ] && echo yes

Output:

~  ./foo.sh; echo $?
1

Would be interesting to know if it really has been working for years, or just went unnoticed because in the majority of cases if that line were in your .bashrc you'd also have nvm installed? Especially because you'd not notice it right-away, except if you directly do echo $? after opening a new shell (or with me I notice it because I use https://github.com/riobard/bash-powerline)

~  bar && echo foo; echo $?
-bash: bar: command not found
127

vs.

if [[ -s foo ]] && [[ -s bar ]]; then echo yes; else echo no; fi
no (return-code = 0)
@ljharb

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2018

I have 3.2.57(1)-release (whatever comes with my Mac) and I definitely see the same thing.

My question is, why does it matter, exactly? The OS is what sources your profile files, and a nonzero return doesn't seem to cause any problems that I can see (other than my prompt, which shows the exit code, showing a 1).

@lifeofguenter

This comment has been minimized.

Copy link
Author

commented May 4, 2018

My question is, why does it matter, exactly

other than my prompt, which shows the exit code, showing a 1

Exactly that is the reason :)

  • cleaner implementation
  • it makes debugging bashrc far more easier, e.g. lets say something is legitimately failing and I want to figure out - if everything else is implemented non-strict I can forget debugging properly..
  • I like my shell starting green :)
  • there might maybe be some situations where one would directly source the bashrc? (ci-server)

I suppose there is no big reason, the PR would make things cleaner though? And might help someone else who stumbled on exactly this issue (like me) - I mean at the end the current code is not readable, as I and you stumbled on wrong assumption of the outcome...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.