fix: don't override existing ASDF_DIR #1008
Conversation
ericbn
changed the title
allowing users to define its value, e.g. when installed with homebrew. Also unset locally used variables at the end.
|
@ericbn is this related to or independent of Homebrew/homebrew-core#81664? |
|
Hi @jthegedus. The changes here still don't fix the problems I've raised in that PR in homebrew-core. So, still unrelated in this sense. |
|
@jthegedus this change allows users to set |
|
@Stratus3D this change looks good to me. What are your thoughts? |
| @@ -21,6 +21,9 @@ setup() { | |||
| mkdir -p $CHILD_DIR | |||
|
|
|||
| cd $PROJECT_DIR | |||
|
|
|||
| # asdf lib needed to run asdf.sh | |||
| cp -rf $BATS_TEST_DIRNAME/../{bin,lib} $ASDF_DIR/ | |||
There was a problem hiding this comment.
Do the tests fail without this change? Which new codepath requires this as I don't see any test changes.
There was a problem hiding this comment.
Yes, tests fail without this change because setup_asdf_dir creates an "empty" ASDF_DIR. It's more like a valid ASDF_DATA_DIR than a valid ASDF_DIR, because the {bin,lib} files are not there. Since with this change the asdf.sh will respect any previously set ASDF_DIR variable, that ASDF_DIR will be used, and the tests fail.
Maybe we should move this cp -rf $BATS_TEST_DIRNAME/../{bin,lib} $ASDF_DIR/ (which is repeated may times after calling setup_asdf_dir in individual tests) to setup_asdf_dir, so this function guarantees that a complete ASDF_DIR was created. WDYT?
There was a problem hiding this comment.
I think I understand after looking through some code. setup_asdf_dir makes sense, but I question why it would be needed even after your change. As I understand, it will not overwrite ASDF_DIR if it is already set. ASDF_DIR is set during the setup_asdf_dir func, so why would these version_commands.bats tests start to fail?
I would expect your change in asdf.sh to only affect tests in asdf_sh.bats, but I don't think there's a test case for this scenarion, so a test should be added there.
There was a problem hiding this comment.
The version_commands.bats tests fail because:
- They call
setup_asdf_dirin the setup step.setup_asdf_dircreates a temp directory with no bin or lib files- and sets the
ASDF_DIRenv variable to be the path to that directory.
- The tests call
. $(dirname "$BATS_TEST_DIRNAME")/asdf.sh. asdf.shwill use the value of the existingASDF_DIRenv variable, that was set bysetup_asdf_dir- and fail because there's no
lib/asdf.shfile in thatASDF_DIR.
This is what I've mentioned in my previous comments, and that's why I've said that "setup_asdf_dir creates an 'empty' ASDF_DIR", and suggested that it should guarantee that the ASDF_DIR it creates is complete.
|
I don't know any fish scripting, but if you agree with this change, maybe the same should be applied to the |
|
It seems that this change breaks something : https://github.com/jbox-web/asdf-squarectl/actions |
|
@n-rodriguez, this works for me: Maybe you already have ASDF_DIR set to something else before running |
|
@n-rodriguez did you figure out what your issue was? I don't see any failures for your CI action. |
|
@Stratus3D sorry I did some clean but I triggered one for you : https://github.com/jbox-web/asdf-squarectl/actions/runs/1426185845 |
|
Actually I can't really tell if it's related. The only thing is synchronicity of events (and the git history ^^) Everything was green for 10 days : https://github.com/jbox-web/asdf-squarectl/actions And then 2 commits appear : https://github.com/asdf-vm/asdf/commits/master (2021-11-02) And then I realize it might be due to 69ff2d0 But one of them break something. Thank you! (and sorry for the noise) |
`asdf` [v0.9.0][1] included a [bug fix][2] to not override an existing ASDF_DIR. However, if `ASDF_DIR` is not set at all, then this causes an error when using bash `set -u`, or `set -o nounset` - see [here][3] for additional info. [1]: https://github.com/asdf-vm/asdf/releases/tag/v0.9.0 [2]: asdf-vm#1008 [3]: https://mywiki.wooledge.org/BashFAQ/112
`asdf` [v0.9.0][1] included a [bug fix][2] to not override an existing ASDF_DIR. However, if `ASDF_DIR` is not set at all, then this causes an error when using bash `set -u`, or `set -o nounset` - see [here][3] for additional info. [1]: https://github.com/asdf-vm/asdf/releases/tag/v0.9.0 [2]: asdf-vm#1008 [3]: https://mywiki.wooledge.org/BashFAQ/112
`asdf` [v0.9.0][1] included a [bug fix][2] to not override an existing ASDF_DIR. However, if `ASDF_DIR` is not set at all, then this causes an error when using bash `set -u`, or `set -o nounset` - see [here][3] for additional info. [1]: https://github.com/asdf-vm/asdf/releases/tag/v0.9.0 [2]: #1008 [3]: https://mywiki.wooledge.org/BashFAQ/112
Summary
Allow users to define their own value of ASDF_DIR before sourcing asdf.sh, e.g. when it was installed with homebrew.
For example, I can use this in my shell startup script:
Also, similar test for
[ -z "$ASDF_DIR" ]is already being done inlib/utils.bash.Other Information
Also unset locally used variables at the end.