Skip to content
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

toolrunner should which tool #220

Merged
merged 1 commit into from Nov 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
70 changes: 15 additions & 55 deletions .github/workflows/unit-tests.yml
Expand Up @@ -6,18 +6,27 @@ on:
pull_request:
paths-ignore:
- '**.md'

jobs:
Ubuntu:
name: Run Ubuntu
runs-on: ubuntu-latest

build:
name: Build

strategy:
matrix:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

leverage matrix instead of duplicate 3 times

runs-on: [ubuntu-latest, macOS-latest, windows-latest]
fail-fast: false

runs-on: ${{ matrix.runs-on }}

steps:
- name: Checkout
uses: actions/checkout@master
uses: actions/checkout@v1

- name: Set Node.js 10.x
- name: Set Node.js 12.x
joshmgross marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/setup-node@master
with:
node-version: 10.x
node-version: 12.x

- name: npm install
run: npm install
Expand All @@ -36,52 +45,3 @@ jobs:

- name: Format
run: npm run format-check
macOS:
name: Run macOS
runs-on: macos-latest
steps:
- name: Checkout
uses: actions/checkout@master

- name: Set Node.js 10.x
uses: actions/setup-node@master
with:
node-version: 10.x

- name: npm install
run: npm install

- name: Bootstrap
run: npm run bootstrap

- name: Compile
run: npm run build

- name: npm test
run: npm test
Windows:
name: Run Windows
runs-on: windows-latest
steps:
- name: Checkout
uses: actions/checkout@master

- name: Set Node.js 10.x
uses: actions/setup-node@master
with:
node-version: 10.x

- name: npm install
run: npm install

- name: Bootstrap
run: npm run bootstrap

- name: Compile
run: npm run build

# TODO: This currently ignores exec due to issues with Node and spawning on Windows, which I think is exacerbated by Jest.
# It doesn't seem to affect behavior in actions themselves, just when testing with Jest.
# See other similar issues here: https://github.com/nodejs/node/issues/25484
- name: npm test
run: npm run test-ci
3 changes: 1 addition & 2 deletions package.json
Expand Up @@ -9,8 +9,7 @@
"format-check": "prettier --check packages/**/*.ts",
"lint": "eslint packages/**/*.ts",
"new-package": "scripts/create-package",
"test": "jest",
"test-ci": "jest --testPathIgnorePatterns=\"<rootDir>/packages/exec/__tests__/exec.test.ts\""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we dont need this command anymore, this was to skip the failing tests on windows

"test": "jest"
},
"devDependencies": {
"@types/jest": "^24.0.11",
Expand Down
2 changes: 1 addition & 1 deletion packages/core/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 2 additions & 5 deletions packages/exec/README.md
Expand Up @@ -48,13 +48,10 @@ await exec.exec('node', ['index.js', 'foo=bar'], options);

#### Exec tools not in the PATH

You can use it in conjunction with the `which` function from `@actions/io` to execute tools that are not in the PATH:
You can specify the full path for tools not in the PATH:

```js
const exec = require('@actions/exec');
const io = require('@actions/io');

const pythonPath: string = await io.which('python', true)

await exec.exec(`"${pythonPath}"`, ['main.py']);
await exec.exec('"/path/to/my-tool"', ['arg1']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the previous example did the exact opposite of the description :(

```
196 changes: 196 additions & 0 deletions packages/exec/__tests__/exec.test.ts
Expand Up @@ -121,6 +121,38 @@ describe('@actions/exec', () => {
}
})

it('Runs exec successfully with command from PATH', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

coverage for existing functionality

const execOptions = getExecOptions()
const outStream = new StringStream()
execOptions.outStream = outStream
let output = ''
execOptions.listeners = {
stdout: (data: Buffer) => {
output += data.toString()
}
}

let exitCode = 1
let tool: string
let args: string[]
if (IS_WINDOWS) {
tool = 'cmd'
args = ['/c', 'echo', 'hello']
} else {
tool = 'sh'
args = ['-c', 'echo hello']
}

exitCode = await exec.exec(tool, args, execOptions)

expect(exitCode).toBe(0)
const rootedTool = await io.which(tool, true)
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${rootedTool} ${args.join(' ')}`
)
expect(output.trim()).toBe(`hello`)
})

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

Expand Down Expand Up @@ -418,6 +450,134 @@ describe('@actions/exec', () => {
fs.unlinkSync(semaphorePath)
})

it('Exec roots relative tool path using unrooted options.cwd', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests to make sure we're not breaking existing behavior

also this confirms that extensions are resolved on windows, which the previous code did not do (but should have done)

let exitCode: number
let command: string
if (IS_WINDOWS) {
command = './print-args-cmd' // let ToolRunner resolve the extension
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note, without extension, toolrunner should resolve the extension

Copy link
Collaborator

@thboop thboop Nov 18, 2019

Choose a reason for hiding this comment

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

Consider adding a \ test for windows

} else {
command = './print-args-sh.sh'
}
const execOptions = getExecOptions()
execOptions.cwd = 'scripts'
const outStream = new StringStream()
execOptions.outStream = outStream
let output = ''
execOptions.listeners = {
stdout: (data: Buffer) => {
output += data.toString()
}
}

const originalCwd = process.cwd()
try {
process.chdir(__dirname)
exitCode = await exec.exec(`${command} hello world`, [], execOptions)
} catch (err) {
process.chdir(originalCwd)
throw err
}

expect(exitCode).toBe(0)
const toolPath = path.resolve(
__dirname,
execOptions.cwd,
`${command}${IS_WINDOWS ? '.cmd' : ''}`
)
if (IS_WINDOWS) {
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${process.env.ComSpec} /D /S /C "${toolPath} hello world"`
)
} else {
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${toolPath} hello world`
)
}
expect(output.trim()).toBe(`args[0]: "hello"${os.EOL}args[1]: "world"`)
})

it('Exec roots relative tool path using rooted options.cwd', async () => {
let command: string
if (IS_WINDOWS) {
command = './print-args-cmd' // let ToolRunner resolve the extension
} else {
command = './print-args-sh.sh'
}
const execOptions = getExecOptions()
execOptions.cwd = path.join(__dirname, 'scripts')
const outStream = new StringStream()
execOptions.outStream = outStream
let output = ''
execOptions.listeners = {
stdout: (data: Buffer) => {
output += data.toString()
}
}

const exitCode = await exec.exec(`${command} hello world`, [], execOptions)

expect(exitCode).toBe(0)
const toolPath = path.resolve(
__dirname,
execOptions.cwd,
`${command}${IS_WINDOWS ? '.cmd' : ''}`
)
if (IS_WINDOWS) {
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${process.env.ComSpec} /D /S /C "${toolPath} hello world"`
)
} else {
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${toolPath} hello world`
)
}
expect(output.trim()).toBe(`args[0]: "hello"${os.EOL}args[1]: "world"`)
})

it('Exec roots relative tool path using process.cwd', async () => {
let exitCode: number
let command: string
if (IS_WINDOWS) {
command = 'scripts/print-args-cmd' // let ToolRunner resolve the extension
} else {
command = 'scripts/print-args-sh.sh'
}
const execOptions = getExecOptions()
const outStream = new StringStream()
execOptions.outStream = outStream
let output = ''
execOptions.listeners = {
stdout: (data: Buffer) => {
output += data.toString()
}
}

const originalCwd = process.cwd()
try {
process.chdir(__dirname)
exitCode = await exec.exec(`${command} hello world`, [], execOptions)
} catch (err) {
process.chdir(originalCwd)
throw err
}

expect(exitCode).toBe(0)
const toolPath = path.resolve(
__dirname,
`${command}${IS_WINDOWS ? '.cmd' : ''}`
)
if (IS_WINDOWS) {
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${process.env.ComSpec} /D /S /C "${toolPath} hello world"`
)
} else {
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${toolPath} hello world`
)
}
expect(output.trim()).toBe(`args[0]: "hello"${os.EOL}args[1]: "world"`)
})

if (IS_WINDOWS) {
// Win specific quoting tests
it('execs .exe with verbatim args (Windows)', async () => {
Expand Down Expand Up @@ -572,6 +732,42 @@ describe('@actions/exec', () => {
)
})

it('execs .cmd from path (Windows)', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the test related to the bug fix.

toolrunner should which the tool before attempting to invoke. otherwise simple commands like npm --version fail on windows (ENOENT) because npm resolves to npm.cmd and node doesnt find it.

// this test validates whether a .cmd is resolved from the PATH when the extension is not specified
const cmd = 'print-args-cmd' // note, not print-args-cmd.cmd
const cmdPath = path.join(__dirname, 'scripts', `${cmd}.cmd`)
const args: string[] = ['my arg 1', 'my arg 2']
const outStream = new StringStream()
let output = ''
const options = {
outStream: <stream.Writable>outStream,
listeners: {
stdout: (data: Buffer) => {
output += data.toString()
}
}
}

const originalPath = process.env['Path']
try {
process.env['Path'] = `${originalPath};${path.dirname(cmdPath)}`
const exitCode = await exec.exec(`${cmd}`, args, options)
expect(exitCode).toBe(0)
expect(outStream.getContents().split(os.EOL)[0]).toBe(
`[command]${
process.env.ComSpec
} /D /S /C "${cmdPath} "my arg 1" "my arg 2""`
)
expect(output.trim()).toBe(
'args[0]: "<quote>my arg 1<quote>"\r\n' +
'args[1]: "<quote>my arg 2<quote>"'
)
} catch (err) {
process.env['Path'] = originalPath
throw err
}
})

it('execs .cmd with arg quoting (Windows)', async () => {
// this test validates .cmd quoting rules are applied, not the default libuv rules
const cmdPath = path.join(
Expand Down
12 changes: 12 additions & 0 deletions packages/exec/__tests__/scripts/print-args-cmd.cmd
@@ -0,0 +1,12 @@
@echo off
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the naming of these script files is a little bit weird, but it's related to the conventions set forth by the existing test case scripts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just copied one of the existing cmd files (but it had spaces in the path, and i didnt want that)

this script basically just echoes the args, e.g.

arg[0]: "hello"
arg[1]: "world"

setlocal
set index=0

:check_arg
set arg=%1
if not defined arg goto :eof
set "arg=%arg:"=<quote>%"
echo args[%index%]: "%arg%"
set /a index=%index%+1
shift
goto check_arg
11 changes: 11 additions & 0 deletions packages/exec/__tests__/scripts/print-args-sh.sh
@@ -0,0 +1,11 @@
#!/usr/bin/env bash

# store arguments in a special array
args=("$@")
# get number of elements
ELEMENTS=${#args[@]}

# echo each element
for (( i=0;i<$ELEMENTS;i++)); do
echo "args[$i]: \"${args[${i}]}\""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same deal here, this script just echos the args

done
2 changes: 1 addition & 1 deletion packages/exec/package.json
Expand Up @@ -32,7 +32,7 @@
"bugs": {
"url": "https://github.com/actions/toolkit/issues"
},
"devDependencies": {
"dependencies": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to call io.which and also some other io helpers

"@actions/io": "^1.0.1"
}
}