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.
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
fix: ensure shims get created when data dir has spaces #996
Conversation
Thanks for the PR @mook-as ! Thank you for catching this, it's definitely something we need to get fixed. Do you think you'd be able to write a basic unit tests for these two functions to prevent this bug from being introduced again in the future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast review!
I've made changes to make it match existing code better, as well changed the tests to just default to having a space in ASDF_DIR
to catch any issues. This does involve changing a lot of the tests to quote things properly; sorry that the PR is so invasive.
I couldn't figure out a nice way to fix shim_exec.bats
, though, so I've opted that one out of having spaces for now. This is because it tests having a tool version of path:…
, but it's unclear from the documentation what should happen if there's a space in the path, given that it's a file with space-delimited fields.
Is there a way to add specific test cases for the code paths you modified to support the space, leaving the large existing set of tests for without a space in place? |
Okay, I've changed things so that having spaces in the test is opt in (per test setup), and only do it for the |
Yes, I think so. We don't need to have every single asdf test cover both cases, just as long as the modified codepaths in
are covered with the existing cases and your new cases. |
I had the exact same thought as @jthegedus. Checking the updated code now. |
@mook-as are you happy with this to be merged? |
@jthegedus I am; I can't figure out what's wrong with the WSL tests, though. |
They do not work. We do not expect them to either. It is my fault they are there. I will be removing those test cases shortly. We do want to test in other envs like wsl2 eventually. |
Summary
If
ASDF_DATA_DIR
has a space in the path (by default, this meansasdf
itself was installed to a path with a space), then the shims will not be created correctly in bash. This PR fixes the issue.Other Information
I have only checked up to the actual executable (in my case, node) running — I do not yet know if there are other issues related to spaces in the path.
I'm using extra variables for the lists (which contain newlines) because POSIX shell wouldn't support
while read … done; while < <(command)
(and is caught by the tests).