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: Quote commands correctly in plugin-test #1078
Conversation
0d0740e
to
d248c24
Compare
d248c24
to
add6df8
Compare
@@ -129,10 +129,10 @@ plugin_test_command() { | |||
fi | |||
|
|||
if [ -n "$plugin_command" ]; then | |||
$plugin_command | |||
"${plugin_command[@]}" |
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.
While we are currently using Bash, our goal is to take the POSIX compat soln where possible to support more Shells in future. Are shift
and [@]
POSIX compatible?
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.
shift
is POSIX per this ref. Unfortunately, POSIX sh does not have arrays at all, so you'll have to do a more thorough rearchitecting in order to make this code portable. Applying this PR won't decrease the portability since all shells with arrays should have [@]
indexing (otherwise they're not much use, since [*]
doesn't handle arguments with spaces correctly).
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.
Looking at the code, I think it would not be too hard in this case to refactor so it does not use arrays at all, would you like me to do that?
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.
@raxod502 which do you think would be better? I only see one line that actually uses this variable as an array (line 25 pushes an element to the array).
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.
Well, if you want to make asdf POSIX compliant in the long run, we might as well get rid of the array usage, since you'll have to do that at some point. I pushed a new commit doing so, and fixing some other quoting errors.
;; | ||
esac | ||
done | ||
|
||
plugin_command="${plugin_command_array[*]}" | ||
if [ "${#plugin_command[@]}" -eq 1 ]; then | ||
plugin_command=(sh -c "${plugin_command[@]}") |
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.
Should this be sh
or bash
?
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.
Pushed a commit making it default to $SHELL
with fallback to POSIX-safe sh
.
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 @raxod502 , lgtm. merge if you're happy @Stratus3D
@raxod502 can you run the |
Done, sorry for not doing it the first time, shfmt wasn't packaged for Ubuntu so I had to write a packaging script to install it:
|
We've got a |
Well, I can't say I shouldn't have seen that coming. |
I observed This PR make asdf-vm/actions/plugin-test@v1 to failed.
These test ran after this PR merged. I think test time |
@tsuyoshicho yeah it does look like we may have broken something here. @raxod502 would you be willing to take a look at @tsuyoshicho builds? Output shows things like:
clearly |
Summary
Quote variables correctly in
command-plugin-test.bash
, eliminating bizarre results when trying to run commands with whitespace in them.Other Information
Before:
After:
The intended behavior was somewhat unclear, but in the new code, if one argument is provided, it is assumed to be a shell command, while if multiple arguments are provided, it is assumed that the first is an executable and the remainder are arguments (so no shell is involved).
The use case that generated this pull request was specifying the following configuration to https://github.com/asdf-vm/actions for jonathanmorley/asdf-bundler#4:
Where we see that the command is passed as a single argument to
asdf plugin-test
, and therefore ought to be interpreted as a shell command, rather than being split on whitespace and then executed without shell interpretation, which is the current behavior (see the bizarre error message above).