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

Default non-Windows bash is not invoked as documented #353

Closed
johnstevenson opened this issue Mar 2, 2020 · 5 comments
Closed

Default non-Windows bash is not invoked as documented #353

johnstevenson opened this issue Mar 2, 2020 · 5 comments
Assignees
Labels
papercut Runner Bug Bug fix scope to the runner

Comments

@johnstevenson
Copy link

Describe the bug
The documentation at https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepsrun implies that when a shell is not specified on a non-Windows platform, bash (if found) will be invoked as bash --noprofile --norc -eo pipefail {0}.

Fail-fast behavior using set -e o pipefail: Default for bash and built-in shell. It is also the default when you don't provide an option on non-Windows platforms.

However, this is not exactly the case.

To Reproduce
Steps to reproduce the behavior:

  1. Create an action without specifying a shell value:
    - name: No shell specified (default bash)
      run: echo Hello, world!
  1. Observe that it is executed as shell: /bin/bash -e {0} as per this example at https://github.com/johnstevenson/runner-actions-test/actions/runs/48283923 which also shows the documented behaviour when the shell is specified.

action-error-bash

Expected behavior
For the command to be executed as shell: /bin/bash --noprofile --norc -e -o pipefail {0}

Runner Version and Platform

Current runner version: 2.165.2
Tested on ubuntu-latest and macos-latest

What's not working?

Regardless of whether this is a documentation problem (or my incorrect interpretation), there is no consistency when invoking bash with and without a shell value.

The ScriptHandler:RunAsync code (also duplicated in ScriptHandler:PrintActionDetails) sets shellCommand as sh even if bash is found, resulting in the wrong format string being used:

if (string.IsNullOrEmpty(shell))
{
#if OS_WINDOWS
shellCommand = "pwsh";
commandPath = WhichUtil.Which(shellCommand, require: false, Trace, prependPath);
if (string.IsNullOrEmpty(commandPath))
{
shellCommand = "powershell";
Trace.Info($"Defaulting to {shellCommand}");
commandPath = WhichUtil.Which(shellCommand, require: true, Trace, prependPath);
}
ArgUtil.NotNullOrEmpty(commandPath, "Default Shell");
#else
shellCommand = "sh";
commandPath = WhichUtil.Which("bash", false, Trace, prependPath) ?? WhichUtil.Which("sh", true, Trace, prependPath);
#endif
argFormat = ScriptHandlerHelpers.GetScriptArgumentsFormat(shellCommand);

So any fix would be:

#if OS_WINDOWS
                shellCommand = "pwsh";
                commandPath = WhichUtil.Which(shellCommand, require: false, Trace, prependPath);
                if (string.IsNullOrEmpty(commandPath))
                {
                    shellCommand = "powershell";
                    Trace.Info($"Defaulting to {shellCommand}");
                    commandPath = WhichUtil.Which(shellCommand, require: true, Trace, prependPath);
                }
                ArgUtil.NotNullOrEmpty(commandPath, "Default Shell");
#else
                shellCommand = "bash";
                commandPath = WhichUtil.Which(shellCommand, require: false, Trace, prependPath)
                if (string.IsNullOrEmpty(commandPath))
                {
                    shellCommand = "sh";
                    Trace.Info($"Defaulting to {shellCommand}");
                    commandPath = WhichUtil.Which(shellCommand, require: true, Trace, prependPath);
                }
                ArgUtil.NotNullOrEmpty(commandPath, "Default Shell");
#endif
                argFormat = ScriptHandlerHelpers.GetScriptArgumentsFormat(shellCommand);

I'd happily PR this if needed and if I could find out how you debug an action in Visual Studio (are there any docs about this?)

@johnstevenson johnstevenson added the bug Something isn't working label Mar 2, 2020
@TingluoHuang TingluoHuang added runner Runner Bug Bug fix scope to the runner and removed bug Something isn't working runner labels Jun 6, 2020
@bendavies
Copy link

Just came across this - this is still an issue.

@johnstevenson
Copy link
Author

The behaviour is still the same: https://github.com/johnstevenson/runner-actions-test/runs/1317090059
and the documentation hasn't changed, so yes.

@amacneil
Copy link

Still appears to be a problem

@thboop
Copy link
Collaborator

thboop commented Mar 14, 2022

This is working as intended, thought I definitely understand with the docs in their current state this is very confusing.

For hosted runners, we know bash is installed, so users can expect their steps to run on it. For self hosted runners, we don't have the same luxury.
-eo pipefail would cause scripts executing on bash to fail where there would have passed on a sh instance, which could be confusing and tricky to debug for workflow authors who don't have great visibility into what software is installed on the runner they landed on (or what happened to be in the PATH at the time of the run).

That being said, we should update the docs to clarify this behavior. I've filed an issue to update the documentation so we can improve this experience.

Thank you for the report! I'm going to close out this issue for now. If you need this behavior, you can use the job defaults shell and specify it as bash to invoke all steps with bash --noprofile --norc -eo pipefail {0}

@thboop thboop closed this as completed Mar 14, 2022
benknoble added a commit to benknoble/qi that referenced this issue Jul 25, 2022
This is _supposed_ to be the default according to
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_id_stepsshell
but actions/runner#353 indicates that this is
_still_ not really the case (but the end result is confusing), so we'll
be explicit here.
benknoble added a commit to benknoble/qi that referenced this issue Jul 25, 2022
This is _supposed_ to be the default according to
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_id_stepsshell
but actions/runner#353 indicates that this is
_still_ not really the case (but the end result is confusing), so we'll
be explicit here.
@Makeshift
Copy link

For self hosted runners, we don't have the same luxury.

I'm aware this issue has been closed for a long time, and this does make sense for self-hosted runners and code running in the self-hosted environment.

However, for containers, wouldn't it be preferable to query the default shell before starting the container and set the job defaults appropriately as part of container start?

eg.

$ docker inspect catthehacker/ubuntu:act-latest | jq -c '.[0].Config.Shell'
["/bin/bash","--login","-e","-o","pipefail","-c"]

Let me know if this should be a new issue/suggestion.

jimmykarily added a commit to kairos-io/kairos that referenced this issue Aug 21, 2023
because when apt fails to fetch information (with errors) the whole job
should fail

actions/runner#353 (comment)
actions/runner#353

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
jimmykarily added a commit to kairos-io/kairos that referenced this issue Aug 21, 2023
* Replace bootargs.cfg file with a stage that generates it

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Move nvidia specific files to cloud-init

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Move alpine files to cloud-init

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Add TODO to fix nvidia partitioning config

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Remove not used overlay file for opensuse

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Install overlay files as a package

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Simplify framework target

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Remove `overlay/` directory from yamlling

because it doesn't exist anymore

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Bump luet repo

to get this fix:

kairos-io/packages#386

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Don't try to lint non-existent dir

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

* Add pipefail to default shell

because when apt fails to fetch information (with errors) the whole job
should fail

actions/runner#353 (comment)
actions/runner#353

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>

---------

Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
papercut Runner Bug Bug fix scope to the runner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants