Skip to content

Optimize CI - #5107

Merged
djc merged 6 commits into
mainfrom
optimize-ci
Sep 25, 2026
Merged

djc merged 6 commits into
mainfrom
optimize-ci

Conversation

@djc

@djc djc commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

(With help from Claude.)

djc added 6 commits September 25, 2026 13:58
The step that records the rustc commit hash wrote `{rust_hash}={...}`
to `$GITHUB_OUTPUT`, so the output was named `{rust_hash}` and
`steps.cargo-target-cache.outputs.rust_hash` always expanded to an
empty string. As a result the cache key never changed when stable
updated: the stale target directory kept hitting the primary key,
every build recompiled from scratch, and nothing was saved back
because actions/cache only saves on a primary-key miss.

It also ran before rustup installed the toolchain the build uses, so
even with the output fixed it would have hashed whatever rustc the
runner image ships. Move the hash and cache steps after the toolchain
install so the key reflects the compiler the build actually runs.
The target-dir cache key was `<base_ref>-<head_ref>-<target>-...` with a
restore key of `<base_ref>-<target>-...`. actions/cache matches restore
keys by prefix, and no saved key ever starts with `<base_ref>-<target>`
because the PR branch name sits in between. Pushes to main and merge
queue runs have an empty base_ref and head_ref, so they saved under
`--<target>-...`, which a PR never asked for either. The net effect was
a cold target directory on every job for the first push of every PR.

Compute the branch to fall back to explicitly: the base branch for PRs
and merge queue runs, the pushed branch otherwise. Put the PR branch
name at the end of the key so the base branch key is a prefix of it,
and add a second restore key without the Cargo.lock hash so a lockfile
bump still starts from a mostly warm cache.
Every target was built in both dev and release mode. For targets that
cannot run the test suite, the dev job compiled the binary in the debug
profile, uploaded nothing and exited. There are no `debug_assertions`
gates in the source, so that job checked nothing the release build of
the same target did not already check.

Make `release` the only value on the `mode` axis and add the dev-mode
jobs for the natively testable targets as explicit `include` entries.
Since an include entry that overrides an axis value becomes its own
matrix entry, this gives each test target a dev job and a release job
while every other target gets a release job only. On a PR this drops
four jobs; on stable it drops around thirty.

Dev mode now implies running the tests, so the `run_tests` marker is
no longer needed, matching what the macOS template already does.
Clippy ran in every Windows release job that was not a mingw build:
three targets on a PR and four on stable, on top of the Linux check
job. Each run is a full check-mode compile, because `--all-features`
differs from the feature set the preceding build used, so almost none
of the build artifacts are reused.

The reason for running it on Windows at all is `cfg(windows)` code the
Linux job never sees. The source has no `target_env` or `target_arch`
gates, so lints in that code are identical across the Windows targets
and one run on x86_64-pc-windows-msvc covers them all.
Without a concurrency group, pushing to a PR twice in a row runs both
full matrices to completion. Group PR runs by ref and cancel the older
run when a new push arrives. Every other event gets a group keyed on
its run id, so pushes to main and stable, which upload artifacts, are
never cancelled or queued behind each other.
Jobs that run the test suite first built the binary without the `test`
feature, then rebuilt the crate with it for `--dump-testament` and the
test targets. The first build has no consumer in those jobs: nothing
uploads or inspects the debug binary, and the release job for the same
target already checks that the crate builds without the `test` feature.

Build only the binary when tests are skipped, and go straight to the
test builds otherwise. This saves one compile of the rustup crate on
each test job, including the FreeBSD one which runs tests in release
mode through the same script.
@djc
djc requested a review from rami3l September 25, 2026 11:58
@rami3l rami3l self-assigned this Sep 25, 2026
@rami3l
rami3l requested a lite review from Copilot September 25, 2026 13:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Windows Clippy no longer covers all previously linted target configurations.

Review effort: Lite
Findings: None

What changed in this PR

Optimizes CI by reducing redundant builds, improving cache reuse, and cancelling superseded PR workflows.

Changes:

  • Restricts tests and builds to necessary targets.
  • Improves Cargo target caching and fallback behavior.
  • Adds PR concurrency cancellation.
  • Narrows Windows Clippy execution.
File Summary
ci/​run.bash Avoids redundant builds.
ci/​actions-templates/​windows-builds-template.yaml Updates matrices, caching, and Clippy configuration; a moderate finding concerns reduced target coverage.
ci/​actions-templates/​macos-builds-template.yaml Updates target caching.
ci/​actions-templates/​linux-builds-template.yaml Updates matrices and target caching.
ci/​actions-templates/​gen-workflows.sh Adds workflow concurrency settings.
ci/​actions-templates/​centos-fmt-clippy-template.yaml Updates caching and Rust setup order.
.github/​workflows/​ci.yaml Regenerated workflow; moderate findings concern reduced Windows Clippy target coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@rami3l rami3l left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm still a bit worrying about the tiny details (many of which I think have to be tested online) but the big picture is looking quite reasonable to me, thanks a lot for helping out :)

View changes since this review

Comment thread .github/workflows/ci.yaml

@Cloud0310 Cloud0310 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@djc
djc added this pull request to the merge queue Sep 25, 2026
Merged via the queue into main with commit b32adec Sep 25, 2026
29 checks passed
@djc
djc deleted the optimize-ci branch September 25, 2026 15:00
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.

4 participants