pkg/tool/exec: Correctly parse and join cmd args

[infogulch]

I'm frustrated that the string form of tool/exec.Run.cmd does not parse args correctly, and that when it prints a command after it exits with an error status it incorrectly joins the args so it is not possible to simply copy the printed command and run it manually. The string form is nice to use for the simple case of running a single command, but it fails to correctly parse the args even if they are appropriately quoted. Also it fails to correctly print out debug information about a command that failed, as it uses plain concatenation instead of properly quoting arguments with spaces or special characters, even if they were passed as separate args in the list form.

For example, consider the following tool file:

package example
import "tool/exec"
command: parse: run: exec.Run & {cmd: "bash -c 'echo hello world'"}
command: join: run: exec.Run & {cmd: ["bash", "-c", "echo hello world; exit 1"]}

$ cue cmd parse
hello: -c: line 1: unexpected EOF while looking for matching `''
hello: -c: line 2: syntax error: unexpected end of file
task failed: command "bash -c 'echo hello world'" failed: exit status 2

$ cue cmd join
hello world
task failed: command "bash -c echo hello world; exit 1" failed: exit status 1

The parse command fails because tool/exec incorrectly parses the final quoted arg as a multiple args. The join command fails intentionally, but the printed error is incorrect, since the string bash -c echo hello world; exit 1 is not the same command as what cue cmd ran.

---

I'd like tool/exec to correctly parse args in the string form and correctly join args in both forms for debug printing. This can be implemented by importing the venerate github.com/kballard/go-shellquote library and updating the code to use it for splitting / joining instead of strings.Fields / plain concatenation. I have implemented this approach in PR #3207 which also includes new tests for these cases and more.

Some alternatives:

• Instead of importing the library it may be better to copy the relevant 250 LOC directly into the repo instead of adding a dependency. This is a tradeoff, but I have no opinion either way.
• Arg parsing/joining may be useful to cue tool authors directly, so we could expose the splitting/joining methods from the library as new tools (say tool/exec.ParseArgs/JoinArgs), and refactor the Run tool to perform the parsing/joining via the new tools within cue instead.

[mvdan]

Also it fails to correctly print out debug information about a command that failed, as it uses plain concatenation instead of properly quoting arguments with spaces or special characters, even if they were passed as separate args in the list form.

That is indeed a bug no matter what; if the input command was exec.Run & {cmd: ["bash", "-c", "echo hello world; exit 1"]}, we should not print it as an error like task failed: command "bash -c echo hello world; exit 1" failed, as that is indeed ambiguous. We could print the list of strings in either CUE or Go syntax, both of which can handle quoting.

The string form is nice to use for the simple case of running a single command, but it fails to correctly parse the args even if they are appropriately quoted.

Just so I understand, why do you want to use the string form when the list form exists? Quoting my reply from #3207 (comment) to join the threads:

I'm sure you have seen, but much like Go's os/exec, we support command arguments as a list of strings, which sidesteps this issue entirely. The majority of users, particularly those needing multiple arguments or arguments involving spaces, should use a list of strings rather than a single string that needs to be split up. This is easier for users to understand and maintain, and also doesn't require anyone to deal with shell syntax, which is unnecessary complexity. Particularly as it requires adding an extra dependency to CUE :)

I hope that makes sense. I would advocate for deprecating the string form of the command because perhaps it's a footgun that it uses https://pkg.go.dev/strings#Fields - it is rather basic. And perhaps some users would expect that it would use shell syntax, and we don't want to give that impression.

It seems to me like deprecating the string form in favor of [...string], or at least documenting that it's only meant for very simple use cases which don't involve whitespace in arguments, is a simpler and better way forward. I do not think we should make string split and unquote fields like a shell because:

• It is a significant amount of added complexity for little benefit. Such quoting exists in shells where the input is just characters, but here in CUE, we have a structured form for the arguments, [...string], with CUE syntax.
• Using CUE syntax is more consistent. ["foo", "bar baz"] reads better and is far more intuitive to CUE readers than "foo 'bar baz'" with nested quoting mixing different syntaxes.
• Shell quoting is a loose standard. Even if we document that we only support POSIX Shell quoting, e.g. no $' from Bash, the popularity of shells like Bash or Zsh may cause confusion and frustration for users down the line.

[mvdan]

For the time being, I've marked this issue as resolved by making the error messages non-ambiguous, and clearly documenting how the string form works with splitting arguments and its limitation. If this is still confusing to users, we can look into more agressive measures such as deprecating or even removing the string form in favor of the list form, with a rewrite in cue fix for existing users. For the time being, that seems premature, as you're the first and only user to run into this issue so far.

Let me know if that makes sense, or if you'd like to reopen this issue to discuss anything else. I'll close the PR for the time being as well, because like I said, we don't intend to add shell quoting support to the tool/exec package.