(view) Claude fixes on top of buildkit#19
Conversation
The earthly fork added GitLogLevel and existingSSHCommand params to getGitSSHCommand. Upstream's test only passes knownHosts. Signed-off-by: Giles Cope <gilescope@gmail.com>
Earthly's OCI hooks feature was still importing containerd v1 types (containers, oci) while the rest of the codebase migrated to v2. Signed-off-by: Giles Cope <gilescope@gmail.com>
Earthly's vendored fsutil fork lacks vtprotobuf's CloneVT method. A simple value copy achieves the same result for this test. Signed-off-by: Giles Cope <gilescope@gmail.com>
The vendored fsutil's wire.proto uses bare import "stat.proto" which needs the fsutil types directory on protoc's include path. Signed-off-by: Giles Cope <gilescope@gmail.com>
After the upstream merge added CDI support, the earthly-specific parameters (hooks, sampleFrequency, cdiManager) were passed in the wrong order at the call site. Signed-off-by: Giles Cope <gilescope@gmail.com>
- Fix Env field incorrectly set to hook.Args instead of hook.Env - Suppress staticcheck SA1019 for intentional legacy prestart support Signed-off-by: Giles Cope <gilescope@gmail.com>
Resolve 47 lint issues introduced by upstream's stricter lint config: - errorlint: use errors.Is/errors.As instead of == and type assertions - forbidigo: use context.WithCancelCause/WithTimeoutCause/Cause - gocritic: fix comment formatting (space after //) - gofmt: reformat 4 files - staticcheck: fix deprecated imports, simplify selectors, nolint pkg names - noctx: use DialContext/Listen with context - unused: remove dead code Signed-off-by: Giles Cope <gilescope@gmail.com>
- Replace interface{} with any
- Use strings.CutPrefix instead of HasPrefix+TrimPrefix
- Use maps.Copy instead of manual k/v loops
- Modernize for loops to range over int
- Remove unnecessary variable copy
- Fix comment formatting (space after //)
Signed-off-by: Giles Cope <gilescope@gmail.com>
Sort maps import alphabetically among stdlib imports. Signed-off-by: Giles Cope <gilescope@gmail.com>
Prefix unused function parameters with _ to satisfy the gopls unusedparams analyzer. Signed-off-by: Giles Cope <gilescope@gmail.com>
Earthly's base64 encoding of git stderr prevented upstream code in source/git/source.go from detecting "would clobber existing tag" and "refname conflict" errors via strings.Contains. Include raw redacted stderr alongside the base64 so pattern matching works. Also update TestGitCLIConfigEnv to match earthly's intentional behavior of not isolating git config (HOME is always passed through). Signed-off-by: Giles Cope <gilescope@gmail.com>
go mod tidy removed unused direct deps (containerd v1, docker/docker, gogo/googleapis, go-fuzz-headers) left over from the upstream merge. go mod vendor then cleaned up 149 files of unused vendored code. Signed-off-by: Giles Cope <gilescope@gmail.com>
- Remove gogoproto.stdtime annotations from control.proto (earthly- specific SessionHistory fields) since gogo protoc is no longer used - Fix vendored fsutil wire.proto bare import to use fully-qualified path - Exclude earthly-specific protos (localhost, pullping, socketforward, registry, gateway) from protoc generation as they use old gogo-style codegen incompatible with modern protoc-gen-go - Add M-flag overrides for fsutil types proto go_package paths - Remove hand-written control_earthly.pb.go (now generated by protoc) - Update control.go to use timestamppb.New() for History timestamps Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
- Fix copylocks warning in client_test.go: types.Stat now contains a mutex via protoimpl.MessageState, use pointers instead of copies - Re-vendor after go mod tidy to sync vendor with updated fsutil Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Earthly's multi-BUILD pattern legitimately produces multiple refs without platform mapping. The upstream verifier added in buildkit treats this as a hard error; downgrade to a warning so earthly builds continue to work. Signed-off-by: Giles Cope <gilescope@gmail.com>
Upstream commit 7aaa797 caches gateway mounts for performance, but this causes stale reads when a persistent cache mount is modified by a RUN and then read by a subsequent SAVE ARTIFACT within the same target. Disable the cache lookup so each operation gets a fresh mount. Signed-off-by: Giles Cope <gilescope@gmail.com>
The buildkitd/buildctl ldflags set version.Package to the legacy github.com/earthly/buildkit. EarthBuild's earthly client compares the buildkit daemon's reported package against github.com/EarthBuild/buildkit and warns "Using a non-EarthBuild version of Buildkit is not supported" on every invocation. Report the current org so our own buildkit is recognised as supported. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The manual runtime.LockOSThread() in run() was added to stop runc processes intermittently exiting with status -1 (the classic PdeathSignal-vs-Go-scheduler race, golang/go#27505). go-runc v1.1.0 (which the upstream merge pulled in) already handles this: because we set Runc.PdeathSignal (updateRuncFieldsForHostOS), Run() -> startCommand() takes the Monitor.StartLocked() path, which calls runtime.LockOSThread() on the goroutine that actually does cmd.Start() (the fork). Our manual lock was on the *caller* goroutine -- not the forking one -- so it no longer does anything useful here. Removing it restores executor_linux.go to the exact upstream version, shrinking the fork delta with no behaviour change.
docopy() dropped action.ModeStr (string chmod, e.g. COPY --chmod=u+x) and hard-disabled action.AlwaysReplaceExistingDestPaths, with a comment claiming "not supported in this fsutil fork". That comment is stale: the vendored fork (EarthBuild/fsutil, pulled in by 5638371) supports both -- copy.CopyInfo.ModeStr (copy.go:197) and AlwaysReplaceExistingDestPaths (copy.go:208) are honoured. The removals dated from an older fsutil and silently degraded string-mode COPY to the integer-mode fallback. Restores the upstream code path; matches base.
Three changes that diverged from upstream with zero functional effect, restored to reduce future merge conflicts: - diffapply_linux.go: uint64(stat.Dev) -> stat.Dev (Dev is uint64 on all linux arches, the cast was a no-op) and Rdev: 0 -> unix.Mkdev(0, 0) (Mkdev(0,0) == 0). - scheduler_test.go: context.TODO() -> t.Context() in two loop bodies. t.Context() (Go 1.24+) is available under go 1.25.5 and is what upstream uses; the downgrade was an older-toolchain artifact.
Reverts util/archutil to upstream: removes the /proc mount in the arch check and the check_nolinux.go stub, and restores check_unix.go's !windows build constraint (it had been narrowed to linux). The patch worked around Rosetta failing to open /proc/self/exe inside the check's chroot, but it is not needed here and carried two upstream regressions: the !windows->linux narrowing dropped the arch check on FreeBSD/Darwin, and the mount error was fatal, breaking the check on rootless/unprivileged daemons. Restoring upstream shrinks the fork delta and removes both regressions.
Removes the earthly diagnostic scaffolding added to chase the "inconsistent graph state" / "return leaving incoming/outgoing open" scheduler errors: the dgstTracker ring buffer (inconsistent_graph_state_error_tracker.go), the helpMe context strings and bklog error logging in scheduler.go dispatch/NewInputRequest, and the dgstTracker.add call sites in jobs.go. The bug is no longer being actively investigated, so this telemetry is no longer needed. All three files now match upstream exactly, removing ~120 lines of delta from the scheduler/jobs hot path -- the most merge-conflict-prone files in the fork.
maps.Clone(opt.FrontendAttrs) returns nil when opt.FrontendAttrs is nil, and the following maps.Copy(frontendAttrs, cacheOpt.frontendAttrs) then panics with "assignment to entry in nil map" whenever cache attrs are present. This is hit on the Client.Build path (earthly's multiImageSolver passes nil FrontendAttrs) with remote/inline cache configured. Regression from upstream "simplify with maps pkg functions" (dfc3527), which replaced a nil-safe explicit map init with maps.Clone/Copy. Present in moby/buildkit master too -- candidate for upstreaming. Re-add the nil guard so frontendAttrs is always writable.
|
| Branch | Total Count |
|---|---|
| main | 217 |
| This PR | 245 |
| Difference | +28 (12.9%) |
📁 Changes by file type:
| File Type | Change |
|---|---|
| Go files (.go) | ❌ +25 |
| Documentation (.md) | ➖ No change |
| Earthfiles | ➖ No change |
Keep up the great work migrating from Earthly to Earthbuild! 🚀
💡 Tips for finding more occurrences
Run locally to see detailed breakdown:
./.github/scripts/count-earthly.shNote that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.
There was a problem hiding this comment.
Code Review
This pull request transitions the codebase from gogo protobuf to the standard Go protobuf API, regenerating protobuf files and removing custom earthly-specific marshaling implementations. It also updates dependencies, disables gRPC ALPN enforcement in buildkitd for compatibility, and skips the mount cache to ensure persistent cache mounts reflect the latest writes. The review feedback highlights a critical resource leak in frontend/gateway/gateway.go where old mounters are overwritten without being unmounted, as well as multiple compilation errors across several files due to name collisions between the standard library errors package and github.com/pkg/errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Earthly: skip mount cache to ensure persistent cache mounts | ||
| // reflect latest writes between consecutive SAVE ARTIFACT calls. | ||
| // Upstream commit 7aaa7974d introduced this cache for performance, | ||
| // but it causes stale reads when the same cache mount is modified | ||
| // between operations within a single target. | ||
| var mountable snapshot.Mountable | ||
| if ref != nil { | ||
| var err error |
There was a problem hiding this comment.
Skipping the mount cache check to ensure persistent cache mounts reflect the latest writes is intentional, but always creating a new mounter and overwriting lbf.mounts[id] without unmounting the previous one will leak the old mount. To prevent resource and mount leaks, the previous mounter must be unmounted before being overwritten.
if oldMounter, ok := lbf.mounts[id]; ok {
if err := oldMounter.Unmount(); err != nil {
return nil, err
}
}
// Earthly: skip mount cache to ensure persistent cache mounts
// reflect latest writes between consecutive SAVE ARTIFACT calls.
// Upstream commit 7aaa7974d introduced this cache for performance,
// but it causes stale reads when the same cache mount is modified
// between operations within a single target.
var mountable snapshot.Mountable
if ref != nil {
var err error| import ( | ||
| "context" | ||
| "io" | ||
|
|
||
| "github.com/pkg/errors" | ||
| context "golang.org/x/net/context" | ||
| "golang.org/x/sync/errgroup" | ||
| ) |
There was a problem hiding this comment.
To avoid a compilation error due to a name collision between the standard library errors package (needed for errors.Is) and github.com/pkg/errors (needed for errors.WithStack), import the standard library errors package as stdErrors and alias github.com/pkg/errors as pkgerrors.
| import ( | |
| "context" | |
| "io" | |
| "github.com/pkg/errors" | |
| context "golang.org/x/net/context" | |
| "golang.org/x/sync/errgroup" | |
| ) | |
| import ( | |
| "context" | |
| stdErrors "errors" | |
| "io" | |
| pkgerrors "github.com/pkg/errors" | |
| "golang.org/x/sync/errgroup" | |
| ) |
| if errors.Is(err, io.EOF) { | ||
| return nil | ||
| } | ||
| return errors.WithStack(err) |
| n, err := conn.Read(buf) | ||
| switch { | ||
| case err == io.EOF: | ||
| case errors.Is(err, io.EOF): |
| if errors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| return errors.WithStack(err) |
There was a problem hiding this comment.
To avoid a compilation error due to a name collision between the standard library errors package (needed for errors.Is) and github.com/pkg/errors (needed for errors.WithStack), you should alias github.com/pkg/errors as pkgerrors in the imports of this file and use pkgerrors.WithStack here.
| if errors.Is(err, io.EOF) { | |
| break | |
| } | |
| return errors.WithStack(err) | |
| if errors.Is(err, io.EOF) { | |
| break | |
| } | |
| return pkgerrors.WithStack(err) |
| if errors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| return errors.Wrapf(err, "failed to read from %s", src) |
There was a problem hiding this comment.
| } else if errors.Is(err, io.EOF) { | ||
| break outer | ||
| default: | ||
| } else { | ||
| return errors.WithStack(err) |
There was a problem hiding this comment.
To avoid a compilation error due to a name collision between the standard library errors package (needed for errors.Is) and github.com/pkg/errors (needed for errors.WithStack), you should alias github.com/pkg/errors as pkgerrors in the imports of this file and use pkgerrors.WithStack here.
| } else if errors.Is(err, io.EOF) { | |
| break outer | |
| default: | |
| } else { | |
| return errors.WithStack(err) | |
| } else if errors.Is(err, io.EOF) { | |
| break outer | |
| } else { | |
| return pkgerrors.WithStack(err) |
| if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), errSessionTimeout) { | ||
| return resp, errors.Errorf("build exceeded max duration of %s", sessionTimeout.String()) |
There was a problem hiding this comment.
To avoid a compilation error due to a name collision between the standard library errors package (needed for errors.Is) and github.com/pkg/errors (needed for errors.Errorf), you should alias github.com/pkg/errors as pkgerrors in the imports of this file and use pkgerrors.Errorf here.
| if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), errSessionTimeout) { | |
| return resp, errors.Errorf("build exceeded max duration of %s", sessionTimeout.String()) | |
| if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), errSessionTimeout) { | |
| return resp, pkgerrors.Errorf("build exceeded max duration of %s", sessionTimeout.String()) |
| if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), errSessionTimeout) { | ||
| return errors.Errorf("build exceeded max duration of %s", sessionTimeout.String()) |
There was a problem hiding this comment.
Use pkgerrors.Errorf to avoid name collision with the standard library errors package.
| if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), errSessionTimeout) { | |
| return errors.Errorf("build exceeded max duration of %s", sessionTimeout.String()) | |
| if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), errSessionTimeout) { | |
| return pkgerrors.Errorf("build exceeded max duration of %s", sessionTimeout.String()) |
| var netErr net.Error | ||
| if errors.As(err, &netErr) && netErr.Timeout() { |
The fork existed only to add a per-file VerboseProgressCB (verbose-only transfer logging, 2022). Stock fsutil already provides the equivalent per-file hook on receive via ReceiveOpt.NotifyHashed (ChangeFunc), which carries path + FileInfo. Rewire the earthly filesync target callbacks to NotifyHashed and use stock fsutil's 4-arg Send everywhere, removing every VerboseProgressCB reference. Drop the go.mod replace directive and re-vendor stock fsutil. Client API: ExportEntry.VerboseProgressCB -> OnReceiveFile (fsutil.ChangeFunc).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Resolves the Dockerfile version-baking conflict: keep the branch's worktree-aware heredoc block (skips version check for git worktrees, sets dev stubs) and apply main's rebrand from #18 (EARTHLY_PKG github.com/earthly/buildkit -> github.com/EarthBuild/buildkit).
Same idea as #17, but based at
67d0d850(the 2026-04-08Merge moby-buildkit/masterpoint) instead ofmoby-merge-base. This narrows the diff to our changes since that upstream merge — i.e. this session's diff-reduction pass plus fixes:t.Context)/procarchutil patchfrontendAttrsmap inClient.solve(fixes a multi-image+cache panic)Review-only view — do not merge. The base branch
moby-merge-base-apr8is a throwaway pointer to67d0d850; keep it while this PR is open.