use sh shell instead of Bash

[badersur]

When installing a nightly build using these commands:

wget https://yarnpkg.com/install.sh
chmod +x install.sh
./install.sh --nightly

yarn installs but I get this annoying messages:

./install.sh: 48: ./install.sh: [[: not found
./install.sh: 53: [: unexpected operator

I know a little about shell scripting & this seems to be a bash related thing (and you use bash in your installation script: curl -o- -L https://yarnpkg.com/install.sh | bash)

I'm suggesting changing sh shebang to bash shebang based on This Stack Overflow question.

I'm using Ubuntu 17.04.

[Daniel15]

We can't change it to Bash because then the script won't work on operating systems that don't include Bash by default. Sent from my phone.

…

On Jun 25, 2017 11:38 AM, "Bader Nasser" ***@***.***> wrote: When installing a nightly build <https://yarnpkg.com/en/docs/nightly> using these commands: wget https://yarnpkg.com/install.sh chmod +x install.sh ./install.sh --nightly yarn installs but I get this *annoying* messages: ./install.sh: 48: ./install.sh: [[: not found ./install.sh: 53: [: unexpected operator I know a little about shell scripting & this seems to be a bash related thing <https://stackoverflow.com/questions/669452/is-preferable-over-in-bash> (and you use bash in your installation script <https://yarnpkg.com/en/docs/install#alternatives-tab>: curl -o- -L https://yarnpkg.com/install.sh | bash) I'm suggesting changing sh shebang to bash shebang based on This Stack Overflow question <https://stackoverflow.com/questions/10376206/what-is-the-preferred-bash-shebang> . ------------------------------ You can view, comment on, or merge this pull request online at: #543 Commit Summary - Change Bash shebang File Changes - *M* install.sh <https://github.com/yarnpkg/website/pull/543/files#diff-0> (2) Patch Links: - https://github.com/yarnpkg/website/pull/543.patch - https://github.com/yarnpkg/website/pull/543.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#543>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFnHXpz3IT6atvxSupTgJspp8B7UBxbks5sHqlBgaJpZM4OEruR> .

[Daniel15]

We should remove any bash-isms from the script rather than forcing it to execute using Bash.

[vvo]

My 2 cents: We had to do the same and it's also what does nvm: https://github.com/creationix/nvm/blob/master/install.sh#L1

I would trust nvm as being a good citizen as for the shebang, has been here for a while now.

[Daniel15]

See yarnpkg/yarn#3338 for prior discussion on a similar change in Yarn itself.

[vvo]

Thanks for that detailed link

[badersur]

I didn't know that bash is not available on other OSes! And I thought you are using Bash from the if syntax.

Should I change the script to use sh shell instead? by

1. Changing the shebang to #!/usr/bin/env sh
2. Replacing [[ with [ for if statements

And modifying the installation doc to use curl -o- -L https://yarnpkg.com/install.sh | sh for the stable release and curl -o- -L https://yarnpkg.com/install.sh | sh -s -- --nightly for the nightly build..

Edit:
I changed the script locally and used it to install different versions: nightly, 0.27.0 & 0.24.6 and it's working fine with no issues!

[badersur]

I've found that:

if [ ! "$REPLY" =~ ^[Yy]$ ]; then
    printf "$red> Aborting$reset\n"
    exit 1
fi

uses a bash syntax & I'm not sure how to convert it to sh!

Edit:
It seems there're several ways to express this if statement in sh shell, like:

# if the reply doesn't start with a "y" char
if [ ! $(echo "$REPLY" | grep "^[yY]") ]; then
  printf "$red> Aborting$reset\n"
  exit 1
fi

Or

case $REPLY in [^yY]* )
    printf "$red> Aborting$reset\n"
    exit 1
    ;;
esac

I prefer the case because it looks more readable to me!

The original script will abort if the $REPLY is anything not a "y" or "Y" but I think anything that doesn't start with a "y" is better (in case someone pressed more keys by mistake!)

It seems like these are the only changes required to use sh. Should I push these changes or there are other things to consider?

[badersur]

Theoretically, the script now uses sh syntax but I'm not sure about some code, such as:

if echo $version | grep -qE "^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$"; then

and

if [ $YARN_PROFILE = *"fish"* ]; then

[Daniel15]

Thanks for continuing to work on this 😃

I'm not sure about some code, such as:

if echo $version | grep -qE "^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$"; then

This one is just grep, so it should be fine as long as the installed version of grep supports it.

and

if [ $YARN_PROFILE = *"fish"* ]; then

I don't think this one will work: https://github.com/koalaman/shellcheck/wiki/SC2081

[badersur]

You're welcome :)

I was just worried about the regex inside grep! I still don't know how to use regexes in shell scripting & I agree it should be fine 👍

I liked the shellcheck project so I installed it & added the VSCode extension! I'm planning to implement their suggestions (I've found that my little case is incorrect 😢)

Thanks for letting me know about this amazing tool 😃

I think it'll be nice to add it to your Travis CI script :)

[badersur]

I'm almost done 😃

There're things I'm not sure about but the script is working fine :)

I'll explain my changes after some break 😄

[badersur]

I tried changing printf to use "..%s.." "$var" syntax but it looks ugly, not easy to read and colours' strings are printed on the terminal as is (without affecting the output with the colour):

printf "%s> Version number must match MAJOR.MINOR.PATCH.%s\n" "$red" "$reset

Maybe my code is wrong but I couldn't achieve previous behaviour using printf (and I like to use echo for no obvious reason!)

I don't understand what's the value of YARN_PROFILE="$(yarn_detect_profile)" will be! The function doesn't return a value (and I don't understand the purpose of local & shellcheck warned me about it so I just commented it out 😄)

Also, there's a warning about ${fish_user_paths} which I don't know from where it'll be obtained (seems like an exported variable!)

No matter what I did, sh kept complaining about read -p "$1 [y/N] " -n 1 -r in the last function so I just converted it to echo and got rid of the headache 😸

@Daniel15 I think the changes are complete but maybe testing from users of fish shell would be helpful to double check :)

[badersur]

In other PR, I've noticed that YARN_PROFILE is checked to be empty then printed to the terminal. I've used these steps to test:

I removed my write permission from profile files using:

cd ~; chmod u-w .bashrc .bash_profile .zshrc .profile

and changed the if statements to use -w, then yarn returned:

> Profile not found. Tried **YARN_PROFILE value missing** (as defined in $PROFILE), ~/.bashrc, ~/.bash_profile, ~/.zshrc, and ~/.profile.
> Create one of them and run this script again
> Create it (touch **... missing**) and run this script again
   OR
> Append the following lines to the correct file yourself:

export PATH="$HOME/.yarn/bin:$PATH"

which doesn't seem alright to me! Maybe we should change the message here and update the script to use -w in that PR.

[badersur]

It turns out that echo isn't portable 😭 and I don't know how to work with printf to colourise the output 😦

[badersur]

Hello, Are there any news about this PR? :)

[andersk]

I wrote #851 before noticing this. It ended up being largely the same, but it avoids the non-portable conversion of printf to echo as follows:

  printf "%b> Downloading tarball...%b\n" "$cyan" "$reset"

and it avoids the case bug as follows:

  case "$REPLY" in
    Y|y)
      ;;
    *)
      printf "%b> Aborting%b\n" "$red" "$reset"
      exit 1
      ;;
  esac