toolrunner should which tool by ericsciple · Pull Request #220 · actions/toolkit

ericsciple · 2019-11-14T17:38:43Z

fixes #218

joshmgross · 2019-11-15T01:52:47Z

packages/exec/src/toolrunner.ts

   * @returns   number
   */
  async exec(): Promise<number> {
+    this.toolPath = await io.which(this.toolPath, true)


Is this a breaking change?

my intention was that it doesnt break anything

for example which returns the path if it is already rooted and executable.

however, now that you got me thinking about it more, i think there are some edge cases wrt relative pathing.... i'll add code to handle.

ok i added the code above to root unrooted paths that contain relative pathing

in addition to checking the PATH, the which function also handles checking rooted paths

now the changes shouldnt be a breaking change. hard to imagine it would have practically broken anyone, but i'd rather cover edge cases to be on the safe side. Also now the changes align better with the default behavior of spawn.

i'll add unit tests tomorrow for the unrooted-relative-pathing cases

No, it's not a breaking change. It's additive. If you were passing in a fully qualified path, which will not changing anything. It will however add the ability to just pass in a tool name and it just works (as well as fixing some other issues and bugs).

@bryanmacfarlane Originally it was breaking, exec of a relative path would no longer work if we automatically which. That should be fixed now with @ericsciple's latest change

packages/exec/README.md

.github/workflows/unit-tests.yml

ericsciple · 2019-11-15T20:42:41Z

.github/workflows/unit-tests.yml

+    name: Build
+
+    strategy:
+      matrix:


leverage matrix instead of duplicate 3 times

ericsciple · 2019-11-15T20:43:16Z

package.json

    "lint": "eslint packages/**/*.ts",
    "new-package": "scripts/create-package",
-    "test": "jest",
-    "test-ci": "jest --testPathIgnorePatterns=\"<rootDir>/packages/exec/__tests__/exec.test.ts\""


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

ericsciple · 2019-11-15T20:43:55Z

packages/exec/README.md

-const pythonPath: string = await io.which('python', true)
-
-await exec.exec(`"${pythonPath}"`, ['main.py']);
+await exec.exec('"/path/to/my-tool"', ['arg1']);


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

ericsciple · 2019-11-15T20:44:28Z

packages/exec/__tests__/exec.test.ts

    }
  })

+  it('Runs exec successfully with command from PATH', async () => {


coverage for existing functionality

ericsciple · 2019-11-15T20:45:33Z

packages/exec/__tests__/exec.test.ts

    fs.unlinkSync(semaphorePath)
  })

+  it('Exec roots relative tool path using unrooted options.cwd', async () => {


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)

ericsciple · 2019-11-15T20:47:23Z

packages/exec/__tests__/exec.test.ts

      )
    })

+    it('execs .cmd from path (Windows)', async () => {


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.

ericsciple · 2019-11-15T20:50:48Z

packages/exec/__tests__/exec.test.ts

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


note, without extension, toolrunner should resolve the extension

ericsciple · 2019-11-15T20:52:18Z

packages/exec/__tests__/scripts/print-args-cmd.cmd

@@ -0,0 +1,12 @@
+@echo off


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

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"

ericsciple · 2019-11-15T20:55:53Z

packages/exec/__tests__/scripts/print-args-sh.sh

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


same deal here, this script just echos the args

ericsciple · 2019-11-15T20:56:49Z

packages/exec/package.json

    "url": "https://github.com/actions/toolkit/issues"
  },
-  "devDependencies": {
+  "dependencies": {


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

thboop · 2019-11-18T20:00:52Z

packages/exec/__tests__/exec.test.ts

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


Consider adding a \ test for windows

thboop · 2019-11-18T20:34:55Z

packages/exec/src/toolrunner.ts

+        (IS_WINDOWS && this.toolPath.includes('\\')))
+    ) {
+      // prefer options.cwd if it is specified, however options.cwd may also need to be rooted
+      this.toolPath = path.resolve(


this is super clean 🥇

thboop

Clean lgtm

thboop · 2019-11-18T21:12:18Z

We have existing tests for exec.exec ("full path to executable"), these tests should cover the rest of the cases and fix this bug

eine · 2019-11-20T13:41:35Z

packages/tool-cache/package-lock.json

 {
 	"name": "@actions/tool-cache",
-	"version": "1.1.1",
+	"version": "1.1.2",


The version of tool-cache was bumped, even though it seems that none of the files in packages/tool-cache was changed. Is this expected?

Should it mean to reflect that tool-cache is updated to use this new feature of exec, I'd expect files such as packages/tool-cache/src/tool-cache.ts to be modified:

toolkit/packages/tool-cache/src/tool-cache.ts

Line 175 in 5c89429

const powershellPath: string = await io.which('powershell', true)

toolkit/packages/tool-cache/src/tool-cache.ts

Line 203 in 5c89429

const tarPath: string = await io.which('tar', true)

toolkit/packages/tool-cache/src/tool-cache.ts

Line 239 in 5c89429

const powershellPath = await io.which('powershell')

toolkit/packages/tool-cache/src/tool-cache.ts

Line 254 in 5c89429

const unzipPath = await io.which('unzip')

ericsciple changed the title ~~toolrunner should which tool before invoking~~ toolrunner should which tool Nov 15, 2019

ericsciple force-pushed the users/ericsciple/m161whichcmd branch from 708b724 to 99befb3 Compare November 15, 2019 01:34

joshmgross approved these changes Nov 15, 2019

View reviewed changes

ericsciple commented Nov 15, 2019

View reviewed changes

packages/exec/README.md Outdated Show resolved Hide resolved

toolrunner should which tool before invoking

a5c2be2

ericsciple force-pushed the users/ericsciple/m161whichcmd branch from 70e0eef to a5c2be2 Compare November 15, 2019 20:38

ericsciple commented Nov 15, 2019

View reviewed changes

.github/workflows/unit-tests.yml Show resolved Hide resolved

ericsciple commented Nov 15, 2019

View reviewed changes

thboop reviewed Nov 18, 2019

View reviewed changes

thboop approved these changes Nov 18, 2019

View reviewed changes

ericsciple merged commit 5c89429 into master Nov 18, 2019

ericsciple deleted the users/ericsciple/m161whichcmd branch November 18, 2019 21:20

eine reviewed Nov 20, 2019

View reviewed changes

Conversation

ericsciple commented Nov 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thboop Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thboop left a comment

Choose a reason for hiding this comment

Uh oh!

thboop commented Nov 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thboop Nov 18, 2019 •

edited

Loading