Skip to content

Set shell: bash by default on all workflows #3091

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.

Sign up for GitHub

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

Merged
redsun82 merged 8 commits into from
Sep 12, 2025
Merged

Set shell: bash by default on all workflows #3091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1b8f0ff
Set `shell: bash` by default on all workflows
redsun82 Sep 9, 2025
0c065fa
Sort out windows CRLF mess
redsun82 Sep 9, 2025
c778749
fix `codeql.yml` codeql invocation on windows
redsun82 Sep 9, 2025
3bf58bb
Merge branch 'main' into redsun82/fix-windows-ci
mbg Sep 9, 2025
d797efb
Merge branch 'main' into redsun82/fix-windows-ci
redsun82 Sep 11, 2025
856e1e5
Address review
redsun82 Sep 11, 2025
4e1dadc
Fix accidental removal of `- shell: bash` lines
redsun82 Sep 11, 2025
2b7d487
Update .github/workflows/codeql.yml
redsun82 Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Sort out windows CRLF mess
  • Loading branch information
@redsun82
redsun82 committed Sep 9, 2025
commit 0c065fa4cf5dc6e4738b887f00d62650c38066ff
4 changes: 4 additions & 0 deletions .github/workflows/pr-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ jobs:
timeout-minutes: 45

steps:
- name: Prepare git (Windows)
if: runner.os == 'Windows'
Copy link
Copy Markdown
Member

@mbg mbg Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@redsun82 redsun82 Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@mbg mbg Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@adityasharad adityasharad Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

run: git config --global core.autocrlf false
Copy link
Copy Markdown
Member

@mbg mbg Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@redsun82 redsun82 Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


- uses: actions/checkout@v5

- name: Set up Node.js
Expand Down
6 changes: 3 additions & 3 deletions pr-checks/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ def writeHeader(checkStream):
})

raw_file = this_dir.parent / ".github" / "workflows" / f"__{checkName}.yml.raw"
with open(raw_file, 'w') as output_stream:
with open(raw_file, 'w', newline='\n') as output_stream:
writeHeader(output_stream)
yaml.dump({
'name': f"PR Check - {checkSpecification['name']}",
Expand Down Expand Up @@ -274,7 +274,7 @@ def writeHeader(checkStream):
}, output_stream)

with open(raw_file, 'r') as input_stream:
with open(this_dir.parent / ".github" / "workflows" / f"__{checkName}.yml", 'w') as output_stream:
with open(this_dir.parent / ".github" / "workflows" / f"__{checkName}.yml", 'w', newline='\n') as output_stream:
content = input_stream.read()
output_stream.write("\n".join(list(map(lambda x:x.rstrip(), content.splitlines()))+['']))
os.remove(raw_file)
Expand Down Expand Up @@ -328,7 +328,7 @@ def writeHeader(checkStream):
}, output_stream)

with open(raw_file, 'r') as input_stream:
with open(this_dir.parent / ".github" / "workflows" / f"__{collection_name}.yml", 'w') as output_stream:
with open(this_dir.parent / ".github" / "workflows" / f"__{collection_name}.yml", 'w', newline='\n') as output_stream:
content = input_stream.read()
output_stream.write("\n".join(list(map(lambda x:x.rstrip(), content.splitlines()))+['']))
os.remove(raw_file)
2 changes: 1 addition & 1 deletion pr-checks/sync.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ set -e

cd "$(dirname "$0")"
python3 -m venv env
source env/bin/activate
source env/*/activate
Copy link
Copy Markdown
Member

@mbg mbg Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python 🤯 🤦🏻‍♂️

pip3 install ruamel.yaml==0.17.31
python3 sync.py

Loading