fix(metabench): rename the duplicate basic example and gate the workspace on unique names - #751
Evgenii (Vaiz) wants to merge 2 commits into
Conversation
…kspace on unique names
`observed` and `metabench` both declared an example target named `basic`.
Cargo writes every example in the workspace into one shared
`target/<profile>/examples/` directory, so both resolved to the same output
file.
Reproduced before changing anything, on `84083b8`:
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 warning for `basic.pdb`. Cargo only warns, then builds both
targets concurrently into that one path.
This exact class already cost the repo a CI failure. PR #676 renamed a
duplicate `tower_service` example after the `link.exe` race it caused turned
into `LNK1104: cannot open file ...\examples\tower_service.exe` on 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.
`metabench`'s target is renamed to `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 referenced from
its own module docs, so renaming that side would have been the worse trade.
The two call sites in `metabench/tests/spawned_benchmark.rs` that spawn the
example by name move with it.
A rename alone repairs this pair and prevents nothing, which is why the
previous occurrence recurred. `automation::check_unique_example_names` now
fails on any duplicate, naming the example and every package that declares
it. `scripts/run-examples.rs` calls it before running anything, and 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 `--exclude`
from the `delta` job, 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 `automation` rather than inline in the script because a
`-Zscript` cargo script has no test harness. There it carries five unit
tests, including one that runs the check against this workspace's real
`cargo metadata` output, so the invariant is enforced by `cargo test` and not
only by the examples job.
Verified:
- The guard fires on the pre-fix tree. Restoring the old name and running
`this_workspace_has_no_colliding_example_names` fails with
"1 name is used by more than one package: - 'basic' is declared by:
metabench, observed".
- `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 through `cargo run --example`.
- `just anvil-clippy`, `just anvil-fmt` and `just anvil-spellcheck` clean.
`just anvil-license-headers` did NOT run: the local `cargo-heather` is
v0.2.1 and the recipe requires v0.3.0. No file gains or loses a header
here, but the check is unrun rather than green.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 634 634
Lines 84746 84746
=======================================
Hits 84746 84746
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
…sion
Review finding on this PR: the guard compared target names byte-for-byte while
the invariant it claims to enforce is an output-file collision. Correct, and
the case-only variant turns out to be the worse of the two.
Both halves were measured rather than argued.
The filesystem folds case. `Cargo.toml` and `CARGO.TOML` stat to one file on
this Windows volume, so `examples/basic.exe` and `examples/Basic.exe` are one
path. Default macOS volumes behave the same way.
Cargo does not warn. Renaming metabench's example to `Basic` against
`observed`'s `basic` and building both:
cargo build -p observed -p metabench --examples --all-features --release
Finished `release` profile [optimized + debuginfo] target(s) in 16.61s
No output-filename-collision warning anywhere -- compare the exact-duplicate
case, which does warn twice. Cargo's own detection compares `PathBuf`s, which
are case-sensitive in Rust, so it is blind to precisely the collision the
filesystem still performs. The exact duplicate at least announces itself; the
case-only one is silent on both sides.
With the fix in place the same tree is rejected:
- 'basic' is declared by: metabench ('Basic'), observed ('basic')
Names are now folded with `to_ascii_lowercase` before comparison. ASCII folding
is exact rather than an approximation here, because Cargo target names are
ASCII -- no Unicode case-folding subtleties apply.
Each package's own spelling is printed only when the spellings actually differ.
Printing it unconditionally would make the common exact-duplicate message
noisier for no gain, and omitting it in the case-only report would leave the
author unable to tell which of the two files to rename.
Three tests added: a case-only collision across two packages, a case-only
collision inside one package (Cargo permits `basic.rs` and `Basic.rs` as
distinct targets, the output paths are not distinct), and one pinning that the
exact-duplicate message keeps its terse `alpha, beta` form.
`cargo test -p automation --all-features` 13 passed; `just anvil-clippy`,
`just anvil-fmt` and `just anvil-spellcheck` clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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.
|
🤖 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.