shell from prependpath #231

dakale · 2019-12-17T21:32:06Z

Fix for: https://github.com/actions/toolkit/issues/232

Setting a prepended path for a tool installation should work for anything downstream of that step (within a job). When we locate the tool via path to use for the run.shell option, we are using just the process env PATH and not the runner-tracked prepended paths

Tested with this sample:

on:
  push:
    paths:
    - .github/workflows/prepend-shell.yaml
     
jobs:
  host:
    runs-on: [ self-hosted, linux ]
    steps:
    - name: Check existing node from bash
      run: echo $PATH && node --version

    - name: Verify existing node via interpreter
      run: 'console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)'
      shell: node {0}

    - uses: actions/setup-node@master
      with:
        node-version: 12.x

    - name: Check new node from bash
      run: node --version

    - name: Verify new node via interpreter
      run: 'console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)'
      shell: node {0}
 
  container:
    runs-on: [ self-hosted, linux ]
    container: node
    steps:
    - name: Check existing node from bash
      run: echo $PATH && node --version

    - name: Verify existing node via interpreter
      run: 'console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)'
      shell: node {0}

    - uses: actions/setup-node@master
      with:
        node-version: 12.x

    - name: Check new node from bash
      run: node --version

    - name: Verify new node via interpreter
      run: 'console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)'
      shell: node {0}

Previously, in the first job the last step (Verify new node via interpreter) would have printed 10 (installed on my vm) instead of 12. For containers it works because we use the container runtime to locate it (we just exec node, not exec by full path since we didnt know the path inside the container)

With this fix the node installed on the host by setup-node is used as the shell

In case anyone cares now or later, heres the (slightly trimmed down) logs from my sample:

2019-12-17T21:14:12.7202576Z ##[group]Run echo $PATH && node --version
2019-12-17T21:14:12.7203344Z �[36;1mecho $PATH && node --version�[0m
2019-12-17T21:14:12.7226176Z shell: /bin/bash -e {0}
2019-12-17T21:14:12.7226652Z ##[endgroup]
2019-12-17T21:14:12.7293142Z /home/dakale/bin:/home/dakale/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/dakale/.dotnet/tools:/home/dakale/n/bin
2019-12-17T21:14:12.7306785Z v10.13.0

2019-12-17T21:14:12.7473744Z ##[group]Run console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)
2019-12-17T21:14:12.7474151Z �[36;1mconsole.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)�[0m
2019-12-17T21:14:12.7495866Z shell: /home/dakale/n/bin/node {0}
2019-12-17T21:14:12.7496187Z ##[endgroup]

2019-12-17T21:14:12.8050405Z PATH: /home/dakale/bin:/home/dakale/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/dakale/.dotnet/tools:/home/dakale/n/bin
2019-12-17T21:14:12.8052462Z ---
2019-12-17T21:14:12.8053138Z Version: v10.13.0

2019-12-17T21:14:12.8182343Z ##[group]Run actions/setup-node@master
2019-12-17T21:14:12.8182672Z with:
2019-12-17T21:14:12.8182891Z   node-version: 12.x
2019-12-17T21:14:12.8183400Z   always-auth: false
2019-12-17T21:14:12.8183668Z ##[endgroup]

2019-12-17T21:14:13.0044876Z ##[group]Run node --version
2019-12-17T21:14:13.0045246Z �[36;1mnode --version�[0m
2019-12-17T21:14:13.0071088Z shell: /bin/bash -e {0}
2019-12-17T21:14:13.0071377Z ##[endgroup]
2019-12-17T21:14:13.0219003Z v12.13.1

2019-12-17T21:14:13.0304519Z ##[group]Run console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)
2019-12-17T21:14:13.0304891Z �[36;1mconsole.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)�[0m
2019-12-17T21:14:13.0306252Z shell: /home/dakale/GitHub/actions/runner/_layout/_work/_tool/node/12.13.1/x64/bin/node {0}
2019-12-17T21:14:13.0306526Z ##[endgroup]

2019-12-17T21:14:13.0604036Z PATH: /home/dakale/GitHub/actions/runner/_layout/_work/_tool/node/12.13.1/x64/bin:/home/dakale/bin:/home/dakale/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/dakale/.dotnet/tools:/home/dakale/n/bin
2019-12-17T21:14:13.0605937Z ---
2019-12-17T21:14:13.0606646Z Version: v12.13.1

2019-12-17T21:14:02.0223617Z ##[group]Run echo $PATH && node --version
2019-12-17T21:14:02.0224129Z �[36;1mecho $PATH && node --version�[0m
2019-12-17T21:14:02.0225754Z shell: sh -e {0}
2019-12-17T21:14:02.0226168Z ##[endgroup]
2019-12-17T21:14:02.3071565Z /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
2019-12-17T21:14:02.3080994Z v13.3.0

2019-12-17T21:14:02.3534791Z ##[group]Run console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)
2019-12-17T21:14:02.3535363Z �[36;1mconsole.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)�[0m
2019-12-17T21:14:02.3536167Z shell: node {0}
2019-12-17T21:14:02.3536400Z ##[endgroup]
2019-12-17T21:14:02.7189231Z PATH: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
2019-12-17T21:14:02.7191462Z ---
2019-12-17T21:14:02.7192244Z Version: v13.3.0

2019-12-17T21:14:02.7745590Z ##[group]Run actions/setup-node@master
2019-12-17T21:14:02.7745974Z with:
2019-12-17T21:14:02.7746166Z   node-version: 12.x
2019-12-17T21:14:02.7746354Z   always-auth: false
2019-12-17T21:14:02.7746538Z ##[endgroup]
2019-12-17T21:14:02.7752215Z ##[command]/usr/bin/docker exec  ce9b8379b79e94165c8ebb81fb7a94b8e0478b5dc26c12f4217fe7ccfb73eb0c sh -c "cat /etc/*release | grep ^ID"
2019-12-17T21:14:03.1148925Z Running JavaScript Action with default external tool: node12

2019-12-17T21:14:03.6762793Z ##[group]Run node --version
2019-12-17T21:14:03.6763138Z �[36;1mnode --version�[0m
2019-12-17T21:14:03.6763693Z shell: sh -e {0}
2019-12-17T21:14:03.6763917Z ##[endgroup]
2019-12-17T21:14:04.0263781Z v12.13.1

2019-12-17T21:14:04.0816365Z ##[group]Run console.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)
2019-12-17T21:14:04.0816760Z �[36;1mconsole.log(`PATH: ${process.env.PATH}\n---\nVersion: ${process.version}`)�[0m
2019-12-17T21:14:04.0817198Z shell: node {0}
2019-12-17T21:14:04.0817405Z ##[endgroup]
2019-12-17T21:14:04.5034874Z PATH: /__w/_tool/node/12.13.1/x64/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
2019-12-17T21:14:04.5038199Z ---
2019-12-17T21:14:04.5038944Z Version: v12.13.1

dakale · 2019-12-17T21:33:05Z

src/Runner.Worker/Handlers/ScriptHandler.cs

@@ -56,26 +57,27 @@ public override void PrintActionDetails(ActionRunStage stage)
            string shellCommand;
            string shellCommandPath = null;
            bool validateShellOnHost = !(StepHost is ContainerStepHost);
+            string prependPath = string.Join(Path.PathSeparator.ToString(), ExecutionContext.PrependPath.Reverse<string>());


This is just duplicated from Handler.AddPrependPathToEnvironment so it should function the same. Might could refactor a bit to reduce duplication

TingluoHuang · 2019-12-17T21:46:07Z

src/Runner.Worker/Handlers/ScriptHandler.cs

@@ -144,23 +146,27 @@ public async Task RunAsync(ActionRunStage stage)
            Inputs.TryGetValue("shell", out var shell);
            var isContainerStepHost = StepHost is ContainerStepHost;

+            // Prepend PATH before locating shell tool
+            AddPrependPathToEnvironment();


do we still need move AddPrependPathToEnvironment up?

Actually no, I misunderstood that it was using System.Environment vs ExecutionContext.Environment so this doesnt do anything. Ill move it back to minimize unnecessary changes

TingluoHuang · 2019-12-17T21:46:52Z

src/Runner.Sdk/Util/WhichUtil.cs

@@ -7,7 +7,7 @@ namespace GitHub.Runner.Sdk
 {
    public static class WhichUtil
    {
-        public static string Which(string command, bool require = false, ITraceWriter trace = null)
+        public static string Which(string command, bool require = false, ITraceWriter trace = null, string prependPath = null)


maybe check all reference of the Which method to make sure we don't miss something else?

I dont see anywhere else that it would be useful. Most/all other uses are "internal" such as locating chmod, chown, and cmd/tar (in self-updater) so it seems like we should avoid letting users do things that might get in the way of runner operations

Other than that, the GitSourceProvider uses it, and some people may want to use an updated version of git, but Im not very familiar with what this is used for, since I thought we just check out via action now. Is that somewhere this would be useful?

i think we should be good, leave the gitsourceprovider along since we already switch to checkout@v2 which has the right behavior anyway. :)

* Prepend path before locating shell tool * Join optional prepended path to path before searching it * Use prepended path when whiching shell tool * Addition prependPath location * Also use prepended paths when writing out run details * Small tweak to undo unnecessary change

David Kale added 5 commits December 17, 2019 12:47

Prepend path before locating shell tool

2ffce0e

Join optional prepended path to path before searching it

092847a

Use prepended path when whiching shell tool

2de5fcb

Addition prependPath location

4608399

Also use prepended paths when writing out run details

a6dc4e1

dakale commented Dec 17, 2019

View reviewed changes

TingluoHuang reviewed Dec 17, 2019

View reviewed changes

TingluoHuang approved these changes Dec 17, 2019

View reviewed changes

Small tweak to undo unnecessary change

a5172c8

dakale merged commit 70a09bc into master Dec 18, 2019

dakale deleted the 275-shell-from-prependpath branch December 18, 2019 20:00

dakale mentioned this pull request Dec 19, 2019

Set custom default shell #240

Closed

eine mentioned this pull request Dec 19, 2019

experimental support for custom shell (multi-line run steps) numworks/setup-msys2#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell from prependpath #231

shell from prependpath #231

dakale commented Dec 17, 2019

dakale Dec 17, 2019

TingluoHuang Dec 17, 2019

dakale Dec 18, 2019

TingluoHuang Dec 17, 2019

dakale Dec 18, 2019

TingluoHuang Dec 18, 2019

shell from prependpath #231

shell from prependpath #231

Conversation

dakale commented Dec 17, 2019

dakale Dec 17, 2019

Choose a reason for hiding this comment

TingluoHuang Dec 17, 2019

Choose a reason for hiding this comment

dakale Dec 18, 2019

Choose a reason for hiding this comment

TingluoHuang Dec 17, 2019

Choose a reason for hiding this comment

dakale Dec 18, 2019

Choose a reason for hiding this comment

TingluoHuang Dec 18, 2019

Choose a reason for hiding this comment