fix!: rework POSIX entrypoint for greater shell support

[hyperupcall]

Summary

Earlier it was discovered that shfmt and shellcheck were not running properly on some entrypoint files (asdf.sh and lib/asdf.sh. As #1397 states, they were identified as Bash when they should have been identified as POSIX shell.

This PR updates both shfmt and shellcheck to parse the aforementioned files as POSIX with one slight nuance:

• shfmt sometimes spuriously prints errors when encountering ZSH-specific syntax (./asdf.sh:34:16: parameter expansion requires a literal). Because of this, shfmt no longer runs on asdf.sh.

I'm not sure if there is still confusion as to whether these files should be treated as (Bash and ZSH) or POSIX, but because we know (from #1138) that users are using shells such as mksh (and at least for me, I know I want asdf to continue to work if i jump into dash or ash), I'm treating these files as POSIX shell files. And POSIX compliance isn't too much work since we're only dealing with two files.

Moving attention back to the changes itself, I make the following fixes:

• Remove any Bash-isms or ZSH-isms that are not parseable with a POSIX-compliant shell
  • Previously, dash would error when parsing asdf.sh
• ASDF_BIN and ASDF_USER_SHIMS are no longer capitalized (they aren't environment variables), and they are now properly unset before lib/asdf.sh is sourced
• Remove current_script_path="$0" (this doesn't work)
  • These files are sourced, so $0 usually holds the path or name the binary of the currently executing shell (ex. /usr/bin/bash, dash)
• Remove _under="$_"
  • $_ rarely actually works in practice (writing something as innocuous as : somevar before source .../asdf.sh breaks this functionality). In my opinion, this flaky behavior is not worth implementing, and I added a workaround for the original Korn shell anyways.
• PATH is no longer modified (i consider it bad practice) (maybe I'll make a separate PR for this)

Other non-essential improvements:

• Behavior according to different shells are clearly segregated according to shell-specific variables ({BASH,ZSH,KSH}_VERSION)
• Uses no subshells and only builtins
• Properly prefix any created variables with _asdf_ and properly unset them on errors
• Implement proper error handling and break out of script if any happen to occur
• Prefix all error messages with asdf: Error so the source of the error is clear (related to No version manager name reported in CLI output, consider adding it #1467)

I tested this with dash, bash, zsh, ksh, and mksh.

• Fixes bug: enable shellcheck & shfmt for asdf entrypoints #1397
• Fixes Improve consistency with bash syntax #1313
• Closes rewrite asdf function to not rely on the "local" keyword #1138
• Closes bug: Installing asdf without sourcing shell #1426
  • This is a related issue, but it should really be closed as our 'source'ing behavior is a design choice rather than a bug
• Closes Support for sh/dash #653

TODO

• Add documentation describing explicit support for dash, sh, and all POSIX-shells

Other Information

I'm thinking that lib/asdf.sh should be removed. I'm not sure what useful purpose it serves, as its contents can easily be written in asdf.sh (same with lib/asdf.fish, etc.). Thoughts?

[jthegedus]

I'm thinking that lib/asdf.sh should be removed. I'm not sure what useful purpose it serves, as its contents can easily be written in asdf.sh (same with lib/asdf.fish, etc.). Thoughts?

That structure predates me, so not sure of motivation. Perhaps @Stratus3D can shed some light on it.

EDIT:
I would say we shouldn't change this for now.

[jthegedus]

This is a monumental change and can potentially break a lot of stuff. I am leaning towards merging other PRs first, releasing them, then merging this after the next release so that people can play with it on HEAD without any other changes confusing bug reports.

[jthegedus]

shfmt sometimes spuriously prints errors when encountering ZSH-specific syntax (./asdf.sh:34:16: parameter expansion requires a literal). Because of this, shfmt no longer runs on asdf.sh.

Right. It seems if we are targeting actual sh then we should not have ZSH specific syntax or specifically ignore those lines, or something. Turning off the formatter on the whole file for one edge case is not the ideal solution.

EDIT:
It seems ZSH specific formatting is coming: mvdan/sh#963 (comment)

However, that doesn't solve the partial support in a single file. So perhaps it is best to extract the .zsh specific stuff into it's own script to be tested (or not) idependently, leaving the sh in this file lint-able for sh specifically. It is a very critical piece of code to turn this off for.

[jthegedus]

@Stratus3D hasn't replied to #1397 (comment) But I believe we're on the same page about treating these files as sh and not bash 👍

[hyperupcall]

This is a monumental change and can potentially break a lot of stuff. I am leaning towards merging other PRs first, releasing them, then merging this after the next release so that people can play with it on HEAD without any other changes confusing bug reports.

That sounds good 👍

Right. It seems if we are targeting actual sh then we should not have ZSH specific syntax or specifically ignore those lines, or something. Turning off the formatter on the whole file for one edge case is not the ideal solution.

I don't really see an issue if the zsh-specific syntax doesn't cause a problem when the file is parsed by POSIX shells. We need to have this zsh specific syntax, or at least some variant of it so ASDF_DIR can be calculated.

However, that doesn't solve the partial support in a single file. So perhaps it is best to extract the .zsh specific stuff into it's own script to be tested (or not) idependently, leaving the sh in this file lint-able for sh specifically. It is a very critical piece of code to turn this off for.

Just to be a bit more clear, it was shfmt having the issues parsing the file - shellcheck works fine.

But I definitely see your point about turning off formatting for a particular file. I think there is another way I can express the same thing while keeping the formatter happy.

[hyperupcall]

Okay so I made the ZSH change, and now shfmt is tripping up again. Here is the change:

diff --git a/asdf.sh b/asdf.sh
index d7dd4c7..5e66fc4 100644
--- a/asdf.sh
+++ b/asdf.sh
@@ -29,11 +29,10 @@ if [ -z "$ASDF_DIR" ]; then
     fi
     unset -v _asdf_old_dir
   elif [ -n "$ZSH_VERSION" ]; then
-    # Use '%x' to expand to path of current file. It must be prefixed
-    # with '(%):-', so it expands in non-prompt-string contexts.
+    # Use '%x' to expand to path of current file. It must be used with
+    # '-P', since depends on prompt expansion.
 
-    # shellcheck disable=SC2296
-    ASDF_DIR=${(%):-%x}
+    print -P -v ASDF_DIR '%x'
     ASDF_DIR=${ASDF_DIR%/*}
   elif [ -n "$KSH_VERSION" ] && [ -z "$PATHSEP" ]; then
     # Only the original KornShell (kornshell.com) has a '.sh.file' variable with the path

With --language-dialect bash, shfmt shows: asdf.sh:43:16: invalid parameter name (at the KSH ${.sh.file})
With --language-dialect posix, shfmt shows: asdf.sh:19:46: arrays are a bash/mksh feature.

It looks impossible to make the asdf.sh file parseable by shfmt. Since it's just the formatter and not linter, are you okay with just dropping the format check? I understand it's not ideal, but there doesn't seem to be much of a choice.

[hyperupcall]

Hey can this get another look? v0.11.2 was just released a week or so ago, so now is an opportune time to merge as mentioned here.

[jthegedus]

It looks impossible to make the asdf.sh file parseable by shfmt. Since it's just the formatter and not linter, are you okay with just dropping the format check?

The errors from shfmt certainly seem like linting errors. Call it what you want but I would like to know information like this: asdf.sh:19:46: arrays are a bash/mksh feature

I believe this is just revealing that we should have separate asdf.* files for each of these other shells, as we do with Fish, Elvish & Nushell.

Just because each of these shells share a lineage with some common syntax or features is a bad reason to keep the code in one place and turn off our automatic validation on such a crucial aspect of our code.

So my suggestion now would be either:

1. Breaking changes:
   • remove the KSH branch of code, we should add asdf.ksh later
   • set shmft to parse the files as bash for now. We should add asdf.bash later
   • after we add asdf.bash, then change shfmt to parse this file as sh
2. Non-breaking but less clear:
   • extract each branch in asdf.sh into their own scripts. Eg: the KSH branch calls asdf-entrypoint.ksh, ZSH branch calls asdf-entrypoint.zsh
   • then we can add specific asdf.ksh, asdf.zsh scripts later and give users a migration path

I'm thinking out loud here. We could just turn off shfmt I guess... Just given the asdf.fish etc it seems that is a better path to go down.

[hyperupcall]

The errors from shfmt certainly seem like linting errors. Call it what you want but I would like to know information like this: asdf.sh:19:46: arrays are a bash/mksh feature

I suppose you could argue that the shfmt formatter is printing "linting errors", but it doesn't matter because there are no "issues" that shfmt detects that shellcheck doesn't:

• With the "arrays are a Bash/mksh feature" ( "${BASH_SOURCE[0]%/*}") we purposefully disable SC3054 because it's a false positive - the code parses on all applicable shells.
• With the ".sh.file is an invalid parameter name", ShellCheck catches that too, but again it's an obvious false positive and SC2296 is supressed - the code parses on all applicable shells.

So nothing (except formatting) is lost when turning shfmt off - ShellCheck already catches everything for us. So since ShellCheck is active, would we agree the whole asdf.sh thing is fine since ShellCheck will catch any errors that are introduced?

[hyperupcall]

I also want to mention, that before my changes, there was no clear separation between shells, functionality was broken for both KSH and POSIX Shells, and ShellCheck wasn't being ran on asdf.sh. My changes added ShellCheck, improved error messages and fixed all aforementioned issues with full backwards compatibility.

Since my changes only fix things, would it be OK to just merge these changes, and if you really wish to separate things into different files (which I would strongly disagree with), to do it in some later PR?

[jthegedus]

So nothing (except formatting) is lost when turning shfmt off - ShellCheck already catches everything for us. So since ShellCheck is active, would we agree the whole asdf.sh thing is fine since ShellCheck will catch any errors that are introduced?

Yes, you're right. Sorry, I lost context of the past conversation as there was a gap in time in which I was able to review and contribute to this conversation.

Since my changes only fix things, would it be OK to just merge these changes, and if you really wish to separate things into different files (which I would strongly disagree with), to do it in some later PR?

Yes, let's just merge this and we can discuss the other changes in future Issues.

[jthegedus]

This is great work, thanks very much @hyperupcall 🙏

We will merge this and let people test it with asdf update --head

[Stratus3D]

This looks like a really great set of changes. Thanks for your hard work on this @hyperupcall and @jthegedus !

[Stratus3D]

I'm thinking that lib/asdf.sh should be removed. I'm not sure what useful purpose it serves, as its contents can easily be written in asdf.sh (same with lib/asdf.fish, etc.). Thoughts?

That structure predates me, so not sure of motivation. Perhaps @Stratus3D can shed some light on it.

EDIT: I would say we shouldn't change this for now.

I think I've wondered the same thing. I seem to remember thinking all the logic in it could either be moved into bin/asdf or asdf.sh. It looks like this has already been done though, so good work!