Move the Go buildpack to a glibc/bookworm base, with an adaptive runtime stage#865
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR updates Go image references from alpine to bookworm, adds Go runtime constants for static and Debian-based images, and makes GoStack cgo-aware. GoStack now uses debian base images, configures Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/stackbuild/golang.go`:
- Around line 273-275: The comment in the Go stackbuild guidance is stale
because it refers to build.cgo = true instead of the current cgo enablement
contract. Update the documentation near the affected stackbuild comment to
reference CGO_ENABLED=1, keeping the rest of the guidance about go:embed and the
slim base unchanged. Use the nearby stackbuild Go comment text as the anchor so
the wording stays aligned with the implementation and PR contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f65ff3fa-f073-4a60-b0fa-c2247898800a
📒 Files selected for processing (8)
pkg/imagerefs/imagerefs.gopkg/imagerefs/imagerefs_test.gopkg/stackbuild/augment_test.gopkg/stackbuild/golang.gopkg/stackbuild/stackbuild.gopkg/stackbuild/stackbuild_test.gopkg/stackbuild/testdata/go-cgo/go.modpkg/stackbuild/testdata/go-cgo/main.go
💤 Files with no reviewable changes (1)
- pkg/stackbuild/stackbuild.go
6fe2be2 to
b6bf036
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/stackbuild/stackbuild_test.go (2)
580-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall
Init()inTestGobeforeGenerateLLB().
GenerateLLB()now readsInit()-derived state (cmdDir,goModVersion,hasVendor,cgoEnabled). Skipping initialization here means the default pure-Go path can still pass while that setup regresses elsewhere.Suggested change
- stack := &GoStack{ - MetaStack: MetaStack{ - dir: dir, - }, - } - state, err := stack.GenerateLLB(dir, BuildOptions{Version: "1.23"}) + opts := BuildOptions{Version: "1.23"} + stack := &GoStack{ + MetaStack: MetaStack{ + dir: dir, + }, + } + stack.Init(opts) + state, err := stack.GenerateLLB(dir, opts)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/stackbuild/stackbuild_test.go` around lines 580 - 586, Initialize the GoStack before generating the LLB in TestGo: call GoStack.Init() on the stack instance before invoking GenerateLLB(), since GenerateLLB() now depends on Init()-populated fields like cmdDir, goModVersion, hasVendor, and cgoEnabled. Update the test setup around GoStack and GenerateLLB() so the initialized state is exercised rather than relying on defaults.
603-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the generated
/etc/groupfile.The pure-Go runtime contract here includes generated passwd and group files, but this test only guards
/etc/passwd. A regression that drops/etc/groupwould still pass.Suggested change
_, hasPasswd := m["etc/passwd"] require.True(t, hasPasswd, "distroless runtime should have an app-user passwd entry") + _, hasGroup := m["etc/group"] + require.True(t, hasGroup, "distroless runtime should have an app-user group entry") }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/stackbuild/stackbuild_test.go` around lines 603 - 604, The stackbuild test currently only verifies the generated passwd entry, so add an assertion in the same test to also check that the distroless runtime output includes the generated /etc/group file. Update the existing validation around the m map in stackbuild_test.go to cover both /etc/passwd and /etc/group, using the same test block and symbols already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/stackbuild/stackbuild_test.go`:
- Around line 580-586: Initialize the GoStack before generating the LLB in
TestGo: call GoStack.Init() on the stack instance before invoking GenerateLLB(),
since GenerateLLB() now depends on Init()-populated fields like cmdDir,
goModVersion, hasVendor, and cgoEnabled. Update the test setup around GoStack
and GenerateLLB() so the initialized state is exercised rather than relying on
defaults.
- Around line 603-604: The stackbuild test currently only verifies the generated
passwd entry, so add an assertion in the same test to also check that the
distroless runtime output includes the generated /etc/group file. Update the
existing validation around the m map in stackbuild_test.go to cover both
/etc/passwd and /etc/group, using the same test block and symbols already
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c76685d0-9220-42e5-9d30-eef3b30d7a58
📒 Files selected for processing (8)
pkg/imagerefs/imagerefs.gopkg/imagerefs/imagerefs_test.gopkg/stackbuild/augment_test.gopkg/stackbuild/golang.gopkg/stackbuild/stackbuild.gopkg/stackbuild/stackbuild_test.gopkg/stackbuild/testdata/go-cgo/go.modpkg/stackbuild/testdata/go-cgo/main.go
💤 Files with no reviewable changes (1)
- pkg/stackbuild/stackbuild.go
✅ Files skipped from review due to trivial changes (1)
- pkg/stackbuild/testdata/go-cgo/go.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/stackbuild/testdata/go-cgo/main.go
- pkg/stackbuild/augment_test.go
- pkg/imagerefs/imagerefs.go
- pkg/stackbuild/golang.go
- pkg/imagerefs/imagerefs_test.go
Go was the last stack still building on alpine/musl while Python, Ruby, Node, Bun, and Rust had all moved to debian/glibc. musl is where a class of prebuilt-binary compatibility problems live, and cgo was the visible symptom: source-compiled cgo built with CGO_ENABLED=0 because the alpine builder ships no C toolchain, and prebuilt-glibc cgo (duckdb's vendored archive) couldn't link against musl at all. Build on golang:<v>-bookworm instead, which ships a C toolchain so cgo works, and flip GoStack.BaseDistro to "debian" so the existing apt-based augmentation machinery applies. bookworm specifically, not trixie: it is the one debian codename published for every Go version an app might pin (trixie images don't exist for Go <= 1.23). To keep images small the build is now multi-stage: it compiles on the heavyweight bookworm builder, then copies the result onto a minimal runtime base. By default the binary is built CGO_ENABLED=0 (static) and ships on distroless/static. Setting CGO_ENABLED=1 (the standard Go env var, via [env]) links against glibc and ships on debian-slim, which is also where JS-augmented apps land. Both run as the app user (uid 2010): debian-slim creates it with adduser, while distroless gets a written /etc/passwd (no shell to run adduser). distroless also gets a static busybox with its applet symlinks, since miren's runtime expects a shell and coreutils: the runner launches the app via `/bin/sh -c`, and `miren sandbox exec` runs commands like echo/ls in the container. cgo is opt-in via the standard CGO_ENABLED env var rather than a Go-specific app.toml field. Automatic detection is a planned follow-up (MIR-1258); the env var stays on afterward as the fast path that skips it.
b6bf036 to
24d0bd3
Compare
Go was the odd one out. Every other language stack (Python, Ruby, Node, Bun, Rust) had already moved to debian/glibc, but the Go buildpack was still building on
golang:<v>-alpine. musl is where a whole class of prebuilt-binary compatibility problems live, and cgo was the most visible symptom: source-compiled cgo fell back toCGO_ENABLED=0because the alpine builder ships no C toolchain, and prebuilt-glibc cgo like duckdb's vendored static archive couldn't link against musl at all.So this moves the Go stack onto
golang:<v>-bookworm, which ships a C toolchain so cgo works, and flipsBaseDistro()to"debian"so the existing apt-vs-apk augmentation machinery applies. It mostly deletes an inconsistency rather than adding surface area. bookworm specifically, not trixie: it's the one debian codename Docker publishes for every Go version an app might pin (trixie images don't exist for Go ≤ 1.23), and keeping builder and runtime on the same codename avoids glibc skew.The catch is image size. The Go stack ships the whole builder image as the final image today (it always has, same as Rust), so there was never a tiny-image story to protect, but a bookworm-with-toolchain builder would make that even chunkier. So the build is now genuinely multi-stage: it compiles on the heavyweight builder, then copies just the result onto a minimal runtime base. By default the binary is built
CGO_ENABLED=0and ships ondistroless/static, the tiny static Go image people expect. cgo apps ship ondebian-slim, where glibc and a real runtime filesystem are available (that's also where JS-augmented apps land, since they carry built assets in/app). Both paths run as the app user, uid 2010, for consistency with every other stack. One wrinkle the blackbox suite caught: miren's runtime expects a shell and coreutils in the image. The runner launches every app as/bin/sh -c <command>(a shell-less image crash-loops at boot), andmiren sandbox execruns commands likeecho/lsin the container. distroless ships neither, so it gets a static musl busybox with its applet symlinks, about a megabyte; debian-slim already has them. The same extract pattern sets up Rust to shed its builder image too, tracked in MIR-1259.On turning cgo on: rather than add a Go-specific
cgofield to the shared app.toml build schema, the buildpack honors the standardCGO_ENABLEDenv var. It's the canonical Go knob, soCGO_ENABLED=1doing a glibc build is the least surprising thing for a Go developer, and it keeps stack-specific concepts out of the schema. It's the explicit opt-in for now; a planned follow-up (MIR-1258) adds automatic detection (building the app to find out whether cgo is what makes it compile, the way #862 auto-detects the real port), at which point this same env var becomes the fast path that skips the extra detection pass.Stacked on #864 (the imagerefs pull-through cleanup), since both touch
GetGolangImage. The two runtime bases (debian:bookworm-slim,distroless/static-debian12) needed adding to the registry proxy allowlist, already applied to prod via mirendev/infra#77.Closes MIR-1248