Skip to content

fix(metabench): rename the duplicate basic example and gate the workspace on unique names - #751

Closed
Evgenii (Vaiz) wants to merge 1 commit into
mainfrom
u/vaiz/2026/09/13/dedupe-basic-example-name
Closed

Evgenii (Vaiz) wants to merge 1 commit into
mainfrom
u/vaiz/2026/09/13/dedupe-basic-example-name

Conversation

@Vaiz

Copy link
Copy Markdown
Contributor

🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.

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
          <https://github.com/rust-lang/cargo/issues/6313>

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_service example after the link.exe race it caused surfaced as 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.

#676 renamed and moved on, so the class recurred four weeks later. This PR closes both halves.

What changed

The rename. 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. The two call sites in metabench/tests/spawned_benchmark.rs that spawn the example by name move with it.

The guard. automation::check_unique_example_names fails on any duplicate, naming the example and every package that declares it:

example target names must be unique across the workspace, but 1 name is used by more than one package:
  - 'basic' is declared by: metabench, observed
All examples share one output directory, so these overwrite each other. Rename one side to a name
that says what distinguishes it, or give it an explicit `[[example]] name = ...` in its Cargo.toml.

scripts/run-examples.rs calls 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 --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, not only by the examples job.

Verification

  • The guard fires on the pre-fix tree. Restoring the old name and running this_workspace_has_no_colliding_example_names fails 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 through cargo run --example.
  • just anvil-clippy, just anvil-fmt, 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 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 the EXCLUDED_EXAMPLES array — a disjoint region from this change, which adds an import and one call. Whichever lands second should merge cleanly. Its new fake_vtune example does not collide with anything.

Copilot AI lite review requested due to automatic review settings September 14, 2026 03:05

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.

🔵 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 basic to single_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 basic and Basic), this check passes while Windows and the usual case-insensitive macOS volumes resolve both to the same target/<profile>/examples path, 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.rs path. The generated Anvil PR test includes anvil-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

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (ec1f6bc) to head (2df9372).
⚠️ Report is 1 commits behind head on main.

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     
Flag Coverage Δ
linux 100.0% <ø> (ø)
linux-arm 100.0% <ø> (ø)
scheduled ?
windows 100.0% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


//! One shared workload and input registered with Criterion and Gungraun.
//!
//! Named for the single benchmark it registers, to contrast with the

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this comment has 0 value

@Vaiz
Evgenii (Vaiz) marked this pull request as draft September 14, 2026 06:10
@Vaiz

Copy link
Copy Markdown
Contributor Author

Both review findings addressed in 108a0e0d. One was correct and is fixed; the other I am pushing back on, with the measurement.

1. Case-only collisions — correct, and worse than the exact duplicate

Fixed. I measured both halves rather than reasoning about them.

The filesystem folds case. Cargo.toml and CARGO.TOML stat to the same file on this Windows volume, so examples/basic.exe and examples/Basic.exe are one path.

Cargo does not warn about it. I renamed metabench's example to Basic against observed's basic and built both:

cargo build -p observed -p metabench --examples --all-features --release
    Finished `release` profile [optimized + debuginfo] target(s) in 16.61s

No collision warning anywhere — compare the exact-duplicate case, which warns twice (.exe and .pdb). Cargo's detection compares PathBufs, which are case-sensitive in Rust, so it is blind to precisely the collision the filesystem still performs. The case-only variant is the more dangerous of the two: the exact duplicate at least announces itself, this one is silent on both sides. That makes this the more valuable half of the guard, not an edge case.

With the fix, that same tree is rejected:

  - 'basic' is declared by: metabench ('Basic'), observed ('basic')

Names are folded with to_ascii_lowercase. ASCII folding is exact here rather than an approximation, because Cargo target names are ASCII — no Unicode folding subtleties apply. Each package's own spelling prints only when the spellings differ, so the common message keeps its terse alpha, beta form while a case-only report still tells you which file to rename. Three tests added, including the case-only regression you asked for and one inside a single package (Cargo permits basic.rs and Basic.rs as distinct targets; the output paths are not distinct).

2. anvil-examples does not invoke the guard — observation right, conclusion wrong

Your observation is accurate: justfiles/anvil/checks/examples.just builds and runs examples directly and never calls scripts/run-examples.rs. But the conclusion — that "the workspace-wide invariant is not enforced by the repository's generated validation path" — does not hold, for a reason the diff makes easy to miss.

The check is not only wired into that script. It is also a unit test in automation, this_workspace_has_no_colliding_example_names, which runs the check against the real cargo metadata output. That executes under cargo test, which anvil-pr-test reaches via anvil-llvm-cov. So the invariant is enforced by the generated path — through the test group rather than the examples group. That was deliberate: a -Zscript cargo script has no test harness, so logic living only there cannot be tested or shown to fire.

I am also not taking the suggested remedy. examples.just carries GENERATED BY cargo-anvil. DO NOT EDIT DIRECTLY — editing it would be overwritten on the next regeneration and would stop automatic updates for this repo. The durable version of your point belongs in cargo-anvil itself, and is already tracked as AB#7871260, which proposes a workspace-wide duplicate-target check covering examples, binaries and benchmarks. This PR delivers the example half inside oxidizer; the generated-recipe half is that work item's.

One caveat worth stating, since it is the real residue of your point: anvil-llvm-cov is impact-scoped, so a PR that adds a colliding example without touching automation could have the test skipped by delta selection. That is a narrower gap than "not enforced", but it is not nothing, and it is the same scoping argument the check itself already makes internally (it reads every package, never the selection). I have left it rather than papering over it, because the fix is the cargo-anvil one above.

CI

050003cd finished 56 success / 2 failing. Both failures were Runtime Analysis (windows-arm) and the Required Anvil checks roll-up that reports it — the job ran 68 minutes and its anvil-run-group step has no conclusion, which is a cancellation rather than an assertion failure, and I could not retrieve the log to confirm (gh returns HTTP 404 for this repo from both configured accounts). It is not reproduced locally and PR #750 passed the same job. I am flagging it as unexplained rather than claiming it is flaky.

@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/13/dedupe-basic-example-name branch from 108a0e0 to 9732519 Compare September 16, 2026 08:26
@Vaiz

Copy link
Copy Markdown
Contributor Author

Rebased onto c5890f3a and force-pushed as 97325192. The 32 failing checks on the previous head were inherited from a stale base, not caused by this branch — worth saying explicitly, because the history now shows a wall of red that has nothing to do with the change under review.

What was failing

anvil-deny was failing on all four Fast Checks legs for two reasons, both fixed on main while this PR sat:

  • rustls 0.23.44 — RUSTSEC advisory, TLS 1.3 handshake messages incorrectly accepted across encryption level boundaries. Fixed by fix: update rustls for RUSTSEC-2026-0285 #761 (6a78eea1).
  • A stale ignore entry. This branch's deny.toml still carried RUSTSEC-2026-0194 / RUSTSEC-2026-0195 for quick-xml, and cargo-deny reported no crate matched advisory criteria for -0195. Those entries are gone from main entirely — the Azure SDK dependency that pinned quick-xml 0.39.x has moved on, so the ignores became not just unnecessary but themselves a failure.

This was repo-wide, not specific to this PR: #746 hit the identical failure.

Verified after the rebase

  • just anvil-denyadvisories ok, bans ok, licenses ok, sources ok. This is the gate that was red.
  • cargo test -p automation --all-features — 13 passed. The workspace test now also covers fake_vtune, the example feat(metabench): add Intel VTune support alongside Linux perf #750 added while this PR was open, and finds no collision.
  • just anvil-clippy — clean against the new base, which is a meaningfully different workspace (compressors and http_compression are new crates).

Both of this branch's commits replayed without conflict, including against #750's edit to scripts/run-examples.rs — that PR touched EXCLUDED_EXAMPLES while this one adds an import and a call, so the two are disjoint as predicted.

Still open

The Runtime Analysis (windows-arm) failure from head 050003cd is not explained by this rebase, and I am not claiming it is resolved. It ran 68 minutes with no conclusion recorded on its anvil-run-group step — a cancellation shape rather than an assertion failure — and I still cannot read the job log (gh returns HTTP 404 for this repository from both configured accounts). If it recurs on this head it needs a human with working log access.

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>
@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/13/dedupe-basic-example-name branch from 9732519 to 2df9372 Compare September 17, 2026 06:10
@Vaiz

Copy link
Copy Markdown
Contributor Author

Rebuilt on ec1f6bc2 and force-pushed as 2df9372a. This one carries a correction I owe the reviewer.

I was wrong on the second finding, and #759 proved it

On 09-15 the Copilot review said the guard "only gates the legacy scripts/run-examples.rs path" and asked for it to be wired into a shared, unscoped validation step. I pushed back, arguing the invariant was enforced anyway through a unit test in the automation crate reached by anvil-llvm-cov.

#759 has since deleted crates/automation/ and scripts/run-examples.rs outright. Both of the things I named as the enforcement path are gone. The reviewer was identifying a structural problem — the check was attached to a path that was already being retired — and I answered it as though it were a coverage question. Calling that file "the legacy path" was accurate and I should have taken it literally.

The guard is now a Pester test in the repository suite:

  • scripts/tests/Pester/unit/workspace/ExampleTargetNames.Tests.ps1
  • runs under just test-scripts, which already gates Required repository checks
  • that workflow is not generated, so it survives cargo anvil regeneration, and it is not impact-scoped, so it cannot be delta-skipped on a PR that touches neither colliding crate

That is what the review asked for, in the place that now exists for it. The narrower residue I conceded last time — that anvil-llvm-cov is impact-scoped — is also closed by the move.

A second thing the rebase exposed

#750 added four new --example basic call sites in crates/metabench/tests/spawned_benchmark.rs while this PR was open. Git merged that with my rename without conflict, because the two touch different lines — and CI then failed vtune_measures_exact_workload_and_writes_metrics and two siblings on all four platforms.

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

  • cargo build --workspace --examples --all-features --release --lockedno filename-collision warning anywhere. The collision reproduces on ec1f6bc2 first (both basic.exe and basic.pdb).
  • cargo test -p metabench --test spawned_benchmark --all-features8 passed, including the four vtune tests that were red on all four platforms.
  • just anvil-clippy, just anvil-fmt, just anvil-spellcheck — clean.
  • The new Pester file passes PowerShell AST parsing and PSScriptAnalyzer with zero findings.

Not verified — please read before approving

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. Static checks pass, but CI's Release script tests job on ubuntu-latest and windows-latest is its first real execution. If it is going to be wrong, that is where it shows up, and I would rather say so than let a green-looking summary imply otherwise.

Still a draft, since that state was set deliberately.

@Vaiz
Evgenii (Vaiz) deleted the u/vaiz/2026/09/13/dedupe-basic-example-name branch September 17, 2026 10:26
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