Asdf: Error running under bash with -u shell option enabled

[lalloni]

When calling a shim from a bash script with set -eEuo pipefail and export SHELLOPTS set on top asdf fails with the following message:

edited-home-dir/.asdf/lib/utils.sh: line 31: ASDF_DATA_DIR: unbound variable
unknown command: terraform. Perhaps you have to reshim?

Even though the shim is installed and works fine when called from user shell and from the script if removing "-u" option.

utils.sh should check for $ASDF_DATA_DIR existence before using or it should disable -u option on top.

[Stratus3D]

We've made several attempts at using Bash strict mode in asdf. See https://github.com/asdf-vm/asdf/pull/520 and https://github.com/asdf-vm/asdf/issues/160. If you want to help feel free to create another PR or work on the existing PR. This isn't high priority right now, so unless someone contributes the changes for this, I don't think it is going to happen soon.

I am going to close this issue. Please use https://github.com/asdf-vm/asdf/issues/160 for further discussion, as I think using set -o nounset -o pipefail -o errexit should fix this issue too.

[lalloni]

Hi @Stratus3D.

While I'm willing to help move this forward sending PRs I don't understand the current situation regarding those two issues you mentioned (#520 & #160). I've went through both of them and yet don't understand why both exists and what is missing.

Can you please explain what's the status and what remains to be done? How can I be most useful?

Having said that, I'm not sure asdf needs to run in strict mode to support a parent shell running in it. This issue is simply asking for supporting that situation which could be done in other ways (like checking vars or perhaps just setting the expected shell configuration on top of asdf entrypoint/s).

For sure it might be a good design choice to move asdf to "strict mode" yet that's another different and probably longer initiative than just ensuring it runs in an environment for which it was designed to run.

[lalloni]

Would a PR unsetting nounset on top of asdf be acceptable?

Rationale: if asdf is written in a way that assumes nounset is not set, then it should ensure it is running in a shell configured as expected.

[Stratus3D]

Can you please explain what's the status and what remains to be done? How can I be most useful?

There are some unknowns here for me. For example, asdf runs fine when the "strict mode" flags are not set, but fails when the flags are set. Flags like -o errexit change a lot. I don't think it's reasonable to try and write code that works the same with and without the errexit option.

If we want to support "strict mode" shells and regular shells we will need to have a way of not allowing the flags from the shell to affect asdf, and that's another thing I'm not sure about. It seems your shell affected how asdf code was executed. But maybe it only affects some of the asdf code, is it just the asdf.sh file that gets sourced by your shell? If so it may be an easy fix to change that file to work with both kinds of shells, since I don't think the logic there is that complicated (and we can check if the variable is set before trying to use it to avoid the issue you ran into).

Would a PR unsetting nounset on top of asdf be acceptable?

I am not sure. If you did that in asdf.sh that change would affect the user's shell which would not be ok.

Sorry, I don't have more answers. But I think probably the best approach for now is to try and work around this issue without touching flags.

Would something like this fix your issue? In utils.sh on lines 41 and 42:

  if [ -n "${ASDF_DATA_DIR:-}" ]; then
    data_dir="${ASDF_DATA_DIR:-}"

I was not able to reproduce the issue when running an interactive shell with set -eEuo pipefail set.

[lalloni]

If we want to support "strict mode" shells and regular shells we will need to have a way of not allowing the flags from the shell to affect asdf, and that's another thing I'm not sure about. It seems your shell affected how asdf code was executed. But maybe it only affects some of the asdf code, is it just the asdf.sh file that gets sourced by your shell? If so it may be an easy fix to change that file to work with both kinds of shells, since I don't think the logic there is that complicated (and we can check if the variable is set before trying to use it to avoid the issue you ran into).

I think ASDF should only care about user shell settings on asdf.sh and asdf.fish which are expected/designed to be "sourced" into a user's running shell instance.

Would a PR unsetting nounset on top of asdf be acceptable?

I am not sure. If you did that in asdf.sh that change would affect the user's shell which would not be ok.

How about setting expected flags on scripts that expects them.

I mean, not in asdf.sh or asdf.fish because, as you well said, this would change the user's shell settings (highly undesirable).

But we could (and probably should) do that on bin/asdf since it is run as a bash script in its own bash instance by design.

Would something like this fix your issue? In utils.sh on lines 41 and 42:

  if [ -n "${ASDF_DATA_DIR:-}" ]; then
    data_dir="${ASDF_DATA_DIR:-}"

Yes, something similar to that was my proposal in my original issue description. :smile:

In fact you can lose the if completely and just do:

data_dir="${ASDF_DATA_DIR:-}"

[lalloni]

BTW, both asdf.fish and asdf.sh should probably not have a shebang line at the beginning since those two are not supposed to be run as standalone scripts but only to be sourced into some other shell instance and having those lines there makes it hard or confusing to understand the expected use cases.

[Stratus3D]

BTW, both asdf.fish and asdf.sh should probably not have a shebang line at the beginning since those two are not supposed to be run as standalone scripts but only to be sourced into some other shell instance and having those lines there makes it hard or confusing to understand the expected use cases.

Good catch, didn't think about that. I definitely welcome a PR for that.

Yes, something similar to that was my proposal in my original issue description. 😄

I would welcome a PR for that too.

But we could (and probably should) do that on bin/asdf since it is run as a bash script in its own bash instance by design.

I'm not sure how bin/asdf inherits from the shell, since it should be running as a child process. Do child processes inherit the bash flags from the parent shell? I should probably know this, but I don't.

[lalloni]

BTW, both asdf.fish and asdf.sh should probably not have a shebang line at the beginning since those two are not supposed to be run as standalone scripts but only to be sourced into some other shell instance and having those lines there makes it hard or confusing to understand the expected use cases.

Good catch, didn't think about that. I definitely welcome a PR for that.

Yes, something similar to that was my proposal in my original issue description. smile

I would welcome a PR for that too.

But we could (and probably should) do that on bin/asdf since it is run as a bash script in its own bash instance by design.

I'm not sure how bin/asdf inherits from the shell, since it should be running as a child process. Do child processes inherit the bash flags from the parent shell? I should probably know this, but I don't.

Child shell script processes doesn't inherit shell options at all. Think the kernel/loader is who actually interprets the shebang line and creates a new process running the named "interpreter" program in that line.

[lalloni]

BTW, both asdf.fish and asdf.sh should probably not have a shebang line at the beginning since those two are not supposed to be run as standalone scripts but only to be sourced into some other shell instance and having those lines there makes it hard or confusing to understand the expected use cases.

Good catch, didn't think about that. I definitely welcome a PR for that.

Here you go #629.

[lalloni]

Yes, something similar to that was my proposal in my original issue description. smile

I would welcome a PR for that too.

And here #630.

[Stratus3D]

Thanks for the PRs @lalloni ! The one has been merged and I'm going to investigate some build issues we are having (unrelated to your changes) before merging the second one.