@actions/exec: add support for using a shell to run commands #285
Conversation
alexkappa
changed the title
| @@ -12,6 +12,9 @@ export interface ExecOptions { | |||
| /** optional. defaults to false */ | |||
| silent?: boolean | |||
|
|
|||
| /** optional. Runs command inside of a shell. A different shell can be specified as a string. Defaults to false */ | |||
| shell?: boolean | string | |||
There was a problem hiding this comment.
How does this affect the quoting rules on windows? linux?
Lots of logic in tool runner to accurately deal with arg quoting for .exe and different rules for .cmd/.bat.
Today not a problem on Linux because iirc node calls execv which takes an arg array. Different on Windows because everything gets joined into a single string. Does this introduce arg quoting challenges on Linux?
There was a problem hiding this comment.
And if so, are the quoting rules different depending on the shell?
There was a problem hiding this comment.
Hi @ericsciple, from the documentation of child_process options:
shell<boolean>|<string>If true, runs command inside of a shell. Uses '/bin/sh' on Unix, and process.env.ComSpec on Windows. A different shell can be specified as a string. See Shell Requirements and Default Windows Shell. Default: false (no shell).windowsVerbatimArguments<boolean>No quoting or escaping of arguments is done on Windows. Ignored on Unix. This is set to true automatically when shell is specified and is CMD. Default: false.
There was a problem hiding this comment.
Perhaps on windows we could set windowsVerbatimArguments to true if shell is specified and is equal to 'CMD'. That way we match the behavior of the stdlib, and allow our quoting to work with that assumption.
On linux I don't know if there are any quoting differences, but I'd be happy to add more tests to find out.
There was a problem hiding this comment.
Thanks. Other thoughts, spitballing...
If drastically different, I wonder whether it would make more sense to be linux/mac only feature.
If quoting issues on Linux, then existing workaround of tool: 'sh', args: ['-c', 'ls -l *.md'] is at least transparent what is happening
There was a problem hiding this comment.
I am not entirely sure how to proceed with this @ericsciple. I understand the workaround, but I thought since node supports it natively, it could be low hanging fruit with a more elegant solution.
Will more tests alleviate your concerns? I understand you are reluctant to accept this as there might be edge cases that break quoting. If so, I might need some help :)
| if (IS_WINDOWS) { | ||
| tool = 'cmd' | ||
| args = ['/c', 'echo', 'hello'] | ||
| opts = {shell: process.env.ComSpec} |
There was a problem hiding this comment.
Suggest also adding a test for running bash shell within Windows. As bash is now supported by windows runners, it provides a universal target for actions wanting to execute shell commands.
|
|
|
Why hasn't this been supported yet? I see that |
This PR adds support for supplying a shell to use when running commands. This is useful with expanding wildcards among other things.
Fixes: #229