feat(install): make install script posix compliant

[dmccaffery]

Description

• update shebang to support posix-compliant shells (like dash)
• replace all usages of echo with printf (to normalise)
• remove locals, which are not posix complaint
• rename command variable to fetch_cmd as it overloads a posix builtin
• rename complete function to completed as it overloads the bash builtin for completion

Motivation and Context

Bash should not be necessary to run the install script on systems that would otherwise not require it. Ubuntu uses dash by default and I use zsh, so no reason to install bash before installing starship. :)

Closes #2226

Screenshots (if appropriate):

How Has This Been Tested?

Ran install scripts on macOS natively, as well as the following docker containers for linux distributions (not the same as native, but as close as I can get with ease):

• debian
• fedora
• centos
• linuxmint
• ubuntu

• I have tested using MacOS
• I have tested using Linux
• I have tested using Windows

Checklist:

• (not relevant) I have updated the documentation accordingly.
• (not relevant) I have updated the tests accordingly.

[davidkna]

I don't really see the value of removing local just for the sake of POSIX compliance, because I can't think of any shell that doesn't support it. Even busybox has support for it. (More elaborate comment on stackoverflow).

[dmccaffery]

Agreed. I removed them because I didn't think dash supported it (default shell in Ubuntu), but will verify.

That being said, they are only global in process and the install script is invoked; not evaluated.

[dmccaffery]

I just realised that modern versions of curl will fail with binary files when not used with a pipe or --output. I tried to use --output - to no avail; not sure how to resolve without dramatically modifying the fetch logic to not write to the stdout buffer.

[vladimyr]

I guess it is time to close #1315

[davidkna]

@vladimyr Maybe. I recently duplicated a lot of your changes from there so merge conflicts are likely significant. But if you still want to get any of the other changes from that PR merged please rebase it.

@dmccaffery Would you please port over download and unpack from #1315 to make sure error handling without pipefail still works?

[dmccaffery]

@davidkna thanks and will do -- apologies for the delay; I've been busy.

@vladimyr -- I'm so sorry I didn't see your PR before opening this one. I did a search for an issue but didn't think to scan current PRs!

[dmccaffery]

@davidkna I made the requested changes and also added a new GitHub Actions workflow for testing the install across a wide array of environments, including Windows WSL. This is isolated on the last commit in case you do not want to incorporate it.

Let me know if there is anything else you need!

Link to latest "Installation" workflow: https://github.com/starship/starship/actions/runs/599593982

[davidkna]

I think it might be better to move the testing changes to a separate PR so this can be merged quickly.

[dmccaffery]

Agreed. I'll pull the testing out and open a separate PR.

[dmccaffery]

@davidkna : removed the install tests, which have been reopened in a WIP/draft PR for now: #2406 -- also made the requested change to the download function, so should be ready for a final review. Thanks!

[davidkna]

LGTM

[chipbuster]

@dmccaffery Thank you for your contributions, and thanks @davidkna for reviewing!

[vladimyr]

@vladimyr -- I'm so sorry I didn't see your PR before opening this one. I did a search for an issue but didn't think to scan current PRs!

@dmccaffery No worries I didn't post my comment to criticize but to spare you some time dealing with conversion. I hope it was helpful and you found useful stuff over there 🤞

@vladimyr Maybe. I recently duplicated a lot of your changes from there so merge conflicts are likely significant.

@davidkna Let's put it this way. I'm in no way emotionally attached to my code but what is the point of duplicating? You could simply continue on top of it or at least copy it completely. Doing neither leads to the omission of important stuff like:

• support PREFIX environment variable

Added support for PREFIX environment variable which enables the following standardish invocation style:

curl -fsSL https://starship.rs/install.sh | PREFIX=$HOME/.local sh
# starship ends up inside $HOME/.local/bin

discussed in detail here: #1315 (comment)

But yeah, you gave me an interesting option:

But if you still want to get any of the other changes from that PR merged please rebase it.

🤐 ...

Let's get back to the technical track...

@davidkna said:

I don't really see the value of removing local just for the sake of POSIX compliance, because I can't think of any shell that doesn't support it. Even busybox has support for it. (More elaborate comment on stackoverflow).

1. SO user literally mentioned ksh and non-GNU/non-BSD systems as potential deal-breakers:

   So unless you're aiming to support non-GNU non-BSD systems like Solaris, or those using standard ksh, etc., you could get away with using local. (Might want to put some comment right at the start of the script, below the shebang line, noting that it is not strictly a POSIX sh script. Just to be not evil.)

   Even though I do agree it is very unlikely to get a bug report from the latter I wouldn't be so surprised to see a ksh user around.

2. The fact that you could get away with it doesn't mean you should! To prove my point let me show you Docker's install script: https://get.docker.com Try to find some locals inside 😉

3. There are reasons why local isn't standardized by POSIX and while following that debian.org discussion link gives you some insight here is the link to the actual Open Group bug tracker https://www.austingroupbugs.net/bug_view_page.php?bug_id=767
   Once you scratch under the surface you figure out that the answer to Are there any shells out there that don't support local? is not that important because that is not the right question to ask in the first place. As this SE user pointed out:

   It's not as simple as supporting local or not.

   https://unix.stackexchange.com/questions/493729/list-of-shells-that-support-local-keyword-for-defining-local-variables

4. Regardless of what you end up doing disabling SC2039 is wrong thing to do! First of all, it is an umbrella warning so you just disabled a whole bunch of other helpful warnings plus that is the exact reason why it is deprecated:

   Note: This warning has been retired in favor of individual SC3xxx warnings for each individual issue.

   https://github.com/koalaman/shellcheck/wiki/SC2039 Unfortunately, that change is relatively new (https://github.com/koalaman/shellcheck/wiki/SC2039/_compare/e241394...6c9fa14 & https://github.com/koalaman/shellcheck/wiki/SC2039/6c9fa14) so those SC3xxx rules haven't been implemented yet. So you either live with shellcheck yelling at you or you trade safety for commodity.

Finally, regardless of all that thanks both of you for your contribution and for moving things forward! 👍

[vladimyr]

FWIW Once the new ShellCheck build gets released we could disable SC3043 only 🎉
https://github.com/koalaman/shellcheck/blob/81acea0/src/ShellCheck/Checks/ShellSupport.hs#L339-L345

[vladimyr]

I wouldn't be so surprised to see a ksh user around.

To further strengthen my argument about ksh's local support let's take a look at the following. I have Termux installed on my phone and I'm using zsh with starship OFC:

~
❯ echo $ZSH_VERSION
5.8

~
❯ type local
local is a reserved word

Switching quickly to bash gives the following output:

~ $ echo $BASH_VERSION
5.1.4(1)-release
~ $ type local
local is a shell builtin

But Android (11) ships with mksh (MirBSD Korn Shell) (R57): https://android.googlesource.com/platform/external/mksh/+/refs/heads/android11-release/src/sh.h#187
So let's switch again to /system/bin/sh to check its local support:

P2a42:/ $ echo $KSH_VERSION
@(#)MIRBSD KSH R52 2016/01/20
P2a42:/ $ type local
local is an alias for '\typeset'

Apart from the fact it reveals I'm using a bit older Android OS/device 🙃, this is still a fairly modern ksh clone and yet local is defined as an alias! ⚠️

[davidkna]

What is the point of duplicating?

I wasn't aware of your PR.

I wouldn't be so surprised to see a ksh user around.

For the use here (locals in functions) it looks like the implementation differences don't matter at all. The only issue is standard ksh which doesn't support local in posix functions and I agree it would be worthwhile to support that shell.

[dmccaffery]

@davidkna / @vladimyr :

Should I remove the locals again? I don't think it will have any side effects. No one should be sourcing the install script. When executed, the scope is the forked process of the sub shell, so there is no need for locals anyway (as far as I know).

[vladimyr]

I wasn't aware of your PR.

Ah, too bad you missed it. However, one way or another installer is finally (almost) POSIX compliant 🎉

The only issue is standard ksh which doesn't support local in posix functions and I agree it would be worthwhile to support that shell.

Should I remove the locals again? I don't think it will have any side effects. No one should be sourcing the install script. When executed, the scope is the forked process of the sub shell, so there is no need for locals anyway (as far as I know).

It seems we have a consensus here. ksh is a worthwhile target to support and, as @dmccaffery nicely explained, ensuring local scope isn't that crucial under terms of preferred/advertised use case. So, @dmccaffery I'm sorry for expanding your todo list 🙈 but feel free to remove locals 😉

[dmccaffery]

... sure, but imma smack you. 😈

[vladimyr]

As of writing this comment Starship's install instructions (https://github.com/starship/starship/blob/4925764/docs/README.md#install-latest-version) suggest the following:

curl -fsSL https://starship.rs/install.sh | bash

First, this needs to get changed to (at least):

curl -fsSL https://starship.rs/install.sh | sh

in order to advertise reaching POSIX compliance to the end-users.
Unless, OFC, you still find it not relevant 😜 #2228 (comment):

Checklist:

• (not relevant) I have updated the documentation accordingly.
• (not relevant) I have updated the tests accordingly.

IMHO there are 4 likely types of workflows users may follow to install Starship using the installer script:

1. simple (c/p) workflow (most likely)
   In this workflow, the value of shebang is ignored as script content is piped directly to the (hopefully POSIX compatible) sh.

2. advanced workflow
   In this workflow, the user makes sure that the script is fully downloaded before starting its execution to prevent possible timing attacks.

   sh -c "$(curl -fsSL https://starship.rs/install.sh)"

   Shebang is ignored in this case because the desired shell (sh) is explicitly invoked.
   It is worth noting that this form should be preferred over the currently advertised one because it is equally convenient (c/p-able) but more secure. For example, take a look at Homebrew installation docs: https://brew.sh

3. cautious workflow
   In this workflow, the user first downloads the installer script:

   curl -fsSL https://starship.rs/install.sh -o install.sh

   They then verify its contents, and eventually run it:

   • by passing it as shell parameter

      sh install.sh

     Shebang is ignored in this case because the desired shell (sh) is explicitly invoked.
     Again, it is worth noting that some projects prefer writing out these steps in detail instead of advertising simple c/p-able shell snippets thus encouraging good safety practices! For example, here is how Docker folks do it: https://docs.docker.com/engine/install/debian/#install-using-the-convenience-script

   • as if it's an executable

      chmod +x install.sh
      ./install.sh

     In this case, the program loader parses the shebang and uses it to spawn a shell that'll be used for executing script contents.

So, only in 1 out 4 possible workflows shebang actually comes into the play. In all other workflows, it serves as a mere indicator of POSIX compliance so the user knows which shell to pick/invoke. In the former case, loader decides which shell will get invoked by parsing the shebang which brings me to my point about @dmccaffery's shebang choice https://github.com/starship/starship/blob/4925764/install/install.sh#L1:

#!/usr/bin/env sh

IMO it is somewhat superfluous. There is hardly any advantage in using env and looking up sh from the PATH instead of going with plain old /bin/sh. Yes, I'm aware that Solaris' /bin/sh is actually a Bourne Shell which is not POSIX compliant and what POSIX standard says about it but bear in mind that:

1. it is part of informative section
2. it is safe to assume Solaris folks are typically advanced users and well aware of that quirk and they'll probably invoke the correct (POSIX compliant) shell directly anyway

Honestly, I never saw anyone suggesting the use of env for POSIX sh lookup but apparently Wikipedia authors think it is a good idea. I definitely don't share their opinion because while it makes it more foolproof it also makes it less secure too. There is no security without knowledge. If the user treats shebang as some sort of magic and rejoices at the perceived flexibility from env usage they are certainly also not aware that a simple tweak to their shell profile (specifically PATH value) together with the introduction of fake sh will end up with them having a very very bad day.

So, long story short, I'm proposing the following:

1. Let's simplify things by using simple /bin/sh shebang. (As we did before with /bin/bash too.)
2. Let's strike a balance between safety and convenience by advertising the following form instead:

   sh -c "$(curl -fsSL https://starship.rs/install.sh)"