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.

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

Jump to bottom

Don't cancel running master builds #54617

Merged
merged 2 commits into from
Jan 20, 2022
Merged

Don't cancel running master builds #54617

merged 2 commits into from
Jan 20, 2022

Conversation

kevingranade
Copy link
Member

Summary

None

Purpose of change

We've been seeing a lot of instances of master builds being interrupted by this concurrency limit, which with bad timing can lead to ccache archives going a long time before being refreshed, which in turn makes the builds themselves take longer.

Describe the solution

With recent improvements to ccache utilization, master builds are now more important to refresh the ccache archives, as well as much, much faster to complete.
It's no longer critical to try to cancel as many jobs as possible, so for master builds this backs off on cancelling in-progress jobs to give them a chance to complete.

Testing

Possibly bump the PR to validate that the jobs are still cancelled as expected.

@kevingranade
Don't cancel running master builds
0192007 
This prioritizes getting master builds completed for refreshing ccaches
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 20, 2022
akrieger
akrieger approved these changes Jan 20, 2022
View reviewed changes
.github/workflows/matrix.yml
@@ -11,7 +11,7 @@ on:
# We only care about the latest revision of a PR, so cancel all previous instances.
concurrency:
group: general-build-${{ github.event.pull_request.number || github.ref_name }}
cancel-in-progress: true
cancel-in-progress: ${{ github.event_name == 'pull_request' }}
Copy link
Member

@akrieger akrieger Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My turn to be (un-)professionally embarrassed for not realizing the solution (to have PRs self-cancel but not master) was as easy as this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was stumped for about 10 minutes, you're not alone.

@kevingranade
Copy link
Member Author

Bumping the branch with a comment triggered cancellation as expected.

@kevingranade kevingranade merged commit 22fb0e2 into master Jan 20, 2022
@kevingranade kevingranade deleted the kevingranade-prevent-mastercide branch January 20, 2022 23:10
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 21, 2022
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Sep 25, 2023
@scarf005 @kevingranade
ci: limit resource usages on repeated pushes
1e27020 
see:
- CleverRaven/Cataclysm-DDA#54306
- CleverRaven/Cataclysm-DDA#54617

Co-authored-by: Kevin Granade <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Sep 27, 2023
@scarf005 @kevingranade
ci: limit resource usages on repeated pushes
390ca1c 
see:
- CleverRaven/Cataclysm-DDA#54306
- CleverRaven/Cataclysm-DDA#54617

Co-authored-by: Kevin Granade <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Oct 1, 2023
@scarf005 @kevingranade
ci: limit resource usages on repeated pushes
36fcbc5 
see:
- CleverRaven/Cataclysm-DDA#54306
- CleverRaven/Cataclysm-DDA#54617

Co-authored-by: Kevin Granade <[email protected]>
scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Oct 3, 2023
@scarf005 @kevingranade
ci: limit resource usages on repeated pushes
8d7cf99 
see:
- CleverRaven/Cataclysm-DDA#54306
- CleverRaven/Cataclysm-DDA#54617

Co-authored-by: Kevin Granade <[email protected]>
github-merge-queue bot pushed a commit to cataclysmbnteam/Cataclysm-BN that referenced this pull request Oct 13, 2023
@scarf005 @BrettDong
@kevingranade @akrieger @irwiss @casswedson @jbytheway @olanti-p
ci(port): various ccache and matrix improvements (#3258)
5f5bf06 
* ci: remove EXTRA_TEST_OPTS

not used ATM

* ci: install SDL dependencies only for tiles build

see: CleverRaven/Cataclysm-DDA#55088

Co-authored-by: Brett Dong <[email protected]>

* ci: order title first

for readability

* ci: limit resource usages on repeated pushes

see:
- CleverRaven/Cataclysm-DDA#54306
- CleverRaven/Cataclysm-DDA#54617

Co-authored-by: Kevin Granade <[email protected]>

* ci: add `ccache_limit`  and `ccache_key`

* ci: correctly cache ccache

see:
- CleverRaven/Cataclysm-DDA#54078
- CleverRaven/Cataclysm-DDA#64553

Co-authored-by: Kevin Granade <[email protected]>
Co-authored-by: Brett Dong <[email protected]>

* ci: ccache stats and cleanup

see: CleverRaven/Cataclysm-DDA#64553

Co-authored-by: Brett Dong <[email protected]>

* ci: write ccache from upload only

see: CleverRaven/Cataclysm-DDA#54558

Co-authored-by: Andrew Krieger <[email protected]>

* ci: better performance ccache options

see: CleverRaven/Cataclysm-DDA@aaffb31

Co-authored-by: Andrew Krieger <[email protected]>

* ci: build only and test only script

see: CleverRaven/Cataclysm-DDA#54435

Co-authored-by: Brett Dong <[email protected]>
Co-authored-by: Alexey <[email protected]>

* ci: add some line breaks

my eyes hurt

* ci: skip via either code or data

see:
- CleverRaven/Cataclysm-DDA@5707742
- CleverRaven/Cataclysm-DDA@f6ec25f

Co-authored-by: Kevin Granade <[email protected]>
Co-authored-by: Andrew Krieger <[email protected]>

* ci: separate build and test step

see: CleverRaven/Cataclysm-DDA#54435

Co-authored-by: Brett Dong <[email protected]>

* ci: bump actions/upload-artifact to v3

see: CleverRaven/Cataclysm-DDA#64962

Co-authored-by: casswedson <[email protected]>

* ci: ignore upload fail after build fails

see: CleverRaven/Cataclysm-DDA@b7dc997

Co-authored-by: John Bytheway <[email protected]>

* ci: emit success artifact

* fix: use num_jobs correctly

* Update .github/workflows/matrix.yml

Co-authored-by: Olanti <[email protected]>

* Update .github/workflows/matrix.yml

Co-authored-by: Olanti <[email protected]>

* ci: remove object-creator

* ci: add run-on-draft flag

* ci: match languages in translation test

* ci: use `upload-artifact`

* Update build-scripts/gha_compile_only.sh

Co-authored-by: Olanti <[email protected]>

* test: fix catch2 tag format

* test: mark overmap lab generation test as flaky

* test: mark ranged aiming test as flaky

* test: use anonymous namespace

* test: adjust grenade lethality to match BN value

* ci: remove gh push trigger

it didn't work

* ci: reduce cache limit

github cache limit is 10GB

* Revert "test: mark overmap lab generation test as flaky"

This reverts commit 71f6840.

* ci: does it work?

* ci: remove `FRAMEWORK` option

* ci: fix restore key

---------

Co-authored-by: Brett Dong <[email protected]>
Co-authored-by: Kevin Granade <[email protected]>
Co-authored-by: Andrew Krieger <[email protected]>
Co-authored-by: Alexey <[email protected]>
Co-authored-by: casswedson <[email protected]>
Co-authored-by: John Bytheway <[email protected]>
Co-authored-by: Olanti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akrieger akrieger akrieger approved these changes

Assignees
No one assigned
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevingranade @akrieger