-
Notifications
You must be signed in to change notification settings - Fork 40
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
Pass the shell arguments directly to bash #44
Conversation
One possibility, I'm open to ideas... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like the change. Specially, getting rid of OLD_PWD
.
However, I'm curious about the behaviour when the default shell is set. FTR, some months ago two different intermediate files were provided: msys2do.cmd
and msys2.cmd
. The former was the original one, which required to use it explicitly (run: msys2do whatever
, and the latter was the one to be used with run: |
. Then, I unified both of them.
Now, according to your changes, I would expect the following case to fail:
job:
defaults:
run:
shell: msys2 {0}
steps:
- uses: actions/checkout@v2
with:
fetch-depth: 0
- name: run action
uses: msys2/setup-msys2@v1
with:
update: true
- run: echo "hello"
That is, the command is echo "hello"
, so the default shell should be msys2 -c {0}
. Nevertheless, that is not the case, because job defaultdirty
is precisely that, and it works. So, what's going on?
To make the point more clear, see jobs shell
and fshell
in https://github.com/msys2/setup-msys2/runs/867268672?check_suite_focus=true
Hence, it seems that the only context where -c
is required is for executing msys2 -c "a command"
inside a cmd
or powershell
. In all other cases, the arguments are passed as an intermediate file. It would be useful to make this explicit in the README.
As a result, although this is a breaking change, I would consider it of minor impact (most users should not be using such syntax). We can include it in v1.2.0.
We could also add a
When you set the shell the content is always passed through a file according to this https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#custom-shell so, at least for me, everything works as expected. Am I missing something? |
I'd rather enforce setting the shell and keeping Calling Hence, I think now is a good moment to introduce this breaking change, before the action is more popular.
No, it is coherent with the current docs. But the environment has been changing and I was slightly outdated. |
This allows us to run the "run" script using "set -eo pipefail" by default. The previous wrapper executed the arguments as a bash command string, which allowed the user to run any command, but didn't give us a chance to special case if the argument was a script passed from the GHA "run" command. This is a breaking change. Porting guide: * Instead of `msys2 mybinary` you now have to run `msys2 -c 'mybinary'` * To revert the bash error settings you can use `set +e` and `set +o pipefail` in your run script
This allows us to run the "run" script using "set -eo pipefail" by default.
The previous wrapper executed the arguments as a bash command string, which
allowed the user to run any command, but didn't give us a chance to special case
if the argument was a script passed from the GHA "run" command.
This is a breaking change.
Porting guide:
msys2 mybinary
you now have to runmsys2 -c 'mybinary'
set +e
andset +o pipefail
in your run script
See #43