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

Use explicit bash shell with pipefail semantics in GitHub actions #823

Merged
merged 11 commits into from
Aug 8, 2023

Conversation

nickpdemarco
Copy link
Collaborator

@nickpdemarco nickpdemarco commented Aug 2, 2023

According to the GitHub Documentation, the shell for Unix platforms when unspecified is bash -e {0}. Notably, this does not set pipefail.

Because pipefail was not set, the pipe to tee was swallowing any error codes during the test run. This PR explicitly sets shell: bash for all run commands, which (per the above docs) produces the internal command bash --noprofile --norc -eo pipefail {0}.

It is baffling that the implicit, default bash shell has different semantics from the explicit bash shell.

@nickpdemarco nickpdemarco changed the title Use YAML "folded" literals in build-and-test.yml Use explicit bash shell with pipefail semantics in GitHub actions Aug 2, 2023
Copy link
Collaborator

@dabrahams dabrahams left a comment

Choose a reason for hiding this comment

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

Overall, great work discovering the truth about the shells (which information I'd been unable to find).

Please think about my questions. Also think about whether we want to remove the top-level &&s.
Finally, remember I asked that you put documentation about whatever you learned into the .yml file?

@@ -42,6 +42,7 @@ jobs:
uses: devcontainers/[email protected]
with:
runCmd: |
set -o pipefail &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this one set -o pipefail and the other one shell: bash?
If shell: bash doesn't work in this context, we should use explicit sets everywhere.

Do we not also want set -e? Let's just be as explicit as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, we're leveraging a custom GitHub action which does not support a shell argument. It probably should, so I raised an issue. For now, I'll be explicit in all steps for consistency, as you ask.

@dabrahams
Copy link
Collaborator

@nickpdemarco Do we need to chat to get this moving forward?

@nickpdemarco
Copy link
Collaborator Author

@dabrahams sorry, no - I got distracted by the compiler explorer work. I'll address your comments now.

@dabrahams
Copy link
Collaborator

@nickpdemarco it looks like a test is still disabled in this PR? Please merge once you've fixed that!

@nickpdemarco nickpdemarco merged commit 1a8788d into main Aug 8, 2023
@nickpdemarco nickpdemarco deleted the npd/ci-stability branch August 8, 2023 17:26
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

Successfully merging this pull request may close these issues.

2 participants