Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: don't override existing ASDF_DIR #1008

Merged
merged 1 commit into from Nov 2, 2021
Merged

fix: don't override existing ASDF_DIR #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 12 additions & 8 deletions asdf.sh
View file
Open in desktop
Expand Up @@ -3,16 +3,18 @@
# For Bash, ${BASH_SOURCE[0]} will be used to obtain this script's path.
# For Zsh and others, $0 (the path to the shell or script) will be used.
_under="$_"
if [ "${BASH_SOURCE[0]}" != "" ]; then
current_script_path="${BASH_SOURCE[0]}"
elif [[ "$_under" == *".sh" ]]; then
current_script_path="$_under"
else
current_script_path="$0"
fi
if [ -z "$ASDF_DIR" ]; then
if [ -n "${BASH_SOURCE[0]}" ]; then
current_script_path="${BASH_SOURCE[0]}"
elif [[ "$_under" == *".sh" ]]; then
current_script_path="$_under"
else
current_script_path="$0"
fi

ASDF_DIR="$(dirname "$current_script_path")"
fi
export ASDF_DIR
ASDF_DIR="$(dirname "$current_script_path")"
# shellcheck disable=SC2016
[ -d "$ASDF_DIR" ] || echo '$ASDF_DIR is not a directory'

Expand All @@ -31,3 +33,5 @@ PATH="${ASDF_USER_SHIMS}:$PATH"
# shellcheck source=lib/asdf.sh
# Load the asdf wrapper function
. "${ASDF_DIR}/lib/asdf.sh"

unset _under current_script_path ASDF_BIN ASDF_USER_SHIMS
3 changes: 3 additions & 0 deletions test/version_commands.bats
View file
Open in desktop
Expand Up @@ -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/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the tests fail without this change? Which new codepath requires this as I don't see any test changes.

Copy link
Contributor Author

@ericbn ericbn Jul 30, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version_commands.bats tests fail because:

  1. They call setup_asdf_dir in the setup step.
    1. setup_asdf_dir creates a temp directory with no bin or lib files
    2. and sets the ASDF_DIR env variable to be the path to that directory.
  2. The tests call . $(dirname "$BATS_TEST_DIRNAME")/asdf.sh.
  3. asdf.sh will use the value of the existing ASDF_DIR env variable, that was set by setup_asdf_dir
  4. and fail because there's no lib/asdf.sh file in that ASDF_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.

}

teardown() {
Expand Down