Improve consistency with bash syntax

[jmcantrell]

Is your feature request related to a problem? Please describe

While reading the code, I've noticed a bit of inconsistency with how bash syntax is used. The one I've most noticed is the usage of the conditional braces ([[ vs [), often within the same file, e.g. this.

It's clear that bash syntax is intended, and that POSIX compatibility isn't desired, which is totally fine. However, if bash is being embraced, it should be embraced fully.

Describe the proposed solution

I would like to see consistency in the syntax usage. This is not just for readability. There are real noticeable affects. For example, the use of [ is actually a completely separate program. Try which [ in a bash shell. If you use [ instead of [[, it's actually creating a new process to do the comparison. If [[ is used, the comparison happens completely in the bash process. Aside from that, the quoting rules are a bit more forgiving with [[.

I'm happy to take on this project, but I would like to know how this PR will be received, first.

Describe similar asdf features and why they are not sufficient

N/A

Describe other workarounds you've considered

N/A

[Stratus3D]

Hi @jmcantrell , thanks for asking. I think this has been an issue for a while. I'll admit I'm not an expert on the differences between Bash syntax and POSIX compatible syntax and that's probably a big part of why the codebase is the way it is. I'd definitely welcome standardization in this area, but is there a way we can automate syntax checks for future PRs? Is there a tool that can enforce one syntax or the other across the board? Left up to manual review this is probably something I'll miss.

Also, some of the scripts are sourced by the user's shell. In my opinion those should be POSIX if possible, but any scripts here containing #!/usr/bin/env bash can use the Bash syntax.

[jmcantrell]

What about adding --enable=require-double-brackets to scripts/shellcheck.bash? It looks like that's already expecting bash syntax. It looks like asdf.sh has mixed bracket styles, so maybe that's safe to switch. Are completions the only other stuff that's sourced?

[doolio]

May I ask why POSIX compatibility isn't desired?

[jmcantrell]

May I ask why POSIX compatibility isn't desired?

They're declared explicitly as bash files (the extension). If POSIX compatibility isn't a goal, it's better to fully embrace bash syntax instead of some mixture of both.

[doolio]

No, I appreciate why you opened this issue. I'm just learning to programme and presumed it was always desirable to be POSIX compliant with one's shell scripts.

[hyperupcall]

For example, the use of [ is actually a completely separate program

@jmcantrell It's a builtin - see help [. What you're referring to hasn't been true for decades, if at all (post SysV)

Concerning performance, I've always placed my bets on [ being faster just because [[ is syntax and causes a different code path to activate, possibly flushing useful bytes in the instruction cache. But that's just a conjecture and it's probably more likely there is no real difference - and it wouldn't make a difference in this codebase due to the high abundance of execs and subshells.

@doolio POSIX compliance is always desired, but it's only really relevant here in the context of calling out to external programs like mv, mkdir, etc. because the runtime is guaranteed to be some version of Bash. Since mv, mkdir, etc. are not builtins - (they could be), extra care needs to take care to ensure that all the flags are POSIX compliant.

[hyperupcall]

I agree that there needs to be consistency

I personally prefer to only use Bash syntax when I need to. So I only use [[ when I need to pull out that regex or glob comparison (if I don't want to use case), or maybe even sometimes if I want to use && within the double brackets. That is what I would advocate.

It's quite convenient because you can easily just copy and paste POSIX compliant code, generally, without having to format things to be consistent. It also makes code a bit more friendly to read in my opinion (to my eyes [[ $ is slightly harsher compared to [ "$), but I guess that's more subjective.

[doolio]

I see. Thanks for the explanation.

[hyperupcall]

PR #1349 partially addresses this by making the tests more consistent

[jthegedus]

@jmcantrell I advocated renaming the files to .bash to reflect what they are today, POSIX was/is(?) a goal.

Shellcheck supports POSIX with -s sh, currently we're using -s bash which validates against the Bash spec.

By setting -s sh in the scripts/shellcheck.bash script and running it, it seems the main issues are: local, array references [@], and [[ ]].

[hyperupcall]

I think it would be beneficial not to remove local. I think I read somewhere that all major strictly POSIX shells support that, even if its not in the spec. Even the Debian Policy Manual even requires local to work. At least for me, it gives me confidence that less variables are leaking (too many variables are already accessible via dynamic scoping).

I think it's worth mentioning that in my experience, it's much easier to improve performance when the script is written in Bash compared to POSIX sh, there are many more tricks (i saw there are quite a few issues on perf). I would also be less confident in the test suite since everything will be running with Bash and therefore the tests wouldn't match the environment the main code is ran under.

I guess #653 is related, and it's a bit confusing to read because there doesn't seem to be an explicit distinction between shells supported when sourcing asdf, and shells supported when the main bin/asdf.sh executable is ran. So I guess I am just trying to say that it would be nice if a choice was made and to stick with it, just from the standpoint of a new contributor, so I know that I can use certain Bash idioms to make the code fast or more semantic.

[jthegedus]

So I guess I am just trying to say that it would be nice if a choice was made and to stick with it

I think we all agree with this.

Neither @Stratus3D or myself are experts in shell-interop/compatibility and so are hesitant to make a call that would break usage for existing users.

When we talk about specific shell support, it's important to draw boundaries about which aspects of the code we mean. If we are talking about everything under lib/ excluding lib/asdf.sh and lib/asdf.fish, then I think we should be okay to commit to Bash.

The test framework we use, Bats, has already had this discussion - https://github.com/bats-core/bats-core/issues/28 It is an interesting read. The main cause of concern was macOS shipping with Bash v3.2, so we would essentially be locked to supporting that version and hence features of Bash.

As we already require systems to support Bash, we wouldn't be paring down the supported environments, just committing to not expanding them.

I'd be happy to vote to commit to Bash. @Stratus3D what do you think?

---

Further information:

Here is the bats compat list:

https://bats-core.readthedocs.io/en/stable/installation.html#supported-bash-versions

Bash versions:
- Everything from 3.2.57(1) and higher (macOS’s highest version)

Operating systems:
- Arch Linux
- Alpine Linux
- Ubuntu Linux
- FreeBSD 10.x and 11.x
- macOS
- Windows 10

Latest version for the following Windows platforms:
- Git for Windows Bash (MSYS2 based)
- Windows Subsystem for Linux
- MSYS2
- Cygwin

A more concrete analysis of POSIX with Shellcheck reveals 17 issues

Script:

# scripts/shellcheck.bash
-#!/usr/bin/env bash
+#!/usr/bin/env sh

-set -euo pipefail
+set -eu

-exec shellcheck -s bash -x \
+exec shellcheck -s sh -x \
  asdf.sh \
  completions/*.bash \
  bin/asdf \
  bin/private/asdf-exec \
  lib/utils.bash \
  lib/commands/*.bash \
  scripts/*.bash \
  test/test_helpers.bash \
  test/fixtures/dummy_plugin/bin/*

Command:

./scripts/shellcheck.bash | grep -Eo "SC...." | sort -u

Results:

SC2086
SC3003
SC3010
SC3011
SC3014
SC3020
SC3024
SC3028
SC3030
SC3040
SC3043
SC3044
SC3045
SC3053
SC3054
SC3057
SC3060

Supporting Dash

Dash support is potentially possible if we support a minimal subset of Bash. Some differences from Googling:

• Dash doesn't have [[, but only [
• Dash doesn't have source, but we use . over source already.
• Dash doesn't have ==, but only =

[Stratus3D]

Yes! I think originally the intention was to standardize on Bash for asdf scripts that are already executed by Bash explicitly via shebang (most of the files in bin and lib). For files sourced by the users shell, asdf.sh and asdf.fish, asdf.elv, will remain in whatever dialect they are.

As @jthegedus said I know nothing about shell-interop/compatibility but mandating a minimum Bash version of 3.2 seems reasonable to me.

The most important contributions around this will be on automating as many checks as possible. We run shfmt, shellcheck, and https://github.com/asdf-vm/asdf/blob/master/test/banned_commands.bats for every PR we receive. There may be additional tools/checks/flags we can add to make for an even stricter criteria for new Bash 3.2 code.

Thanks everyone!

[jthegedus]

There is a feature request for Shellcheck to support specifically validating against Bash 3.2 because everyone now must support it as a minimum thanks to Apple - Allow configuring to reject code that is incompatible with bash 3.2. #2615