fix: Quote commands correctly in plugin-test

[raxod502]

Summary

Quote variables correctly in command-plugin-test.bash, eliminating bizarre results when trying to run commands with whitespace in them.

Other Information

Before:

% asdf plugin-test bundler https://github.com/jonathanmorley/asdf-bundler.git 'bundler --version | grep 2.2.29'
Updating bundler...
Already on 'master'
Your branch is up to date with 'origin/master'.
ERROR: "bundler version" was called with arguments ["|", "grep", "2.2.29"]
Usage: "bundler version"
FAILED: bundler --version | grep 2.2.29 failed with exit code 0

After:

% ./bin/asdf plugin-test bundler https://github.com/jonathanmorley/asdf-bundler.git 'bundler --version | grep 2.2.29'
Updating bundler to master
Already on 'master'
Your branch is up to date with 'origin/master'.
Bundler version 2.2.29

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:

      - name: asdf_plugin_test
        uses: asdf-vm/actions/plugin-test@v1
        with:
          command: bundle --version | grep -F "${{ matrix.version }}"

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).

[jthegedus]

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?

[raxod502]

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).

[raxod502]

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?

[Stratus3D]

@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).

[raxod502]

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.

[Stratus3D]

Should this be sh or bash?

[raxod502]

Pushed a commit making it default to $SHELL with fallback to POSIX-safe sh.

[jthegedus]

thanks @raxod502 , lgtm. merge if you're happy @Stratus3D

[jthegedus]

@raxod502 can you run the scripts/shfmt.bash script?

[raxod502]

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:

#!/usr/bin/env bash

set -euxo pipefail

ver="$(curl -fsSL https://api.github.com/repos/mvdan/sh/releases | jq 'map(.name) | .[]' -r | sed 's/^v//' | sort -V | tail -n1)"

pkg="${PWD}/shfmt-${ver}"

rm -rf "${pkg}"
install -d "${pkg}/usr/bin"
wget "https://github.com/mvdan/sh/releases/download/v${ver}/shfmt_v${ver}_linux_amd64" -O "${pkg}/usr/bin/shfmt"
chmod +x "${pkg}/usr/bin/shfmt"

install -d "${pkg}/DEBIAN"
tee "${pkg}/DEBIAN/control" <<EOF >/dev/null
Package: shfmt
Version: ${ver}
Section: shells
Priority: optional
Architecture: amd64
Maintainer: Radon Rosborough <radon.neon@gmail.com>
Description: Formats shell programs
EOF

fakeroot dpkg-deb --build "${pkg}" "shfmt-${ver}.deb"
if dpkg -s shfmt &>/dev/null; then
    sudo apt remove -y shfmt
fi
sudo apt install -y "./shfmt-${ver}.deb"

[jthegedus]

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 .tool-versions in this repo with the deps for managing via asdf 😉

[raxod502]

We've got a .tool-versions in this repo with the deps

Well, I can't say I shouldn't have seen that coming.

[tsuyoshicho]

I observed This PR make asdf-vm/actions/plugin-test@v1 to failed.

• https://github.com/MetricMike/asdf-awscli/runs/4087328230?check_suite_focus=true
• https://github.com/tsuyoshicho/asdf-vim/runs/4085137146?check_suite_focus=true

These test ran after this PR merged.

I think test time ASDF_INSTALL_VERSION set as --asdf-plugin-gitref

[Stratus3D]

@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:

asdf-awscli: Could not download https://github.com/aws/aws-cli/archive/--asdf-plugin-gitref.tar.gz

clearly --asdf-plugin-gitref was a flag for the tag, not the tag itself.