[DO NOT MERGE] test: swap testrunner hack for buildkit passthrough op#1084
Draft
cpuguy83 wants to merge 1 commit into
Draft
[DO NOT MERGE] test: swap testrunner hack for buildkit passthrough op#1084cpuguy83 wants to merge 1 commit into
cpuguy83 wants to merge 1 commit into
Conversation
Swap dalec's testrunner "force-build-but-discard-fs" hack for buildkit's new passthrough op (State.Requires) from moby/buildkit#6829 (fixes #6809, which cites this dalec hack as motivation). This is a feasibility test; back-compat is intentionally out of scope. Behavior changes: - doValidate runs the validation as a writable-rootfs exec and attaches it to the input as a build-only dependency via State.Requires, instead of a readonly exec consumed through a discard AddMount. The validation filesystem is discarded; only the pass/fail dependency edge is kept. - mergeStateOptions forces each check to build as a build-only dependency of the input (output = input) rather than diffing+merging branches. This narrows its contract from "merge branch diffs" to "force branch builds"; sound because every current testrunner option is validation-only (returns the input state). - noopCommand.WithOutput (WithFinalState) returns out.Requires(id, in), making out the output while forcing in to build, replacing the no-op `true` exec plus discard mount. - Unit tests marshal from a base state with a real output (passthrough rejects a nil-output receiver) and drop the now-removed __internal_state mount asserts. This requires a buildkitd that understands the passthroughop capability: - go.mod pins buildkit to tonistiigi/buildkit@passthrough-op. - CI adds a build-buildkit job that builds that buildkitd (from a local clone with tags so version detection yields a usable version) and pushes it to ghcr; the integration and e2e jobs now run every dalec-spec solve against a docker-container builder backed by that image. Trade-offs: - The passthrough vertex carries no progress group / platform / source location (State.Requires takes no ConstraintsOpt); accepted as cosmetic. - Failure is only re-observed on a cache miss, since Requires deps can be served from cache. Existing negative fixtures use fresh inputs, which is sufficient. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
What this is
A feasibility test for the new buildkit passthrough op from moby/buildkit#6829 (which fixes moby/buildkit#6809 — that issue cites dalec's testrunner hack as the motivation). Not for merge: back-compat is intentionally out of scope, and
go.modis pinned to an unmerged buildkit fork.Testrunner change
dalec's testrunner currently fakes "run for validation only, discard fs changes, but force the op to build" with a no-op
trueexec + readonly rootfs + discard mount, plus a diff/merge to strip test diffs. This swaps that forState.Requires(id, deps...):doValidate— runs the validation as a writable-rootfs exec and attaches it to the input as a build-only dependency (in.Requires(...)), instead of a readonly exec consumed via a discardAddMount. The validation filesystem is discarded; only the pass/fail edge is kept.mergeStateOptions— forces each check to build as a build-only dependency of the input (output = input) instead of diff+merge. Contract narrows from "merge branch diffs" to "force branch builds"; sound because every current testrunner option is validation-only.noopCommand.WithOutput/WithFinalState— returnsout.Requires(id, in).__internal_statemount assertions.CI: running against a custom buildkitd
The op bakes the
passthroughopcapability into the LLB, so a stock buildkitd rejects it. Every CI job that solves a dalec spec must use the PR daemon:build-buildkitprep job builds buildkitd fromtonistiigi/buildkit@passthrough-op(from a local clone with tags, so version detection yields a usable version — the harness gates on>= 0.12) and pushes it to ghcr.integrationande2ejobs now create adocker-containerbuilder from that image (BUILDX_BUILDER,--bootstrap, abuildkitd --versionsmoke check, and PR-buildkitd log capture on failure). Forintegration, the prebuild bakes are cache-warming on that same builder; the harness rebuilds the frontend/workers from source in-graph.Trade-offs / notes
State.Requirestakes noConstraintsOpt) — cosmetic.Local validation passes:
go build ./...,go test --test.short ./...,go run ./cmd/lint ./...,go generate(no diff). The real signal is whether the integration + e2e suites stay green against the custom buildkitd in CI.