perf(cargo-anvil): parallelize Miri test artifacts - #123
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the cargo-anvil Miri check implementation to compile the selected workspace scope once and then execute discovered profile.test = true libtest artifacts concurrently via cargo-miri runner, with deterministic log replay and aggregated failures. It also adds a package-level opt-out knob ([package.metadata.anvil.miri] exclude = true) and applies the updated templates to this repository, including documentation and snapshot updates.
Changes:
- Add a shared
_anvil-miri-testJust recipe that compiles once (--no-run), discovers test artifacts from Cargo JSON, and runs them in parallel withANVIL_MIRI_JOBScontrolling concurrency. - Update all Miri profiles (
miri,miri-tree-borrows,miri-strict-provenance,miri-race-coverage) to delegate execution to the shared runner and preserve profile-specific flags. - Extend
cargo-anvilcontract tests/docs/snapshots and update.anvil.lockto reflect regenerated template outputs.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| justfiles/anvil/checks/miri.just | Introduces _anvil-miri-test and routes anvil-miri through the shared parallel artifact runner. |
| justfiles/anvil/checks/miri-tree-borrows.just | Delegates Tree Borrows profile execution to _anvil-miri-test. |
| justfiles/anvil/checks/miri-strict-provenance.just | Delegates Strict Provenance profile execution to _anvil-miri-test. |
| justfiles/anvil/checks/miri-race-coverage.just | Delegates Race Coverage profile execution to _anvil-miri-test. |
| crates/cargo-anvil/templates/justfiles/anvil/checks/miri.just | Template source for the new shared parallel Miri artifact runner. |
| crates/cargo-anvil/templates/justfiles/anvil/checks/miri-tree-borrows.just | Template updated to call the shared runner. |
| crates/cargo-anvil/templates/justfiles/anvil/checks/miri-strict-provenance.just | Template updated to call the shared runner. |
| crates/cargo-anvil/templates/justfiles/anvil/checks/miri-race-coverage.just | Template updated to call the shared runner. |
| crates/cargo-anvil/src/lib.rs | Updates public crate documentation to describe compile-once + parallel artifact execution, plus package opt-out metadata. |
| crates/cargo-anvil/docs/design/local.md | Documents the new Miri concurrency model and the package-level exclude metadata. |
| crates/cargo-anvil/docs/design/checks.md | Updates the miri check contract to match the new artifact runner behavior and profile delegation. |
| crates/cargo-anvil/src/anvil/artifacts/justfile.rs | Adds a regression test ensuring Miri profiles delegate to the shared runner and key behaviors are present in the generated check file bodies. |
| crates/cargo-anvil/tests/recipe_contracts.rs | Adds contract tests with a fake cargo miri/cargo-miri runner to validate exclusion logic, parallelism, flag propagation, and failure aggregation. |
| crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap | Updates expected generated output snapshot to include _anvil-miri-test and delegation. |
| crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap | Updates expected generated output snapshot to include _anvil-miri-test and delegation. |
| crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap | Updates expected generated output snapshot to include _anvil-miri-test and delegation. |
| crates/cargo-anvil/README.md | Regenerated README content reflecting the updated Miri behavior and metadata opt-out. |
| .anvil.lock | Updates catalog checksum and file/region checksums for regenerated Anvil-managed files. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
08135d3 to
f748586
Compare
f748586 to
661df32
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
justfiles/anvil/checks/miri.just:177
- The PR description claims “memory telemetry”, but this runner currently doesn’t emit any memory usage/pressure info (it only prints artifact/job counts). Either add explicit telemetry (and document what it measures) or adjust the PR description so it doesn’t promise this behavior.
Write-Host "anvil miri: running $($artifacts.Count) test artifacts with $jobs concurrent process(es)"
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (97.6%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
=====================================
Coverage 97.6% 97.6%
=====================================
Files 290 290
Lines 65860 65860
=====================================
+ Hits 64316 64318 +2
+ Misses 1544 1542 -2
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:
|
Sander Saares (sandersaares)
left a comment
Sander Saares (sandersaares)
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Published 19 findings. No finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Hit |
There was a problem hiding this comment.
[Copilot speaking]
Document the shared Miri runner architecture in the implementation guide
The shared _anvil-miri-test recipe introduces a substantial internal execution subsystem: it maps Cargo metadata to package exclusions and working directories, compiles once into a Cargo JSON artifact manifest, selects and de-duplicates test artifacts, reconstructs the pinned cargo-miri runner environment, dispatches artifacts through bounded PowerShell workers, captures isolated logs, replays results deterministically, aggregates failures, and cleans up temporary state. The user-visible behavior is documented in the design chapters and README, but crates/cargo-anvil/docs/implementation.md contains no corresponding Miri architecture or synchronization boundaries.
Reproducible reasoning: The implementation guide is the repository's designated location for high-level internal decisions that keep emitted behavior aligned with the design. Correctness of the new runner depends on coordinated invariants across the canonical template, generated recipes, Cargo metadata and JSON protocols, package working directories, MIRI_SYSROOT and MIRI_BE_RUSTC, profile delegates, parallel log and failure coordination, contract tests, structural assertions, and snapshots. These are implementation decisions rather than only user-visible settings or guarantees. Without an implementation-guide section, maintainers must reverse engineer the recipe, generated copies, tests, and pinned Miri protocol to identify which mechanisms are architectural and which files must remain synchronized.
Consequence: A later maintenance change can preserve the documented command-line behavior while breaking artifact selection, package mapping, standalone cargo-miri environment restoration, generated-file synchronization, deterministic reporting, or verification coverage because the subsystem's internal contracts and ownership boundaries are undocumented.
Recommended action: Add a focused Miri-runner section to crates/cargo-anvil/docs/implementation.md. Describe the canonical-template and generated-copy ownership boundary; the compile, discover, and execute stages; package exclusion, artifact selection, package-ID mapping, and working-directory invariants; the pinned cargo-miri runner environment contract; profile delegation; bounded worker, isolated logging, deterministic replay, failure aggregation, and cleanup; and the structural, executable contract, and snapshot tests that guard these boundaries. Keep configuration and other user-visible promises in the existing design chapters.
References:
- crates/cargo-anvil/docs/implementation.md
- crates/cargo-anvil/docs/design/checks.md
- crates/cargo-anvil/docs/design/local.md
- crates/cargo-anvil/README.md
- crates/cargo-anvil/templates/justfiles/anvil/checks/miri.just
- crates/cargo-anvil/tests/recipe_contracts.rs
Impacted locations:
crates/cargo-anvil/docs/design/checks.mdcrates/cargo-anvil/docs/design/local.mdcrates/cargo-anvil/README.mdcrates/cargo-anvil/docs/implementation.mdcrates/cargo-anvil/src/anvil/artifacts/justfile.rscrates/cargo-anvil/templates/justfiles/anvil/checks/miri.justcrates/cargo-anvil/tests/recipe_contracts.rscrates/cargo-anvil/tests/snapshots/snapshots__github_backend.snapcrates/cargo-anvil/tests/snapshots/snapshots__local_only.snapjustfiles/anvil/checks/miri.justjustfiles/anvil/checks/miri-tree-borrows.justjustfiles/anvil/checks/miri-strict-provenance.justjustfiles/anvil/checks/miri-race-coverage.just
| | `miri-race-coverage` | Uses the shared compile-once artifact runner with `MIRIFLAGS="-Zmiri-many-seeds=<low>..<high>"` and `RUSTFLAGS='--cfg miri_race_coverage'`. The `<low>..<high>` window rotates daily based on day-of-month (day N -> seeds `2N-1..2N+1`, exclusive upper bound -> 2 seeds/day, about 62 seeds/month). Per-test opt-outs use `#[cfg_attr(miri_race_coverage, ignore = "<reason>")]`. | oxidizer-github | | ||
|
|
||
| These profiles each cost hours per leg (oxidizer caps `miri-race-coverage` at 12 h), which is why they live in scheduled rather than PR. They share `miri`'s setup and run sequentially within the `scheduled-runtime-analysis` group. Each uses a profile-specific `MIRIFLAGS` (the actual miri mode, e.g. `-Zmiri-tree-borrows`) plus a profile-specific `--cfg miri_<profile>` in `RUSTFLAGS`; the distinct cfg is what lets a test opt out of just one profile via `#[cfg_attr(miri_<profile>, ignore = "…")]` without affecting the others. The OS matrix matches `pr-runtime-analysis` (4 legs on GitHub, 2 on ADO) so any OS already considered "worth running miri on" gets the harder profiles too -- the single-tier-per-group rule forbids running tree-borrows on a strict subset of the OSes where stacked-borrows runs, which would silently hide tree-borrows-only UB on the dropped legs. | ||
| These profiles each cost hours per leg, which is why they live in scheduled rather than PR. They share `miri`'s setup and run sequentially within the `scheduled-runtime-analysis` group; only independent test artifacts inside one profile run concurrently. Each uses profile-specific `MIRIFLAGS` plus a profile-specific `--cfg miri_<profile>` in `RUSTFLAGS`, letting a test opt out of one profile without affecting the others. The OS matrix matches `pr-runtime-analysis` (4 legs on GitHub, 2 on ADO) so any OS considered worth running Miri on gets the harder profiles too. |
There was a problem hiding this comment.
[Copilot speaking]
Explain why stricter Miri profiles retain the full Miri OS matrix
The design states that the stricter scheduled Miri profiles use the same operating-system matrix as pr-runtime-analysis, but the explanation only restates the policy: operating systems considered worth standard Miri also receive the harder profiles. It does not document why the expensive profiles must not use a smaller matrix.
Reproducible reasoning: Tree Borrows, strict provenance, and race coverage cost hours per leg, so reducing their operating-system coverage is an attractive future optimization. The current text does not give a maintainer enough information to evaluate that change. The decision depends on the catalog's single-tier-per-group constraint and on preserving detection of profile-specific undefined behavior on every operating system where Miri is supported; those considerations are not evident from the matrix counts themselves.
Consequence: A maintainer trying to reduce scheduled cost could narrow the stricter profiles to fewer operating systems and unintentionally remove coverage for undefined behavior that appears only under one of those profiles on an omitted platform.
Recommended action: Document the constraint and intended coverage outcome next to the matrix statement: using a smaller matrix would leave profile-specific behavior unchecked on supported Miri platforms, and the tier/group structure does not provide a separate subset without changing the catalog organization.
Impacted locations:
crates/cargo-anvil/docs/design/checks.md:255
There was a problem hiding this comment.
[Copilot speaking]
Confirm whether the enlarged test snapshots belong in the published crate
The refreshed pull request enlarges each packaged backend snapshot by 9,871 bytes: snapshots__ado_backend.snap, snapshots__github_backend.snap, and snapshots__local_only.snap. Because crates/cargo-anvil/Cargo.toml has no explicit include or exclude package-file configuration, Cargo's default packaging behavior includes these integration-test snapshots and adds 29,613 bytes to the published source package.
Reproducible reasoning: All three reviewed files are committed snapshot-test data under crates/cargo-anvil/tests/snapshots/. Comparing the refreshed merge base and source commits shows the same generated Miri fixture expansion in each file. The owning manifest does not constrain packaged files, so the test tree is included by default. Although that packaging policy predates the pull request, the package-size increase is introduced by this change and therefore requires an explicit decision.
Consequence: Each release carries the additional snapshot data in its crate archive, increasing registry storage and user download and extraction work even though the files are used only by integration tests.
Recommended action: Confirm whether publishing the test snapshots is intentional. If they are not publication inputs, add an explicit package include allowlist or equivalent exclude configuration that omits the snapshots while retaining every source and embedded asset required to build the crate. If they are intentionally published, explicitly accept and document the package-size increase after confirming the resulting package contents.
References:
- Cargo package layout
- crates/cargo-anvil/docs/verification.md
Impacted locations:
crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap:2841-3097crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap:2720-2976crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap:1593-1849crates/cargo-anvil/Cargo.toml
| $artifacts = [System.Collections.Generic.List[object]]::new() | ||
| $seenArtifacts = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::Ordinal) | ||
| foreach ($line in Get-Content -LiteralPath $artifactManifest) { | ||
| try { | ||
| $message = $line | ConvertFrom-Json -ErrorAction Stop | ||
| } catch { | ||
| Write-Error "anvil miri: could not parse Cargo JSON output: $($_.Exception.Message)" | ||
| exit 1 | ||
| } | ||
| if ($message.reason -eq 'compiler-message' -and $message.message.rendered) { | ||
| [Console]::Error.Write($message.message.rendered) | ||
| continue | ||
| } | ||
| if ($message.reason -ne 'compiler-artifact' -or -not $message.executable -or $message.profile.test -ne $true) { | ||
| continue | ||
| } | ||
| $artifactPath = [string]$message.executable | ||
| if (-not $seenArtifacts.Add($artifactPath)) { continue } | ||
| $packageId = [string]$message.package_id |
There was a problem hiding this comment.
[Copilot speaking]
Explain the artifact admission and deduplication boundary
The runner accepts only executable compiler-artifact messages whose Cargo profile is marked as a test, deduplicates them by exact executable path, and then rejects artifacts owned by package IDs that opted out. These predicates decide which compiled outputs Miri executes, but the overview does not state which checks are Cargo-format requirements and which are defensive safeguards.
Reproducible reasoning: The message loop first handles rendered compiler diagnostics, then excludes non-artifact records, records without an executable, and non-test profiles. It subsequently suppresses repeated paths and enforces package-ID exclusion even after selection arguments were filtered. The code shows what happens, but not whether repeated paths or excluded-package test artifacts are expected Cargo emissions or safeguards against future or irregular streams. That distinction matters because a maintainer cannot judge whether either apparently redundant guard can be removed.
Consequence: A cleanup can remove deduplication or the post-selection exclusion guard and cause a test executable to run more than once or allow an opted-out package target to execute when Cargo output differs from the common case.
Recommended action: Add a concise comment above the artifact loop defining the admission boundary: only executable test-profile compiler artifacts are runnable; executable paths are unique work identities; and package-ID exclusion is enforced as a final opt-out guard. State whether duplicate records are observed or merely defended against so the condition can be reevaluated with evidence. Keep the canonical template as the source and regenerate the managed recipe and snapshots.
References:
Impacted locations:
crates/cargo-anvil/templates/justfiles/anvil/checks/miri.just:102-120justfiles/anvil/checks/miri.just:102-120
| | Check | Invocation | Source | | ||
| |-----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------| | ||
| | `miri` | `cargo +<pinned-nightly> miri test --all-features --tests` over the impact-affected packages. Uses libtest (one process per test binary), **not** `cargo miri nextest run`: under miri, nextest's process-per-test model pays miri's expensive std-initialization re-interpretation for *every* test and roughly doubles wall-time on a large suite (the dominant cost on the PR critical path). `--tests` runs lib/bin unit tests and integration tests (the same target set nextest ran) while excluding doctests, which miri can't run; it is used in preference to `--lib --tests` because `--lib` errors with "no library targets found" on a bin-only affected package under impact scoping. Slow tests opt out per-test with `#[cfg_attr(miri, ignore)]` -- anvil doesn't pass exotic `MIRIFLAGS`; the per-test opt-out is the canonical mechanism. libtest exits 0 when a binary's tests are all skipped, so no `--no-tests=pass` workaround is needed. The recipe reads its scope from the `target/anvil/impact/` cache via `_anvil-impact-include`; because it depends on `anvil-impact`, a clean direct or PR invocation is impact-scoped (unaffected packages are skipped). It runs the full workspace only when scoping is off — the scheduled/full tiers set `ANVIL_IMPACT=off`, and a dirty local tree widens for safety. | oxidizer, oxidizer-github | | ||
| | `miri` | Compiles `cargo +<pinned-nightly> miri test --all-features --tests --no-run` once over the impact-affected packages, parses Cargo's JSON artifact stream, then runs independent libtest binaries concurrently through `cargo-miri runner`. Compiling the selected scope together preserves Cargo feature unification; artifact workers avoid serializing every binary. The default is one worker per logical processor, clamped to the artifact count; `ANVIL_MIRI_JOBS` accepts a positive integer override. Output is isolated per artifact, replayed deterministically, and every artifact completes before failures are aggregated. Only artifacts with `profile.test = true` run, excluding build scripts and ordinary executables. `--tests` selects lib/bin unit tests and integration tests while excluding doctests, benches, and examples. Slow or unsupported individual tests opt out with `#[cfg_attr(miri, ignore)]`. A package can omit all its own Miri targets through `[package.metadata.anvil.miri] exclude = true`; it may still compile as a dependency of a selected consumer. The recipe remains impact-scoped for clean direct and PR invocations and full-workspace for scheduled/full tiers or a dirty local tree. | oxidizer, oxidizer-github | |
There was a problem hiding this comment.
[Copilot speaking]
Separate the public Miri contract from the pinned runner protocol
The check catalog, generated README, and crate-level API documentation expose Cargo JSON parsing and the hidden cargo-miri runner phase as though consumers must rely on them. The supported public behavior is instead that one Cargo invocation compiles the selected scope into Miri test executables, each executable is scheduled as one concurrency unit, ANVIL_MIRI_JOBS controls the worker limit, package exclusions and selected-scope feature behavior are preserved, and output and failures follow the documented rules. Internally, the parallel path depends on a pinned cargo-miri protocol: cargo miri test --no-run produces run-information files, then the recipe prepares the Miri sysroot, locates cargo-miri in the pinned toolchain, establishes the ambient MIRI_BE_RUSTC sentinel, and invokes the hidden runner phase for each executable.
Reproducible reasoning: Consumers observe selection, compilation, executable-level concurrency, configuration, exclusions, diagnostics, and exit status; they do not consume Cargo messages or invoke the runner phase. The pinned cargo-miri source shows that runner is an internal target-runner phase rather than a documented subcommand. It also shows why the bootstrap is a single protocol: the compile phase serializes build-time arguments, environment, and working directory; phase_runner restores that state, uses the runtime working directory as MIRI_CWD, and starts the Miri driver. The driver command begins with MIRI_BE_RUSTC removed, while the matching ambient sentinel lets restoration recognize the recorded value as unchanged so the removal remains effective during interpretation. The nightly pin constrains when these mechanics can change, but it does not turn them into a supported public API.
Consequence: Publishing Cargo JSON and the hidden runner phase makes a replaceable discovery or dispatch mechanism appear to be a compatibility promise. Separately, a pinned-nightly upgrade or local cleanup can remove or alter an apparently incidental sysroot, executable-lookup, sentinel, or runner step and break every generated Miri execution even when supported cargo miri commands remain compatible.
Recommended action: Keep public documentation behavioral: describe compiling the selected scope once and running the resulting Miri test executables concurrently under the documented worker, exclusion, output, and failure behavior, without naming Cargo message fields or the hidden runner phase; update src/lib.rs and regenerate the README from it. Isolate the Cargo message schema, executable-artifact selection criteria, and complete pinned runner protocol in docs/implementation.md; add one block comment before the bootstrap covering the prepared sysroot, pinned-toolchain cargo-miri lookup, ambient sentinel, serialized run information, and later runner invocation, and explain the sentinel's role without duplicating its literal value. Treat each pinned-nightly upgrade as a required revalidation of this protocol and its executable contract, and keep structural tests focused on stable delegation rather than incidental protocol tokens.
References:
- crates/cargo-anvil/docs/implementation.md
- crates/cargo-anvil/docs/design/checks.md
- crates/cargo-anvil/docs/design/local.md
- Pinned cargo-miri phase dispatch and hidden runner invocation
- Pinned cargo-miri phase implementation
- Pinned cargo-miri serialized environment model
- crates/cargo-anvil/docs/verification.md
Impacted locations:
crates/cargo-anvil/docs/design/checks.md:199crates/cargo-anvil/README.mdcrates/cargo-anvil/src/lib.rscrates/cargo-anvil/src/anvil/artifacts/justfile.rscrates/cargo-anvil/templates/justfiles/anvil/checks/miri.just:138-163crates/cargo-anvil/templates/justfiles/anvil/checks/miri.just:215justfiles/anvil/checks/miri.just:138-163justfiles/anvil/checks/miri.just:215
661df32 to
7bec4b6
Compare
Compile each Miri profile once to preserve workspace feature unification, then execute independent test artifacts concurrently through cargo-miri. Add configurable concurrency, package-level exclusions, grouped artifact output, memory telemetry, and aggregated failure reporting for GitHub and Azure DevOps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98e52fe3-a435-412e-9154-73a557fef79f
Remove tuning telemetry inherited from oxidizer #706, including the process-wide forced collections around every artifact. Keep the generic concurrency control through ANVIL_MIRI_JOBS and document the private recipe clearly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98e52fe3-a435-412e-9154-73a557fef79f
Use the minimal raw-string delimiter accepted by Rust 1.97 and clear inherited GitHub and ADO markers so each recipe-contract test selects its output backend explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98e52fe3-a435-412e-9154-73a557fef79f
Use the resolved stable toolchain for metadata and expose the pinned nightly directly on Miri invocations, matching cargo-anvil's deterministic toolchain contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98e52fe3-a435-412e-9154-73a557fef79f
7bec4b6 to
d855c41
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new _anvil-miri-test runner uses rustc ... --print sysroot output as a scalar, which can break if PowerShell captures multiple lines/arrays; the sysroot parsing should be made robust (comments added in both miri.just copies).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Lite
| $toolchainSysroot = & rustc $toolchain --print sysroot | ||
| if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrWhiteSpace($toolchainSysroot)) { | ||
| Write-Error "anvil miri: could not resolve the $toolchain toolchain sysroot" | ||
| exit 1 | ||
| } | ||
| $toolchainBin = Join-Path $toolchainSysroot.Trim() 'bin' |
| $toolchainSysroot = & rustc $toolchain --print sysroot | ||
| if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrWhiteSpace($toolchainSysroot)) { | ||
| Write-Error "anvil miri: could not resolve the $toolchain toolchain sysroot" | ||
| exit 1 | ||
| } | ||
| $toolchainBin = Join-Path $toolchainSysroot.Trim() 'bin' |
Forward the Miri worker cap into containers, handle fully excluded workspaces, and identify artifacts by package and target. Strengthen impact/exclusion contracts and document the corresponding behavior and rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98e52fe3-a435-412e-9154-73a557fef79f
There was a problem hiding this comment.
🟡 Changes recommended
The new Miri runner currently uses Cargo’s path-based package_id directly for labels/sorting, which can leak machine-specific paths and make output ordering environment-dependent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
crates/cargo-anvil/templates/justfiles/anvil/checks/miri.just:196
- Sorting artifacts by raw
PackageIdmakes the replay order depend on Cargo’s path-based package-id format (e.g.path+file:///...), which varies across machines/CI. Once you introduce a stablePackageLabel(name/version), sort by that instead to keep output deterministic across environments.
$orderedArtifacts = @($artifacts | Sort-Object PackageId, TargetKind, TargetName, Path)
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Lite
| $targetKind = $targetKinds -join '/' | ||
| $artifactFileName = Split-Path -Leaf $artifactPath | ||
| $artifacts.Add([pscustomobject]@{ | ||
| Path = $artifactPath | ||
| PackageId = $packageId | ||
| TargetName = $targetName | ||
| TargetKind = $targetKind | ||
| Label = "$packageId :: $targetKind $targetName ($artifactFileName)" | ||
| WorkingDirectory = $packageDirectories[$packageId] | ||
| }) |
| $ErrorActionPreference = 'Stop' | ||
| $packageArgs = @('{{ replace(package_args, "'", "''") }}' -split '\s+' | Where-Object { $_ }) | ||
|
|
||
| $metadataJson = & cargo {{_anvil_stable_toolchain_args}} metadata --no-deps --format-version 1 |
There was a problem hiding this comment.
🤖: Miri now compares package IDs emitted by two different Cargo versions, which makes the check fail for adopters whose stable/MSRV Cargo predates the package-ID format introduced in Cargo 1.78.
Please obtain the metadata with the same pinned nightly Cargo that emits the artifact stream, or join the records through an identity that is stable across Cargo versions. With an older MSRV, $packageDirectories contains the legacy ID spelling while cargo +nightly miri emits the current spelling, so every test artifact reaches package metadata not found instead of running.
Match deterministic replay group headers instead of parallel progress messages, and canonicalize fixture working directories before comparing Windows path spellings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98e52fe3-a435-412e-9154-73a557fef79f
There was a problem hiding this comment.
🔵 Needs a closer look
The test harness currently inherits ANVIL_MIRI_JOBS from the outer environment, which can make contract/snapshot tests flaky or fail unexpectedly unless it’s explicitly cleared.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-anvil/tests/recipe_contracts.rs:394
- The test harness clears ANVIL_IMPACT and CI backend markers, but it still inherits ANVIL_MIRI_JOBS from the outer environment. If a developer/CI has ANVIL_MIRI_JOBS set (or set to an invalid value), unrelated snapshot/contract tests can become flaky or fail unexpectedly. Clear ANVIL_MIRI_JOBS here for isolation; tests that need it already pass it explicitly.
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
cargo-miri runner, defaulting to one worker per logical processor with anANVIL_MIRI_JOBSoverride.This brings the generic Miri orchestration developed in microsoft/oxidizer#706 into
cargo-anvil. Repository-specific workload reductions and artifact priorities remain outside cargo-anvil.Behavior
The four scheduled Miri profiles remain sequential. Within each profile, Cargo compiles the selected scope once with
--no-run, and only Cargo artifacts markedprofile.test = trueare executed concurrently.Packages can exclude their own test targets while remaining available as dependencies:
ANVIL_MIRI_JOBSaccepts a positive integer override.Validation
cargo-anviltest suitecargo anvil --dry-runafter applying the templates