Set `shell: bash` by default on all workflows #3091

redsun82 · 2025-09-09T10:24:13Z

The default behaviour of steps in absence of a shell settings is error prone:

if the steps runs on multiple platforms, it will use a different shell on POSIX and Windows, which can lead to surprising effects.
if the step runs multiple lines, it won't fail if anything else than the last line fails, which can also lead to workflows hiding failures.

This change just sets defaults: {run: {shell: bash}} on all workflows (generated and written by hand), and removes shell: bash from individual steps as no more needed. With this convention in place, we don't risk falling in that trap again.

To add up motivation to why we want to do this: this has uncovered some checks that we weren't really running on Windows, and that wereactually failing there because of CRLF vs LF issues. Luckily this was only concerning dev functionality rather than production one. In any case this PR fixes those issues as well.

Risk assessment

For internal use only. Please select the risk level of this change:

Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.
High risk: Changes are not fully under feature flags, have limited visibility and/or cannot be tested outside of production.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Consider adding a changelog entry for this change.
Confirm the readme and docs have been updated if necessary.

Copilot

Pull Request Overview

This PR implements a consistent shell configuration across all GitHub Actions workflows by setting shell: bash as the default. This addresses potential issues where workflows might use different shells on different platforms (POSIX vs Windows) and ensures proper error handling for multi-line steps by preventing workflows from hiding failures when intermediate commands fail.

Key changes:

Add defaults: { run: { shell: bash } } to all workflow files (both manually written and auto-generated)
Remove individual shell: bash declarations from workflow steps since they're now redundant
Update the workflow generation script to include the default shell setting in generated workflows

Reviewed Changes

Copilot reviewed 114 out of 114 changed files in this pull request and generated 1 comment.

File	Description
pr-checks/sync.py	Updated workflow generation script to include bash shell default in all generated workflows
pr-checks/checks/*.yml	Removed redundant `shell: bash` declarations from individual workflow steps in template files
.github/workflows/*.yml	Added default shell configuration and removed redundant shell declarations from manually written workflows
.github/workflows/__*.yml	Added default shell configuration and removed redundant shell declarations from auto-generated workflows

Comments suppressed due to low confidence (8)

pr-checks/checks/unset-environment.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Check database locations" or similar.

name: "Test unsetting environment variables"

pr-checks/checks/go-tracing-legacy-workflow.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify Go database creation" or similar.

name: "Go: tracing with legacy workflow"

pr-checks/checks/go-tracing-custom-build-steps.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify build environment and database" or similar.

name: "Go: tracing with custom build steps"

pr-checks/checks/go-tracing-autobuilder.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify Go autobuilder execution" or similar.

name: "Go: tracing with autobuilder step"

pr-checks/checks/go-indirect-tracing-workaround.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify indirect tracing workaround" or similar.

name: "Go: workaround for indirect tracing"

pr-checks/checks/cpp-deptrace-enabled.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify dependency installation" or similar.

name: "C/C++: autoinstalling dependencies (Linux)"

pr-checks/checks/cpp-deptrace-enabled-on-macos.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify dependency installation is disabled" or similar.

name: "C/C++: autoinstalling dependencies is skipped (macOS)"

pr-checks/checks/cpp-deptrace-disabled.yml:1

This step is missing a name field, which makes it harder to identify in workflow logs. Consider adding a descriptive name like "Verify dependency installation is disabled" or similar.

name: "C/C++: disabling autoinstalling dependencies (Linux)"

pr-checks/sync.py

mbg

Thanks for noticing the problem, investigating the issue, and proposing this fix!

This broadly looks good. I only have a few minor, non-blocking suggestions and thoughts.

mbg · 2025-09-09T14:44:12Z

pr-checks/sync.sh

 cd "$(dirname "$0")"
 python3 -m venv env
-source env/bin/activate
+source env/*/activate


Python 🤯 🤦🏻‍♂️

mbg · 2025-09-09T14:58:40Z

.github/workflows/pr-checks.yml


    steps:
+      - name: Prepare git (Windows)
+        if: runner.os == 'Windows'


Minor: Checking matrix.os == 'windows-latest' might be a bit easier to work with here, since the string appears in the matrix and is therefore less of a magic value. I.e. without checking what possible values runner.os has, I wouldn't be sure that 'Windows' is correct.

on the other hand, checking runner.os will work even if we change the runners (for example pinning the runner version) or if we copy such a snippet in another workflow. All in all, I prefer the stability and independence of context of runner.os, it's just less prone to errors. One thing to note, all string comparisons on actions are case insensitive, so runner.os == 'windows' works as well. runner.os and runner.arch are good things to keep in mind when writing workflows.

My comment was less about whether string comparison is case-sensitive or what runner properties exist, but more that runner.os could theoretically have other plausible values like 'win' or 'win11' etc. -- we know as routine Actions users that this may not be the case, but it is not as obvious generally as the matrix.os comparison.

I think runner.os is fairly stable - it's always one of Linux/Windows/macOS according to https://docs.github.com/en/actions/reference/workflows-and-actions/contexts#runner-context (and we've relied on that in the past). Agree it's not super obvious from your workflow though.

mbg · 2025-09-09T14:59:25Z

.github/workflows/pr-checks.yml

    steps:
+      - name: Prepare git (Windows)
+        if: runner.os == 'Windows'
+        run: git config --global core.autocrlf false


I'm mildly surprised that there isn't an option for actions/checkout@v5 to set this / that true is the default on Windows.

indeed, it has been discussed before. FYI, we also do this on all semmle-code workflows

.github/workflows/codeql.yml

henrymercer · 2025-09-10T10:42:03Z

pr-checks/checks/cpp-deptrace-disabled.yml

      working-directory: autobuild-dir
    env:
      CODEQL_EXTRACTOR_CPP_AUTOINSTALL_DEPENDENCIES: false
-  - shell: bash


Here, and in a couple of other places, the PR removes the - for the start of the new step as well as just the shell directive, so the workflow becomes invalid. I think we want to start a new step with the run.

Unfortunately, we just get pending checks rather than failures for these invalid workflows. Perhaps a tool like actionlint could surface these errors more visibly?

I guess it might complain that the steps have both uses and run which isn't valid. At the same time, I feel like this could be a CodeQL actions query too.

aaah, hadn't noticed in my automatic search and remove, good catch!

mbg

@henrymercer noticed that some of the workflows didn't start because you accidentally removed -s where shell was the first key in a step definition. I have marked all the places where I have spotted that problem, but it would be good if you could check as well to make sure I haven't missed any.

mbg · 2025-09-10T10:43:24Z

pr-checks/checks/cpp-deptrace-disabled.yml

      working-directory: autobuild-dir
    env:
      CODEQL_EXTRACTOR_CPP_AUTOINSTALL_DEPENDENCIES: false
-  - shell: bash


You removed the - here by accident. May be worth adding a name to make this more obvious.

mbg · 2025-09-10T10:43:32Z

pr-checks/checks/cpp-deptrace-enabled-on-macos.yml

      working-directory: autobuild-dir
    env:
      CODEQL_EXTRACTOR_CPP_AUTOINSTALL_DEPENDENCIES: true
-  - shell: bash


mbg · 2025-09-10T10:43:40Z

pr-checks/checks/cpp-deptrace-enabled.yml

      working-directory: autobuild-dir
    env:
      CODEQL_EXTRACTOR_CPP_AUTOINSTALL_DEPENDENCIES: true
-  - shell: bash


mbg · 2025-09-10T10:43:56Z

pr-checks/checks/go-indirect-tracing-workaround.yml

-    shell: bash
    run: go build main.go
  - uses: ./../action/analyze
-  - shell: bash


mbg · 2025-09-10T10:44:02Z

pr-checks/checks/go-tracing-autobuilder.yml

      tools: ${{ steps.prepare-test.outputs.tools-url }}
  - uses: ./../action/autobuild
  - uses: ./../action/analyze
-  - shell: bash


mbg · 2025-09-10T10:44:10Z

pr-checks/checks/go-tracing-custom-build-steps.yml

-    shell: bash
    run: go build main.go
  - uses: ./../action/analyze
-  - shell: bash


mbg · 2025-09-10T10:44:14Z

pr-checks/checks/go-tracing-legacy-workflow.yml

      languages: go
      tools: ${{ steps.prepare-test.outputs.tools-url }}
  - uses: ./../action/analyze
-  - shell: bash


mbg · 2025-09-10T10:44:53Z

pr-checks/checks/unset-environment.yml

    id: analysis
    with:
      upload-database: false
-  - shell: bash


mbg · 2025-09-11T16:08:33Z

.github/workflows/pr-checks.yml


    steps:
+      - name: Prepare git (Windows)
+        if: runner.os == 'Windows'


My comment was less about whether string comparison is case-sensitive or what runner properties exist, but more that runner.os could theoretically have other plausible values like 'win' or 'win11' etc. -- we know as routine Actions users that this may not be the case, but it is not as obvious generally as the matrix.os comparison.

.github/workflows/codeql.yml

Co-authored-by: Henry Mercer <[email protected]>

Set shell: bash by default on all workflows

1b8f0ff

redsun82 requested a review from a team as a code owner September 9, 2025 10:24

Copilot AI review requested due to automatic review settings September 9, 2025 10:24

Copilot AI reviewed Sep 9, 2025

View reviewed changes

pr-checks/sync.py Show resolved Hide resolved

redsun82 force-pushed the redsun82/fix-windows-ci branch 2 times, most recently from 6e07bb5 to 0c065fa Compare September 9, 2025 12:00

redsun82 added 2 commits September 9, 2025 14:00

Sort out windows CRLF mess

0c065fa

fix codeql.yml codeql invocation on windows

c778749

mbg reviewed Sep 9, 2025

View reviewed changes

Merge branch 'main' into redsun82/fix-windows-ci

3bf58bb

henrymercer reviewed Sep 10, 2025

View reviewed changes

mbg requested changes Sep 10, 2025

View reviewed changes

redsun82 added 3 commits September 11, 2025 17:41

Merge branch 'main' into redsun82/fix-windows-ci

d797efb

Address review

856e1e5

Fix accidental removal of - shell: bash lines

4e1dadc

redsun82 requested review from mbg and henrymercer September 11, 2025 15:55

mbg previously approved these changes Sep 11, 2025

View reviewed changes

redsun82 enabled auto-merge September 11, 2025 16:16

henrymercer reviewed Sep 12, 2025

View reviewed changes

.github/workflows/codeql.yml Outdated Show resolved Hide resolved

Update .github/workflows/codeql.yml

2b7d487

Co-authored-by: Henry Mercer <[email protected]>

redsun82 dismissed mbg’s stale review via 2b7d487 September 12, 2025 16:20

henrymercer approved these changes Sep 12, 2025

View reviewed changes

redsun82 merged commit aa90e97 into main Sep 12, 2025
291 checks passed

redsun82 deleted the redsun82/fix-windows-ci branch September 12, 2025 16:47

github-actions bot mentioned this pull request Sep 25, 2025

Merge main into releases/v3 #3149

Merged

8 tasks

Set shell: bash by default on all workflows #3091

Set shell: bash by default on all workflows #3091

Conversation

redsun82 commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk assessment

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

mbg left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbg Sep 10, 2025 • 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!

mbg left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Set `shell: bash` by default on all workflows #3091

Set `shell: bash` by default on all workflows #3091

redsun82 commented Sep 9, 2025 •

edited

Loading

mbg Sep 10, 2025 •

edited

Loading