ci: drop duplicate matrix legs and consolidate caches - #2614
ilblackdragon wants to merge 2 commits into
Conversation
Combines five reductions targeting the same root cause: clippy and cargo check were running the same compile work multiple times across matrix legs and platforms. - Drop `windows-build` from test.yml. Its `cargo check` matrix is fully subsumed by `clippy-windows`, which already type-checks `--all --benches --tests --examples` on the same configs. - Collapse PR-blocking clippy to `--all-features` only (Linux + Windows). Lint findings are almost never feature-gated, and the remaining configs (`default`, `libsql-only`) now run as `clippy-extra` / `clippy-windows-extra` jobs gated to `push` events on long-lived branches, where they verify feature combinations without slowing PRs. - Share rust-cache slot between clippy and tests via `shared-key: build` (Linux) and `shared-key: build-windows`. Clippy and `cargo test` produce overlapping artifacts for the same feature set, so a single slot serves both jobs and avoids duplicating multi-GB target/ caches. - Replace `cargo install cargo-component --locked || true` with a composite action backed by `taiki-e/install-action`. Drops a multi-minute source build to seconds and stops swallowing failures. Applied to test.yml, coverage.yml, and release.yml. - Add `paths-ignore` filters for top-level docs, LICENSE files, PNG assets, issue/PR templates, and `docs/**` (mintlify site). Conservative scope: any `.md` file actually `include_str!()`'d by the binary (workspace seeds, engine prompts, channel READMEs) remains unfiltered. Includes `save-if` guards on every rust-cache step so PR jobs are restore-only — complementary to #2609 and #2610. Gates `no-panics` to PR events (it diffs against the PR base SHA, unavailable on push) and teaches the `code-style` roll-up to accept `skipped` for the push-only / PR-only conditional jobs.
There was a problem hiding this comment.
Code Review
This pull request introduces a new GitHub composite action to install cargo-component using precompiled binaries via taiki-e/install-action, which significantly reduces CI setup time compared to source-based installation. Feedback suggests pinning the cargo-component tool to a specific version to ensure build reproducibility and avoid non-deterministic behavior in the CI environment.
| - name: Install cargo-component | ||
| uses: taiki-e/install-action@62b0f2dec647a8e604c6a0fda0e38530180dce20 # v2 | ||
| with: | ||
| tool: cargo-component |
There was a problem hiding this comment.
To ensure build reproducibility and avoid unexpected CI failures when a new version of cargo-component is released, consider pinning the tool to a specific version (e.g., cargo-component@0.20.0). This aligns with the project's practice of using --locked for deterministic builds, as seen in the local setup scripts. Without a version specifier, install-action will fetch the latest available version, which may introduce non-deterministic behavior in the build process.
References
- When applying a best practice, such as using --locked for reproducible builds, it should be applied consistently across the codebase. Pinning tool versions in CI is the equivalent best practice for binary installers to ensure a deterministic environment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Reduces duplicated compile work in GitHub Actions by shrinking CI matrices, consolidating rust-cache usage across jobs, and speeding up cargo-component installation via a composite action.
Changes:
- Add
paths-ignorefilters to skip CI on docs-only/template/image changes. - Consolidate Swatinem rust-cache usage via
shared-keyandsave-ifgating, and removewindows-buildfromtest.yml. - Replace ad-hoc
cargo install cargo-componentwith a composite action usingtaiki-e/install-action.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/test.yml | Adds paths-ignore, shares rust-cache slot with code_style, adds save gating, removes windows-build, uses composite installer. |
| .github/workflows/code_style.yml | Adds paths-ignore + push trigger, collapses PR clippy coverage, shares caches, adjusts roll-up for skipped conditional jobs. |
| .github/workflows/coverage.yml | Uses composite action for cargo-component installation. |
| .github/workflows/release.yml | Uses composite action for cargo-component installation. |
| .github/actions/install-cargo-component/action.yml | New composite action to install cargo-component via prebuilt binaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shared-key: build | ||
| save-if: ${{ github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} |
There was a problem hiding this comment.
tests is a 3-leg matrix, but the rust-cache uses a single shared-key: build and is allowed to save on push. Because GitHub caches are immutable, whichever leg finishes first will “win” the cache slot, likely leaving it populated by a subset build (e.g. libsql-only) and preventing the all-features leg from ever refreshing the shared cache. Consider making only the all-features leg eligible to save (and keep the other legs restore-only), or split cache slots per feature-set.
| shared-key: build | |
| save-if: ${{ github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} | |
| # | |
| # Only the broadest feature-set leg is allowed to save this immutable | |
| # shared cache; the narrower matrix legs remain restore-only so they | |
| # cannot win the slot with a subset build. | |
| shared-key: build | |
| save-if: ${{ matrix.name == 'all-features' && github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} |
There was a problem hiding this comment.
Resolved differently in #2566. After merging this work into #2566, tests keeps the per-leg key: ${{ matrix.name }} (no shared-key: build), and #2566's dynamic matrix means PRs only run the all-features leg while pushes run the full matrix on independent keys — no race possible. (This PR is being closed in favor of #2566.)
| with: | ||
| key: clippy-${{ matrix.name }} | ||
| shared-key: build | ||
| save-if: ${{ github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} |
There was a problem hiding this comment.
clippy-extra shares shared-key: build with the PR-blocking clippy job and is also configured to save on push. Since caches can’t be overwritten, an extra (subset) leg can win the race and permanently prevent the all-features clippy job from refreshing the shared cache on that push. Recommend making clippy-extra restore-only (no save) and letting only the Clippy (all-features) job save build on pushes.
| save-if: ${{ github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} | |
| save-if: false |
There was a problem hiding this comment.
Resolved in #2566. The merged design has a single clippy job (no clippy-extra split) with #2566's dynamic matrix. The shared-key: clippy slot is now guarded by save-if: matrix.name == 'all-features' && push to main/staging — only the all-features leg can write the slot, so subset legs never race against it. (This PR is being closed in favor of #2566.)
| with: | ||
| key: clippy-windows-${{ matrix.name }} | ||
| shared-key: build-windows | ||
| save-if: ${{ github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} |
There was a problem hiding this comment.
clippy-windows-extra shares shared-key: build-windows with the Clippy Windows (all-features) job and is allowed to save on push. Because cache keys are immutable, a subset leg can win and block the all-features job from refreshing the slot, reducing cache usefulness for future PRs. Recommend making clippy-windows-extra restore-only and letting only the all-features Windows job save the shared slot on pushes.
| save-if: ${{ github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/staging') }} | |
| save-if: false |
There was a problem hiding this comment.
| # Windows compilation is covered by the clippy-windows job in | ||
| # code_style.yml (clippy implies cargo check), so no Windows job is needed | ||
| # here. | ||
| needs: [tests, heavy-integration-tests, telegram-tests, wasm-wit-compat, docker-build, version-check, bench-compile] |
There was a problem hiding this comment.
This workflow drops windows-build based on Windows compilation being covered by code_style.yml’s clippy job, but test.yml is also used as a reusable workflow (e.g. via staging-ci.yml), which does not run code_style.yml. As a result, those callers will lose any Windows compile signal entirely. If that coverage is still desired for batch/staging runs, consider adding a Windows leg back under workflow_call, or have the caller workflow also invoke the relevant Windows lint/check workflow.
There was a problem hiding this comment.
…undant-jobs # Conflicts: # .github/workflows/code_style.yml # .github/workflows/test.yml
|
Closing in favor of #2566. The full set of changes from this PR (drop |
Summary
Combines five reductions targeting the duplicate compile work the analysis flagged: clippy and `cargo check` were running the same compile multiple times across matrix legs and platforms.
Interaction with #2609 / #2610
Independent and complementary. This PR includes `save-if` guards on every rust-cache step (so it's self-contained if those land later) and changes cache keys (`clippy` → `build` for Linux; `clippy-windows` + `windows-build` → `build-windows` for Windows). Expect trivial line-level merge conflicts in the rust-cache blocks; the intent layers cleanly:
Also: gates `no-panics` to PR-only (it diffs against the PR base SHA, empty on push) and updates the `code-style` roll-up to accept `skipped` for the push-only / PR-only conditional jobs.
Expected impact per PR
Test plan
Watch-outs