From 2df9372abbd9c717a0100ed083ee62395c1fe4fd Mon Sep 17 00:00:00 2001 From: Vaiz <4908982+Vaiz@users.noreply.github.com> Date: Thu, 17 Sep 2026 04:04:44 +0100 Subject: [PATCH] fix(metabench): rename the duplicate `basic` example and gate the workspace on unique names `observed` and `metabench` both declare an example target named `basic`. Cargo writes every example in the workspace into one shared `target//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 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> --- .../{basic.rs => single_benchmark.rs} | 4 + crates/metabench/tests/spawned_benchmark.rs | 14 +-- .../workspace/ExampleTargetNames.Tests.ps1 | 90 +++++++++++++++++++ 3 files changed, 101 insertions(+), 7 deletions(-) rename crates/metabench/examples/{basic.rs => single_benchmark.rs} (81%) create mode 100644 scripts/tests/Pester/unit/workspace/ExampleTargetNames.Tests.ps1 diff --git a/crates/metabench/examples/basic.rs b/crates/metabench/examples/single_benchmark.rs similarity index 81% rename from crates/metabench/examples/basic.rs rename to crates/metabench/examples/single_benchmark.rs index f6d4b2750..062f4d65b 100644 --- a/crates/metabench/examples/basic.rs +++ b/crates/metabench/examples/single_benchmark.rs @@ -2,6 +2,10 @@ // Licensed under the MIT License. //! One shared workload and input registered with Criterion and Gungraun. +//! +//! Named for the single benchmark it registers, to contrast with the +//! `parameterized` example and to keep example target names unique across the +//! workspace -- Cargo writes every example to one shared output directory. use criterion::Criterion; diff --git a/crates/metabench/tests/spawned_benchmark.rs b/crates/metabench/tests/spawned_benchmark.rs index 6eeae646a..c45222698 100644 --- a/crates/metabench/tests/spawned_benchmark.rs +++ b/crates/metabench/tests/spawned_benchmark.rs @@ -66,7 +66,7 @@ fn run(arguments: &[&str]) -> Output { "--profile", "bench", "--example", - "basic", + "single_benchmark", "--", ]) .args(arguments) @@ -162,7 +162,7 @@ exit "$status" "--profile", "bench", "--example", - "basic", + "single_benchmark", "--", "--perf", "--show-engine-output", @@ -251,7 +251,7 @@ fn fake_vtune_directory() -> &'static Path { /// /// This guard only covers the vtune tests added alongside it; it is not a /// claim that the rest of this file (`run`/`run_target`'s unconditional -/// `--example basic`/`--example parameterized` invocations) can run from a +/// `--example single_benchmark`/`--example parameterized` invocations) can run from a /// published tarball. Those tests have depended on unpackaged examples since /// before this fixture existed, and fixing that pre-existing, file-wide gap /// is a separate concern from hardening the new vtune coverage this guard @@ -327,7 +327,7 @@ fn vtune_measures_exact_workload_and_writes_metrics() { "--profile", "bench", "--example", - "basic", + "single_benchmark", "--", "--vtune", "--show-engine-output", @@ -402,7 +402,7 @@ fn vtune_suppresses_report_output_without_show_engine_output() { "--profile", "bench", "--example", - "basic", + "single_benchmark", "--", "--vtune", "--no-baseline", @@ -450,7 +450,7 @@ fn vtune_command_failure_surfaces_vtune_control_error() { "--profile", "bench", "--example", - "basic", + "single_benchmark", "--", "--vtune", "--show-engine-output", @@ -486,7 +486,7 @@ fn vtune_pause_failure_surfaces_vtune_control_error() { "--profile", "bench", "--example", - "basic", + "single_benchmark", "--", "--vtune", "--show-engine-output", diff --git a/scripts/tests/Pester/unit/workspace/ExampleTargetNames.Tests.ps1 b/scripts/tests/Pester/unit/workspace/ExampleTargetNames.Tests.ps1 new file mode 100644 index 000000000..11971f4c7 --- /dev/null +++ b/scripts/tests/Pester/unit/workspace/ExampleTargetNames.Tests.ps1 @@ -0,0 +1,90 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +BeforeAll { + . (Join-Path $PSScriptRoot '..\..\_common\TestHelpers.ps1') + + $script:RepoRoot = Get-OxiRepoRoot +} + +Describe 'Workspace example target names' { + # Cargo writes every example in the workspace into one shared + # target//examples/ directory, so two packages declaring the same + # example name resolve to the same output file. Cargo only *warns* about + # the exact-duplicate case ("output filename collision ... this may become + # a hard error in the future", rust-lang/cargo#6313) and then builds both + # targets concurrently into that one path. On Windows the resulting + # link.exe race is fatal -- PR #676 renamed a duplicate `tower_service` + # example after it produced LNK1104 on an unrelated pull request. On Linux + # and macOS one binary silently overwrites the other, which is worse: the + # example that runs is not the one that was selected and nothing fails. + # + # This lives in the repository checks rather than in an Anvil recipe for + # two reasons. 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 would pass on exactly the pull + # requests that did not touch either colliding crate. + It 'declares every example target name exactly once across the workspace' { + Push-Location $script:RepoRoot + try { + $metadataJson = & cargo metadata --no-deps --format-version 1 + $LASTEXITCODE | Should -Be 0 -Because 'cargo metadata must succeed' + } finally { + Pop-Location + } + + $metadata = $metadataJson | ConvertFrom-Json + + $declarations = foreach ($package in $metadata.packages) { + foreach ($target in $package.targets) { + if ($target.kind -contains 'example') { + [pscustomobject]@{ + Package = $package.name + Example = $target.name + } + } + } + } + + $declarations | Should -Not -BeNullOrEmpty -Because 'the workspace has example targets to check' + + # Grouped case-insensitively on purpose. Windows and the default + # case-insensitive macOS volumes resolve `basic` and `Basic` to one + # path, but Cargo compares PathBufs -- which are case-sensitive in Rust + # -- so it does not even emit its usual warning for that variant. The + # case-only collision is therefore the more dangerous of the two: + # silent on both sides. Cargo target names are ASCII, so folding with + # ToLowerInvariant is exact here rather than an approximation. + $collisions = @( + $declarations | + Group-Object -Property { $_.Example.ToLowerInvariant() } | + Where-Object Count -GT 1 + ) + + if ($collisions.Count -gt 0) { + $detail = foreach ($collision in $collisions) { + # Spell out each package's own casing only when the spellings + # actually differ, so the common exact-duplicate message stays + # terse and a case-only collision is not mistaken for a + # reporting bug. + $spellings = @($collision.Group.Example | Sort-Object -Unique) + $owners = if ($spellings.Count -eq 1) { + (@($collision.Group.Package | Sort-Object) -join ', ') + } else { + (@($collision.Group | Sort-Object Package | ForEach-Object { "$($_.Package) ('$($_.Example)')" }) -join ', ') + } + " - '$($collision.Name)' is declared by: $owners" + } + + $message = @( + "Example target names must be unique across the workspace, but $($collisions.Count) name(s) are used by more than one package:" + $detail + '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.' + ) -join [Environment]::NewLine + + throw $message + } + } +}