Skip to content

Conversation

tlevine
Copy link
Contributor

@tlevine tlevine commented Feb 8, 2016

My bash is not installed in /bin/bash.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2016

@tlevine Thanks - should this also be done in the install script?

@hlechner
Copy link

On FreeBSD it's not on /bin/bash too, The scripts that I made for Linux and FreeBSD uses this approach too. /usr/bin/env bash

But I think it should be done in the rest of files that uses /bin/bash

@tlevine
Copy link
Contributor Author

tlevine commented Feb 14, 2016

I see but one more file that uses /bin/bash.

$ grep -r '#!/bin/bash' test/
test/fast/Unit tests/nvm_ls_current:echo "#!/bin/bash" > "$TEST_DIR/node"

To my surprise, when I change this to /bin/sh or to
/usr/bin/env bash, tests pass only when run in bash.

$ urchin -t -s bash test/fast/Unit\ tests | grep nvm_ls_current
ok 28 - nvm_ls_current
$ urchin -t -s dash test/fast/Unit\ tests | grep nvm_ls_current
not ok 28 - nvm_ls_current

I suggest that we merge the changes that I have already made before we deal with this last file.

@ljharb
Copy link
Member

ljharb commented Feb 14, 2016

@tlevine what about install.sh?

@hlechner
Copy link

@tlevine This approach should be used on the nvm script files too not just the test ones.

So, here the files that still use #!/bin/bash:

  • install.sh
  • nvm-exec

Thanks mate.

@tlevine
Copy link
Contributor Author

tlevine commented Feb 14, 2016

Oh hah I was only looking at the tests.

install.sh and nvm-exec have #!/bin/bash, so I just changed them in the env-bash branch.

In case anyone was curious, #!/bin/sh doesn't work; it has to be bash.

@tlevine
Copy link
Contributor Author

tlevine commented Feb 14, 2016

I sent the previous comment before reading Henrique's.
I independently itemized the same two files as Henrique, and this gives me some confidence that we have really found all of them.

On the other hand, I have now found two more uses of /bin/bash.

./test/install_script/nvm_detect_profile:NVM_DETECT_PROFILE="$(SHELL="/bin/bash"; unset PROFILE; nvm_detect_profile)"
./test/install_script/nvm_detect_profile:NVM_DETECT_PROFILE="$(SHELL="/bin/bash"; PROFILE="test_profile"; nvm_detect_profile)"

@hlechner
Copy link

Thanks!

Just to comment about the bash vs sh. some distro have the symlink sh -> bash

So it's possible that a script that uses sh act different in different distros.

In my personal case, I always avoid use sh instead of bash

@tlevine
Copy link
Contributor Author

tlevine commented Feb 14, 2016

I ran the tests with different shells (with urchin's -s flag) to determine that sh wouldn't work. This is sort of like having /bin/sh linked to different shells.

@ljharb
Copy link
Member

ljharb commented Feb 14, 2016

@tlevine thanks, this looks great! any chance you'd mind rebasing it on top of the latest master?

@tlevine
Copy link
Contributor Author

tlevine commented Feb 14, 2016

Oops

No chance I would mind

I have rebased.

@tlevine
Copy link
Contributor Author

tlevine commented Feb 14, 2016

Oh and I should note: Some tests in test/installation/io.js/ fail for me in both master and in the rebased branch.
But I think it's the same tests.

ljharb added a commit that referenced this pull request Feb 14, 2016
[Fix] use env bash rather than /bin/bash
@ljharb ljharb merged commit dc9020b into nvm-sh:master Feb 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants