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

Set custom default shell #240

Closed
eine opened this issue Nov 27, 2019 · 5 comments
Closed

Set custom default shell #240

eine opened this issue Nov 27, 2019 · 5 comments
Assignees

Comments

@eine
Copy link

eine commented Nov 27, 2019

Current usage of action setup-msys2 requires commands to be preprended with msys2do, which is a cmd file (d:\a\_tmp\msys\msys2do.cmd). For example:

  - uses: numworks/setup-msys2@v1
  - run: msys2do makepkg-mingw -sCLfc --noconfirm --noprogressbar

On the one hand, it would be more natural if the shell was selectable as the built-in ones (cmd, powershell, bash), as it would allow a more consistent syntax:

  - uses: numworks/setup-msys2@v1
  - shell: msys2
    run: makepkg-mingw -sCLfc --noconfirm --noprogressbar

On the other hand, such a syntax would hopefully allow to use multi-line run fields, which is not possible ATM (numworks/setup-msys2#8):

  - uses: numworks/setup-msys2@v1
  - shell: msys2
    run: |
      uname -a
      makepkg-mingw -sCLfc --noconfirm --noprogressbar

Apart from that, it'd be useful to have a function in the toolkit core which allows to change the default shell for all the following steps in a job. A possible solution is to allow to set shell at job level (as it is supported for env). However, I think that supporting it in the toolkit would still be useful.

For example, until a month ago, the default shell in windows-latest was cmd. Then, it was changed to powershell: https://github.blog/changelog/2019-10-17-github-actions-default-shell-on-windows-runners-is-changing-to-powershell/. As a result, it is/was necessary to explicitly set shell: cmd in all the steps that won't work with powershell. This is still the case for workflows where certain tests are to be executed on cmd, powershell and/or bash.


Currently, when run: msys2do is used, either with cmd or powershell, msys2do.cmd is found in the path and it is successfully executed. However, it seems that shell: msys2do {0} does not take the PATH set through the toolkit into account,: ##[error]File not found: 'msys2do'

d:\a\_temp\msys\msys2do.cmd {0} is not supported, either: ##[error]Second path fragment must not be a drive or UNC name. (Parameter 'expression')

Using a relative path (..\_temp\msys\msys2do.cmd {0}), shows that the workdir of the parent script is: ##[error]Could not find a part of the path 'C:\hostedtoolcache\windows\Python\3.6.8\_temp\msys'..

Therefore, it seems that a possible solution is to place a dummy file in C:\hostedtoolcache\windows\Python\3.6.8\x64\msys2.cmd, so that a relative path is used. That file will execute d:\a\_temp\msys\msys2do.cmd, which will itself execute bash. Precisely, the following js snippet works:

    fs.writeFileSync('C:/hostedtoolcache/windows/Python/3.6.8/x64/msys2.cmd', [
      `setlocal`,
      `@echo off`,
      `set "args=%*"`,
      `set "args=%args:\\=/%"`,
      cmd + ` %args%`
    ].join('\r\n'));
  - shell: ./msys2.cmd {0}
    run: |
      pacman -Syu --noconfirm
      uname -a

Nonetheless, it feels hackish, specially because of the hardcoded location of msys2.cmd.

@eine
Copy link
Author

eine commented Dec 3, 2019

The same approach works on ubuntu-latest too, but it seems that the runner on GNU/Linux is written in Rust instead of Python:

https://github.com/1138-4EB/actions/blob/fef6d105b756a6ca057adf1721b05793555a5e9f/.github/workflows/push.yml#L35-L39

@ericsciple
Copy link
Collaborator

@dakale re shell feedback. It sounds like custom shell should consider any exported add-path when which'ing the tool

@dakale dakale transferred this issue from actions/toolkit Dec 19, 2019
@dakale
Copy link
Contributor

dakale commented Dec 19, 2019

I just made this change in #231

(We just open sourced the runner, so transferring this issue there)

This will go out with the 2.164.0 runner which is currently in prerelease but not rolled out to GitHub hosted machines

See: releases https://github.com/actions/runner/releases

You can download the prerelease package if you want to run it self-hosted to test

@eine
Copy link
Author

eine commented Jan 19, 2020

@dakale, what about the following comment? Should I open a new issue?

Apart from that, it'd be useful to have a function in the toolkit core which allows to change the default shell for all the following steps in a job. A possible solution is to allow to set shell at job level (as it is supported for env). However, I think that supporting it in the toolkit would still be useful.

@dakale
Copy link
Contributor

dakale commented Jan 21, 2020

@eine Feel free to open an issue in https://github.com/actions/toolkit so we can discuss it, but my gut is that it will need to be preceded by some additional conversation about the product in general, because we likely would want some formalized concept of run or job level defaults or configuration first, followed by expressing those values in .yaml, finally adding a way to dynamically mutate them at runtime (the toolkit function you propose)

So imo the toolkit doesnt feel like the best place to begin that discussion, but I am also not sure what would be better so we can start there and move it around as necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants