fix(metabench): rename the duplicate basic example and gate the workspace on unique names - #751
Evgenii (Vaiz) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Case-only collisions remain undetected, and generated Anvil examples validation does not invoke the guard.
Pull request overview
Renames the metabench example and adds workspace-wide validation against duplicate Cargo example names.
Changes:
- Renames
basictosingle_benchmark. - Updates spawned benchmark references.
- Adds duplicate-name detection and invokes it from the example runner.
File summaries
| File | Summary |
|---|---|
scripts/run-examples.rs |
Runs workspace example-name validation. |
crates/metabench/tests/spawned_benchmark.rs |
Uses the renamed example target. |
crates/metabench/examples/single_benchmark.rs |
Defines the renamed benchmark example. |
crates/automation/src/lib.rs |
Re-exports validation functionality. |
crates/automation/src/cargo_metadata.rs |
Implements validation and unit tests. |
Review details
Suppressed comments (2)
crates/automation/src/cargo_metadata.rs:78
- This map compares target names byte-for-byte, but the invariant being guarded is an output-file collision across the workspace. If two examples are named only by case (for example
basicandBasic), this check passes while Windows and the usual case-insensitive macOS volumes resolve both to the sametarget/<profile>/examplespath, so the race this guard documents remains possible. Compare normalized output names (or otherwise apply the host filesystem's case rules) and add a case-only regression test.
let mut owners: BTreeMap<&str, Vec<&str>> = BTreeMap::new();
for package in packages {
for target in &package.targets {
if target.kind.iter().any(|kind| kind == "example") {
owners.entry(target.name.as_str()).or_default().push(package.name.as_str());
scripts/run-examples.rs:78
- This call only gates the legacy
scripts/run-examples.rspath. The generated Anvil PR test includesanvil-examples, whose recipe builds and runs examples directly (justfiles/anvil/checks/examples.just:11-35, 83-96) and never invokes this script. A future duplicate can therefore still pass the current Anvil gate (Cargo only warns about the collision), so the workspace-wide invariant is not enforced by the repository's generated validation path. Wire the check into a shared/unscoped Anvil validation step or otherwise make the Anvil examples check invoke it.
automation::check_unique_example_names(&packages)?;
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #751 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 640 639 -1
Lines 85528 85484 -44
=======================================
- Hits 85528 85484 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| //! One shared workload and input registered with Criterion and Gungraun. | ||
| //! | ||
| //! Named for the single benchmark it registers, to contrast with the |
There was a problem hiding this comment.
this comment has 0 value
|
Both review findings addressed in 1. Case-only collisions — correct, and worse than the exact duplicateFixed. I measured both halves rather than reasoning about them. The filesystem folds case. Cargo does not warn about it. I renamed No collision warning anywhere — compare the exact-duplicate case, which warns twice ( With the fix, that same tree is rejected: Names are folded with 2.
|
108a0e0 to
9732519
Compare
|
Rebased onto What was failing
This was repo-wide, not specific to this PR: #746 hit the identical failure. Verified after the rebase
Both of this branch's commits replayed without conflict, including against #750's edit to Still openThe Leaving the PR as a draft, since that state was set deliberately. |
…kspace on unique names
`observed` and `metabench` both declare an example target named `basic`. Cargo
writes every example in the workspace into one shared
`target/<profile>/examples/` directory, so both resolve to the same output file.
Reproduced on `ec1f6bc2` before changing anything:
cargo build -p observed -p metabench --example basic --release
warning: output filename collision at target\release\examples\basic.exe
= note: the example target `basic` in package `observed v0.26.0` has the
same output filename as the example target `basic` in package
`metabench v0.1.1`
= note: this may become a hard error in the future; see
<rust-lang/cargo#6313>
plus the same for `basic.pdb`. Cargo only warns, then builds both targets
concurrently into that one path.
This class already cost a CI failure. PR #676 renamed a duplicate
`tower_service` example after the `link.exe` race it caused surfaced as
`LNK1104: cannot open file ...` on an unrelated pull request. On Linux and
macOS the race silently overwrites the binary instead, so only the Windows leg
surfaces it -- the example that runs is not the one that was selected, and
nothing fails. #676 renamed and moved on, so the class recurred.
`metabench`'s target becomes `single_benchmark`, which also says what
distinguishes it from the sibling `parameterized` example. `observed`'s `basic`
is the published crate's introductory example and is named in its own module
docs, so renaming that side would have been the worse trade.
Six call sites in `crates/metabench/tests/spawned_benchmark.rs` spawn the
example by name through `cargo run --example` and move with it. Four of those
are the vtune tests added by #750, which is the reason a rename alone is not
enough: #750 introduced new references to `basic` while this work was in
flight. Git merged the two changes without conflict because they touch
different lines, and the result still failed -- `cargo nextest` reported
`vtune_measures_exact_workload_and_writes_metrics` and two siblings failing on
all four platforms. A textually clean merge is not a semantically clean one,
and nothing in the toolchain catches a rename that misses a call site.
So the rename is paired with a check. `ExampleTargetNames.Tests.ps1` fails when
two packages declare the same example name, naming the example and every
package that declares it.
It lives in the repository Pester suite, not in an Anvil recipe. The Anvil
recipes are generated (`DO NOT EDIT DIRECTLY`), so a check added there is
reverted on the next regeneration, and the Anvil groups are impact-scoped while
a duplicate example name is a property of the whole workspace -- a scoped check
passes on exactly the pull requests that did not touch either colliding crate.
`just test-scripts` is unscoped and already gates on `Required repository
checks`.
Names are compared case-insensitively. Windows and the default
case-insensitive macOS volumes resolve `basic` and `Basic` to one path, but
Cargo compares `PathBuf`s, which are case-sensitive in Rust, so it does not
emit even its usual warning for that variant. Measured earlier in this work:
renaming one side to `Basic` and building both produced no collision warning at
all, where the exact duplicate warns twice. The case-only collision is the more
dangerous of the two because it is silent on both sides.
Verified:
- `cargo build --workspace --examples --all-features --release --locked` --
no filename-collision warning anywhere.
- `cargo test -p metabench --test spawned_benchmark --all-features` -- 8
passed, including the four vtune tests that were failing on all four
platforms in CI.
- `just anvil-clippy`, `just anvil-fmt`, `just anvil-spellcheck` clean.
- The new Pester file passes PowerShell AST parsing and PSScriptAnalyzer with
no findings.
NOT verified: the Pester test was not executed locally. The local Pester
install fails to load (`The cloud file provider exited unexpectedly` reading
Pester 6.0.0 from a OneDrive-backed module path) and this environment could not
invoke the pinned 5.7.1 directly. CI's `Release script tests` job on
ubuntu-latest and windows-latest is its first real execution.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9732519 to
2df9372
Compare
|
Rebuilt on I was wrong on the second finding, and #759 proved itOn 09-15 the Copilot review said the guard "only gates the legacy #759 has since deleted The guard is now a Pester test in the repository suite:
That is what the review asked for, in the place that now exists for it. The narrower residue I conceded last time — that A second thing the rebase exposed#750 added four new I had twice predicted these PRs were "disjoint". They were textually disjoint and semantically not. Worth stating plainly, because it is the same failure mode this PR exists to prevent: a rename that misses a call site, with nothing in the toolchain to catch it. All six call sites now move together. Verified
Not verified — please read before approvingThe Pester test was not executed locally. The local Pester install fails to load ( Still a draft, since that state was set deliberately. |
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
observedandmetabenchboth declared an example target namedbasic. Cargo writes every example in the workspace into one sharedtarget/<profile>/examples/directory, so both resolved to the same output file.Reproduced before changing anything, on
84083b8:plus the same warning for
basic.pdb. Cargo only warns, then builds both targets concurrently into that one path.Why this is worth a guard and not just a rename
This class already cost a CI failure here. #676 renamed a duplicate
tower_serviceexample after thelink.exerace it caused surfaced asLNK1104: cannot open file ...\examples\tower_service.exeon an unrelated pull request. On Linux and macOS the same race silently overwrites the binary instead, so only the Windows leg surfaces it — the example that runs is simply not the one that was selected, and nothing fails.#676 renamed and moved on, so the class recurred four weeks later. This PR closes both halves.
What changed
The rename.
metabench's target becomessingle_benchmark, which also says what distinguishes it from the siblingparameterizedexample.observed'sbasicis the published crate's introductory example and is named in its own module docs, so renaming that side would have been the worse trade. The two call sites inmetabench/tests/spawned_benchmark.rsthat spawn the example by name move with it.The guard.
automation::check_unique_example_namesfails on any duplicate, naming the example and every package that declares it:scripts/run-examples.rscalls it before running anything. It is checked over every workspace package rather than the selection: a collision is a property of the workspace, and CI narrows that script with--excludefrom thedeltajob, so checking only the selected packages would hide a collision on exactly the pull requests that did not touch either colliding crate.The logic lives in
automationrather than inline in the script because a-Zscriptcargo script has no test harness. There it carries five unit tests, including one that runs the check against this workspace's realcargo metadataoutput — so the invariant is enforced bycargo test, not only by the examples job.Verification
this_workspace_has_no_colliding_example_namesfails with the message quoted above. A check that has only ever been observed passing is not a check.cargo build --workspace --examples --all-features --release --locked— completes with no filename-collision warning anywhere.cargo test -p automation --all-features— 10 passed.cargo test -p metabench --test spawned_benchmark --all-features— 4 passed, exercising the renamed example throughcargo run --example.just anvil-clippy,just anvil-fmt,just anvil-spellcheck— clean.just anvil-license-headersdid not run: the localcargo-heatheris v0.2.1 and the recipe requires v0.3.0. No file here gains or loses a header, but I am reporting that check as unrun rather than green.Note on #750
#750 also touches
scripts/run-examples.rs, but only theEXCLUDED_EXAMPLESarray — a disjoint region from this change, which adds an import and one call. Whichever lands second should merge cleanly. Its newfake_vtuneexample does not collide with anything.