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

mac big sur uses zsh #2300

Closed
wants to merge 1 commit into from
Closed

Conversation

8secz-johndpope
Copy link

No description provided.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This won't actually work, because the install script is written to only work in bash.

It must always be used with | bash, regardless of what your current shell is.

@ljharb ljharb added the installing nvm Problems installing nvm itself label Sep 11, 2020
@8secz-johndpope
Copy link
Author

it does seem to correctly target the updates necessary for .zshrc file though.

@ljharb
Copy link
Member

ljharb commented Sep 12, 2020

In that case, I'd prefer something more generic like | $SHELL, and then we'd have to rewrite the install script to work in POSIX.

What does your echo $SHELL say prior to running the install script?

@8secz-johndpope
Copy link
Author

echo $SHELL
/bin/zsh

@ljharb
Copy link
Member

ljharb commented Sep 12, 2020

In that case, I'd expect #2260 to address it a bit?

@8secz-johndpope
Copy link
Author

@ljharb
Copy link
Member

ljharb commented Sep 12, 2020

If in fact it already works in all supported shells, I'd be fine changing the | bash line to something like | "${SHELL:-bash}, and also adding test runs to CI for those use cases.

@hinell
Copy link

hinell commented Nov 19, 2020

The installation script explicitly states that it uses bash (shebang line), so piping it into zsh makes little sense, so does adding it to the docs. As I stated in related PR Switching the shell to something other than bash risks breaking backward compatibility.

nvm/install.sh

Lines 1 to 2 in 21c0c05

#!/usr/bin/env bash

@sladyn98
Copy link
Contributor

The installation script explicitly states that it uses bash (shebang line), so piping it into zsh makes little sense, so does adding it to the docs. As I stated in related PR Switching the shell to something other than bash risks breaking backward compatibility.

@ljharb in view of this, would we like to close this ? or would we still want to pursue a PR in which we pipe it with other shells, although it might risk the backward compatibility

@ljharb
Copy link
Member

ljharb commented Mar 26, 2021

Yes, I think the proper solution is to improve the install script's shell detection behavior.

@ljharb ljharb closed this Mar 26, 2021
@ljharb ljharb mentioned this pull request Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installing nvm Problems installing nvm itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants