Skip to content

Fail build.sh if any command in it fails #2931

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mbg
Copy link
Member

@mbg mbg commented Jun 12, 2025

This PR updates build.sh so that it fails if any step in the script fails. Previously, the script would continue running and thus mask unexpected failures. For example, Go is no longer available by default on runners and so an explicit setup-go step was required. However, because the script didn't fail, we didn't notice that failure until now.

To address the Go issue, I updated sync.sh to understand a new installGo input that injects the setup-go step into the workflow.

I tried adding it to the local prepare-test action first, but this resulted in a number of further issues:

  • Adding setup-go there, which has a post-action, forced the entire prepare-test action to have a post-action, which caused problems because the workflow also moves the location of the prepare-test action to outside of the workspace.
  • Moving the local actions back to the workspace then caused a bunch of further issues with workflows not expecting there to be a meaningful .github folder in the workspace.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@mbg mbg force-pushed the mbg/fail-build.sh-on-error branch from 71d8dee to 63beaad Compare June 12, 2025 14:31
@mbg mbg force-pushed the mbg/fail-build.sh-on-error branch from 63beaad to 8f71d47 Compare June 16, 2025 11:28
@mbg mbg marked this pull request as ready for review June 16, 2025 11:43
@Copilot Copilot AI review requested due to automatic review settings June 16, 2025 11:43
@mbg mbg requested a review from a team as a code owner June 16, 2025 11:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that Go is explicitly installed in all local workflow runs by adding an actions/setup-go@v5 step, so that build.sh and related scripts will correctly fail if Go is missing.

  • Introduces an Install Go step to every workflow to guarantee a supported Go version is available.
  • Aligns all workflows on go-version: '>=1.21.0' and disables caching of Go installs.
  • Prepares the ground for build.sh to error out early when dependencies are not met.

Reviewed Changes

Copilot reviewed 48 out of 48 changed files in this pull request and generated no comments.

Show a summary per file
File Description
.github/workflows/__test-local-codeql.yml Added Install Go step before fetching bundle
.github/workflows/__swift-custom-build.yml Added Install Go step before init action
.github/workflows/__split-workflow.yml Added Install Go step before init action
.github/workflows/__remote-config.yml Added Install Go step before init action
.github/workflows/__packaging-inputs-js.yml Added Install Go step before init action
.github/workflows/__packaging-config-js.yml Added Install Go step before init action
.github/workflows/__packaging-config-inputs-js.yml Added Install Go step before init action
.github/workflows/__packaging-codescanning-config-inputs-js.yml Added Install Go step before init action
.github/workflows/__multi-language-autodetect.yml Added Install Go step before init action
.github/workflows/__go-tracing-legacy-workflow.yml Replaced old setup-go usage with named step
.github/workflows/__go-tracing-custom-build-steps.yml Replaced old setup-go usage with named step
.github/workflows/__go-tracing-autobuilder.yml Replaced old setup-go usage with named step
.github/workflows/__go-indirect-tracing-workaround.yml Added Install Go step before init action
.github/workflows/__go-indirect-tracing-workaround-no-file-program.yml Added Install Go step before remove step
.github/workflows/__go-indirect-tracing-workaround-diagnostic.yml Added Install Go step before init action
.github/workflows/__go-custom-queries.yml Added Install Go step before init action
.github/workflows/__export-file-baseline-information.yml Added Install Go step before init action
.github/workflows/__build-mode-manual.yml Added Install Go step before init action
.github/workflows/__analyze-ref-input.yml Added Install Go step before init action
.github/workflows/__all-platform-bundle.yml Added Install Go step before init action
Comments suppressed due to low confidence (1)

.github/workflows/__test-local-codeql.yml:48

  • This Install Go step is duplicated across many workflows. Consider extracting it into a reusable workflow snippet or composite action to simplify updates and reduce duplication.
-      - name: Install Go

Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

Style nits only on my side.

Comment on lines +111 to +113
installGo = False
if checkSpecification.get('installGo'):
installGo = True if checkSpecification['installGo'].lower() == "true" else False
Copy link
Contributor

Choose a reason for hiding this comment

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

if we just use

Suggested change
installGo = False
if checkSpecification.get('installGo'):
installGo = True if checkSpecification['installGo'].lower() == "true" else False
installGo = checkSpecification.get('installGo')

then we can use a more natural

installGo: true

in the test specification, without using stringly typed values. Although I see we use "true" for other cases as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, like with the other point, I went with this to mirror the existing inputs. I would be happy to change this though if we can't think of a downside / motivation for why the others are strings.

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