fix(cargo-anvil): let a repository with no root toolchain file build the container image - #161
Draft
martinhavelka (wukchung) wants to merge 10 commits into
Draft
Conversation
`anvil-msrv-test-setup` resolves the MSRV by scanning the root `Cargo.toml`, and `just anvil-setup` reaches it through the PR tier. The container image is built with no repository source in its context by design, so that scan threw and the install layer failed. No image was produced, which left every `anvil-container` recipe unusable after a cold build. The MSRV is the one version anvil installs that is declared in the source rather than pinned in `versions.just`, and the image already carries the repository's other toolchain declaration, `rust-toolchain.toml`. Close that asymmetry by making the MSRV an image input: `anvil-container-tag` resolves the declared value through a new `_anvil-resolve-stable root-msrv` action and hashes it, the build passes it as `ARG ANVIL_ROOT_MSRV`, and the resolver reads it only when no root manifest is present, so it can never shadow a real declaration. The value is carried rather than the manifest holding it. Copying `Cargo.toml` into the context would rename the image on every dependency edit, obliging a publisher to rebuild and republish for changes that cannot alter a byte the image contains, while `rust-version` moves perhaps once. A repository that declares no MSRV sends `none`, which is an answer; an unset variable is not, so a build that drops the argument stops rather than producing an image silently missing a toolchain it claims to install. Validated with a cold image build on Docker 29.7.1: the build completes and `rustup toolchain list` in the resulting image reports the declared MSRV alongside the pinned stable and nightlies. Contract tests cover manifest precedence over the override, the `none` declaration, the refusal when nothing is declared, and that the tag follows the MSRV while ignoring unrelated manifest edits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80f12d44-5a83-4650-9806-d3e247175858
…ainer-msrv # Conflicts: # .anvil.lock
…ainer-msrv # Conflicts: # .anvil.lock
…old it The setup region installs the toolchain named by the repository's declared MSRV, which lives in the root `Cargo.toml` rather than in `versions.just`. That value reached the build as `ARG ANVIL_ROOT_MSRV`, resolved on the host and passed as a build argument, with the resolver reading the variable whenever it found no manifest. That channel is fragile in a way the composed Dockerfile makes likely. The setup region is a documented replacement point, and the `ARG` sat inside it, so a repository substituting its own region kept receiving the build argument while silently losing the declaration that receives it. The resulting failure is `failed to install MSRV toolchain '1.92'`, which sends the reader hunting a toolchain that is not the problem. Admit the root manifest to the build context and copy it to `/opt/anvil` instead. That directory is already the root the recipes resolve against: it is `justfile_directory()`, and it holds `justfiles/` and the toolchain pin. Copying the manifest alongside them completes it for the one question the setup asks, and the resolver reads it there exactly as it does on a developer's machine. `tools.just` therefore returns to what it was, minus the new `root-msrv` action the tag still needs. The variable, its `none` sentinel, its error message and the `--build-arg` all go away, and with them a general resolver's knowledge of containers. Dropping the `COPY` now fails with `Cargo.toml not found at repository root`, which names the missing thing. The workspace members the manifest lists stay out of the context: they are a checkout, and the image is not one. The one path that would need them, workspace MSRV validation, returns early whenever a root toolchain file selects the compiler, which this image requires and copies. The tag continues to hash the resolved value rather than the file. The manifest is the busiest file in a workspace while `rust-version` moves perhaps once in a repository's life, so hashing it would rename the image, and oblige a publisher to rebuild and republish, for a stream of edits that cannot alter a byte the image contains. Contracts follow the mechanism: the context admits the manifest, the setup region copies it, and no build argument survives anywhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2963084a-ef08-4cf7-adc6-e78e61556a84
The prose ran about four lines per line of code, well past what the change needs. Keep the reasons a reader cannot recover from the code -- why the value is hashed rather than the manifest, why the members stay out of the context, why the declared value rather than the mapped one -- and drop the restatement around them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2963084a-ef08-4cf7-adc6-e78e61556a84
…n rests on Workspace MSRV validation reads every member manifest, and the build context carries none. It stays out of reach only because a root toolchain file selects the compiler and makes the resolver return early -- which holds today by accident, since the unconditional COPY means a repository without one cannot build an image at all. Making that COPY conditional is a reasonable fix for those repositories, and it would silently put the branch back in reach of a partial workspace. State the dependency so that change has to confront this one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2963084a-ef08-4cf7-adc6-e78e61556a84
…the image The setup region named `rust-toolchain.toml` in a COPY and the tag recipe listed it as a required input, so a repository that pins its compiler by other means could not build an image at all: the build failed at the first COPY, and `anvil-container-tag` refused before that. `microsoft/oxidizer` is such a repository. No engine anvil supports offers a portable COPY of a path that may not exist, so the region names no input at all and copies the context whole. The ignore file already scopes that context to precisely the image's inputs, so what it admits and what the image contains become the same set. It now admits both toolchain-file spellings: naming only the TOML would leave a repository that pins with the extensionless file building an image whose compiler silently disagreed with its own checkout. The tag discovers the file rather than requiring it, in both spellings. Absence is one fewer record in the digest, so the two states cannot share a reference, and the file's mode still comes from the index. The ignore file stays a named, required input for the reason it always was. The design note this replaces claimed the toolchain file kept workspace MSRV validation out of reach of a memberless context. It does not: that validation hangs off the `-validate-prereqs` recipes, and `anvil-setup` -- the only thing the image runs -- depends on none of them. `just --dry-run anvil-setup binstall` reaches `_anvil-resolve-stable install-msrv` and nothing else, so no resolver change is needed and the doc and contract test now state the real reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (97.5%) 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 @@
## u/mhavelka/anvil-container-msrv #161 +/- ##
=================================================================
- Coverage 97.5% 97.5% -0.1%
=================================================================
Files 299 299
Lines 67766 67766
=================================================================
- Hits 66129 66122 -7
- Misses 1637 1644 +7
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:
|
The tag hashes the declared MSRV rather than the manifest, so an edit that leaves rust-version alone computes the same tag. With the manifest left in place that tag named two different filesystems, and a reused or published image carried a Cargo.toml matching no checkout. Deleting it in the same RUN that reads it makes the identity exact again, and restores the design note's claim that those edits cannot alter a byte the image contains. Three statements in containers.md that still described the old three-path context are corrected alongside it, including the replacement rule a downstream catalog reads verbatim. Also drops an env_remove for ANVIL_ROOT_MSRV: nothing on this branch reads that variable, so the call could not affect what a fixture resolves and its comment described a mechanism that no longer exists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2963084a-ef08-4cf7-adc6-e78e61556a84
…able The comments added with the previous commit ran well past the change. Several restated the assertion message directly below them; others explained what the code already says. Keep the reasons a reader cannot recover: why the context is copied whole, why both toolchain-file spellings are admitted and neither required, and why the tag discovers the file rather than requiring it. Drop the rest. No behaviour changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… into u/mhavelka/anvil-container-toolchain-optional # Conflicts: # .anvil.lock # .anvil/container/Dockerfile # crates/cargo-anvil/docs/design/containers.md # crates/cargo-anvil/templates/anvil/container/Dockerfile.setup.region # crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap # crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap # crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Authored by an AI agent. Please verify before acting.
Problem
The container setup region named the toolchain file directly:
and
anvil-container-taglisted the same path among the inputs it requires. A repository that pins its compiler by other means owns no rootrust-toolchain/rust-toolchain.toml, sojust anvil-containerfailed at the tag withcontainer image input is missing, and would have failed at the firstCOPYhad it got that far. Nothing else about the feature was reachable.microsoft/oxidizeris exactly such a repository, which is where this was reported.Change
The region names no input and copies the context whole:
No engine anvil supports offers a portable
COPYof a path that may not exist -- a glob that matches nothing is an error on the classic builder and on buildah, and BuildKit's opt-out is a frontend flag -- so a conditionalCOPYwas not available.Dockerfile.dockerignorealready scopes the context to precisely the image's inputs, so deferring to it costs no breadth and makes what the context admits and what the image contains the same set by construction..anvil/container/rides along; it is committed source, it has to be in the context for a gapCOPYto work at all, and the image never runs it.The ignore file now admits both toolchain-file spellings. Admitting only the TOML while naming neither in a
COPYwould leave a repository that pins with the extensionlessrust-toolchainbuilding an image whose compiler silently disagreed with its own checkout -- a worse outcome than today's hard failure.The tag discovers the toolchain file rather than requiring it, in both spellings. Absence is one fewer record in the length-prefixed stream, so a repository with a toolchain file and one without cannot share a tag, and either spelling is its own input. The file's git mode still comes from the index. The ignore file stays a named, required input for the reason it always was: the walk cannot notice its absence, and without it the context widens to files the digest never hashed.
The coupled constraint in the bug does not hold
The bug (and the last commit of #155) said the unconditional
COPYwas load-bearing: that workspace MSRV validation stays out of reach of a memberless context only because a root toolchain file makes the resolver return early, so making theCOPYconditional would putcargo metadatain front of a manifest whose members are absent.That is not what protects it.
Assert-WorkspaceMsrvCompatibilityis reached throughanvil-tool-rustc-validate-prereqs, and no-setuprecipe depends on any-validate-prereqsrecipe. The image runsjust anvil-setup binstalland nothing else:reaches
_anvil-resolve-stable install-msrvand no other resolver action. Inside a running container the validation does execute, but against/workspace-- a real checkout, with its members. So no resolver change is needed;tools.justis untouched. The design note and the contract test that asserted the old reason now state the real one.Validation
cargo test -p cargo-anvil --all-features(523 passed),cargo fmt,cargo clippy -p cargo-anvil --all-targets --all-features -D warnings,cargo spellcheckviajust spellcheck,just readme, andcargo anvil --dry-runreporting the in-tree state current.New coverage: a contract test that the setup region names no toolchain file and that the context admits both spellings, and a behavioural test driving
anvil-container-tagthrough four states -- no toolchain file,rust-toolchain.toml, an edit to it, and the extensionless spelling with identical bytes -- asserting each produces a distinct tag and none refuses. The three emitted-tree snapshots were re-recorded and reviewed; they carry only the intended changes.Validated end to end against the repository the report came from,
microsoft/oxidizer(PR 728, which adopts 0.7.0 and owns norust-toolchain*file), on Docker 29.7.1:just anvil-container-tagfails withanvil: container image input is missing: rust-toolchain.toml, before anyCOPYis reached.COPY . ./and the tag now resolves.anvil: installing stable toolchain '1.95', andrustup toolchain listinside the image reports1.95 (active, default)beside the two pinned nightlies.just anvil-container <command>then runs in it.just --dry-run anvil-setup binstallthere reaches_anvil-resolve-stable install-msrvand no other resolver action, which is the claim above holding in oxidizer's own generated tree rather than only in this one.No regression for a repository that does own one: a cold build of this repository produces
1.97 (active)fromrust-toolchain.tomlbeside the1.95MSRV, matching what #155 recorded before theCOPYchanged.Podman is unverified; only Docker was available here.
COPY . ./has no engine-specific semantics, which is why it was chosen over a conditionalCOPY, but that is an argument rather than an observation.No version bump or CHANGELOG entry: this repo generates the CHANGELOG at release time from conventional-commit messages.
WI: https://o365exchange.visualstudio.com/O365%20Core/_workitems/edit/7840047