Skip to content

fix(cargo-anvil): make the container image's root toolchain file optional - #161

Merged
martinhavelka (wukchung) merged 21 commits into
mainfrom
u/mhavelka/anvil-container-toolchain-optional
Sep 9, 2026
Merged

fix(cargo-anvil): make the container image's root toolchain file optional#161
martinhavelka (wukchung) merged 21 commits into
mainfrom
u/mhavelka/anvil-container-toolchain-optional

Conversation

@wukchung

@wukchung martinhavelka (wukchung) commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Authored by an AI agent. Please verify before acting.

Stacked on #155. Base is u/mhavelka/anvil-container-msrv; review the last commit only.

Problem

The container setup region named the toolchain file directly:

COPY justfiles ./justfiles
COPY rust-toolchain.toml ./
COPY Cargo.toml ./

and anvil-container-tag listed the same path among the inputs it requires. A repository that pins its compiler by other means owns no root rust-toolchain / rust-toolchain.toml, so just anvil-container failed at the tag with container image input is missing, and would have failed at the first COPY had it got that far. Nothing else about the feature was reachable. microsoft/oxidizer is exactly such a repository, which is where this was reported.

Change

The region names no input and copies the context whole:

WORKDIR /opt/anvil
COPY . ./

No engine anvil supports offers a portable COPY of 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 conditional COPY was not available. Dockerfile.dockerignore already 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 gap COPY to 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 COPY would leave a repository that pins with the extensionless rust-toolchain building an image whose compiler silently disagreed with its own checkout -- a worse outcome than today's hard failure.

The image now names its default toolchain instead of inheriting one. rustup-init runs with --default-toolchain none, and rustup sets the default as a side effect of the first install that finds none set, so the MSRV became the default only because _anvil-resolve-stable install-msrv happens to precede the nightly installs in the setup graph. A checkout with a toolchain file never notices, because the file overrides the default; one without has nothing else to select a compiler. The region runs rustup default on the declared MSRV, after anvil-setup and before the manifest it is read from is deleted.

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 COPY was 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 the COPY conditional would put cargo metadata in front of a manifest whose members are absent.

That is not what protects it. Assert-WorkspaceMsrvCompatibility is reached through anvil-tool-rustc-validate-prereqs, and no -setup recipe depends on any -validate-prereqs recipe. The image runs just anvil-setup binstall and nothing else:

just --dry-run anvil-setup binstall

reaches _anvil-resolve-stable install-msrv and 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.just is 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 (543 passed), cargo fmt, cargo clippy -p cargo-anvil --all-targets --all-features -D warnings, cargo spellcheck via just spellcheck, just readme, and cargo anvil --dry-run reporting 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-tag through 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, plus a contract test that the region names its default toolchain, sources it from the declared MSRV, and does so before the manifest that value is read from is deleted. 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 no rust-toolchain* file), on Docker 29.7.1:

  1. Reproduced on that branch as released: just anvil-container-tag fails with anvil: container image input is missing: rust-toolchain.toml, before any COPY is reached.
  2. Regenerated with this build; the emitted region is COPY . ./ and the tag now resolves.
  3. Cold image build from that context succeeds. With no toolchain file the compiler comes from the declared MSRV, as it would on a host: anvil: installing stable toolchain '1.95', and rustup toolchain list inside the image reports 1.95 (active, default) beside the two pinned nightlies. just anvil-container <command> then runs in it.
  4. Also checked in isolation: a replica of the image context -- root manifest, recipe tree, no members, no toolchain file -- resolves the MSRV, and just --dry-run anvil-setup binstall there reaches _anvil-resolve-stable install-msrv and no other resolver action, which is the claim above holding in oxidizer's own generated tree rather than only in this one.

The default-toolchain change was executed rather than argued. In a replica of the file-absent case (this repository's context with the toolchain file removed, on the image's own tools layer), a nightly was installed before the MSRV so that the implicit default would be the wrong one. Rustup did set it: info: default toolchain set to nightly-2025-11-27. The new line then read [1.95] from _anvil-resolve-stable root-msrv under sh capture and ran rustup default 1.95, which reported using existing install and unchanged -- no download, so the name matched the installed toolchain. From /workspace with no toolchain file the image reports 1.95 (active, default), rustc 1.95.0 and cargo 1.95.0.

No regression for a repository that does own one: a cold build of this repository produces 1.97 (active) from rust-toolchain.toml beside the 1.95 MSRV, matching what #155 recorded before the COPY changed.

Podman is unverified; only Docker was available here. COPY . ./ has no engine-specific semantics, which is why it was chosen over a conditional COPY, 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

`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
…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-commenter

Codecov Comments Bot (codecov-commenter) commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.5%. Comparing base (3bf8d54) to head (aa7b055).
⚠️ Report is 1 commits behind head on main.

❌ 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           @@
##            main    #161     +/-   ##
=======================================
- Coverage   97.5%   97.5%   -0.1%     
=======================================
  Files        300     300             
  Lines      68538   68538             
=======================================
- Hits       66876   66875      -1     
- Misses      1662    1663      +1     
Flag Coverage Δ
linux 97.5% <ø> (ø)
linux-arm 97.5% <ø> (+<0.1%) ⬆️
scheduled ?
windows 97.7% <ø> (-0.1%) ⬇️

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.

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
Base automatically changed from u/mhavelka/anvil-container-msrv to main September 4, 2026 15:26
@wukchung
martinhavelka (wukchung) marked this pull request as ready for review September 7, 2026 10:53
Copilot AI lite review requested due to automatic review settings September 7, 2026 10:53

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.

🟡 Changes recommended

There are a few concrete contract/correctness gaps (notably Cargo.toml missing from the tag’s symlink guard and rustdoc/README identity drift) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes cargo-anvil’s container-image workflow so repositories without a root rust-toolchain / rust-toolchain.toml can still build an anvil exec image, by switching the setup layer to copy the (scoped) build context wholesale and by making the image tag computation discover (rather than require) toolchain files while also incorporating the declared root MSRV into the tag.

Changes:

  • Update the container setup region to COPY . ./ and rely on Dockerfile.dockerignore to precisely scope what enters the image, including admitting both toolchain-file spellings and the root Cargo.toml.
  • Extend anvil-container-tag to (a) discover optional root toolchain files, and (b) hash the declared root MSRV value as its own record.
  • Add/adjust contract + behavior tests and update docs/snapshots to reflect the new container build/tag contracts.
File summaries
File Description
justfiles/anvil/tools.just Adds _anvil-resolve-stable root-msrv action for tag computation.
justfiles/anvil/container.just Updates tag computation to discover toolchain files and include the declared root MSRV in the digest stream.
crates/cargo-anvil/tests/snapshots/snapshots__local_only.snap Updates snapshots for new setup region and dockerignore behavior.
crates/cargo-anvil/tests/snapshots/snapshots__github_backend.snap Updates snapshots for new setup region and dockerignore behavior.
crates/cargo-anvil/tests/snapshots/snapshots__ado_backend.snap Updates snapshots for new setup region and dockerignore behavior.
crates/cargo-anvil/tests/recipe_contracts.rs Adds contract/behavior tests for container context + tag changes and MSRV hashing.
crates/cargo-anvil/templates/justfiles/anvil/tools.just Propagates root-msrv action into generated template source.
crates/cargo-anvil/templates/justfiles/anvil/container.just Propagates container-tag changes into generated template source.
crates/cargo-anvil/templates/anvil/container/Dockerfile.setup.region Switches setup layer to COPY . ./ and deletes Cargo.toml after setup.
crates/cargo-anvil/templates/anvil/container/Dockerfile.dockerignore Admits Cargo.toml and both toolchain-file spellings; documents that it now defines image contents.
crates/cargo-anvil/src/lib.rs Updates public rustdoc for container prerequisites/identity (but currently misses two new identity inputs).
crates/cargo-anvil/src/anvil/artifacts/container.rs Adjusts tests to assert the composed Dockerfile now uses COPY . ./ and toolchain file is not named.
crates/cargo-anvil/README.md Updates user-facing docs for container prerequisites/identity (but currently misses two new identity inputs).
crates/cargo-anvil/docs/implementation.md Documents the new root-msrv resolver action and its purpose.
crates/cargo-anvil/docs/design/containers.md Updates the container design/contract to match the new context + tag scheme.
.anvil/container/Dockerfile.dockerignore Updates in-repo dockerignore used by the composed Dockerfile build.
.anvil/container/Dockerfile Updates the checked-in composed Dockerfile setup region to copy scoped context.
.anvil.lock Updates catalog checksums for the changed managed artifacts.
Review details
  • Files reviewed: 17/18 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/cargo-anvil/templates/justfiles/anvil/container.just Outdated
Comment thread crates/cargo-anvil/README.md Outdated
Comment thread crates/cargo-anvil/src/lib.rs Outdated
…ainer-toolchain-optional

# Conflicts:
#	.anvil.lock
#	.anvil/container/Dockerfile
#	.anvil/container/Dockerfile.dockerignore
#	crates/cargo-anvil/README.md
#	crates/cargo-anvil/docs/design/containers.md
#	crates/cargo-anvil/src/lib.rs
#	crates/cargo-anvil/templates/anvil/container/Dockerfile.dockerignore
#	crates/cargo-anvil/templates/anvil/container/Dockerfile.setup.region
#	crates/cargo-anvil/templates/justfiles/anvil/container.just
#	crates/cargo-anvil/tests/recipe_contracts.rs
#	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
#	justfiles/anvil/container.just
Copilot AI review requested due to automatic review settings September 7, 2026 11:02

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

It changes core container image construction and identity hashing behavior across engines, which merits final human verification despite the added/updated tests and design updates.

Review details
  • Files reviewed: 12/13 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

… MSRV validation

The image builds against a context carrying the root manifest and none of the members it names, and runs `just anvil-setup binstall` there. Workspace MSRV validation resolves every member through `cargo metadata`, so it has to stay outside that graph. It does: it hangs off `anvil-tool-rustc-validate-prereqs`, and no `-setup` recipe depends on a `-validate-prereqs` recipe.

Nothing declared that, and it is the property the conditional COPY rests on. A dependency edge added later would surface as a cargo path error inside an image build, naming a manifest rather than the edge that reached it.

The whole emitted tree is planned rather than a fixture subset, because the edge could be added in any tier, group or check file, and the plan is matched on the invocation spelling rather than the bare action name, which the resolver's own body lists. Verified to fail: adding `anvil-tool-rustc-validate-prereqs` to `anvil-full-setup` trips it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 7, 2026 11:13

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.

🟢 Approval recommended

The changes consistently update the Dockerfile, tag logic, docs, and tests to support repositories without a root toolchain file without weakening build-context scoping or image identity.

Review details
  • Files reviewed: 12/13 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@wukchung martinhavelka (wukchung) changed the title fix(cargo-anvil): let a repository with no root toolchain file build the container image fix(cargo-anvil): make the container image's root toolchain file optional Sep 7, 2026
Six places said something the reader already had. The two new contract tests carried doc comments repeating their assertion messages, and one repeated the portability argument the design document and the setup region already make. The tag recipe opened a comment by naming what the next line does. Two design-document paragraphs restated the sentence above them, and one described the choice approvingly rather than stating it.

Keep the reasons a reader cannot recover from the code: why a repository may own no toolchain file, why both spellings are checked, why the plan is matched on the invocation rather than the action name. Drop the rest. No behaviour changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 7, 2026 12:27

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

A few concrete correctness/robustness issues remain (notably missing link-guard coverage for Cargo.toml and brittle line-ending-sensitive assertions in tests) that should be addressed before approval.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

crates/cargo-anvil/src/anvil/artifacts/container.rs:431

  • This assertion is sensitive to line endings (\n vs \r\n) and also ties correctness to the exact newline placement in the file. To avoid false negatives on CRLF checkouts while still avoiding the rust-toolchain.toml substring match, assert on whole lines instead.
    crates/cargo-anvil/tests/recipe_contracts.rs:588
  • The contains("!rust-toolchain\n") check is brittle with CRLF checkouts and depends on the exact newline sequence. Since the goal is to distinguish !rust-toolchain from !rust-toolchain.toml, prefer checking for exact lines.

crates/cargo-anvil/templates/justfiles/anvil/container.just:204

  • The symlink/reparse-point guard doesn’t include Cargo.toml, even though the container build context explicitly admits it (via Dockerfile.dockerignore) and the tag computation reads it indirectly via root-msrv. If Cargo.toml is a symlink, root-msrv will follow it on the host, but COPY . ./ will copy the link into the image, which can make the image build fail or select a different MSRV than the tag was derived from. Add Cargo.toml to the declared paths checked for ReparsePoint so this fails early with the existing helpful error.
    foreach ($declared in @('rust-toolchain.toml', 'rust-toolchain', '.anvil', '.anvil/container', 'justfiles', 'justfiles/anvil')) {
  • Files reviewed: 12/13 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

…d from

The crate documentation listed the Dockerfile, the toolchain and the recipe tree. The tag also covers the Dockerfile's ignore file and the declared root MSRV, and after this branch the toolchain file is an input only where a repository owns one, so the summary named neither the full set nor the optional member.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 7, 2026 13:19
…ainer-toolchain-optional

# Conflicts:
#	.anvil.lock

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

Two updated tests assert !rust-toolchain via contains("...\\n"), which is brittle under CRLF checkouts and should be made line-ending agnostic.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/cargo-anvil/src/anvil/artifacts/container.rs:430

  • This check looks for the exact substring "!rust-toolchain\n", which can fail on CRLF checkouts (\r\n). Using a line-based match (e.g., DOCKERIGNORE.lines().any(|l| l == "!rust-toolchain")) avoids coupling the test to a specific newline style while still distinguishing it from !rust-toolchain.toml.
    crates/cargo-anvil/tests/recipe_contracts.rs:586
  • This assertion relies on a literal "\n" to distinguish !rust-toolchain from !rust-toolchain.toml. That makes the test sensitive to CRLF checkouts (e.g., \r\n), where the extensionless entry may be present but the contains("!rust-toolchain\n") needle won’t match. Consider checking for an exact line match via .lines() instead, which is line-ending agnostic.
  • Files reviewed: 14/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 7, 2026 13:24

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.

🟢 Approval recommended

The behavior change is consistently applied across templates, emitted artifacts, docs, and tests, with only a minor doc-comment accuracy nit noted in the review.

Review details
  • Files reviewed: 14/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/cargo-anvil/tests/recipe_contracts.rs Outdated
…e image it protects

The doc comment opened on the image's build context, which reads as a description of the fixture. The fixture is an ordinary workspace, and has to be: the test plans the emitted recipe graph, and the container's filesystem is what that graph protects rather than what it reproduces.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ainer-toolchain-optional

# Conflicts:
#	.anvil.lock
Copilot AI review requested due to automatic review settings September 8, 2026 09:04

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.

🟢 Approval recommended

The change consistently updates the tag logic, build context scoping, templates, tests, and documentation to support repositories without a root toolchain file, with no inconsistencies found in the reviewed diffs.

Review details
  • Files reviewed: 14/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread crates/cargo-anvil/templates/justfiles/anvil/container.just Outdated
Comment thread crates/cargo-anvil/templates/anvil/container/Dockerfile.setup.region Outdated
Comment thread crates/cargo-anvil/docs/design/containers.md

@martin-kolinek martin-kolinek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖: Approved.

…ainer-toolchain-optional

# Conflicts:
#	.anvil.lock
#	crates/cargo-anvil/README.md
`COPY . ./` made the ignore file the definition of the image's contents, and `!rust-toolchain` re-includes a directory's whole subtree by the same parent re-inclusion that makes `!justfiles/anvil` admit its recipes. Discovery skipped a non-leaf, and the digest walks only `.anvil/container/` and `justfiles/anvil/`, so a repository with a root `rust-toolchain/` directory shipped its contents into the image and hashed none of them: two images differing anywhere under it computed one tag, with no refusal. The link guard could not catch it, because a plain directory is not a reparse point. Both states are new -- before this branch `*` excluded that directory, and a directory at `rust-toolchain.toml` failed the required-input check.

Two claims are corrected alongside it. The setup region and section 3 said `anvil-setup` depends on no `-validate-prereqs` recipe; it reaches `anvil-tool-pwsh-validate-prereqs` through `anvil-pr-title-setup`. The conclusion holds and the contract test already pins it, but the sentence ships into every adopter's Dockerfile as the justification for dropping a previously load-bearing COPY, so it now names `anvil-tool-rustc-validate-prereqs` -- the sole caller of the validation -- instead of the family. Section 9's claim that the digest covers every file the context admits is true only of the ignore file anvil ships, and section 5 sanctions widening it; both now say so, and the region table row still described the named-input copy model.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 9, 2026 06:45

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.

🟢 Approval recommended

The changes consistently implement the “optional root toolchain file” contract across tagging, build context scoping, tests, snapshots, and documentation without introducing inconsistencies in the reviewed diffs.

Review details
  • Files reviewed: 14/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

The image initializes rustup with `--default-toolchain none`, so nothing but rustup's own side effect -- it sets the default from the first install that finds none set -- gave the image a default. That is the MSRV only because of the order `anvil-setup` reaches the install recipes in. A checkout with a root toolchain file never notices, because the file overrides the default; a checkout with none has nothing else to select a compiler, so plain `cargo` in the container would follow whichever toolchain the setup graph installed first.

The setup region now runs `rustup default` on the declared MSRV, read before the manifest it comes from is deleted. A repository declaring no MSRV is left alone, since the setup installs no stable toolchain for it either.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 9, 2026 08:03

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.

🟢 Approval recommended

The diffs consistently implement “optional root toolchain file” across the Dockerfile, digest/tagging logic, tests, snapshots, and docs without introducing any verified correctness gaps in the reviewed changes.

Review details
  • Files reviewed: 14/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@wukchung
martinhavelka (wukchung) merged commit 7dd2441 into main Sep 9, 2026
29 checks passed
@wukchung
martinhavelka (wukchung) deleted the u/mhavelka/anvil-container-toolchain-optional branch September 9, 2026 09:30
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.

5 participants