Skip to content

@actions/exec: add support for using a shell to run commands #285

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

@actions/exec: add support for using a shell to run commands #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
654dba3
add support for using a shell to run commands
alexkappa Jan 6, 2020
f0583ec
formatting
alexkappa Jan 6, 2020
e849c06
remove blank line
alexkappa Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/exec/__tests__/exec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,23 @@ describe('@actions/exec', () => {
expect(output.trim()).toBe(`hello`)
})

it('Runs exec successfully with shell wildcard substitution', async () => {
let tool: string
let args: string[]
let opts: im.ExecOptions
if (IS_WINDOWS) {
tool = 'cmd'
args = ['/c', 'echo', 'hello']
opts = {shell: process.env.ComSpec}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
tool = 'ls'
args = ['-l', '*.md']
opts = {shell: '/bin/sh'}
}
const exitCode = await exec.exec(tool, args, opts)
expect(exitCode).toBe(0)
})

it('Exec fails with error on bad call', async () => {
const _testExecOptions = getExecOptions()

Expand Down
3 changes: 3 additions & 0 deletions packages/exec/src/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if so, are the quoting rules different depending on the shell?

Copy link
Author

@alexkappa alexkappa Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** optional out stream to use. Defaults to process.stdout */
outStream?: stream.Writable

Expand Down
1 change: 1 addition & 0 deletions packages/exec/src/toolrunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ export class ToolRunner extends events.EventEmitter {
const result = <child.SpawnOptions>{}
result.cwd = options.cwd
result.env = options.env
result.shell = options.shell
result['windowsVerbatimArguments'] =
options.windowsVerbatimArguments || this._isCmdFile()
if (options.windowsVerbatimArguments) {
Expand Down