ci(rust): skip unnecessary setup steps - #3749
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3749 +/- ##
=============================================
- Coverage 75.56% 40.73% -34.83%
Complexity 969 969
=============================================
Files 1317 1315 -2
Lines 154975 133898 -21077
Branches 128600 107600 -21000
=============================================
- Hits 117105 54548 -62557
- Misses 34327 76495 +42168
+ Partials 3543 2855 -688
🚀 New features to boost your workflow:
|
b089984 to
4526ce8
Compare
|
The cpp bdd failure looks like a transient network failure |
|
very nice! i'll review this today |
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
the direction here is good, and the PR closes a real pre-existing hole worth naming: previously test-1 and test-2 each ran their own git fetch origin master and cargo rail plan, so two racing fetches could resolve different origin/master, produce different -E filters, and let a test fall out of both partitions. one build shipping a single nextest-filter.txt makes the partitions complementary by construction.
one blocker, on the artifact name - see the comment at line 323 of the composite action. everything else is should-fix or polish.
one finding lands outside the diff so it has no line to attach to: Install cargo-nextest in setup-rust-with-cache still carries continue-on-error: true, but this PR deletes the cargo test fallback that made a missing nextest survivable. a curl flake now shows a green setup step and dies several steps later at cargo nextest archive with "no such command: nextest". the flag has outlived its reason - worth dropping while you're in there.
| if: inputs.task == 'test-build' | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt }} |
There was a problem hiding this comment.
the artifact name embeds github.run_attempt, and the download at line 333 uses the same expression. "re-run failed jobs" bumps run_attempt but does not re-execute an already-successful needs job, so build-rust-tests never uploads under the new attempt number and the partition goes looking for rust-nextest-archive-<run_id>-2, which never existed. it stays broken for attempts 3, 4 and so on - only "re-run all jobs" recovers, at the cost of a full rebuild. with retries = 3 in .config/nextest.toml a flaky partition is routine, and re-running just that partition worked before this PR.
the fix needs three parts, not two: drop -${{ github.run_attempt }} from both names, add overwrite: true to the upload, and raise retention-days from 1 at line 327. the overwrite part matters - artifacts are run-scoped and survive across attempts, so without it "re-run all jobs" hits a duplicate-name rejection on its upload. and github allows re-runs for 30 days, so a 1-day retention reopens the same hole from the other side.
There was a problem hiding this comment.
Good catch! Thanks for pointing this out. I’ll fix it.
| test_duration=$((test_end - test_start)) | ||
| echo "::notice::Partition ${PARTITION_INDEX}/2 executed in ${test_duration}s ($(date -ud @${test_duration} +'%M:%S'))" | ||
|
|
||
| cargo llvm-cov report --codecov --output-path codecov.json |
There was a problem hiding this comment.
cargo llvm-cov report only ever runs here, in test-run-*, and the build job has no reports-upload step - so the profraw written during cargo build and cargo nextest archive dies with that runner. that silently drops coverage for code which only executes at build time, i.e. proc-macro expansion inside rustc.
measured on this PR's run against the previous flow, union of both partitions: core/configs_derive/src/config_env.rs goes from 222/237 regions hit to 0/237, and core/configs_derive/src/lib.rs from 4/4 to 0/4. region totals are identical across the two runs, so it is the same code with the counters zeroed. the control is inside the same crate family - files with real #[test]s keep their coverage, only the #[proc_macro_*] entry points go to zero. the new run also had a strictly larger scope, so it is not a DAG-filter artifact.
nothing goes red, because codecov statuses here are informational - which is what makes it worth fixing. also emit cargo llvm-cov report --codecov in test-build and upload from there. don't ship the build-phase profraw inside the artifact instead, since both partitions would merge it and double-count.
|
|
||
| run-rust-tests: | ||
| if: ${{ startsWith(inputs.component, 'rust') && inputs.task == 'test' }} | ||
| needs: build-rust-tests |
There was a problem hiding this comment.
worth stating this tradeoff in the PR description - the results tables only cover the first commit, and the archive split itself is unmeasured.
needs: makes the gap between the build finishing and the partitions starting structural: 82s and 113s on this PR's run. on top of that the split adds roughly half a minute of tar and upload after the archive completes, plus a whole extra job setup - the disk reclaim alone was 49s in the build job and 93s and 53s in the two run jobs. and it removes nothing from the critical path, because the old two jobs already built concurrently. so wall clock can only get worse. the job-minute saving is the real win and it is worth having.
at 2 partitions that fixed overhead is paid to dedupe a single build. at 4 it dedupes three for roughly the same cost and wins on both axes - which is the argument for unblocking the partition count, see the note on line 388 of the composite action.
There was a problem hiding this comment.
I’ve updated the PR description and added test-partition-count input to remove the hardcoded partition count.
|
|
||
| jobs: | ||
| run: | ||
| if: ${{ !(startsWith(inputs.component, 'rust') && inputs.task == 'test') }} |
There was a problem hiding this comment.
nothing fails closed on an unknown task name. every step in the composite action is an if: on a string literal, and this PR takes the number of literals that have to agree from one (test-) to several: test here and at lines 256 and 270, test in components.yml, plus test-build and test-run- inside the action.
rename the task in components.yml and this negation flips true, the generic run job invokes the composite with a task matching zero steps, and the job passes green having done nothing. the one fail-closed check the PR does add - ::error::Invalid archived test task - can never fire, since its only caller is test-run-${{ matrix.partition }}.
a terminal step in the composite asserting inputs.task is in a known set would close it.
There was a problem hiding this comment.
I’ve added a unified allowlist check for Rust tasks at the end of the composite action.
| --workspace-remap "$GITHUB_WORKSPACE" \ | ||
| --no-fail-fast \ | ||
| --profile ci \ | ||
| --partition "hash:${PARTITION_INDEX}/2" \ |
There was a problem hiding this comment.
the partition denominator 2 is hardcoded here, again in the notice at line 392, in the matrix at _test.yml:277, and in the task description at line 23 - four places that have to stay in lockstep.
growing the matrix fails loudly, since nextest rejects hash:3/2. shrinking it to [1] fails silently: partition 2's tests simply never run and the gate stays green. deriving the denominator from strategy.job-total would collapse all four.
| required: false | ||
| default: "false" | ||
| install-system-dependencies: | ||
| description: "Whether to install system packages required by Rust builds" |
There was a problem hiding this comment.
"required by Rust builds" undersells this - it is load-bearing at run time too. hwlocality links libhwloc.so.15 dynamically (the vendored fallback is gated on target_env = "musl"), and the runner image doesn't ship it: the apt log on this PR's run shows libhwloc15 being newly installed. so this step is the sole provider on the test-run-* legs, which compile nothing.
as written the description invites the next person to switch it off for those legs as an obvious optimisation, and break CI at process load. worth rewording now while the input is new.
| - name: Install cargo-llvm-cov | ||
| if: startsWith(inputs.task, 'test-') | ||
| if: inputs.task == 'test-build' || startsWith(inputs.task, 'test-run-') | ||
| uses: taiki-e/install-action@v2 |
There was a problem hiding this comment.
splitting one job into two means the build job and the run jobs now resolve cargo-llvm-cov independently on separate runners, so a release landing between them is a producer/consumer skew that could not happen before. same for cargo-nextest, which setup-rust-with-cache curls from the floating get.nexte.st/latest url.
no tool is version-pinned anywhere in this file, so the unpinned pattern is pre-existing - it is the split that turns it into a correctness risk. tool: cargo-llvm-cov@<version> in both jobs would close it.
| # restore is pure overhead. Compiling legs (check/clippy/doctest/test-*) | ||
| # keep it; a cold cache there recompiles the whole dep tree. | ||
| read-cache: ${{ (inputs.task == 'fmt' || inputs.task == 'sort' || inputs.task == 'machete') && 'false' || 'true' }} | ||
| read-cache: ${{ (inputs.task == 'fmt' || inputs.task == 'sort' || inputs.task == 'machete' || startsWith(inputs.task, 'test-run-')) && 'false' || 'true' }} |
There was a problem hiding this comment.
the comment two lines up still says compiling legs "(check/clippy/doctest/test-*)" keep the cache, which this line now contradicts for test-run-*.
knock-on worth knowing: Configure Cargo for CI in setup-rust-with-cache is gated on read-cache == 'true', so turning the cache off here also stops exporting CARGO_INCREMENTAL=0 and the profile debug settings on those legs. harmless today since nothing compiles there, but it is an accidental coupling of two unrelated concerns rather than a decision - read-cache is now doing double duty as "is this a compiling job".
| source <(cargo llvm-cov show-env --sh) | ||
|
|
||
| if [[ "$RUNNER_OS" == "Linux" ]]; then | ||
| sudo sysctl -w vm.max_map_count=2000000 || true |
There was a problem hiding this comment.
moving the sysctl from the build phase to here is right - doris boots at test time, not build time. but the six-line comment explaining why went with it. it named the kernel constraint and pointed at the testcontainers fixture, which is exactly what the next reader won't reconstruct from a bare || true. worth carrying over.
| @@ -1,4 +1,4 @@ | |||
| #!/bin/bash | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
unrelated to either commit in this PR, and incomplete - four other scripts under scripts/ci still use #!/bin/bash. either split it out or convert all of them.
github-actions
Bot
added
S-waiting-on-author
Signed-off-by: StandingMan <jmtangcs@gmail.com>
|
/ready |
github-actions
Bot
added
S-waiting-on-review
|
@Standing-Man something is still wrong: it's building /author |
github-actions
Bot
added
S-waiting-on-author
Signed-off-by: StandingMan <jmtangcs@gmail.com>
| run: | | ||
| TASK="${{ inputs.task }}" | ||
| case "$TASK" in | ||
| check|check-msrv|fmt|clippy|sort|machete|doctest|verify-publish) |
There was a problem hiding this comment.
build is missing from this allowlist, and it's a live task. rust-bench-dashboard declares tasks: ["build"], _detect.yml only routes the exact name rust into rust_matrix so it falls into other_matrix, and _test.yml's "Run Rust task" step still gates on startsWith(inputs.component, 'rust') - so this composite really does get invoked with task: build. see run 30009212161 job 89212758174: Other • rust-bench-dashboard/build / run passed with every task step skipped. after this change it exits 1, test-other goes red and finalize_pr blocks. it won't show up on this PR because it doesn't touch core/bench/dashboard/**. the _test_rust.yml split doesn't affect this path - rust-bench-dashboard never went through test-rust.
the check is doing its job - that task has never built anything, and nothing is lost by deleting it. the dashboard crates are already compiled by the rust component: rust-bench matches core/bench/** and rust depends_on it, so any core/bench/dashboard/** change pulls in the whole rust task set through dependency resolution, build-aarch64-gnu included. the only path rust-bench-dashboard owns alone is scripts/dashboard/**, two shell scripts with nothing to compile. so either add build here to keep today's behaviour, or drop the task from components.yml and let the component stay path-trigger-only like the other rust-* ones.
| uses: taiki-e/install-action@v2 | ||
| with: | ||
| tool: cargo-llvm-cov | ||
| tool: cargo-llvm-cov@0.8.6 |
There was a problem hiding this comment.
0.8.7 is the current release, and the unpinned install was already resolving to it - see the cargo-llvm-cov install log on job 89212757735. pinning is the right move, but this pins one behind what CI was actually running. unless 0.8.6 was picked for a reason, bump it.
| CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} | ||
|
|
||
| build-rust-tests: | ||
| if: inputs.task == 'test' |
There was a problem hiding this comment.
this cuts the noise a lot but doesn't remove it. the previous run had 85 *-rust-tests rows, 82 of them skipped, across 41 callers. now that only test-rust reaches these jobs the SDK callers drop to zero, but each of the eleven non-test rust tasks still materialises a skipped build-rust-tests and a skipped run-rust-tests, so about 22 skipped rows remain plus one for the skipped run forward on rust/test.
to get to zero the split has to move a level up - have _detect.yml emit the test task separately from the rest of rust_matrix, then this workflow holds only the build and partition jobs and never needs the run forward at all. not required here, but that's the shape that actually makes the check list clean.
| timeout-minutes: 60 | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v7.0.0 |
There was a problem hiding this comment.
also at line 96.
actions/checkout@v7.0.0 on both new jobs while _test.yml:51 is already on v7.0.1 (dependabot #3772, which is in the merge base). worth bumping these two by hand.
Signed-off-by: StandingMan <jmtangcs@gmail.com>
4 participants
Rationale
Rust CI was doing redundant work in two areas:
cargo macheteinstalled nextest and system dependencies that theydid not use.
coverage-instrumented workspace before running their respective test
subsets.
This increased both CI setup time and total compute usage.
What changed?
platform-specific system dependencies.
coverage-instrumented nextest archive;
hash partitions.
integration tests can continue to resolve runtime plugins.
setup, and per-partition Codecov reports.
Rust setup paths.
portable Bash shebang.
Results
The setup optimization was measured over three runs:
A pre-merge comparison showed:
The benchmark link.
Archive split tradeoff
The archive split reduces total runner usage by compiling the
coverage-instrumented test suite once. It does not improve the critical
path with the current two partitions because the previous two jobs
already performed their builds concurrently.
In the measured PR run:
needshandoff introduced gaps of 82 s and 113 s between thebuild completing and the partitions starting;
finished creating the archive;
partition jobs;
The primary benefit at two partitions is therefore lower aggregate
runner minutes, with increased wall-clock latency as the tradeoff.
The partition count is now derived from the workflow matrix rather than
being hardcoded in the composite action. Increasing the matrix to four
partitions would deduplicate three builds for approximately the same
build-and-upload cost while reducing each partition's test workload.
That configuration should be measured separately before changing the
default.
Local Execution
Passed
Ran
AI Usage