From 43c6620328a02035c389fce229d970e943794e28 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 14:30:53 +0200 Subject: [PATCH 01/11] feat(registry): extend Store with annotations + LookupBySpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Push now accepts an Annotations value carrying manifest-level metadata (BundleName, SpecHash). LookupBySpec walks the layout index for a manifest whose annotations match a given (name, spec-hash) pair — the foundation for phpup build's cache-probe short-circuit (arrives in PR 2 Tasks 4-5). Remote backend's LookupBySpec is a no-op returning (zero, false, nil); anonymous OCI registries don't expose index walks, so build callers should use an oci-layout registry as their cache store and publish to remote only after a fresh build produces an artifact. Breaking change: Store.Push signature gains an Annotations value. Only internal callers (layout_test.go, remote_test.go) were affected and have been updated in the same commit. --- internal/registry/annotations.go | 33 +++++++ internal/registry/annotations_test.go | 121 ++++++++++++++++++++++++++ internal/registry/layout.go | 45 +++++++++- internal/registry/layout_test.go | 12 +-- internal/registry/registry.go | 11 ++- internal/registry/remote.go | 12 ++- internal/registry/remote_test.go | 2 +- 7 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 internal/registry/annotations.go create mode 100644 internal/registry/annotations_test.go diff --git a/internal/registry/annotations.go b/internal/registry/annotations.go new file mode 100644 index 0000000..79630e7 --- /dev/null +++ b/internal/registry/annotations.go @@ -0,0 +1,33 @@ +package registry + +// annotationSpecHash is the OCI-annotation key where the build spec-hash +// lives on the manifest. annotationBundleName is defined in layout.go. +const annotationSpecHash = "io.buildrush.bundle.spec-hash" + +// Annotations is a well-known, backend-agnostic set of manifest annotations +// that callers can request on Push and query on LookupBySpec. Keys are OCI +// annotation strings under the io.buildrush namespace so external OCI +// inspection tools (oras discover, skopeo inspect) see them too. +type Annotations struct { + // BundleName is the logical bundle kind+name (e.g. "php-core", + // "php-ext-redis"). Mirrors the annotationBundleName key used by PR 1. + BundleName string + + // SpecHash is the deterministic hash of the inputs that produced the + // bundle (builder scripts + catalog entry + os/arch/php/ts). Used by + // the build subcommand's cache probe to skip redundant rebuilds. + SpecHash string +} + +// asMap returns the OCI annotation map for the Annotations value. +// Empty fields are omitted so we don't write empty-string annotations. +func (a Annotations) asMap() map[string]string { + m := map[string]string{} + if a.BundleName != "" { + m[annotationBundleName] = a.BundleName + } + if a.SpecHash != "" { + m[annotationSpecHash] = a.SpecHash + } + return m +} diff --git a/internal/registry/annotations_test.go b/internal/registry/annotations_test.go new file mode 100644 index 0000000..11ec28d --- /dev/null +++ b/internal/registry/annotations_test.go @@ -0,0 +1,121 @@ +package registry + +import ( + "bytes" + "context" + "path/filepath" + "testing" +) + +func TestAnnotations_AsMap_OmitsEmpty(t *testing.T) { + if got := (Annotations{}).asMap(); len(got) != 0 { + t.Errorf("empty Annotations produced non-empty map: %v", got) + } + got := Annotations{BundleName: "x", SpecHash: "sha256:abc"}.asMap() + if got[annotationBundleName] != "x" || got[annotationSpecHash] != "sha256:abc" { + t.Errorf("asMap = %v", got) + } + // BundleName only + got2 := Annotations{BundleName: "x"}.asMap() + if _, ok := got2[annotationSpecHash]; ok { + t.Errorf("asMap should omit empty SpecHash: %v", got2) + } + // SpecHash only + got3 := Annotations{SpecHash: "sha256:abc"}.asMap() + if _, ok := got3[annotationBundleName]; ok { + t.Errorf("asMap should omit empty BundleName: %v", got3) + } +} + +// pushForTest pushes a bundle with the given (name, spec-hash) pair via +// the new Annotations API. It is intentionally minimal so the cache-probe +// tests below read as declarative fixtures rather than re-exercising the +// full Push surface. +func pushForTest(t *testing.T, s *layoutStore, name, specHash string, payload []byte) { + t.Helper() + err := s.Push(context.Background(), Ref{Name: name}, bytes.NewReader(payload), nil, + Annotations{BundleName: name, SpecHash: specHash}) + if err != nil { + t.Fatalf("Push: %v", err) + } +} + +func TestLayoutStore_LookupBySpec_Hit(t *testing.T) { + s, err := openLayout(filepath.Join(t.TempDir(), "layout")) + if err != nil { + t.Fatalf("openLayout: %v", err) + } + pushForTest(t, s, "php-core", "sha256:abc", []byte("bundle")) + ref, hit, err := s.LookupBySpec(context.Background(), "php-core", "sha256:abc") + if err != nil { + t.Fatalf("LookupBySpec: %v", err) + } + if !hit { + t.Fatal("want hit, got miss") + } + if ref.Name != "php-core" || ref.Digest == "" { + t.Fatalf("ref = %+v", ref) + } +} + +func TestLayoutStore_LookupBySpec_Miss_EmptyLayout(t *testing.T) { + s, err := openLayout(filepath.Join(t.TempDir(), "layout")) + if err != nil { + t.Fatalf("openLayout: %v", err) + } + ref, hit, err := s.LookupBySpec(context.Background(), "php-core", "sha256:abc") + if err != nil { + t.Fatalf("LookupBySpec on empty layout: err = %v, want nil", err) + } + if hit { + t.Fatal("want miss, got hit") + } + if ref.Name != "" || ref.Digest != "" { + t.Fatalf("ref should be zero: %+v", ref) + } +} + +func TestLayoutStore_LookupBySpec_WrongNameIsMiss(t *testing.T) { + s, err := openLayout(filepath.Join(t.TempDir(), "layout")) + if err != nil { + t.Fatalf("openLayout: %v", err) + } + pushForTest(t, s, "php-core", "sha256:abc", []byte("x")) + _, hit, err := s.LookupBySpec(context.Background(), "php-ext-redis", "sha256:abc") + if err != nil { + t.Fatalf("LookupBySpec: %v", err) + } + if hit { + t.Fatal("lookup with wrong name should miss") + } +} + +func TestLayoutStore_LookupBySpec_WrongHashIsMiss(t *testing.T) { + s, err := openLayout(filepath.Join(t.TempDir(), "layout")) + if err != nil { + t.Fatalf("openLayout: %v", err) + } + pushForTest(t, s, "php-core", "sha256:abc", []byte("x")) + _, hit, err := s.LookupBySpec(context.Background(), "php-core", "sha256:xyz") + if err != nil { + t.Fatalf("LookupBySpec: %v", err) + } + if hit { + t.Fatal("lookup with wrong hash should miss") + } +} + +func TestRemoteStore_LookupBySpec_AlwaysMiss(t *testing.T) { + host := startTestRegistry(t) + s, err := Open(host + "/buildrush") + if err != nil { + t.Fatalf("Open: %v", err) + } + _, hit, err := s.LookupBySpec(context.Background(), "php-core", "sha256:abc") + if err != nil { + t.Fatalf("LookupBySpec: %v", err) + } + if hit { + t.Fatal("remote LookupBySpec should always miss in PR 1/2 scope") + } +} diff --git a/internal/registry/layout.go b/internal/registry/layout.go index 6a88db4..3a9766e 100644 --- a/internal/registry/layout.go +++ b/internal/registry/layout.go @@ -59,7 +59,7 @@ func (s *layoutStore) open() (layout.Path, error) { return layout.FromPath(s.root) } -func (s *layoutStore) Push(_ context.Context, ref Ref, body io.Reader, meta *Meta) error { +func (s *layoutStore) Push(_ context.Context, ref Ref, body io.Reader, meta *Meta, ann Annotations) error { if ref.Name == "" { return errors.New("layout.Push: ref.Name required") } @@ -86,7 +86,15 @@ func (s *layoutStore) Push(_ context.Context, ref Ref, body io.Reader, meta *Met // partial.Descriptor does not propagate manifest-level annotations into the // index), while the manifest-level annotation keeps the round-trip // self-describing for tools that inspect the OCI image directly. - annotations := map[string]string{annotationBundleName: ref.Name} + // + // Callers supply the desired annotation set via Annotations. For backward + // compatibility with PR 1 semantics, if the caller didn't set BundleName + // (or any annotation at all) we fall back to deriving it from ref.Name so + // Has/Fetch still find the manifest. + annotations := ann.asMap() + if annotations[annotationBundleName] == "" { + annotations[annotationBundleName] = ref.Name + } annotated, ok := mutate.Annotations(img, annotations).(v1.Image) if !ok { return errors.New("layout.Push: mutate.Annotations did not return v1.Image") @@ -102,6 +110,39 @@ func (s *layoutStore) Push(_ context.Context, ref Ref, body io.Reader, meta *Met return nil } +// LookupBySpec walks the index for a manifest whose annotations match BOTH +// the given bundle name and spec-hash. Returns (Ref, true, nil) on hit. +// An absent layout is a valid miss (not an error) so callers can probe +// empty caches without a pre-check. Any other open failure propagates. +func (s *layoutStore) LookupBySpec(_ context.Context, name, specHash string) (Ref, bool, error) { + p, err := s.open() + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return Ref{}, false, nil + } + return Ref{}, false, fmt.Errorf("layout.LookupBySpec: open %q: %w", s.root, err) + } + idx, err := p.ImageIndex() + if err != nil { + return Ref{}, false, fmt.Errorf("layout.LookupBySpec: index: %w", err) + } + manifest, err := idx.IndexManifest() + if err != nil { + return Ref{}, false, fmt.Errorf("layout.LookupBySpec: index manifest: %w", err) + } + for i := range manifest.Manifests { + m := &manifest.Manifests[i] + if m.Annotations[annotationBundleName] != name { + continue + } + if m.Annotations[annotationSpecHash] != specHash { + continue + } + return Ref{Name: name, Digest: m.Digest.String()}, true, nil + } + return Ref{}, false, nil +} + func (s *layoutStore) Has(_ context.Context, ref Ref) (bool, error) { p, err := layout.FromPath(s.root) if err != nil { diff --git a/internal/registry/layout_test.go b/internal/registry/layout_test.go index 10218ae..6e21b12 100644 --- a/internal/registry/layout_test.go +++ b/internal/registry/layout_test.go @@ -43,7 +43,7 @@ func TestLayoutStore_RoundTrip(t *testing.T) { t.Fatal("Has returned true on empty layout") } - if err := s.Push(ctx, ref, bytes.NewReader(payload), meta); err != nil { + if err := s.Push(ctx, ref, bytes.NewReader(payload), meta, Annotations{BundleName: "php-core"}); err != nil { t.Fatalf("Push: %v", err) } @@ -100,7 +100,7 @@ func TestLayoutStore_FetchMissingRef_Errors(t *testing.T) { // Populated layout, wrong digest — must still error (exercises the // "not found in index" branch, not just the open() failure). - if err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader([]byte("x")), nil); err != nil { + if err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader([]byte("x")), nil, Annotations{BundleName: "php-core"}); err != nil { t.Fatalf("Push: %v", err) } missing := Ref{Name: "php-core", Digest: "sha256:" + strings.Repeat("0", 64)} @@ -162,10 +162,10 @@ func TestLayoutStore_DigestOnlyFallback_PrefersExactAnnotationMatch(t *testing.T s := newTestLayoutStore(t) // Two pushes with distinct payloads → distinct digests. - if err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader([]byte("core-bytes")), nil); err != nil { + if err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader([]byte("core-bytes")), nil, Annotations{BundleName: "php-core"}); err != nil { t.Fatalf("Push core: %v", err) } - if err := s.Push(ctx, Ref{Name: "php-ext-redis"}, bytes.NewReader([]byte("redis-bytes")), nil); err != nil { + if err := s.Push(ctx, Ref{Name: "php-ext-redis"}, bytes.NewReader([]byte("redis-bytes")), nil, Annotations{BundleName: "php-ext-redis"}); err != nil { t.Fatalf("Push redis: %v", err) } @@ -231,7 +231,7 @@ func TestLayoutStore_DigestOnlyFallback_RejectsManifestWithWrongAnnotation(t *te ctx := context.Background() s := newTestLayoutStore(t) - if err := s.Push(ctx, Ref{Name: "php-ext-redis"}, bytes.NewReader([]byte("redis-bytes")), nil); err != nil { + if err := s.Push(ctx, Ref{Name: "php-ext-redis"}, bytes.NewReader([]byte("redis-bytes")), nil, Annotations{BundleName: "php-ext-redis"}); err != nil { t.Fatalf("Push: %v", err) } redisDigest := indexEntryForName(t, s, "php-ext-redis") @@ -258,7 +258,7 @@ func TestLayoutStore_TolerateMissingMeta(t *testing.T) { ctx := context.Background() s := newTestLayoutStore(t) payload := []byte("legacy-bundle") - if err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader(payload), nil); err != nil { + if err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader(payload), nil, Annotations{BundleName: "php-core"}); err != nil { t.Fatalf("Push nil meta: %v", err) } got, err := s.list(ctx) diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 24fd3b5..04987b9 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -65,8 +65,17 @@ type Meta struct { type Store interface { Kind() string Fetch(ctx context.Context, ref Ref) (io.ReadCloser, *Meta, error) - Push(ctx context.Context, ref Ref, body io.Reader, meta *Meta) error + // Push writes a bundle to the store under the given ref, attaching + // the supplied Annotations to the manifest. Remote backends that + // don't support anonymous writes return ErrUnsupported. + Push(ctx context.Context, ref Ref, body io.Reader, meta *Meta, ann Annotations) error Has(ctx context.Context, ref Ref) (bool, error) + // LookupBySpec finds a manifest whose annotations include both + // BundleName==name AND SpecHash==specHash. Returns (Ref, true, nil) + // on hit, (zero, false, nil) when absent, (zero, false, err) on + // backend error. Used by the build subcommand's cache probe to + // short-circuit redundant rebuilds. + LookupBySpec(ctx context.Context, name, specHash string) (Ref, bool, error) ResolveDigest(ctx context.Context, name string) (string, error) } diff --git a/internal/registry/remote.go b/internal/registry/remote.go index c13c003..23adecc 100644 --- a/internal/registry/remote.go +++ b/internal/registry/remote.go @@ -136,10 +136,20 @@ func (s *remoteStore) Fetch(ctx context.Context, ref Ref) (io.ReadCloser, *Meta, // Push is deliberately unsupported in PR 1; remote publication lands with the // `phpup build` subcommand in a follow-up PR. -func (s *remoteStore) Push(_ context.Context, _ Ref, _ io.Reader, _ *Meta) error { +func (s *remoteStore) Push(_ context.Context, _ Ref, _ io.Reader, _ *Meta, _ Annotations) error { return ErrUnsupported } +// LookupBySpec on the remote backend is a no-op returning (zero, false, nil). +// Anonymous OCI registries don't expose an index walk for annotation-keyed +// probes, so build callers should use an oci-layout registry as their cache +// store and publish to the remote only after a fresh build produces an +// artifact. A non-error miss keeps the build subcommand's default flow +// correct: "don't know" -> "build anyway". +func (s *remoteStore) LookupBySpec(_ context.Context, _, _ string) (Ref, bool, error) { + return Ref{}, false, nil +} + func (s *remoteStore) ResolveDigest(ctx context.Context, reference string) (string, error) { ref, err := name.ParseReference(reference) if err != nil { diff --git a/internal/registry/remote_test.go b/internal/registry/remote_test.go index b742c1e..c9a0c49 100644 --- a/internal/registry/remote_test.go +++ b/internal/registry/remote_test.go @@ -148,7 +148,7 @@ func TestRemoteStore_PushReturnsUnsupported(t *testing.T) { ctx := context.Background() host := startTestRegistry(t) s, _ := Open(host + "/buildrush") - err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader([]byte("x")), nil) + err := s.Push(ctx, Ref{Name: "php-core"}, bytes.NewReader([]byte("x")), nil, Annotations{BundleName: "php-core"}) if !errors.Is(err, ErrUnsupported) { t.Fatalf("Push err = %v, want ErrUnsupported", err) } From b2d3e8fcaa2b54abd577d629acac29302d905c03 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 14:42:34 +0200 Subject: [PATCH 02/11] fix(registry): tighten LookupBySpec contract + back-compat coverage - layout.LookupBySpec now rejects empty name or specHash explicitly so a Ref{} zero-value probe can't false-positive on un-annotated manifests. Add regression test. - remote.LookupBySpec now returns ErrUnsupported so callers who wire only a remote store see the structural capability gap instead of a silent miss every call. Build callers can convert to soft-miss via errors.Is(err, ErrUnsupported). - New test exercises the empty-Annotations back-compat fallback where Push derives BundleName from ref.Name. - Godoc clarifications on asMap's never-nil contract and on how to call LookupBySpec from an Annotations value. --- internal/registry/annotations.go | 3 ++ internal/registry/annotations_test.go | 54 +++++++++++++++++++++++---- internal/registry/layout.go | 3 ++ internal/registry/registry.go | 3 ++ internal/registry/remote.go | 18 +++++---- 5 files changed, 66 insertions(+), 15 deletions(-) diff --git a/internal/registry/annotations.go b/internal/registry/annotations.go index 79630e7..c6a721f 100644 --- a/internal/registry/annotations.go +++ b/internal/registry/annotations.go @@ -21,6 +21,9 @@ type Annotations struct { // asMap returns the OCI annotation map for the Annotations value. // Empty fields are omitted so we don't write empty-string annotations. +// The returned map is always non-nil and writable — callers can mutate +// it in place (e.g. the layout backend's back-compat fallback writes +// BundleName from ref.Name when the Annotations value is zero). func (a Annotations) asMap() map[string]string { m := map[string]string{} if a.BundleName != "" { diff --git a/internal/registry/annotations_test.go b/internal/registry/annotations_test.go index 11ec28d..62e1e49 100644 --- a/internal/registry/annotations_test.go +++ b/internal/registry/annotations_test.go @@ -3,6 +3,7 @@ package registry import ( "bytes" "context" + "errors" "path/filepath" "testing" ) @@ -105,17 +106,54 @@ func TestLayoutStore_LookupBySpec_WrongHashIsMiss(t *testing.T) { } } -func TestRemoteStore_LookupBySpec_AlwaysMiss(t *testing.T) { +func TestRemoteStore_LookupBySpec_ReturnsUnsupported(t *testing.T) { host := startTestRegistry(t) - s, err := Open(host + "/buildrush") - if err != nil { - t.Fatalf("Open: %v", err) - } + s, _ := Open(host + "/buildrush") _, hit, err := s.LookupBySpec(context.Background(), "php-core", "sha256:abc") - if err != nil { - t.Fatalf("LookupBySpec: %v", err) + if !errors.Is(err, ErrUnsupported) { + t.Fatalf("LookupBySpec on remote: err = %v, want ErrUnsupported", err) } if hit { - t.Fatal("remote LookupBySpec should always miss in PR 1/2 scope") + t.Fatal("hit should be false even when returning ErrUnsupported") + } +} + +func TestLayoutStore_LookupBySpec_EmptyInput_Errors(t *testing.T) { + s, _ := openLayout(filepath.Join(t.TempDir(), "layout")) + // Pre-populate so "empty match" would succeed if the guard were absent. + _ = s.Push(context.Background(), Ref{Name: "php-core"}, + bytes.NewReader([]byte("x")), nil, Annotations{BundleName: "php-core"}) + + cases := []struct { + name, sh string + }{ + {"", ""}, + {"php-core", ""}, + {"", "sha256:abc"}, + } + for _, c := range cases { + _, hit, err := s.LookupBySpec(context.Background(), c.name, c.sh) + if err == nil { + t.Errorf("LookupBySpec(%q, %q): want error, got nil (hit=%v)", c.name, c.sh, hit) + } + if hit { + t.Errorf("LookupBySpec(%q, %q): hit=true on empty input", c.name, c.sh) + } + } +} + +func TestLayoutStore_Push_EmptyAnnotationsFallsBackToRefName(t *testing.T) { + ctx := context.Background() + s, _ := openLayout(filepath.Join(t.TempDir(), "layout")) + if err := s.Push(ctx, Ref{Name: "php-core"}, + bytes.NewReader([]byte("x")), nil, Annotations{}); err != nil { + t.Fatalf("Push: %v", err) + } + got, err := s.list(ctx) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(got) != 1 || got[0].Name != "php-core" { + t.Fatalf("list = %+v; want single php-core entry derived from ref.Name fallback", got) } } diff --git a/internal/registry/layout.go b/internal/registry/layout.go index 3a9766e..6bcdb15 100644 --- a/internal/registry/layout.go +++ b/internal/registry/layout.go @@ -115,6 +115,9 @@ func (s *layoutStore) Push(_ context.Context, ref Ref, body io.Reader, meta *Met // An absent layout is a valid miss (not an error) so callers can probe // empty caches without a pre-check. Any other open failure propagates. func (s *layoutStore) LookupBySpec(_ context.Context, name, specHash string) (Ref, bool, error) { + if name == "" || specHash == "" { + return Ref{}, false, errors.New("layout.LookupBySpec: name and specHash required") + } p, err := s.open() if err != nil { if errors.Is(err, fs.ErrNotExist) { diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 04987b9..052a55b 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -75,6 +75,9 @@ type Store interface { // on hit, (zero, false, nil) when absent, (zero, false, err) on // backend error. Used by the build subcommand's cache probe to // short-circuit redundant rebuilds. + // + // Callers with an `Annotations` value already in scope should pass + // `ann.BundleName` and `ann.SpecHash` positionally. LookupBySpec(ctx context.Context, name, specHash string) (Ref, bool, error) ResolveDigest(ctx context.Context, name string) (string, error) } diff --git a/internal/registry/remote.go b/internal/registry/remote.go index 23adecc..4c82456 100644 --- a/internal/registry/remote.go +++ b/internal/registry/remote.go @@ -140,14 +140,18 @@ func (s *remoteStore) Push(_ context.Context, _ Ref, _ io.Reader, _ *Meta, _ Ann return ErrUnsupported } -// LookupBySpec on the remote backend is a no-op returning (zero, false, nil). -// Anonymous OCI registries don't expose an index walk for annotation-keyed -// probes, so build callers should use an oci-layout registry as their cache -// store and publish to the remote only after a fresh build produces an -// artifact. A non-error miss keeps the build subcommand's default flow -// correct: "don't know" -> "build anyway". +// LookupBySpec is not supported on the remote backend: anonymous OCI +// registries don't expose an index walk for annotation-keyed probes. +// Callers should use an oci-layout registry as their cache store and +// publish to the remote only after a fresh build succeeds. Build +// callers that want "no cache => build anyway" semantics should +// treat ErrUnsupported as a soft miss: +// +// if errors.Is(err, ErrUnsupported) { +// hit, err = false, nil // no cache available on this backend +// } func (s *remoteStore) LookupBySpec(_ context.Context, _, _ string) (Ref, bool, error) { - return Ref{}, false, nil + return Ref{}, false, ErrUnsupported } func (s *remoteStore) ResolveDigest(ctx context.Context, reference string) (string, error) { From c6030938e8d41d1307c95087f7cd1fd199c3fec3 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 14:55:15 +0200 Subject: [PATCH 03/11] feat(build): internal/build.ComputeSpecHash wrapper + exported planner helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract HashFiles / PerVersionYAMLFromFile / ExtensionYAMLFromFile / ParseEnvValue from cmd/planner/main.go inlining into exported internal/planner helpers so phpup build (PR 2 Task 4/5) can compute spec-hashes from the same inputs CI planning uses. Values are byte-identical across the refactor — verified against the full catalog (10 PHP cells, 172 ext cells) before/after: zero spec_hash diffs. internal/build.ComputeSpecHash accepts CLI-friendly SpecHashInputs (kind/name/version/os/arch/ts/repo-root) and wraps planner.ComputeSpecHash with repo-relative path resolution. Tests use tempdir fixtures; no real catalog or builders touched. The wrapper also includes a cross-check test that builds the inputs by hand the planner's way and asserts the wrapper's output matches, so any future drift between the two code paths surfaces at CI time. --- cmd/planner/main.go | 44 ++-- internal/build/spechash.go | 137 +++++++++++ internal/build/spechash_test.go | 392 +++++++++++++++++++++++++++++++ internal/planner/helpers.go | 75 ++++++ internal/planner/helpers_test.go | 258 ++++++++++++++++++++ 5 files changed, 882 insertions(+), 24 deletions(-) create mode 100644 internal/build/spechash.go create mode 100644 internal/build/spechash_test.go create mode 100644 internal/planner/helpers.go create mode 100644 internal/planner/helpers_test.go diff --git a/cmd/planner/main.go b/cmd/planner/main.go index a76cf6e..0a2f643 100644 --- a/cmd/planner/main.go +++ b/cmd/planner/main.go @@ -53,30 +53,28 @@ func main() { // Hash builder scripts and shared support files the builders source. Changes // to any of these change the bundle contents; fold them into builderHash so - // spec_hash invalidates and bundles rebuild. - builderHashPHP, err := planner.HashFile(filepath.Join("builders", "linux", "build-php.sh")) + // spec_hash invalidates and bundles rebuild. Ordering mirrors the historical + // inline concatenation (build-.sh + schema env + capture + pack) so + // spec_hash values stay byte-identical across this refactor. + common := []string{ + filepath.Join("builders", "common", "bundle-schema-version.env"), + filepath.Join("builders", "common", "capture-hermetic-libs.sh"), + filepath.Join("builders", "common", "pack-bundle.sh"), + } + builderHashPHP, err := planner.HashFiles(append( + []string{filepath.Join("builders", "linux", "build-php.sh")}, + common..., + )) if err != nil { log.Fatalf("hash php builder: %v", err) } - builderHashExt, err := planner.HashFile(filepath.Join("builders", "linux", "build-ext.sh")) + builderHashExt, err := planner.HashFiles(append( + []string{filepath.Join("builders", "linux", "build-ext.sh")}, + common..., + )) if err != nil { log.Fatalf("hash ext builder: %v", err) } - schemaEnvHash, err := planner.HashFile(filepath.Join("builders", "common", "bundle-schema-version.env")) - if err != nil { - log.Fatalf("hash schema env: %v", err) - } - captureHash, err := planner.HashFile(filepath.Join("builders", "common", "capture-hermetic-libs.sh")) - if err != nil { - log.Fatalf("hash capture script: %v", err) - } - packHash, err := planner.HashFile(filepath.Join("builders", "common", "pack-bundle.sh")) - if err != nil { - log.Fatalf("hash pack-bundle script: %v", err) - } - commonHash := schemaEnvHash + ":" + captureHash + ":" + packHash - builderHashPHP = builderHashPHP + ":" + commonHash - builderHashExt = builderHashExt + ":" + commonHash // Expand PHP matrix phpCells := planner.ExpandPHPMatrix(cat.PHP) @@ -134,13 +132,11 @@ func readBuilderOS(path string) (string, error) { if err != nil { return "", fmt.Errorf("read %s: %w", path, err) } - for _, line := range strings.Split(string(data), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "BUILDER_OS=") { - return strings.TrimPrefix(line, "BUILDER_OS="), nil - } + v := planner.ParseEnvValue(data, "BUILDER_OS") + if v == "" { + return "", fmt.Errorf("%s: BUILDER_OS not found", path) } - return "", fmt.Errorf("%s: BUILDER_OS not found", path) + return v, nil } func filterExisting(ctx context.Context, cells []planner.MatrixCell, lf *lockfile.Lockfile, client *oci.Client) []planner.MatrixCell { diff --git a/internal/build/spechash.go b/internal/build/spechash.go new file mode 100644 index 0000000..79f2b1e --- /dev/null +++ b/internal/build/spechash.go @@ -0,0 +1,137 @@ +// Package build docker-wraps builders/linux/*.sh to produce OCI bundles +// and writes them to a registry.Store. Exposed via `phpup build php|ext` +// subcommands. +package build + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/buildrush/setup-php/internal/planner" +) + +// SpecHashInputs groups the inputs needed to compute a bundle's spec-hash. +// All fields are string for CLI-friendliness; the caller is responsible for +// normalising (e.g. "linux" instead of "Linux"). The mapping onto +// planner.MatrixCell depends on Kind: +// +// - For Kind == "php": Version is the PHP minor (e.g. "8.4"); Name and +// PHPABI are ignored. +// - For Kind == "ext": Name is the extension name (e.g. "redis"); Version +// is the extension version (e.g. "6.2.0") which maps onto +// MatrixCell.ExtVer; PHPABI is the combined "-" (e.g. "8.4-nts"). +type SpecHashInputs struct { + Kind string // "php" or "ext" + Name string // empty for PHP; extension name for ext (e.g. "redis") + Version string // PHP version (e.g. "8.4") or extension version (e.g. "6.2.0") + OS string // "linux" + Arch string // "x86_64" or "aarch64" + PHPABI string // e.g. "8.4-nts" (for ext only) + TS string // "nts" or "zts" (for php only) + Repo string // absolute path to the setup-php repo root +} + +// ComputeSpecHash produces the canonical spec-hash string for the inputs. +// It reads the same builder + catalog files the planner reads and defers to +// planner.ComputeSpecHash for the actual hashing, so local `phpup build` +// invocations and CI planner invocations produce identical hashes for the +// same inputs. The pointer receiver avoids copying the SpecHashInputs struct +// on every call (it's ~128 bytes). +func ComputeSpecHash(in *SpecHashInputs) (string, error) { + builderFiles, err := builderFilesFor(in.Kind, in.Repo) + if err != nil { + return "", err + } + builderHash, err := planner.HashFiles(builderFiles) + if err != nil { + return "", fmt.Errorf("spechash: hash builders: %w", err) + } + + builderOS, err := readBuilderOS(filepath.Join(in.Repo, "builders", "common", "builder-os.env")) + if err != nil { + return "", fmt.Errorf("spechash: read builder-os.env: %w", err) + } + + var catalogBytes []byte + switch in.Kind { + case "php": + catalogBytes, err = planner.PerVersionYAMLFromFile( + filepath.Join(in.Repo, "catalog", "php.yaml"), in.Version) + case "ext": + catalogBytes, err = planner.ExtensionYAMLFromFile( + filepath.Join(in.Repo, "catalog", "extensions", in.Name+".yaml")) + default: + return "", fmt.Errorf("spechash: unknown kind %q", in.Kind) + } + if err != nil { + return "", fmt.Errorf("spechash: load catalog: %w", err) + } + + cell := cellFor(in) + return planner.ComputeSpecHash(cell, catalogBytes, builderHash, builderOS), nil +} + +// cellFor builds the planner.MatrixCell shape the planner would have produced +// for these inputs. For ext entries the planner leaves Version empty and puts +// the extension version in ExtVer; we mirror that exactly because +// planner.ComputeSpecHash's wire format hashes cell.Version (empty for ext) +// and the ext version is instead encoded via the catalog YAML bytes. +func cellFor(in *SpecHashInputs) *planner.MatrixCell { + switch in.Kind { + case "php": + return &planner.MatrixCell{ + Version: in.Version, + OS: in.OS, + Arch: in.Arch, + TS: in.TS, + } + case "ext": + return &planner.MatrixCell{ + Extension: in.Name, + ExtVer: in.Version, + PHPAbi: in.PHPABI, + OS: in.OS, + Arch: in.Arch, + TS: in.TS, + } + } + return &planner.MatrixCell{} +} + +// builderFilesFor returns the ordered list of builder files whose hash +// contributes to this kind's builder hash. Ordering must match the planner's +// historical per-kind concatenation: "build-.sh" first, then the shared +// common files. For ext builds the planner did NOT historically include +// fetch-core.sh in the hash (it's only sourced at bundle-assembly time, not +// at spec-hash time), so we preserve that. +func builderFilesFor(kind, repo string) ([]string, error) { + common := []string{ + filepath.Join(repo, "builders", "common", "bundle-schema-version.env"), + filepath.Join(repo, "builders", "common", "capture-hermetic-libs.sh"), + filepath.Join(repo, "builders", "common", "pack-bundle.sh"), + } + switch kind { + case "php": + return append([]string{ + filepath.Join(repo, "builders", "linux", "build-php.sh"), + }, common...), nil + case "ext": + return append([]string{ + filepath.Join(repo, "builders", "linux", "build-ext.sh"), + }, common...), nil + } + return nil, fmt.Errorf("spechash: unknown kind %q", kind) +} + +func readBuilderOS(path string) (string, error) { + data, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + return "", err + } + v := planner.ParseEnvValue(data, "BUILDER_OS") + if v == "" { + return "", fmt.Errorf("%s: BUILDER_OS not found", path) + } + return v, nil +} diff --git a/internal/build/spechash_test.go b/internal/build/spechash_test.go new file mode 100644 index 0000000..e7b1b49 --- /dev/null +++ b/internal/build/spechash_test.go @@ -0,0 +1,392 @@ +package build + +import ( + "os" + "path/filepath" + "testing" + + "github.com/buildrush/setup-php/internal/planner" +) + +// fakeRepo lays down a minimal tree that mimics the setup-php repo layout +// well enough for ComputeSpecHash to find every file it needs. Contents are +// arbitrary but stable; tests that want to see the hash change mutate the +// relevant file under this tempdir. +func fakeRepo(t *testing.T) string { + t.Helper() + root := t.TempDir() + + mk := func(rel, content string) { + t.Helper() + abs := filepath.Join(root, rel) + if err := os.MkdirAll(filepath.Dir(abs), 0o750); err != nil { + t.Fatalf("mkdir %s: %v", rel, err) + } + if err := os.WriteFile(abs, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", rel, err) + } + } + + mk("builders/linux/build-php.sh", "#!/bin/bash\necho build-php v1\n") + mk("builders/linux/build-ext.sh", "#!/bin/bash\necho build-ext v1\n") + mk("builders/common/bundle-schema-version.env", "BUNDLE_SCHEMA_VERSION=1\n") + mk("builders/common/capture-hermetic-libs.sh", "#!/bin/bash\necho capture v1\n") + mk("builders/common/pack-bundle.sh", "#!/bin/bash\necho pack v1\n") + mk("builders/common/fetch-core.sh", "#!/bin/bash\necho fetch v1\n") + mk("builders/common/builder-os.env", "BUILDER_OS=ubuntu-22.04\n") + + mk("catalog/php.yaml", `name: php +versions: + "8.4": + bundled_extensions: [core] + sources: + url: https://example/php-8.4.tar.xz + abi_matrix: + os: [linux] + arch: [x86_64] + ts: [nts] +`) + + mk("catalog/extensions/redis.yaml", `name: redis +kind: pecl +source: + pecl_package: redis +versions: + - "6.2.0" +abi_matrix: + php: ["8.4"] + os: ["linux"] + arch: ["x86_64"] + ts: ["nts"] +`) + + return root +} + +func phpIn(repo string) *SpecHashInputs { + return &SpecHashInputs{ + Kind: "php", + Version: "8.4", + OS: "linux", + Arch: "x86_64", + TS: "nts", + Repo: repo, + } +} + +func extIn(repo string) *SpecHashInputs { + return &SpecHashInputs{ + Kind: "ext", + Name: "redis", + Version: "6.2.0", + OS: "linux", + Arch: "x86_64", + PHPABI: "8.4-nts", + TS: "nts", + Repo: repo, + } +} + +func TestComputeSpecHash_PHP_StableAcrossCalls(t *testing.T) { + repo := fakeRepo(t) + h1, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("first call: %v", err) + } + h2, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("second call: %v", err) + } + if h1 != h2 { + t.Errorf("stable call returned different hashes:\n1: %s\n2: %s", h1, h2) + } +} + +func TestComputeSpecHash_PHP_ChangesWhenCatalogChanges(t *testing.T) { + repo := fakeRepo(t) + h1, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h1: %v", err) + } + + // Mutate the per-version subtree. Adding a new field to the 8.4 entry + // must flow through PerVersionYAMLFromFile into the hash. + newPHP := `name: php +versions: + "8.4": + bundled_extensions: [core, opcache] + sources: + url: https://example/php-8.4.tar.xz + abi_matrix: + os: [linux] + arch: [x86_64] + ts: [nts] +` + if err := os.WriteFile(filepath.Join(repo, "catalog", "php.yaml"), []byte(newPHP), 0o600); err != nil { + t.Fatalf("rewrite php.yaml: %v", err) + } + + h2, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h2: %v", err) + } + if h1 == h2 { + t.Errorf("catalog change did not alter spec_hash, both = %s", h1) + } +} + +func TestComputeSpecHash_PHP_ChangesWhenBuilderChanges(t *testing.T) { + repo := fakeRepo(t) + h1, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h1: %v", err) + } + if err := os.WriteFile(filepath.Join(repo, "builders", "linux", "build-php.sh"), + []byte("#!/bin/bash\necho build-php v2\n"), 0o600); err != nil { + t.Fatalf("rewrite builder: %v", err) + } + h2, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h2: %v", err) + } + if h1 == h2 { + t.Errorf("builder change did not alter spec_hash, both = %s", h1) + } +} + +func TestComputeSpecHash_PHP_ChangesWhenCommonBuilderChanges(t *testing.T) { + // The common files (capture-hermetic-libs, pack-bundle, schema env) are + // folded into the builder hash alongside build-php.sh. Mutating any of + // them must bust spec_hash too — otherwise a pack-script fix would ship + // silently and the lockfile would never invalidate. + repo := fakeRepo(t) + h1, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h1: %v", err) + } + if err := os.WriteFile(filepath.Join(repo, "builders", "common", "pack-bundle.sh"), + []byte("#!/bin/bash\necho pack v2\n"), 0o600); err != nil { + t.Fatalf("rewrite pack-bundle: %v", err) + } + h2, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h2: %v", err) + } + if h1 == h2 { + t.Errorf("common-builder change did not alter spec_hash, both = %s", h1) + } +} + +func TestComputeSpecHash_PHP_ChangesWhenBuilderOSChanges(t *testing.T) { + repo := fakeRepo(t) + h1, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h1: %v", err) + } + if err := os.WriteFile(filepath.Join(repo, "builders", "common", "builder-os.env"), + []byte("BUILDER_OS=ubuntu-24.04\n"), 0o600); err != nil { + t.Fatalf("rewrite builder-os.env: %v", err) + } + h2, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("h2: %v", err) + } + if h1 == h2 { + t.Errorf("builder-os change did not alter spec_hash, both = %s", h1) + } +} + +func TestComputeSpecHash_Ext_StableAcrossCalls(t *testing.T) { + repo := fakeRepo(t) + h1, err := ComputeSpecHash(extIn(repo)) + if err != nil { + t.Fatalf("h1: %v", err) + } + h2, err := ComputeSpecHash(extIn(repo)) + if err != nil { + t.Fatalf("h2: %v", err) + } + if h1 != h2 { + t.Errorf("stable call returned different hashes:\n1: %s\n2: %s", h1, h2) + } +} + +func TestComputeSpecHash_Ext_ChangesWhenExtensionCatalogChanges(t *testing.T) { + repo := fakeRepo(t) + h1, err := ComputeSpecHash(extIn(repo)) + if err != nil { + t.Fatalf("h1: %v", err) + } + + newExt := `name: redis +kind: pecl +source: + pecl_package: redis +versions: + - "6.2.0" + - "6.3.0" +abi_matrix: + php: ["8.4"] + os: ["linux"] + arch: ["x86_64"] + ts: ["nts"] +` + if err := os.WriteFile(filepath.Join(repo, "catalog", "extensions", "redis.yaml"), []byte(newExt), 0o600); err != nil { + t.Fatalf("rewrite redis.yaml: %v", err) + } + h2, err := ComputeSpecHash(extIn(repo)) + if err != nil { + t.Fatalf("h2: %v", err) + } + if h1 == h2 { + t.Errorf("ext catalog change did not alter spec_hash, both = %s", h1) + } +} + +// The PHP and ext kinds have distinct builder-file sets (build-php.sh vs +// build-ext.sh), plus different catalog bytes. They must never collide even +// if the other axes are zeroed. +func TestComputeSpecHash_PHP_And_Ext_Differ(t *testing.T) { + repo := fakeRepo(t) + hp, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("php: %v", err) + } + he, err := ComputeSpecHash(extIn(repo)) + if err != nil { + t.Fatalf("ext: %v", err) + } + if hp == he { + t.Errorf("php and ext produced the same spec_hash: %s", hp) + } +} + +// ComputeSpecHash must mirror planner.ComputeSpecHash exactly for equivalent +// inputs. This test composes the inputs by hand the way the planner does it +// and verifies the wrapper returns the same string. +func TestComputeSpecHash_PHP_MatchesPlannerOutput(t *testing.T) { + repo := fakeRepo(t) + got, err := ComputeSpecHash(phpIn(repo)) + if err != nil { + t.Fatalf("wrapper: %v", err) + } + + builderHash, err := planner.HashFiles([]string{ + filepath.Join(repo, "builders", "linux", "build-php.sh"), + filepath.Join(repo, "builders", "common", "bundle-schema-version.env"), + filepath.Join(repo, "builders", "common", "capture-hermetic-libs.sh"), + filepath.Join(repo, "builders", "common", "pack-bundle.sh"), + }) + if err != nil { + t.Fatalf("hash builders: %v", err) + } + catalogBytes, err := planner.PerVersionYAMLFromFile(filepath.Join(repo, "catalog", "php.yaml"), "8.4") + if err != nil { + t.Fatalf("catalog bytes: %v", err) + } + cell := &planner.MatrixCell{Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts"} + want := planner.ComputeSpecHash(cell, catalogBytes, builderHash, "ubuntu-22.04") + if got != want { + t.Errorf("wrapper diverges from planner:\n wrapper = %s\n planner = %s", got, want) + } +} + +func TestComputeSpecHash_Ext_MatchesPlannerOutput(t *testing.T) { + repo := fakeRepo(t) + got, err := ComputeSpecHash(extIn(repo)) + if err != nil { + t.Fatalf("wrapper: %v", err) + } + + builderHash, err := planner.HashFiles([]string{ + filepath.Join(repo, "builders", "linux", "build-ext.sh"), + filepath.Join(repo, "builders", "common", "bundle-schema-version.env"), + filepath.Join(repo, "builders", "common", "capture-hermetic-libs.sh"), + filepath.Join(repo, "builders", "common", "pack-bundle.sh"), + }) + if err != nil { + t.Fatalf("hash builders: %v", err) + } + catalogBytes, err := planner.ExtensionYAMLFromFile(filepath.Join(repo, "catalog", "extensions", "redis.yaml")) + if err != nil { + t.Fatalf("catalog bytes: %v", err) + } + // Note: ext cells use ExtVer/PHPAbi/Extension — the planner leaves + // Version empty for ext entries. Matching that avoids silently changing + // the hash. + cell := &planner.MatrixCell{ + Extension: "redis", + ExtVer: "6.2.0", + PHPAbi: "8.4-nts", + OS: "linux", + Arch: "x86_64", + TS: "nts", + } + want := planner.ComputeSpecHash(cell, catalogBytes, builderHash, "ubuntu-22.04") + if got != want { + t.Errorf("wrapper diverges from planner:\n wrapper = %s\n planner = %s", got, want) + } +} + +func TestComputeSpecHash_UnknownKind_Errors(t *testing.T) { + repo := fakeRepo(t) + in := &SpecHashInputs{Kind: "tool", Name: "composer", Version: "2.7", Repo: repo} + if _, err := ComputeSpecHash(in); err == nil { + t.Error("unknown kind must error") + } +} + +func TestComputeSpecHash_MissingCatalog_Errors(t *testing.T) { + repo := fakeRepo(t) + // Delete the per-version catalog so load fails. + if err := os.Remove(filepath.Join(repo, "catalog", "php.yaml")); err != nil { + t.Fatalf("remove php.yaml: %v", err) + } + if _, err := ComputeSpecHash(phpIn(repo)); err == nil { + t.Error("missing catalog must error") + } +} + +func TestComputeSpecHash_MissingExtCatalog_Errors(t *testing.T) { + repo := fakeRepo(t) + if err := os.Remove(filepath.Join(repo, "catalog", "extensions", "redis.yaml")); err != nil { + t.Fatalf("remove redis.yaml: %v", err) + } + if _, err := ComputeSpecHash(extIn(repo)); err == nil { + t.Error("missing ext catalog must error") + } +} + +func TestComputeSpecHash_MissingBuilderOS_Errors(t *testing.T) { + repo := fakeRepo(t) + if err := os.Remove(filepath.Join(repo, "builders", "common", "builder-os.env")); err != nil { + t.Fatalf("remove builder-os.env: %v", err) + } + if _, err := ComputeSpecHash(phpIn(repo)); err == nil { + t.Error("missing builder-os.env must error") + } +} + +func TestComputeSpecHash_EmptyBuilderOS_Errors(t *testing.T) { + // Silent-fallback guard: if BUILDER_OS isn't found in the env file we + // must fail loudly, because emitting "" into the hash would make every + // lockfile entry look up-to-date but reflect the wrong runner. + repo := fakeRepo(t) + if err := os.WriteFile(filepath.Join(repo, "builders", "common", "builder-os.env"), + []byte("# no BUILDER_OS here\n"), 0o600); err != nil { + t.Fatalf("rewrite builder-os.env: %v", err) + } + if _, err := ComputeSpecHash(phpIn(repo)); err == nil { + t.Error("missing BUILDER_OS key must error") + } +} + +func TestComputeSpecHash_MissingBuilderScript_Errors(t *testing.T) { + repo := fakeRepo(t) + if err := os.Remove(filepath.Join(repo, "builders", "linux", "build-php.sh")); err != nil { + t.Fatalf("remove build-php.sh: %v", err) + } + if _, err := ComputeSpecHash(phpIn(repo)); err == nil { + t.Error("missing builder script must error") + } +} diff --git a/internal/planner/helpers.go b/internal/planner/helpers.go new file mode 100644 index 0000000..f3126f8 --- /dev/null +++ b/internal/planner/helpers.go @@ -0,0 +1,75 @@ +package planner + +import ( + "strings" + + "github.com/buildrush/setup-php/internal/catalog" +) + +// HashFiles computes a deterministic, order-sensitive hash string across the +// given files. The output is the colon-joined concatenation of each file's +// individual HashFile result, i.e. for paths=[a, b, c] it returns +// "sha256::sha256::sha256:". This is the same shape the planner has +// historically produced by manually concatenating HashFile calls, so using +// HashFiles in place of that inline code keeps spec_hash values byte-identical. +// +// Missing or unreadable files bail via HashFile's error semantics so callers +// cannot silently produce an empty prefix and skip rebuilds when a builder +// script has been deleted or moved. +func HashFiles(paths []string) (string, error) { + parts := make([]string, 0, len(paths)) + for _, p := range paths { + h, err := HashFile(p) + if err != nil { + return "", err + } + parts = append(parts, h) + } + return strings.Join(parts, ":"), nil +} + +// PerVersionYAMLFromFile loads the PHP catalog YAML at path and returns the +// marshaled YAML for a single version, suitable for hashing. It wraps +// catalog.LoadPHPSpec + PerVersionYAML so callers that have only a file path +// (e.g. phpup build) get the same byte-level output as the planner. Unknown +// versions are an error; see PerVersionYAML for the marshaling contract. +func PerVersionYAMLFromFile(path, version string) ([]byte, error) { + spec, err := catalog.LoadPHPSpec(path) + if err != nil { + return nil, err + } + return PerVersionYAML(spec, version) +} + +// ExtensionYAMLFromFile loads an extension catalog YAML at path and returns +// the marshaled YAML for hashing. It wraps catalog.LoadExtensionSpec + +// ExtensionYAML so callers that have only a file path get the same byte-level +// output as the planner. +func ExtensionYAMLFromFile(path string) ([]byte, error) { + spec, err := catalog.LoadExtensionSpec(path) + if err != nil { + return nil, err + } + return ExtensionYAML(spec) +} + +// ParseEnvValue scans a minimal .env file byte slice and returns the value for +// the given key, or "" if the key is absent. Lines are split on "\n"; the +// first line matching "=" (after trimming) wins. Blank lines and +// lines starting with "#" are ignored. Values are not unquoted — callers that +// need quote handling must do it themselves. This mirrors the parser that +// lived inline in cmd/planner/main.go for BUILDER_OS and is deliberately +// minimal; extend only with a test-driven reason. +func ParseEnvValue(data []byte, key string) string { + prefix := key + "=" + for _, line := range strings.Split(string(data), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + if strings.HasPrefix(trimmed, prefix) { + return strings.TrimPrefix(trimmed, prefix) + } + } + return "" +} diff --git a/internal/planner/helpers_test.go b/internal/planner/helpers_test.go new file mode 100644 index 0000000..7adfef4 --- /dev/null +++ b/internal/planner/helpers_test.go @@ -0,0 +1,258 @@ +package planner + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestHashFiles_Deterministic_AcrossCalls(t *testing.T) { + dir := t.TempDir() + a := filepath.Join(dir, "a.txt") + b := filepath.Join(dir, "b.txt") + if err := os.WriteFile(a, []byte("alpha"), 0o600); err != nil { + t.Fatalf("write a: %v", err) + } + if err := os.WriteFile(b, []byte("beta"), 0o600); err != nil { + t.Fatalf("write b: %v", err) + } + + h1, err := HashFiles([]string{a, b}) + if err != nil { + t.Fatalf("HashFiles 1: %v", err) + } + h2, err := HashFiles([]string{a, b}) + if err != nil { + t.Fatalf("HashFiles 2: %v", err) + } + if h1 != h2 { + t.Errorf("HashFiles not deterministic:\n%s\n---\n%s", h1, h2) + } +} + +// The planner relied historically on colon-joined per-file sha256 strings for +// the builder hash. HashFiles must reproduce that exact shape or spec_hash +// values would silently change across the refactor. +func TestHashFiles_MatchesColonJoinedHashFile(t *testing.T) { + dir := t.TempDir() + a := filepath.Join(dir, "a.txt") + b := filepath.Join(dir, "b.txt") + c := filepath.Join(dir, "c.txt") + if err := os.WriteFile(a, []byte("alpha"), 0o600); err != nil { + t.Fatalf("write a: %v", err) + } + if err := os.WriteFile(b, []byte("beta"), 0o600); err != nil { + t.Fatalf("write b: %v", err) + } + if err := os.WriteFile(c, []byte("gamma"), 0o600); err != nil { + t.Fatalf("write c: %v", err) + } + + ha, _ := HashFile(a) + hb, _ := HashFile(b) + hc, _ := HashFile(c) + want := ha + ":" + hb + ":" + hc + + got, err := HashFiles([]string{a, b, c}) + if err != nil { + t.Fatalf("HashFiles: %v", err) + } + if got != want { + t.Errorf("HashFiles = %q, want %q", got, want) + } +} + +func TestHashFiles_ChangesWhenFileChanges(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "f.txt") + if err := os.WriteFile(p, []byte("v1"), 0o600); err != nil { + t.Fatalf("write v1: %v", err) + } + h1, err := HashFiles([]string{p}) + if err != nil { + t.Fatalf("HashFiles v1: %v", err) + } + if err := os.WriteFile(p, []byte("v2"), 0o600); err != nil { + t.Fatalf("write v2: %v", err) + } + h2, err := HashFiles([]string{p}) + if err != nil { + t.Fatalf("HashFiles v2: %v", err) + } + if h1 == h2 { + t.Errorf("HashFiles should differ after mutation, both = %q", h1) + } +} + +func TestHashFiles_OrderSensitive(t *testing.T) { + dir := t.TempDir() + a := filepath.Join(dir, "a.txt") + b := filepath.Join(dir, "b.txt") + if err := os.WriteFile(a, []byte("alpha"), 0o600); err != nil { + t.Fatalf("write a: %v", err) + } + if err := os.WriteFile(b, []byte("beta"), 0o600); err != nil { + t.Fatalf("write b: %v", err) + } + hab, _ := HashFiles([]string{a, b}) + hba, _ := HashFiles([]string{b, a}) + if hab == hba { + t.Errorf("HashFiles should be order-sensitive, both = %q", hab) + } +} + +func TestHashFiles_MissingFile_Errors(t *testing.T) { + dir := t.TempDir() + if _, err := HashFiles([]string{filepath.Join(dir, "nope")}); err == nil { + t.Error("HashFiles should error on missing file") + } +} + +func TestPerVersionYAMLFromFile_ExtractsVersion(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "php.yaml") + content := `name: php +versions: + "8.3": + bundled_extensions: [core] + sources: + url: https://example/php-8.3.tar.xz + abi_matrix: + os: [linux] + arch: [x86_64] + ts: [nts] + "8.4": + bundled_extensions: [core, opcache] + sources: + url: https://example/php-8.4.tar.xz + abi_matrix: + os: [linux] + arch: [x86_64] + ts: [nts] +` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + got, err := PerVersionYAMLFromFile(path, "8.4") + if err != nil { + t.Fatalf("PerVersionYAMLFromFile: %v", err) + } + s := string(got) + // Per-version YAML contains only the one version's subtree — it should + // mention the url for 8.4 but not 8.3's url, since 8.3 is a sibling key + // at the parent that gets filtered out by the per-version marshaling. + if !strings.Contains(s, "php-8.4") { + t.Errorf("expected 8.4 url in output:\n%s", s) + } + if strings.Contains(s, "php-8.3") { + t.Errorf("per-version yaml must not leak sibling version:\n%s", s) + } +} + +func TestPerVersionYAMLFromFile_MissingVersion_Errors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "php.yaml") + content := `name: php +versions: + "8.4": + bundled_extensions: [core] +` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + if _, err := PerVersionYAMLFromFile(path, "9.9"); err == nil { + t.Error("missing version must error") + } +} + +func TestPerVersionYAMLFromFile_MissingFile_Errors(t *testing.T) { + dir := t.TempDir() + if _, err := PerVersionYAMLFromFile(filepath.Join(dir, "nope.yaml"), "8.4"); err == nil { + t.Error("missing file must error") + } +} + +func TestExtensionYAMLFromFile_ReadsRawBytes(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "redis.yaml") + content := `name: redis +kind: pecl +source: + pecl_package: redis +versions: + - "6.2.0" +abi_matrix: + php: ["8.4"] + os: ["linux"] + arch: ["x86_64"] + ts: ["nts"] +` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + got, err := ExtensionYAMLFromFile(path) + if err != nil { + t.Fatalf("ExtensionYAMLFromFile: %v", err) + } + s := string(got) + if !strings.Contains(s, "redis") { + t.Errorf("expected redis in marshaled yaml:\n%s", s) + } + if !strings.Contains(s, "6.2.0") { + t.Errorf("expected version in marshaled yaml:\n%s", s) + } +} + +func TestExtensionYAMLFromFile_MissingFile_Errors(t *testing.T) { + dir := t.TempDir() + if _, err := ExtensionYAMLFromFile(filepath.Join(dir, "nope.yaml")); err == nil { + t.Error("missing file must error") + } +} + +func TestParseEnvValue_FindsKey(t *testing.T) { + data := []byte("BUILDER_OS=ubuntu-22.04\n") + if got := ParseEnvValue(data, "BUILDER_OS"); got != "ubuntu-22.04" { + t.Errorf("ParseEnvValue = %q, want ubuntu-22.04", got) + } +} + +func TestParseEnvValue_MissingKey_ReturnsEmpty(t *testing.T) { + data := []byte("OTHER=value\n") + if got := ParseEnvValue(data, "BUILDER_OS"); got != "" { + t.Errorf("ParseEnvValue for missing key = %q, want empty", got) + } +} + +func TestParseEnvValue_IgnoresComments(t *testing.T) { + data := []byte("# BUILDER_OS=ignored\nBUILDER_OS=ubuntu-24.04\n") + if got := ParseEnvValue(data, "BUILDER_OS"); got != "ubuntu-24.04" { + t.Errorf("ParseEnvValue = %q, want ubuntu-24.04", got) + } +} + +func TestParseEnvValue_IgnoresBlankLines(t *testing.T) { + data := []byte("\n\nBUILDER_OS=ubuntu-22.04\n\n") + if got := ParseEnvValue(data, "BUILDER_OS"); got != "ubuntu-22.04" { + t.Errorf("ParseEnvValue = %q, want ubuntu-22.04", got) + } +} + +func TestParseEnvValue_FirstMatchWins(t *testing.T) { + data := []byte("BUILDER_OS=first\nBUILDER_OS=second\n") + if got := ParseEnvValue(data, "BUILDER_OS"); got != "first" { + t.Errorf("ParseEnvValue = %q, want first (first match wins)", got) + } +} + +func TestParseEnvValue_TrimsWhitespace(t *testing.T) { + // The inline planner parser used strings.TrimSpace before prefix-matching, + // so leading/trailing whitespace around the whole line should be tolerated. + data := []byte(" BUILDER_OS=ubuntu-22.04 \n") + if got := ParseEnvValue(data, "BUILDER_OS"); got != "ubuntu-22.04" { + t.Errorf("ParseEnvValue = %q, want ubuntu-22.04", got) + } +} From 4ea1737d1c53378cbdba9582b4e250114ecb68de Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 15:14:19 +0200 Subject: [PATCH 04/11] feat(build): internal/build.DockerRun with swappable runner Thin wrapper over `docker run --rm` that PR 2 Tasks 4-5 drive. A package-level RunnerFunc var is swappable via SetRunner(fn) (ret a restore callback) so unit tests don't need real docker to assert the argv phpup would build. The realDockerRun default goes through exec.CommandContext; env keys are sorted for deterministic argv. A gated smoke test exercises real docker with `alpine:3 echo hello`, skipped under -short or when docker is absent. --- internal/build/docker.go | 179 ++++++++++++++++++++++++++++++++++ internal/build/docker_test.go | 178 +++++++++++++++++++++++++++++++++ 2 files changed, 357 insertions(+) create mode 100644 internal/build/docker.go create mode 100644 internal/build/docker_test.go diff --git a/internal/build/docker.go b/internal/build/docker.go new file mode 100644 index 0000000..a65d9db --- /dev/null +++ b/internal/build/docker.go @@ -0,0 +1,179 @@ +package build + +import ( + "context" + "fmt" + "io" + "os" + "os/exec" + "sort" + "sync" +) + +// DockerRunOpts describes a single `docker run --rm` invocation. Fields map +// one-for-one onto docker CLI flags so the argv phpup builds is easy to audit +// in test assertions. +type DockerRunOpts struct { + // Image is the container image reference (required). Example: "ubuntu:22.04". + Image string + // Platform is the target platform override. Example: "linux/amd64". + // Empty string skips the --platform flag and lets docker pick the host default. + Platform string + // Network is a named docker network the container joins. Empty string + // skips the --network flag and uses docker's default bridge network. + Network string + // Mounts is the ordered list of volume binds. Caller order is preserved + // in argv so readability matches the caller's intent. + Mounts []Mount + // Env is the set of environment variables to export inside the container. + // Keys are sorted alphabetically when turned into argv so the -e flags + // appear in a deterministic order — useful for test assertions; docker + // itself doesn't care. + Env map[string]string + // Cmd is the argv passed after the image. Empty leaves the image's + // default entrypoint/cmd intact. + Cmd []string + // Stdout is where container stdout is streamed. Nil defaults to os.Stdout. + Stdout io.Writer + // Stderr is where container stderr is streamed. Nil defaults to os.Stderr. + Stderr io.Writer +} + +// Mount describes a single bind mount. Both Host and Container MUST be +// absolute paths; the caller is responsible for resolving relative inputs. +type Mount struct { + // Host is the absolute host-side path (required). + Host string + // Container is the absolute container-side path (required). + Container string + // ReadOnly appends ":ro" to the bind spec when true. + ReadOnly bool +} + +// RunnerFunc runs a single docker invocation. Tests substitute a fake via +// SetRunner to assert the argv phpup would construct without needing real +// docker in the test environment. +type RunnerFunc func(ctx context.Context, opts DockerRunOpts) error + +// runnerMu protects defaultRunner during SetRunner swaps. Swaps are rare +// (test setup/teardown only) so the mutex cost is negligible, but it rules +// out a data race when two tests happen to swap concurrently. That said, +// tests that call SetRunner MUST NOT use t.Parallel() because the runner +// is a package-level global: a swap in one test would leak into another. +var runnerMu sync.Mutex + +// defaultRunner is the RunnerFunc DockerRun dispatches through when no test +// has overridden it via SetRunner. The lambda adapts the value-type +// RunnerFunc signature (pinned by the public API) to realDockerRun's +// pointer-receiver shape so the underlying function doesn't need to copy +// the 136-byte DockerRunOpts on every call. +var defaultRunner RunnerFunc = func(ctx context.Context, opts DockerRunOpts) error { + return realDockerRun(ctx, &opts) +} + +// DockerRun dispatches the invocation through the currently-installed +// runner. Production callers go through the default realDockerRun; tests +// call SetRunner first to install a fake. +// +//nolint:gocritic // hugeParam: RunnerFunc signature takes DockerRunOpts by value by design — the tests in docker_test.go assign recorder.got = opts without dereferencing, and the per-call 136-byte copy is negligible vs. a docker invocation's seconds-scale runtime. +func DockerRun(ctx context.Context, opts DockerRunOpts) error { + runnerMu.Lock() + r := defaultRunner + runnerMu.Unlock() + return r(ctx, opts) +} + +// SetRunner swaps the package-level RunnerFunc and returns a restore +// function that callers MUST defer to revert. The typical test pattern is: +// +// restore := SetRunner(myFake) +// defer restore() +// +// The package-level state means tests that call SetRunner must not run in +// parallel — they share one global runner. +func SetRunner(r RunnerFunc) func() { + runnerMu.Lock() + prev := defaultRunner + defaultRunner = r + runnerMu.Unlock() + return func() { + runnerMu.Lock() + defaultRunner = prev + runnerMu.Unlock() + } +} + +// realDockerRun shells out to the `docker` binary via exec.CommandContext +// and streams stdio to the caller-provided writers (defaulting to +// os.Stdout/Stderr). Takes a pointer so the 136-byte DockerRunOpts doesn't +// need to be copied per invocation; the adapter in defaultRunner converts +// from the value-type RunnerFunc shape. Context cancellation relies on +// exec.CommandContext's built-in SIGKILL behaviour. +func realDockerRun(ctx context.Context, opts *DockerRunOpts) error { + if opts.Image == "" { + return fmt.Errorf("DockerRun: Image is required") + } + args := argvFor(opts) + //nolint:gosec // G204: args come exclusively from typed DockerRunOpts fields above; callers are first-party Go code (Task 4/5), no shell interpretation occurs. + cmd := exec.CommandContext(ctx, "docker", args...) + cmd.Stdout = coalesceWriter(opts.Stdout, os.Stdout) + cmd.Stderr = coalesceWriter(opts.Stderr, os.Stderr) + if err := cmd.Run(); err != nil { + return fmt.Errorf("DockerRun %s: %w", opts.Image, err) + } + return nil +} + +// argvFor returns the exact argv (minus the leading "docker") that +// realDockerRun would pass to exec.CommandContext. Extracted as a pure +// function so unit tests can assert on it without invoking exec. +// Takes a pointer so callers can assert on large opts without copy cost; +// the function reads the struct without mutating it. +func argvFor(opts *DockerRunOpts) []string { + // Preallocate with a conservative estimate: "run" + "--rm" + two per + // platform/network, two per mount, two per env, image, plus Cmd. + args := make([]string, 0, 2+4+2*len(opts.Mounts)+2*len(opts.Env)+1+len(opts.Cmd)) + args = append(args, "run", "--rm") + if opts.Platform != "" { + args = append(args, "--platform", opts.Platform) + } + if opts.Network != "" { + args = append(args, "--network", opts.Network) + } + for _, m := range opts.Mounts { + spec := m.Host + ":" + m.Container + if m.ReadOnly { + spec += ":ro" + } + args = append(args, "-v", spec) + } + for _, k := range sortedKeys(opts.Env) { + args = append(args, "-e", k+"="+opts.Env[k]) + } + args = append(args, opts.Image) + args = append(args, opts.Cmd...) + return args +} + +// sortedKeys returns the map's keys in alphabetical order. Empty and nil +// maps both produce a nil slice, which append handles fine. +func sortedKeys(m map[string]string) []string { + if len(m) == 0 { + return nil + } + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// coalesceWriter returns w if non-nil, else fallback. Used to let callers +// opt into a custom destination while defaulting to the process's stdio. +func coalesceWriter(w, fallback io.Writer) io.Writer { + if w != nil { + return w + } + return fallback +} diff --git a/internal/build/docker_test.go b/internal/build/docker_test.go new file mode 100644 index 0000000..84e2718 --- /dev/null +++ b/internal/build/docker_test.go @@ -0,0 +1,178 @@ +package build + +import ( + "context" + "errors" + "os/exec" + "reflect" + "strings" + "testing" + "time" +) + +// recorder is a RunnerFunc test double that captures the last DockerRunOpts +// it received. Tests install it via SetRunner to assert the argv phpup would +// build without invoking real docker. The run closure is defined inline in +// each test (rather than as a method on *recorder) so the value-type opts +// parameter required by RunnerFunc doesn't hit gocritic's hugeParam linter. +type recorder struct { + got DockerRunOpts + err error +} + +// asRunner returns a RunnerFunc closure that captures into r and returns r.err. +func (r *recorder) asRunner() RunnerFunc { + return func(_ context.Context, opts DockerRunOpts) error { + r.got = opts + return r.err + } +} + +// TestDockerRun_PropagatesOptsToRunner verifies that DockerRun passes every +// field of the opts struct to the installed runner unchanged. This is the +// foundational guarantee Task 4/5 rely on when asserting the argv phpup +// would construct for a given build. +func TestDockerRun_PropagatesOptsToRunner(t *testing.T) { + r := &recorder{} + restore := SetRunner(r.asRunner()) + defer restore() + + opts := DockerRunOpts{ + Image: "ubuntu:22.04", + Platform: "linux/arm64", + Network: "test-net", + Mounts: []Mount{ + {Host: "/a", Container: "/mnt/a", ReadOnly: true}, + {Host: "/b", Container: "/mnt/b"}, + }, + Env: map[string]string{"FOO": "bar", "BAZ": "qux"}, + Cmd: []string{"bash", "-c", "echo hi"}, + } + if err := DockerRun(context.Background(), opts); err != nil { + t.Fatalf("DockerRun: %v", err) + } + if !reflect.DeepEqual(r.got, opts) { + t.Errorf("recorder.got = %+v, want %+v", r.got, opts) + } +} + +// TestDockerRun_PropagatesError verifies that an error returned by the +// installed runner flows back through DockerRun unchanged. Task 4/5 rely on +// this so build failures surface to the CLI caller. +func TestDockerRun_PropagatesError(t *testing.T) { + r := &recorder{err: errors.New("boom")} + restore := SetRunner(r.asRunner()) + defer restore() + + err := DockerRun(context.Background(), DockerRunOpts{Image: "alpine:3"}) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatalf("err = %v, want containing \"boom\"", err) + } +} + +// TestDockerRun_ContextCancellation verifies that DockerRun returns promptly +// once the caller cancels the context, even when the underlying runner would +// otherwise block indefinitely. The real runner relies on +// exec.CommandContext's SIGKILL behaviour; here we simulate that with a +// runner that blocks on ctx.Done and returns ctx.Err. +func TestDockerRun_ContextCancellation(t *testing.T) { + slow := func(ctx context.Context, _ DockerRunOpts) error { + <-ctx.Done() + return ctx.Err() + } + restore := SetRunner(slow) + defer restore() + + ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) + go func() { errCh <- DockerRun(ctx, DockerRunOpts{Image: "alpine:3"}) }() + cancel() + select { + case err := <-errCh: + if !errors.Is(err, context.Canceled) { + t.Errorf("err = %v, want context.Canceled", err) + } + case <-time.After(2 * time.Second): + t.Fatal("DockerRun did not return after cancel within 2s") + } +} + +// TestRealDockerRun_BuildsExpectedArgs asserts the exact argv phpup's real +// runner would pass to `docker`. argvFor is extracted precisely so this +// test can exercise argv construction without invoking exec. The expected +// ordering — run/--rm first, flags in field order, env sorted, image, cmd — +// is the contract Task 4/5 depend on. +func TestRealDockerRun_BuildsExpectedArgs(t *testing.T) { + args := argvFor(&DockerRunOpts{ + Image: "ubuntu:22.04", + Platform: "linux/amd64", + Network: "build-net", + Mounts: []Mount{ + {Host: "/src", Container: "/workspace", ReadOnly: true}, + {Host: "/out", Container: "/tmp"}, + }, + Env: map[string]string{"Z": "last", "A": "first"}, + Cmd: []string{"bash", "-c", "echo hi"}, + }) + want := []string{ + "run", "--rm", + "--platform", "linux/amd64", + "--network", "build-net", + "-v", "/src:/workspace:ro", + "-v", "/out:/tmp", + "-e", "A=first", // env keys sorted alphabetically + "-e", "Z=last", + "ubuntu:22.04", + "bash", "-c", "echo hi", + } + if !reflect.DeepEqual(args, want) { + t.Errorf("argv mismatch\n got = %v\nwant = %v", args, want) + } +} + +// TestRealDockerRun_BuildsMinimalArgs asserts that optional flags (platform, +// network, mounts, env, cmd) are omitted when their corresponding +// DockerRunOpts fields are zero. Verifies argvFor doesn't synthesise an +// empty --platform flag value or similar garbage. +func TestRealDockerRun_BuildsMinimalArgs(t *testing.T) { + args := argvFor(&DockerRunOpts{Image: "alpine:3"}) + want := []string{"run", "--rm", "alpine:3"} + if !reflect.DeepEqual(args, want) { + t.Errorf("argv mismatch\n got = %v\nwant = %v", args, want) + } +} + +// TestRealDockerRun_ImageRequired asserts the guardrail on the real runner: +// an empty Image is a programmer error, not a docker error to pipe through. +func TestRealDockerRun_ImageRequired(t *testing.T) { + err := realDockerRun(context.Background(), &DockerRunOpts{}) + if err == nil || !strings.Contains(err.Error(), "Image is required") { + t.Errorf("err = %v, want containing \"Image is required\"", err) + } +} + +// TestRealDockerRun_SmokeIntegration exercises the real docker binary end to +// end with a tiny `alpine:3 echo hello` invocation. Skipped under -short and +// when docker isn't on PATH, so CI without docker is unaffected. +func TestRealDockerRun_SmokeIntegration(t *testing.T) { + if testing.Short() { + t.Skip("skipping real docker smoke under -short") + } + if _, err := exec.LookPath("docker"); err != nil { + t.Skipf("docker not found in PATH: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + out := &strings.Builder{} + err := DockerRun(ctx, DockerRunOpts{ + Image: "alpine:3", + Cmd: []string{"echo", "hello"}, + Stdout: out, + }) + if err != nil { + t.Fatalf("DockerRun: %v", err) + } + if !strings.Contains(out.String(), "hello") { + t.Errorf("stdout = %q, want contains \"hello\"", out.String()) + } +} From d997b02182f8217ff0660aa84dffae55c43a5459 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 15:19:30 +0200 Subject: [PATCH 05/11] fix(build): convert DockerRun to pointer-receiver; keep only the G204 false positive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gocritic:hugeParam on DockerRun wasn't a false positive — the ~136B DockerRunOpts is a real large-struct copy. Switch the public API to *DockerRunOpts (and RunnerFunc accordingly), eliminating the //nolint:gocritic suppression entirely. gosec:G204 on exec.CommandContext IS a genuine false positive: CommandContext passes argv directly to execve(2) without shell interpretation, and all argv strings come from typed DockerRunOpts fields. Keep the //nolint:gosec with a tightened justification invoking the false-positive criterion rather than prior-art config exclusions. --- internal/build/docker.go | 27 +++++++++------------------ internal/build/docker_test.go | 24 ++++++++++++------------ 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/internal/build/docker.go b/internal/build/docker.go index a65d9db..fa9f68e 100644 --- a/internal/build/docker.go +++ b/internal/build/docker.go @@ -52,8 +52,9 @@ type Mount struct { // RunnerFunc runs a single docker invocation. Tests substitute a fake via // SetRunner to assert the argv phpup would construct without needing real -// docker in the test environment. -type RunnerFunc func(ctx context.Context, opts DockerRunOpts) error +// docker in the test environment. Takes a pointer so the ~136-byte +// DockerRunOpts isn't copied on every call. +type RunnerFunc func(ctx context.Context, opts *DockerRunOpts) error // runnerMu protects defaultRunner during SetRunner swaps. Swaps are rare // (test setup/teardown only) so the mutex cost is negligible, but it rules @@ -63,20 +64,13 @@ type RunnerFunc func(ctx context.Context, opts DockerRunOpts) error var runnerMu sync.Mutex // defaultRunner is the RunnerFunc DockerRun dispatches through when no test -// has overridden it via SetRunner. The lambda adapts the value-type -// RunnerFunc signature (pinned by the public API) to realDockerRun's -// pointer-receiver shape so the underlying function doesn't need to copy -// the 136-byte DockerRunOpts on every call. -var defaultRunner RunnerFunc = func(ctx context.Context, opts DockerRunOpts) error { - return realDockerRun(ctx, &opts) -} +// has overridden it via SetRunner. +var defaultRunner RunnerFunc = realDockerRun // DockerRun dispatches the invocation through the currently-installed // runner. Production callers go through the default realDockerRun; tests // call SetRunner first to install a fake. -// -//nolint:gocritic // hugeParam: RunnerFunc signature takes DockerRunOpts by value by design — the tests in docker_test.go assign recorder.got = opts without dereferencing, and the per-call 136-byte copy is negligible vs. a docker invocation's seconds-scale runtime. -func DockerRun(ctx context.Context, opts DockerRunOpts) error { +func DockerRun(ctx context.Context, opts *DockerRunOpts) error { runnerMu.Lock() r := defaultRunner runnerMu.Unlock() @@ -105,17 +99,14 @@ func SetRunner(r RunnerFunc) func() { // realDockerRun shells out to the `docker` binary via exec.CommandContext // and streams stdio to the caller-provided writers (defaulting to -// os.Stdout/Stderr). Takes a pointer so the 136-byte DockerRunOpts doesn't -// need to be copied per invocation; the adapter in defaultRunner converts -// from the value-type RunnerFunc shape. Context cancellation relies on -// exec.CommandContext's built-in SIGKILL behaviour. +// os.Stdout/Stderr). Context cancellation relies on exec.CommandContext's +// built-in SIGKILL behaviour. func realDockerRun(ctx context.Context, opts *DockerRunOpts) error { if opts.Image == "" { return fmt.Errorf("DockerRun: Image is required") } args := argvFor(opts) - //nolint:gosec // G204: args come exclusively from typed DockerRunOpts fields above; callers are first-party Go code (Task 4/5), no shell interpretation occurs. - cmd := exec.CommandContext(ctx, "docker", args...) + cmd := exec.CommandContext(ctx, "docker", args...) //nolint:gosec // G204 false positive: exec.CommandContext passes argv directly to execve(2) (no shell), and all args come from typed DockerRunOpts fields assembled by internal callers — the wrapper's purpose is precisely to spawn docker with dynamic argv, so a fixed argv is impossible by design. cmd.Stdout = coalesceWriter(opts.Stdout, os.Stdout) cmd.Stderr = coalesceWriter(opts.Stderr, os.Stderr) if err := cmd.Run(); err != nil { diff --git a/internal/build/docker_test.go b/internal/build/docker_test.go index 84e2718..75acfdf 100644 --- a/internal/build/docker_test.go +++ b/internal/build/docker_test.go @@ -12,9 +12,9 @@ import ( // recorder is a RunnerFunc test double that captures the last DockerRunOpts // it received. Tests install it via SetRunner to assert the argv phpup would -// build without invoking real docker. The run closure is defined inline in -// each test (rather than as a method on *recorder) so the value-type opts -// parameter required by RunnerFunc doesn't hit gocritic's hugeParam linter. +// build without invoking real docker. The captured value is stored by +// dereference rather than by pointer so assertions remain immune to any +// post-return mutation of the caller's opts struct. type recorder struct { got DockerRunOpts err error @@ -22,8 +22,8 @@ type recorder struct { // asRunner returns a RunnerFunc closure that captures into r and returns r.err. func (r *recorder) asRunner() RunnerFunc { - return func(_ context.Context, opts DockerRunOpts) error { - r.got = opts + return func(_ context.Context, opts *DockerRunOpts) error { + r.got = *opts return r.err } } @@ -37,7 +37,7 @@ func TestDockerRun_PropagatesOptsToRunner(t *testing.T) { restore := SetRunner(r.asRunner()) defer restore() - opts := DockerRunOpts{ + opts := &DockerRunOpts{ Image: "ubuntu:22.04", Platform: "linux/arm64", Network: "test-net", @@ -51,8 +51,8 @@ func TestDockerRun_PropagatesOptsToRunner(t *testing.T) { if err := DockerRun(context.Background(), opts); err != nil { t.Fatalf("DockerRun: %v", err) } - if !reflect.DeepEqual(r.got, opts) { - t.Errorf("recorder.got = %+v, want %+v", r.got, opts) + if !reflect.DeepEqual(r.got, *opts) { + t.Errorf("recorder.got = %+v, want %+v", r.got, *opts) } } @@ -64,7 +64,7 @@ func TestDockerRun_PropagatesError(t *testing.T) { restore := SetRunner(r.asRunner()) defer restore() - err := DockerRun(context.Background(), DockerRunOpts{Image: "alpine:3"}) + err := DockerRun(context.Background(), &DockerRunOpts{Image: "alpine:3"}) if err == nil || !strings.Contains(err.Error(), "boom") { t.Fatalf("err = %v, want containing \"boom\"", err) } @@ -76,7 +76,7 @@ func TestDockerRun_PropagatesError(t *testing.T) { // exec.CommandContext's SIGKILL behaviour; here we simulate that with a // runner that blocks on ctx.Done and returns ctx.Err. func TestDockerRun_ContextCancellation(t *testing.T) { - slow := func(ctx context.Context, _ DockerRunOpts) error { + slow := func(ctx context.Context, _ *DockerRunOpts) error { <-ctx.Done() return ctx.Err() } @@ -85,7 +85,7 @@ func TestDockerRun_ContextCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) errCh := make(chan error, 1) - go func() { errCh <- DockerRun(ctx, DockerRunOpts{Image: "alpine:3"}) }() + go func() { errCh <- DockerRun(ctx, &DockerRunOpts{Image: "alpine:3"}) }() cancel() select { case err := <-errCh: @@ -164,7 +164,7 @@ func TestRealDockerRun_SmokeIntegration(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() out := &strings.Builder{} - err := DockerRun(ctx, DockerRunOpts{ + err := DockerRun(ctx, &DockerRunOpts{ Image: "alpine:3", Cmd: []string{"echo", "hello"}, Stdout: out, From 966d62ac556a1066333b141e0197db0f00922428 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 15:26:39 +0200 Subject: [PATCH 06/11] feat(build): phpup build php wraps builders/linux/build-php.sh in docker phpup build php [--php 8.4] [--os jammy] [--arch x86_64] [--ts nts] [--registry oci-layout:./out/oci-layout] [--repo .] Computes the spec-hash from the same builder + catalog inputs the planner uses, probes the target registry with LookupBySpec, and short-circuits on hit. On miss: spins up a bare ubuntu:22.04 (jammy) or ubuntu:24.04 (noble) container under the requested platform (linux/amd64 or linux/arm64), mounts the repo read-only + a tempdir output mount, runs builders/linux/build-php.sh unchanged, reads the resulting bundle.tar.zst + meta.json, and Pushes to the registry with bundle-name + spec-hash annotations. Callers with a remote-only registry (no oci-layout cache) are tolerated: LookupBySpec's ErrUnsupported is treated as a soft miss. BuildExt is declared as a stub returning an explanatory error until PR 2 Task 5 implements it. --- cmd/phpup/main.go | 10 ++ internal/build/build.go | 238 ++++++++++++++++++++++++++++++++ internal/build/build_test.go | 254 +++++++++++++++++++++++++++++++++++ 3 files changed, 502 insertions(+) create mode 100644 internal/build/build.go create mode 100644 internal/build/build_test.go diff --git a/cmd/phpup/main.go b/cmd/phpup/main.go index 0799a6e..f89bb9e 100644 --- a/cmd/phpup/main.go +++ b/cmd/phpup/main.go @@ -14,6 +14,7 @@ import ( "sort" "strings" + "github.com/buildrush/setup-php/internal/build" "github.com/buildrush/setup-php/internal/cache" "github.com/buildrush/setup-php/internal/catalog" "github.com/buildrush/setup-php/internal/compat" @@ -54,6 +55,15 @@ func main() { return } + // `phpup build …` is dispatched before the setup-flow flag.Parse so the + // two argv universes never collide. build.Main uses its own FlagSet. + if len(os.Args) > 1 && os.Args[1] == "build" { + if err := build.Main(os.Args[2:]); err != nil { + log.Fatalf("%v", err) + } + return + } + registryFlag := flag.String("registry", "", "OCI artifact store (e.g., ghcr.io/buildrush or oci-layout:./out/oci-layout). Overrides INPUT_REGISTRY / PHPUP_REGISTRY; defaults to ghcr.io/buildrush.") flag.Parse() diff --git a/internal/build/build.go b/internal/build/build.go new file mode 100644 index 0000000..9b5cdbd --- /dev/null +++ b/internal/build/build.go @@ -0,0 +1,238 @@ +package build + +import ( + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/buildrush/setup-php/internal/registry" +) + +// Main is the entry point for `phpup build …`. args is the tail after +// the "build" subcommand token (so args[0] is "php" or "ext"). Returning +// a nil error means the requested build (or cache hit) succeeded; a +// non-nil error is safe to pass straight to log.Fatalf. +func Main(args []string) error { + if len(args) == 0 { + return errors.New("phpup build: usage: phpup build (php|ext) [flags]") + } + ctx := context.Background() + switch args[0] { + case "php": + return BuildPHP(ctx, args[1:]) + case "ext": + return BuildExt(ctx, args[1:]) + default: + return fmt.Errorf("phpup build: unknown kind %q (want php or ext)", args[0]) + } +} + +// BuildPHP runs the php-core build end to end. args is everything after +// "build php" (flags). Returns nil on success, or an error with the +// "phpup build php: " prefix the CLI dispatcher expects. +func BuildPHP(ctx context.Context, args []string) error { + opts, err := parsePHPFlags(args) + if err != nil { + return err + } + + // 1. Spec-hash. + specHash, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "php", + Version: opts.Version, + OS: "linux", + Arch: opts.Arch, + TS: opts.TS, + Repo: opts.Repo, + }) + if err != nil { + return fmt.Errorf("phpup build php: %w", err) + } + + // 2. Open target store + cache-probe. + store, err := registry.Open(opts.Registry) + if err != nil { + return fmt.Errorf("phpup build php: open registry: %w", err) + } + // Remote backends return ErrUnsupported for LookupBySpec; treat that + // as a soft miss so callers without an oci-layout cache fall through + // to building. Hard errors from a layout backend still propagate. + ref, hit, err := store.LookupBySpec(ctx, "php-core", specHash) + if errors.Is(err, registry.ErrUnsupported) { + hit, err = false, nil + } + if err != nil { + return fmt.Errorf("phpup build php: lookup by spec: %w", err) + } + if hit { + fmt.Printf("phpup build php: cache hit %s (spec-hash %s)\n", ref.Digest, specHash) + return nil + } + + // 3. Prepare output mount dir. + outDir, err := os.MkdirTemp("", "phpup-build-php-*") + if err != nil { + return fmt.Errorf("phpup build php: mktemp: %w", err) + } + defer func() { _ = os.RemoveAll(outDir) }() + + // 4. Invoke builder in docker. + image, err := ubuntuImage(opts.OS) + if err != nil { + return fmt.Errorf("phpup build php: %w", err) + } + platform, err := dockerPlatform(opts.Arch) + if err != nil { + return fmt.Errorf("phpup build php: %w", err) + } + runOpts := &DockerRunOpts{ + Image: image, + Platform: platform, + Mounts: []Mount{ + {Host: opts.Repo, Container: "/workspace", ReadOnly: true}, + {Host: outDir, Container: "/tmp/out", ReadOnly: false}, + }, + Env: map[string]string{ + "PHP_VERSION": opts.Version, + "ARCH": opts.Arch, + "OUTPUT_DIR": "/tmp/out", + "WORKSPACE": "/workspace", + }, + Cmd: []string{"bash", "-c", + "apt-get update >/dev/null 2>&1 && " + + "apt-get install -y --no-install-recommends curl xz-utils ca-certificates >/dev/null 2>&1 && " + + "/workspace/builders/linux/build-php.sh"}, + } + if err := DockerRun(ctx, runOpts); err != nil { + return fmt.Errorf("phpup build php: docker: %w", err) + } + + // 5. Read the bundle + meta from the mount dir. + bundlePath := filepath.Join(outDir, "bundle.tar.zst") + metaPath := filepath.Join(outDir, "meta.json") + bundle, err := os.Open(filepath.Clean(bundlePath)) + if err != nil { + return fmt.Errorf("phpup build php: open bundle: %w", err) + } + defer func() { _ = bundle.Close() }() + meta, err := parseMetaJSONFile(metaPath) + if err != nil { + return fmt.Errorf("phpup build php: parse meta.json: %w", err) + } + + // 6. Push to the store. + pushRef := registry.Ref{Name: "php-core"} + ann := registry.Annotations{BundleName: "php-core", SpecHash: specHash} + if err := store.Push(ctx, pushRef, bundle, meta, ann); err != nil { + return fmt.Errorf("phpup build php: push bundle: %w", err) + } + + fmt.Printf("phpup build php: built and pushed php-core (spec-hash %s) to %s\n", specHash, opts.Registry) + return nil +} + +// BuildExt is declared but not implemented yet; it lands in Task 5. The +// stub returns a recognisable error so Main's dispatch compiles and +// callers see a clear "not yet" rather than an obscure panic. +func BuildExt(_ context.Context, _ []string) error { + return errors.New("phpup build ext: implemented in PR2 Task 5") +} + +// phpOpts is the parsed flag set for `phpup build php`. Repo is resolved +// to an absolute path during parsing so downstream code (spec-hash, docker +// bind mount) can use it directly without re-resolving. +type phpOpts struct { + Version string // "8.4" + OS string // "jammy" or "noble" → maps to ubuntu:22.04 / ubuntu:24.04 + Arch string // "x86_64" or "aarch64" → maps to linux/amd64 / linux/arm64 + TS string // "nts" or "zts" + Registry string // "oci-layout:./out/oci-layout" or "ghcr.io/..." + Repo string // absolute path to setup-php repo root +} + +// parsePHPFlags parses the flag tail for `phpup build php`. The FlagSet +// uses ContinueOnError so callers get back an error instead of a process +// exit — makes the surface testable without os.Exit acrobatics. +func parsePHPFlags(args []string) (*phpOpts, error) { + fs := flag.NewFlagSet("phpup build php", flag.ContinueOnError) + version := fs.String("php", "", "PHP version, e.g. 8.4 (required)") + osFlag := fs.String("os", "jammy", "Ubuntu flavour: jammy (22.04) or noble (24.04)") + arch := fs.String("arch", "x86_64", "Target arch: x86_64 or aarch64") + ts := fs.String("ts", "nts", "Thread safety: nts or zts") + registryFlag := fs.String("registry", "oci-layout:./out/oci-layout", + "Target registry URI (oci-layout: or ghcr.io/)") + repo := fs.String("repo", ".", "Path to setup-php repo root") + if err := fs.Parse(args); err != nil { + return nil, err + } + if *version == "" { + return nil, errors.New("phpup build php: --php is required") + } + absRepo, err := filepath.Abs(*repo) + if err != nil { + return nil, fmt.Errorf("phpup build php: resolve repo path: %w", err) + } + return &phpOpts{ + Version: *version, + OS: *osFlag, + Arch: *arch, + TS: *ts, + Registry: *registryFlag, + Repo: absRepo, + }, nil +} + +// ubuntuImage maps a short OS flavour name onto the concrete docker image +// tag that builders/linux/build-php.sh expects. Accepts both the short +// ("jammy"/"noble") and long ("ubuntu-22.04"/"ubuntu-24.04") spellings +// because the planner emits the long form and humans tend to type the +// short form; either is unambiguous. +func ubuntuImage(osFlag string) (string, error) { + switch strings.ToLower(osFlag) { + case "jammy", "ubuntu-22.04": + return "ubuntu:22.04", nil + case "noble", "ubuntu-24.04": + return "ubuntu:24.04", nil + default: + return "", fmt.Errorf("unknown os %q (want jammy|noble)", osFlag) + } +} + +// dockerPlatform maps the caller-facing arch name onto the docker +// --platform value. The caller-facing names match the builder script's +// ARCH env contract ("x86_64"/"aarch64"); the docker aliases ("amd64"/ +// "arm64") are accepted too for ergonomics. +func dockerPlatform(arch string) (string, error) { + switch arch { + case "x86_64", "amd64": + return "linux/amd64", nil + case "aarch64", "arm64": + return "linux/arm64", nil + default: + return "", fmt.Errorf("unknown arch %q (want x86_64|aarch64)", arch) + } +} + +// parseMetaJSONFile reads the builder's meta.json sidecar into a +// registry.Meta. Missing schema_version defaults to 1 to match +// internal/registry/layout.go's legacy-bundle tolerance; callers should +// not rely on this default — the builder writes the real version. +func parseMetaJSONFile(path string) (*registry.Meta, error) { + data, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + return nil, err + } + var m registry.Meta + if err := json.Unmarshal(data, &m); err != nil { + return nil, err + } + if m.SchemaVersion == 0 { + m.SchemaVersion = 1 + } + return &m, nil +} diff --git a/internal/build/build_test.go b/internal/build/build_test.go new file mode 100644 index 0000000..39486d2 --- /dev/null +++ b/internal/build/build_test.go @@ -0,0 +1,254 @@ +package build + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/buildrush/setup-php/internal/registry" +) + +// writeRepoFixture creates a minimal on-disk repo skeleton under dir so +// ComputeSpecHash can find the files it needs during the test. The files +// are stand-ins — the builder scripts are single-line no-ops, but their +// contents still participate in the spec-hash so a realistic structure is +// required for cache-hit probes to line up with cache-miss builds. +func writeRepoFixture(t *testing.T, dir string) { + t.Helper() + mustWrite := func(rel, content string) { + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", rel, err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", rel, err) + } + } + mustWrite("builders/linux/build-php.sh", "#!/bin/bash\nexit 0\n") + mustWrite("builders/linux/build-ext.sh", "#!/bin/bash\nexit 0\n") + mustWrite("builders/common/bundle-schema-version.env", "SCHEMA_VERSION=3\n") + mustWrite("builders/common/capture-hermetic-libs.sh", "#!/bin/bash\n") + mustWrite("builders/common/pack-bundle.sh", "#!/bin/bash\n") + mustWrite("builders/common/fetch-core.sh", "#!/bin/bash\n") + mustWrite("builders/common/builder-os.env", "BUILDER_OS=ubuntu-22.04\n") + mustWrite("catalog/php.yaml", "versions:\n \"8.4\":\n sources:\n url: https://example.com/php-8.4.0.tar.xz\n") + mustWrite("catalog/extensions/redis.yaml", "name: redis\nversions:\n - \"6.2.0\"\n") +} + +// seedLayout pushes a manifest into an oci-layout so BuildPHP's cache +// probe returns a hit. Returns the registry URI pointing at the layout. +func seedLayout(t *testing.T, dir, bundleName, specHash string) string { + t.Helper() + layoutURI := "oci-layout:" + dir + s, err := registry.Open(layoutURI) + if err != nil { + t.Fatalf("open layout: %v", err) + } + err = s.Push(context.Background(), registry.Ref{Name: bundleName}, + bytes.NewReader([]byte("fake")), nil, + registry.Annotations{BundleName: bundleName, SpecHash: specHash}) + if err != nil { + t.Fatalf("seed layout: %v", err) + } + return layoutURI +} + +// fakeRunner is a RunnerFunc that writes a valid bundle + meta.json into +// the output mount so BuildPHP's read-push step can proceed without a +// real docker invocation. +func fakeRunner(bundleBytes []byte) RunnerFunc { + return func(_ context.Context, opts *DockerRunOpts) error { + var outHost string + for _, m := range opts.Mounts { + if m.Container == "/tmp/out" { + outHost = m.Host + break + } + } + if outHost == "" { + return errors.New("fakeRunner: no /tmp/out mount") + } + if err := os.WriteFile(filepath.Join(outHost, "bundle.tar.zst"), bundleBytes, 0o644); err != nil { + return err + } + meta := map[string]any{"schema_version": 3, "kind": "php-core"} + mjson, err := json.Marshal(meta) + if err != nil { + return err + } + return os.WriteFile(filepath.Join(outHost, "meta.json"), mjson, 0o644) + } +} + +func TestBuildPHP_CacheHit_ShortCircuitsWithoutRunning(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutDir := filepath.Join(t.TempDir(), "layout") + + hash, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "php", Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts", Repo: repo, + }) + if err != nil { + t.Fatalf("ComputeSpecHash: %v", err) + } + layoutURI := seedLayout(t, layoutDir, "php-core", hash) + + var called bool + restore := SetRunner(func(_ context.Context, _ *DockerRunOpts) error { + called = true + return errors.New("runner should not be called on cache hit") + }) + defer restore() + + out := captureStdout(t, func() { + err = BuildPHP(context.Background(), []string{ + "--php", "8.4", + "--registry", layoutURI, + "--repo", repo, + }) + }) + if err != nil { + t.Fatalf("BuildPHP: %v", err) + } + if called { + t.Fatal("runner was called on cache hit") + } + if !strings.Contains(out, "cache hit") { + t.Errorf("stdout = %q, want contains \"cache hit\"", out) + } +} + +func TestBuildPHP_CacheMiss_InvokesRunnerThenPushes(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutDir := filepath.Join(t.TempDir(), "layout") + layoutURI := "oci-layout:" + layoutDir + + restore := SetRunner(fakeRunner([]byte("synthetic-bundle"))) + defer restore() + + err := BuildPHP(context.Background(), []string{ + "--php", "8.4", + "--registry", layoutURI, + "--repo", repo, + }) + if err != nil { + t.Fatalf("BuildPHP: %v", err) + } + + // Verify the layout now contains a manifest annotated with + // php-core + spec-hash so a subsequent run lands on the cache-hit + // path. + s, _ := registry.Open(layoutURI) + hash, _ := ComputeSpecHash(&SpecHashInputs{ + Kind: "php", Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts", Repo: repo, + }) + ref, hit, err := s.LookupBySpec(context.Background(), "php-core", hash) + if err != nil || !hit { + t.Fatalf("LookupBySpec after build: hit=%v err=%v", hit, err) + } + if ref.Digest == "" { + t.Error("pushed ref has empty digest") + } +} + +func TestBuildPHP_RunnerError_Propagates(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutURI := "oci-layout:" + filepath.Join(t.TempDir(), "layout") + + restore := SetRunner(func(_ context.Context, _ *DockerRunOpts) error { + return errors.New("boom") + }) + defer restore() + + err := BuildPHP(context.Background(), []string{ + "--php", "8.4", + "--registry", layoutURI, + "--repo", repo, + }) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Errorf("BuildPHP err = %v, want containing \"boom\"", err) + } +} + +func TestBuildPHP_MissingVersionFlag_Errors(t *testing.T) { + err := BuildPHP(context.Background(), []string{"--registry", "oci-layout:/tmp/x"}) + if err == nil || !strings.Contains(err.Error(), "--php") { + t.Errorf("BuildPHP err = %v, want --php required", err) + } +} + +func TestBuildPHP_UnknownOS_Errors(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutURI := "oci-layout:" + filepath.Join(t.TempDir(), "layout") + + err := BuildPHP(context.Background(), []string{ + "--php", "8.4", "--os", "bogus", + "--registry", layoutURI, "--repo", repo, + }) + if err == nil || !strings.Contains(err.Error(), "unknown os") { + t.Errorf("BuildPHP err = %v, want unknown os", err) + } +} + +func TestBuildPHP_UnknownArch_Errors(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutURI := "oci-layout:" + filepath.Join(t.TempDir(), "layout") + + err := BuildPHP(context.Background(), []string{ + "--php", "8.4", "--arch", "bogus", + "--registry", layoutURI, "--repo", repo, + }) + if err == nil || !strings.Contains(err.Error(), "unknown arch") { + t.Errorf("BuildPHP err = %v, want unknown arch", err) + } +} + +func TestMain_UnknownKind_Errors(t *testing.T) { + err := Main([]string{"tool"}) + if err == nil || !strings.Contains(err.Error(), "unknown kind") { + t.Errorf("Main err = %v, want unknown kind", err) + } +} + +func TestMain_EmptyArgs_Errors(t *testing.T) { + err := Main(nil) + if err == nil { + t.Fatal("Main(nil) want error, got nil") + } +} + +func TestMain_ExtDispatch_ReturnsStub(t *testing.T) { + err := Main([]string{"ext"}) + if err == nil || !strings.Contains(err.Error(), "Task 5") { + t.Errorf("Main([]string{\"ext\"}) err = %v, want containing \"Task 5\"", err) + } +} + +// captureStdout redirects os.Stdout for the duration of fn and returns +// what was written. Simple helper; doesn't need to be fancy. Tests that +// use this helper must not run with t.Parallel() because os.Stdout is a +// process-level global. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + old := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stdout = w + fn() + _ = w.Close() + os.Stdout = old + out, _ := io.ReadAll(r) + return string(out) +} From 2ca76d26699a7fd1bdd9e2143bb33057778c16b5 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 15:48:30 +0200 Subject: [PATCH 07/11] fix(build): correct mount path + reject ZTS + normalize arch aliases - Container mount for bundle output was /tmp/out; builders write to /tmp via pack-bundle.sh's hardcoded OUTPUT_PATH. Real docker runs would have lost the bundle on container exit. Mount outDir at /tmp directly; keep OUTPUT_DIR=/tmp/out for make install INSTALL_ROOT staging inside the mount. Unit tests bypassed this via fake runners. - Add TestBuildPHP_RealDockerSmoke that swaps the repo fixture's build-php.sh for a 3-line synthetic and exercises real docker to catch mount-contract regressions in seconds. - Reject --ts zts at parse time: the flag was cache-key-only with no builder support, so accepting it silently cached an NTS artifact under a ZTS key. Fail loudly until builder support lands. - Normalize --arch aliases (amd64 -> x86_64, arm64 -> aarch64) at parse time so spec-hashes are stable regardless of caller convention; dockerPlatform simplified. - Stream apt output through to the user's stderr instead of silencing it; extract linuxAptPreamble constant so Task 5 can share. - Drop dead SchemaVersion default and linter-theater filepath.Clean calls. Revise BuildExt stub wording for end-user readability. --- internal/build/build.go | 88 +++++++++++++++++------- internal/build/build_test.go | 129 +++++++++++++++++++++++++++++++++-- 2 files changed, 188 insertions(+), 29 deletions(-) diff --git a/internal/build/build.go b/internal/build/build.go index 9b5cdbd..c97837a 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -13,6 +13,15 @@ import ( "github.com/buildrush/setup-php/internal/registry" ) +// linuxAptPreamble installs the minimal host-side packages needed for +// the Ubuntu build containers to run the builder scripts. Shared +// between build-php and build-ext; keep in sync with Makefile's +// bundle-php / bundle-ext targets. Output is NOT silenced so apt +// diagnostics (mirror outages, DNS, missing packages) stream through +// to the user's stderr via DockerRun's default Stdout/Stderr wiring. +const linuxAptPreamble = "apt-get update && " + + "apt-get install -y --no-install-recommends curl xz-utils ca-certificates && " + // Main is the entry point for `phpup build …`. args is the tail after // the "build" subcommand token (so args[0] is "php" or "ext"). Returning // a nil error means the requested build (or cache hit) succeeded; a @@ -95,7 +104,14 @@ func BuildPHP(ctx context.Context, args []string) error { Platform: platform, Mounts: []Mount{ {Host: opts.Repo, Container: "/workspace", ReadOnly: true}, - {Host: outDir, Container: "/tmp/out", ReadOnly: false}, + // Mount at /tmp (not /tmp/out) because builders/linux/build-php.sh + // invokes pack-bundle.sh with the hardcoded output path + // /tmp/bundle.tar.zst, and pack-bundle.sh writes meta.json as a + // sibling of the output tar. Mounting /tmp/out would leave both + // files in the container's ephemeral /tmp and lose them on exit. + // OUTPUT_DIR=/tmp/out still lives INSIDE the mount so the + // builder's INSTALL_ROOT staging tree is preserved unchanged. + {Host: outDir, Container: "/tmp", ReadOnly: false}, }, Env: map[string]string{ "PHP_VERSION": opts.Version, @@ -103,19 +119,18 @@ func BuildPHP(ctx context.Context, args []string) error { "OUTPUT_DIR": "/tmp/out", "WORKSPACE": "/workspace", }, - Cmd: []string{"bash", "-c", - "apt-get update >/dev/null 2>&1 && " + - "apt-get install -y --no-install-recommends curl xz-utils ca-certificates >/dev/null 2>&1 && " + - "/workspace/builders/linux/build-php.sh"}, + Cmd: []string{"bash", "-c", linuxAptPreamble + "/workspace/builders/linux/build-php.sh"}, } if err := DockerRun(ctx, runOpts); err != nil { return fmt.Errorf("phpup build php: docker: %w", err) } - // 5. Read the bundle + meta from the mount dir. + // 5. Read the bundle + meta from the mount dir. Paths are constructed + // internally from os.MkdirTemp output, not user input; no filepath.Clean + // needed (and the linter's G304 is excluded project-wide). bundlePath := filepath.Join(outDir, "bundle.tar.zst") metaPath := filepath.Join(outDir, "meta.json") - bundle, err := os.Open(filepath.Clean(bundlePath)) + bundle, err := os.Open(bundlePath) if err != nil { return fmt.Errorf("phpup build php: open bundle: %w", err) } @@ -140,7 +155,7 @@ func BuildPHP(ctx context.Context, args []string) error { // stub returns a recognisable error so Main's dispatch compiles and // callers see a clear "not yet" rather than an obscure panic. func BuildExt(_ context.Context, _ []string) error { - return errors.New("phpup build ext: implemented in PR2 Task 5") + return errors.New("phpup build ext: not yet supported in this build; will land in a subsequent release") } // phpOpts is the parsed flag set for `phpup build php`. Repo is resolved @@ -162,8 +177,8 @@ func parsePHPFlags(args []string) (*phpOpts, error) { fs := flag.NewFlagSet("phpup build php", flag.ContinueOnError) version := fs.String("php", "", "PHP version, e.g. 8.4 (required)") osFlag := fs.String("os", "jammy", "Ubuntu flavour: jammy (22.04) or noble (24.04)") - arch := fs.String("arch", "x86_64", "Target arch: x86_64 or aarch64") - ts := fs.String("ts", "nts", "Thread safety: nts or zts") + arch := fs.String("arch", "x86_64", "Target arch: x86_64 or aarch64 (amd64/arm64 aliases accepted)") + ts := fs.String("ts", "nts", "Thread safety: nts (zts not yet supported)") registryFlag := fs.String("registry", "oci-layout:./out/oci-layout", "Target registry URI (oci-layout: or ghcr.io/)") repo := fs.String("repo", ".", "Path to setup-php repo root") @@ -173,6 +188,17 @@ func parsePHPFlags(args []string) (*phpOpts, error) { if *version == "" { return nil, errors.New("phpup build php: --php is required") } + // ZTS differs the spec-hash (so a future ZTS builder gets its own + // cache key), but the current build-php.sh has no --enable-zts + // conditional — accepting zts here would silently cache an NTS + // artifact under a ZTS key. Reject until builder support lands. + if *ts != "nts" { + return nil, fmt.Errorf("phpup build php: --ts %q not yet supported (only nts)", *ts) + } + archNormalized, err := normalizeArch(*arch) + if err != nil { + return nil, fmt.Errorf("phpup build php: %w", err) + } absRepo, err := filepath.Abs(*repo) if err != nil { return nil, fmt.Errorf("phpup build php: resolve repo path: %w", err) @@ -180,7 +206,7 @@ func parsePHPFlags(args []string) (*phpOpts, error) { return &phpOpts{ Version: *version, OS: *osFlag, - Arch: *arch, + Arch: archNormalized, TS: *ts, Registry: *registryFlag, Repo: absRepo, @@ -203,15 +229,28 @@ func ubuntuImage(osFlag string) (string, error) { } } -// dockerPlatform maps the caller-facing arch name onto the docker -// --platform value. The caller-facing names match the builder script's -// ARCH env contract ("x86_64"/"aarch64"); the docker aliases ("amd64"/ -// "arm64") are accepted too for ergonomics. -func dockerPlatform(arch string) (string, error) { +// normalizeArch canonicalizes arch aliases to the forms used in the +// planner/lockfile ("x86_64" / "aarch64"), so spec-hashes are stable +// regardless of whether the caller used the docker-style ("amd64"/"arm64") +// or the uname-style ("x86_64"/"aarch64") spelling. +func normalizeArch(arch string) (string, error) { switch arch { case "x86_64", "amd64": - return "linux/amd64", nil + return "x86_64", nil case "aarch64", "arm64": + return "aarch64", nil + default: + return "", fmt.Errorf("unknown arch %q (want x86_64 or aarch64)", arch) + } +} + +// dockerPlatform maps the canonical arch name (as produced by +// normalizeArch) onto the docker --platform value. +func dockerPlatform(arch string) (string, error) { + switch arch { + case "x86_64": + return "linux/amd64", nil + case "aarch64": return "linux/arm64", nil default: return "", fmt.Errorf("unknown arch %q (want x86_64|aarch64)", arch) @@ -219,11 +258,15 @@ func dockerPlatform(arch string) (string, error) { } // parseMetaJSONFile reads the builder's meta.json sidecar into a -// registry.Meta. Missing schema_version defaults to 1 to match -// internal/registry/layout.go's legacy-bundle tolerance; callers should -// not rely on this default — the builder writes the real version. +// registry.Meta. The builder is the source of truth for schema_version +// (pack-bundle.sh always writes the current SCHEMA_VERSION from +// builders/common/bundle-schema-version.env); we do not fill a default +// here. Callers that need legacy tolerance (e.g. remote Fetch reading an +// older, pre-schema_version bundle from a registry) handle it there. +// Path is constructed internally from os.MkdirTemp; no filepath.Clean +// needed (G304 is excluded project-wide). func parseMetaJSONFile(path string) (*registry.Meta, error) { - data, err := os.ReadFile(filepath.Clean(path)) + data, err := os.ReadFile(path) if err != nil { return nil, err } @@ -231,8 +274,5 @@ func parseMetaJSONFile(path string) (*registry.Meta, error) { if err := json.Unmarshal(data, &m); err != nil { return nil, err } - if m.SchemaVersion == 0 { - m.SchemaVersion = 1 - } return &m, nil } diff --git a/internal/build/build_test.go b/internal/build/build_test.go index 39486d2..e0f3bd8 100644 --- a/internal/build/build_test.go +++ b/internal/build/build_test.go @@ -7,9 +7,11 @@ import ( "errors" "io" "os" + "os/exec" "path/filepath" "strings" "testing" + "time" "github.com/buildrush/setup-php/internal/registry" ) @@ -61,18 +63,19 @@ func seedLayout(t *testing.T, dir, bundleName, specHash string) string { // fakeRunner is a RunnerFunc that writes a valid bundle + meta.json into // the output mount so BuildPHP's read-push step can proceed without a -// real docker invocation. +// real docker invocation. Looks for the mount at /tmp (matching the +// production mount contract — see BuildPHP's Mounts comment for why). func fakeRunner(bundleBytes []byte) RunnerFunc { return func(_ context.Context, opts *DockerRunOpts) error { var outHost string for _, m := range opts.Mounts { - if m.Container == "/tmp/out" { + if m.Container == "/tmp" { outHost = m.Host break } } if outHost == "" { - return errors.New("fakeRunner: no /tmp/out mount") + return errors.New("fakeRunner: no /tmp mount") } if err := os.WriteFile(filepath.Join(outHost, "bundle.tar.zst"), bundleBytes, 0o644); err != nil { return err @@ -213,6 +216,122 @@ func TestBuildPHP_UnknownArch_Errors(t *testing.T) { } } +// TestBuildPHP_ZTSNotSupported_Errors: --ts zts differs the spec-hash (so +// a future ZTS builder gets its own cache key) but today's build-php.sh +// has no --enable-zts path. Accepting zts would silently cache an NTS +// artifact under a ZTS key — reject until builder support lands. +func TestBuildPHP_ZTSNotSupported_Errors(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + err := BuildPHP(context.Background(), []string{ + "--php", "8.4", "--ts", "zts", + "--registry", "oci-layout:" + filepath.Join(t.TempDir(), "layout"), + "--repo", repo, + }) + if err == nil || !strings.Contains(err.Error(), "zts") { + t.Errorf("BuildPHP err = %v, want zts rejection", err) + } +} + +// TestBuildPHP_AmdAliasNormalizes verifies that --arch amd64 produces the +// same spec-hash as --arch x86_64 — otherwise callers using the docker +// spelling would get a distinct cache entry for the same build. +func TestBuildPHP_AmdAliasNormalizes(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + // Reference hash computed from the canonical spelling. + h1, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "php", Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts", Repo: repo, + }) + if err != nil { + t.Fatalf("ComputeSpecHash: %v", err) + } + // Route through parsePHPFlags to get the normalized value. + opts, err := parsePHPFlags([]string{"--php", "8.4", "--arch", "amd64", "--repo", repo}) + if err != nil { + t.Fatalf("parsePHPFlags: %v", err) + } + if opts.Arch != "x86_64" { + t.Fatalf("normalizeArch failure: Arch = %q, want x86_64", opts.Arch) + } + h2, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "php", Version: opts.Version, OS: "linux", Arch: opts.Arch, TS: opts.TS, Repo: repo, + }) + if err != nil { + t.Fatalf("ComputeSpecHash (amd64): %v", err) + } + if h1 != h2 { + t.Fatalf("spec-hash differs: %q vs %q", h1, h2) + } +} + +// TestBuildPHP_RealDockerSmoke exercises the real docker mount contract +// end to end — unit tests that use fakeRunner write into the output host +// dir directly, bypassing the container's filesystem. This test swaps +// build-php.sh for a 3-line synthetic that emulates pack-bundle.sh's +// output (writes /tmp/bundle.tar.zst + /tmp/meta.json) so we can verify +// the mount actually captures those files without paying a 10-minute +// PHP compile. Catches mount-path bugs (e.g. mounting /tmp/out instead +// of /tmp) that unit tests cannot. +func TestBuildPHP_RealDockerSmoke(t *testing.T) { + if testing.Short() { + t.Skip("skipping real docker smoke under -short") + } + if _, err := exec.LookPath("docker"); err != nil { + t.Skipf("docker not found in PATH: %v", err) + } + repo := t.TempDir() + writeRepoFixture(t, repo) + // Overwrite build-php.sh with a synthetic that emulates what real + // build-php.sh does: produce /tmp/bundle.tar.zst + /tmp/meta.json. + fakeBuilder := "#!/usr/bin/env bash\n" + + "set -euo pipefail\n" + + "mkdir -p /tmp\n" + + "echo 'synthetic bundle' > /tmp/bundle.tar.zst\n" + + `echo '{"schema_version":3,"kind":"php-core"}' > /tmp/meta.json` + "\n" + builderPath := filepath.Join(repo, "builders", "linux", "build-php.sh") + if err := os.WriteFile(builderPath, []byte(fakeBuilder), 0o755); err != nil { + t.Fatalf("write fake builder: %v", err) + } + // os.WriteFile preserves the mode of the pre-existing fixture file + // (0o644 from writeRepoFixture). Force +x so the container can execv. + if err := os.Chmod(builderPath, 0o755); err != nil { + t.Fatalf("chmod fake builder: %v", err) + } + + layoutURI := "oci-layout:" + filepath.Join(t.TempDir(), "layout") + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + err := BuildPHP(ctx, []string{ + "--php", "8.4", + "--registry", layoutURI, + "--repo", repo, + }) + if err != nil { + t.Fatalf("BuildPHP: %v", err) + } + + // Verify the layout has the manifest at the expected spec-hash. + hash, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "php", Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts", Repo: repo, + }) + if err != nil { + t.Fatalf("ComputeSpecHash: %v", err) + } + s, err := registry.Open(layoutURI) + if err != nil { + t.Fatalf("registry.Open: %v", err) + } + ref, hit, err := s.LookupBySpec(ctx, "php-core", hash) + if err != nil || !hit { + t.Fatalf("LookupBySpec after real-docker build: hit=%v err=%v", hit, err) + } + if ref.Digest == "" { + t.Fatal("pushed ref has empty digest after real-docker build") + } +} + func TestMain_UnknownKind_Errors(t *testing.T) { err := Main([]string{"tool"}) if err == nil || !strings.Contains(err.Error(), "unknown kind") { @@ -229,8 +348,8 @@ func TestMain_EmptyArgs_Errors(t *testing.T) { func TestMain_ExtDispatch_ReturnsStub(t *testing.T) { err := Main([]string{"ext"}) - if err == nil || !strings.Contains(err.Error(), "Task 5") { - t.Errorf("Main([]string{\"ext\"}) err = %v, want containing \"Task 5\"", err) + if err == nil || !strings.Contains(err.Error(), "not yet supported") { + t.Errorf("Main([]string{\"ext\"}) err = %v, want containing \"not yet supported\"", err) } } From 51904a6dd6827e5c24491a6a283480828fb097b6 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 15:56:47 +0200 Subject: [PATCH 08/11] fix(build): default docker output to project-relative ./build/ (gitignored) phpup build php's mounted output directory was a tempdir under /tmp. Per user redirect, switch the default to /build/php/- --/ so artifacts persist next to the source tree for easy inspection, keyed deterministically on the spec tuple (repeated runs overwrite the same dir instead of piling up tempdirs). Add --out-dir flag for explicit override; tests pass t.TempDir() via the flag to keep the worktree clean. .gitignore gains /build/ (docker output) and /out/ (default oci-layout registry target). Leading slashes anchor the patterns to the repo root so they don't shadow internal/build/ (the Go package). --- .gitignore | 8 +++++ internal/build/build.go | 67 +++++++++++++++++++++++++++++++----- internal/build/build_test.go | 38 ++++++++++++++++++++ 3 files changed, 104 insertions(+), 9 deletions(-) diff --git a/.gitignore b/.gitignore index bea2cc3..b07eca9 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,14 @@ bin/ dist/ cmd/phpup/bundles.lock +# Docker-wrapped build output (phpup build php|ext). +# Leading slash anchors to repo root so we don't accidentally shadow +# internal/build/ (the Go package) when adding new files to it. +/build/ + +# Default OCI-layout registry target (phpup build --registry flag). +/out/ + # Dependencies vendor/ node_modules/ diff --git a/internal/build/build.go b/internal/build/build.go index c97837a..a41ec6a 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -83,12 +83,21 @@ func BuildPHP(ctx context.Context, args []string) error { return nil } - // 3. Prepare output mount dir. - outDir, err := os.MkdirTemp("", "phpup-build-php-*") + // 3. Prepare output mount dir. Default is /build/php// so + // artifacts persist next to the source tree for easy inspection; + // --out-dir overrides verbatim. We wipe before writing so stale files + // from an aborted previous run don't masquerade as fresh output. + // Docker requires absolute host paths for bind mounts, so absolutize + // whatever we end up with. No defer-cleanup — artifacts are meant to + // persist; the next run's cache-hit path reads from registry.Store. + outDir := resolveOutDir(opts) + if err := prepareOutDir(outDir); err != nil { + return fmt.Errorf("phpup build php: %w", err) + } + absOutDir, err := filepath.Abs(outDir) if err != nil { - return fmt.Errorf("phpup build php: mktemp: %w", err) + return fmt.Errorf("phpup build php: resolve out dir: %w", err) } - defer func() { _ = os.RemoveAll(outDir) }() // 4. Invoke builder in docker. image, err := ubuntuImage(opts.OS) @@ -111,7 +120,7 @@ func BuildPHP(ctx context.Context, args []string) error { // files in the container's ephemeral /tmp and lose them on exit. // OUTPUT_DIR=/tmp/out still lives INSIDE the mount so the // builder's INSTALL_ROOT staging tree is preserved unchanged. - {Host: outDir, Container: "/tmp", ReadOnly: false}, + {Host: absOutDir, Container: "/tmp", ReadOnly: false}, }, Env: map[string]string{ "PHP_VERSION": opts.Version, @@ -126,10 +135,11 @@ func BuildPHP(ctx context.Context, args []string) error { } // 5. Read the bundle + meta from the mount dir. Paths are constructed - // internally from os.MkdirTemp output, not user input; no filepath.Clean - // needed (and the linter's G304 is excluded project-wide). - bundlePath := filepath.Join(outDir, "bundle.tar.zst") - metaPath := filepath.Join(outDir, "meta.json") + // internally from resolveOutDir (repo-derived or explicit --out-dir), + // not arbitrary user input; no filepath.Clean needed (and the linter's + // G304 is excluded project-wide). + bundlePath := filepath.Join(absOutDir, "bundle.tar.zst") + metaPath := filepath.Join(absOutDir, "meta.json") bundle, err := os.Open(bundlePath) if err != nil { return fmt.Errorf("phpup build php: open bundle: %w", err) @@ -168,6 +178,10 @@ type phpOpts struct { TS string // "nts" or "zts" Registry string // "oci-layout:./out/oci-layout" or "ghcr.io/..." Repo string // absolute path to setup-php repo root + // OutDir is the docker output mount path. Empty = derive the default + // under /build/php/---/; non-empty = + // use verbatim (may be absolute or relative — absolutized in BuildPHP). + OutDir string } // parsePHPFlags parses the flag tail for `phpup build php`. The FlagSet @@ -182,6 +196,8 @@ func parsePHPFlags(args []string) (*phpOpts, error) { registryFlag := fs.String("registry", "oci-layout:./out/oci-layout", "Target registry URI (oci-layout: or ghcr.io/)") repo := fs.String("repo", ".", "Path to setup-php repo root") + outDir := fs.String("out-dir", "", + "Docker output directory (defaults to /build/php/---/)") if err := fs.Parse(args); err != nil { return nil, err } @@ -210,9 +226,42 @@ func parsePHPFlags(args []string) (*phpOpts, error) { TS: *ts, Registry: *registryFlag, Repo: absRepo, + OutDir: *outDir, }, nil } +// resolveOutDir derives the project-relative build output directory from +// the input tuple when the caller didn't pass --out-dir explicitly. The +// directory lives under /build/ so it's project-local (gitignored +// via the repo's .gitignore). Path shape: +// +// /build/php/---/ +// +// Keying on the same tuple as the spec-hash makes the path deterministic: +// repeated runs with the same inputs overwrite the same dir instead of +// piling up random tempdirs. Task 5 (BuildExt) will mirror this pattern +// under /build/ext/----/. +func resolveOutDir(opts *phpOpts) string { + if opts.OutDir != "" { + return opts.OutDir + } + slug := opts.Version + "-" + opts.OS + "-" + opts.Arch + "-" + opts.TS + return filepath.Join(opts.Repo, "build", "php", slug) +} + +// prepareOutDir wipes any previous content at path (so stale files from an +// aborted earlier run don't masquerade as fresh output) and creates a +// clean directory. +func prepareOutDir(path string) error { + if err := os.RemoveAll(path); err != nil { + return fmt.Errorf("clean out dir: %w", err) + } + if err := os.MkdirAll(path, 0o750); err != nil { + return fmt.Errorf("create out dir: %w", err) + } + return nil +} + // ubuntuImage maps a short OS flavour name onto the concrete docker image // tag that builders/linux/build-php.sh expects. Accepts both the short // ("jammy"/"noble") and long ("ubuntu-22.04"/"ubuntu-24.04") spellings diff --git a/internal/build/build_test.go b/internal/build/build_test.go index e0f3bd8..108d2bf 100644 --- a/internal/build/build_test.go +++ b/internal/build/build_test.go @@ -114,6 +114,7 @@ func TestBuildPHP_CacheHit_ShortCircuitsWithoutRunning(t *testing.T) { "--php", "8.4", "--registry", layoutURI, "--repo", repo, + "--out-dir", t.TempDir(), }) }) if err != nil { @@ -140,6 +141,7 @@ func TestBuildPHP_CacheMiss_InvokesRunnerThenPushes(t *testing.T) { "--php", "8.4", "--registry", layoutURI, "--repo", repo, + "--out-dir", t.TempDir(), }) if err != nil { t.Fatalf("BuildPHP: %v", err) @@ -175,6 +177,7 @@ func TestBuildPHP_RunnerError_Propagates(t *testing.T) { "--php", "8.4", "--registry", layoutURI, "--repo", repo, + "--out-dir", t.TempDir(), }) if err == nil || !strings.Contains(err.Error(), "boom") { t.Errorf("BuildPHP err = %v, want containing \"boom\"", err) @@ -196,6 +199,7 @@ func TestBuildPHP_UnknownOS_Errors(t *testing.T) { err := BuildPHP(context.Background(), []string{ "--php", "8.4", "--os", "bogus", "--registry", layoutURI, "--repo", repo, + "--out-dir", t.TempDir(), }) if err == nil || !strings.Contains(err.Error(), "unknown os") { t.Errorf("BuildPHP err = %v, want unknown os", err) @@ -210,6 +214,7 @@ func TestBuildPHP_UnknownArch_Errors(t *testing.T) { err := BuildPHP(context.Background(), []string{ "--php", "8.4", "--arch", "bogus", "--registry", layoutURI, "--repo", repo, + "--out-dir", t.TempDir(), }) if err == nil || !strings.Contains(err.Error(), "unknown arch") { t.Errorf("BuildPHP err = %v, want unknown arch", err) @@ -227,6 +232,7 @@ func TestBuildPHP_ZTSNotSupported_Errors(t *testing.T) { "--php", "8.4", "--ts", "zts", "--registry", "oci-layout:" + filepath.Join(t.TempDir(), "layout"), "--repo", repo, + "--out-dir", t.TempDir(), }) if err == nil || !strings.Contains(err.Error(), "zts") { t.Errorf("BuildPHP err = %v, want zts rejection", err) @@ -307,6 +313,7 @@ func TestBuildPHP_RealDockerSmoke(t *testing.T) { "--php", "8.4", "--registry", layoutURI, "--repo", repo, + "--out-dir", t.TempDir(), }) if err != nil { t.Fatalf("BuildPHP: %v", err) @@ -332,6 +339,37 @@ func TestBuildPHP_RealDockerSmoke(t *testing.T) { } } +// TestResolveOutDir_DefaultShape asserts the derived default path when +// --out-dir is not supplied: /build/php/---/. +// Shape is load-bearing because it participates in the gitignore contract +// (build/ is ignored) and the Task 5 ext mirror will reuse /build/. +// Use a temp dir as the repo root — keeps the test OS-agnostic (so it +// works on Windows CI too where "/abs/repo" isn't absolute) and side- +// steps gocritic's "filepath.Join on a string that already contains a +// separator" complaint. +func TestResolveOutDir_DefaultShape(t *testing.T) { + repo := t.TempDir() + opts := &phpOpts{ + Version: "8.4", OS: "jammy", Arch: "x86_64", TS: "nts", + Repo: repo, + } + got := resolveOutDir(opts) + want := filepath.Join(repo, "build", "php", "8.4-jammy-x86_64-nts") + if got != want { + t.Errorf("resolveOutDir = %q, want %q", got, want) + } +} + +// TestResolveOutDir_ExplicitOverride asserts --out-dir is honored verbatim +// (pass-through, no derivation), which is how tests keep the worktree +// clean by pointing at t.TempDir(). +func TestResolveOutDir_ExplicitOverride(t *testing.T) { + opts := &phpOpts{OutDir: "/custom/path"} + if got := resolveOutDir(opts); got != "/custom/path" { + t.Errorf("resolveOutDir with --out-dir = %q, want /custom/path", got) + } +} + func TestMain_UnknownKind_Errors(t *testing.T) { err := Main([]string{"tool"}) if err == nil || !strings.Contains(err.Error(), "unknown kind") { From 09162dfe6708095bd3efac6b7443cc42970430cf Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 16:14:23 +0200 Subject: [PATCH 09/11] feat(build): phpup build ext + distribution:3 sidecar registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit phpup build ext --ext redis --ext-version 6.2.0 --php-abi 8.4-nts \ --php-core-digest sha256:... [--arch x86_64] [--os jammy] \ [--registry oci-layout:./out/oci-layout] [--out-dir ./build/ext/...] Spins up an ephemeral distribution:3 registry on a fresh docker network, copies the prerequisite php-core from the source registry.Store to the sidecar via remote.Write (name.Insecure — distribution speaks HTTP by default), then runs builders/linux/build-ext.sh inside a container on the same network with REGISTRY overridden to the sidecar. fetch-core.sh pulls from the sidecar without any script change. Strict spec compliance: builders/** stays unchanged. After the build exits, phpup reads the ext bundle + meta.json from the output mount and pushes back to the destination Store with bundle-name + spec-hash annotations. Cache probe via LookupBySpec short-circuits redundant rebuilds. Default output dir mirrors Task 4's pattern: /build/ext/----/ (gitignored). SidecarLifecycle is a package-level var swappable via SetSidecarLifecycle for unit tests; a fakeSidecar exercises BuildExt's orchestration without real docker. A gated TestSidecar_LifecycleAndSeed_Real covers the real distribution:3 path (skipped under -short or when docker is absent). --- internal/build/build.go | 321 ++++++++++++++++++++++++++++- internal/build/build_test.go | 359 ++++++++++++++++++++++++++++++++- internal/build/sidecar.go | 313 ++++++++++++++++++++++++++++ internal/build/sidecar_test.go | 239 ++++++++++++++++++++++ 4 files changed, 1222 insertions(+), 10 deletions(-) create mode 100644 internal/build/sidecar.go create mode 100644 internal/build/sidecar_test.go diff --git a/internal/build/build.go b/internal/build/build.go index a41ec6a..148fb6b 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -9,6 +9,9 @@ import ( "os" "path/filepath" "strings" + "time" + + "gopkg.in/yaml.v3" "github.com/buildrush/setup-php/internal/registry" ) @@ -161,11 +164,319 @@ func BuildPHP(ctx context.Context, args []string) error { return nil } -// BuildExt is declared but not implemented yet; it lands in Task 5. The -// stub returns a recognisable error so Main's dispatch compiles and -// callers see a clear "not yet" rather than an obscure panic. -func BuildExt(_ context.Context, _ []string) error { - return errors.New("phpup build ext: not yet supported in this build; will land in a subsequent release") +// BuildExt runs the php-ext build end to end. args is everything after +// "build ext" (flags). Returns nil on success, or an error with the +// "phpup build ext: " prefix the CLI dispatcher expects. +// +// Unlike BuildPHP, the ext build needs a prerequisite php-core bundle — +// the extension is dynamically linked against a specific PHP. Per spec, +// builders/** stays unchanged, so instead of modifying fetch-core.sh we +// spin up an ephemeral distribution:3 sidecar registry, copy the +// prerequisite php-core@sha256 into it, and override REGISTRY for the +// build container so fetch-core.sh pulls from the sidecar transparently. +func BuildExt(ctx context.Context, args []string) error { + opts, err := parseExtFlags(args) + if err != nil { + return err + } + + // 1. Spec-hash. + specHash, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "ext", + Name: opts.Name, + Version: opts.Version, + OS: "linux", + Arch: opts.Arch, + PHPABI: opts.PHPABI, + TS: tsFromPHPABI(opts.PHPABI), + Repo: opts.Repo, + }) + if err != nil { + return fmt.Errorf("phpup build ext: %w", err) + } + + // 2. Open target store + cache-probe. + store, err := registry.Open(opts.Registry) + if err != nil { + return fmt.Errorf("phpup build ext: open registry: %w", err) + } + bundleName := "php-ext-" + opts.Name + // Remote backends return ErrUnsupported for LookupBySpec; treat that + // as a soft miss so callers without an oci-layout cache fall through + // to building. Hard errors from a layout backend still propagate. + ref, hit, err := store.LookupBySpec(ctx, bundleName, specHash) + if errors.Is(err, registry.ErrUnsupported) { + hit, err = false, nil + } + if err != nil { + return fmt.Errorf("phpup build ext: lookup by spec: %w", err) + } + if hit { + fmt.Printf("phpup build ext: cache hit %s (spec-hash %s)\n", ref.Digest, specHash) + return nil + } + + // 3. Start sidecar + seed prerequisite core. The sidecar is tied to + // this invocation's lifetime; teardown is unconditional (including + // on error paths below) via defer. + lifecycle := currentSidecarLifecycle() + sc, stopSidecar, err := lifecycle.Start(ctx) + if err != nil { + return fmt.Errorf("phpup build ext: start sidecar: %w", err) + } + defer func() { + // Fresh context with a generous timeout so teardown still runs + // if the caller's ctx has already been cancelled (which is the + // common case when the build itself failed). 30s is + // comfortably more than `docker rm -f` + `docker network rm` + // need under load. + stopCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _ = stopSidecar(stopCtx) + }() + + // Core tag matches what fetch-core.sh constructs inside the build + // container: "---". This MUST line up or the + // oras pull will miss the seeded image. + coreTag := coreTagForFetch(opts.PHPABI, opts.Arch) + coreRef := registry.Ref{Name: "php-core", Digest: opts.CoreDigest} + if err := lifecycle.SeedCore(ctx, sc, store, coreRef, "buildrush", coreTag); err != nil { + return fmt.Errorf("phpup build ext: seed core: %w", err) + } + + // 4. Prepare output dir (default: /build/ext// ; + // --out-dir overrides verbatim). + outDir := resolveExtOutDir(opts) + if err := prepareOutDir(outDir); err != nil { + return fmt.Errorf("phpup build ext: %w", err) + } + absOutDir, err := filepath.Abs(outDir) + if err != nil { + return fmt.Errorf("phpup build ext: resolve out dir: %w", err) + } + + // 5. Load build_deps.linux from catalog (build-ext.sh reads + // BUILD_DEPS env). Absent build_deps = empty = no-op in the + // builder script. + buildDeps, err := loadExtBuildDeps(filepath.Join(opts.Repo, "catalog", "extensions", opts.Name+".yaml")) + if err != nil { + return fmt.Errorf("phpup build ext: load build_deps: %w", err) + } + + // 6. Run build container on the sidecar's network so + // fetch-core.sh can reach the sidecar by in-network hostname. + image, err := ubuntuImage(opts.OS) + if err != nil { + return fmt.Errorf("phpup build ext: %w", err) + } + platform, err := dockerPlatform(opts.Arch) + if err != nil { + return fmt.Errorf("phpup build ext: %w", err) + } + runOpts := &DockerRunOpts{ + Image: image, + Platform: platform, + Network: sc.Network, + Mounts: []Mount{ + {Host: opts.Repo, Container: "/workspace", ReadOnly: true}, + // Mount at /tmp (not /tmp/out) because builders/linux/build-ext.sh + // calls pack-bundle.sh with the hardcoded output path + // /tmp/bundle.tar.zst — mirroring the php-core build. See + // BuildPHP for the full rationale. + {Host: absOutDir, Container: "/tmp", ReadOnly: false}, + }, + Env: map[string]string{ + "EXT_NAME": opts.Name, + "EXT_VERSION": opts.Version, + "PHP_ABI": opts.PHPABI, + "ARCH": opts.Arch, + "WORKSPACE": "/workspace", + "OUTPUT_DIR": "/tmp/ext-out", + // REGISTRY override: fetch-core.sh defaults to + // ghcr.io/buildrush but honours REGISTRY if set. We point + // it at the sidecar so the builder pulls the seeded + // php-core without any script change. + "REGISTRY": sc.InNetworkHost + "/buildrush", + "BUILD_DEPS": buildDeps, + }, + Cmd: []string{"bash", "-c", linuxAptPreamble + "/workspace/builders/linux/build-ext.sh"}, + } + if err := DockerRun(ctx, runOpts); err != nil { + return fmt.Errorf("phpup build ext: docker: %w", err) + } + + // 7. Read the bundle + meta from the mount dir. + bundlePath := filepath.Join(absOutDir, "bundle.tar.zst") + metaPath := filepath.Join(absOutDir, "meta.json") + bundle, err := os.Open(bundlePath) + if err != nil { + return fmt.Errorf("phpup build ext: open bundle: %w", err) + } + defer func() { _ = bundle.Close() }() + meta, err := parseMetaJSONFile(metaPath) + if err != nil { + return fmt.Errorf("phpup build ext: parse meta.json: %w", err) + } + + // 8. Push to the store. + pushRef := registry.Ref{Name: bundleName} + ann := registry.Annotations{BundleName: bundleName, SpecHash: specHash} + if err := store.Push(ctx, pushRef, bundle, meta, ann); err != nil { + return fmt.Errorf("phpup build ext: push bundle: %w", err) + } + + fmt.Printf("phpup build ext: built and pushed %s (spec-hash %s) to %s\n", bundleName, specHash, opts.Registry) + return nil +} + +// extOpts is the parsed flag set for `phpup build ext`. Repo is +// resolved to an absolute path during parsing so downstream code +// (spec-hash, docker bind mount) can use it directly. CoreDigest +// addresses the prerequisite php-core bundle that gets seeded into +// the sidecar — the caller is responsible for resolving it (typically +// via the lockfile or a prior BuildPHP invocation). +type extOpts struct { + Name string // "redis" + Version string // "6.2.0" + PHPABI string // "8.4-nts" + OS string // "jammy" or "noble" (after normalisation) + Arch string // "x86_64" or "aarch64" (after normalisation) + Registry string + Repo string + OutDir string + CoreDigest string // "sha256:..." — required; resolved by caller +} + +// parseExtFlags parses the flag tail for `phpup build ext`. The FlagSet +// uses ContinueOnError so callers get back an error instead of a process +// exit — makes the surface testable without os.Exit acrobatics. +func parseExtFlags(args []string) (*extOpts, error) { + fs := flag.NewFlagSet("phpup build ext", flag.ContinueOnError) + extName := fs.String("ext", "", "Extension name, e.g. redis (required)") + extVer := fs.String("ext-version", "", "Extension version (required)") + phpAbi := fs.String("php-abi", "", "PHP ABI, e.g. 8.4-nts (required)") + osFlag := fs.String("os", "jammy", "Ubuntu flavour: jammy (22.04) or noble (24.04)") + arch := fs.String("arch", "x86_64", "Target arch: x86_64 or aarch64 (amd64/arm64 aliases accepted)") + registryFlag := fs.String("registry", "oci-layout:./out/oci-layout", + "Target registry URI (oci-layout: or ghcr.io/)") + repo := fs.String("repo", ".", "Path to setup-php repo root") + outDir := fs.String("out-dir", "", + "Docker output directory (defaults to /build/ext/----/)") + coreDigest := fs.String("php-core-digest", "", + "Digest of the prerequisite php-core bundle (sha256:...). Required.") + if err := fs.Parse(args); err != nil { + return nil, err + } + if *extName == "" { + return nil, errors.New("phpup build ext: --ext is required") + } + if *extVer == "" { + return nil, errors.New("phpup build ext: --ext-version is required") + } + if *phpAbi == "" { + return nil, errors.New("phpup build ext: --php-abi is required") + } + if *coreDigest == "" { + return nil, errors.New("phpup build ext: --php-core-digest is required") + } + + archNormalized, err := normalizeArch(*arch) + if err != nil { + return nil, fmt.Errorf("phpup build ext: %w", err) + } + absRepo, err := filepath.Abs(*repo) + if err != nil { + return nil, fmt.Errorf("phpup build ext: resolve repo path: %w", err) + } + + return &extOpts{ + Name: *extName, + Version: *extVer, + PHPABI: *phpAbi, + OS: *osFlag, + Arch: archNormalized, + Registry: *registryFlag, + Repo: absRepo, + OutDir: *outDir, + CoreDigest: *coreDigest, + }, nil +} + +// resolveExtOutDir derives the project-relative build output directory +// from the input tuple when the caller didn't pass --out-dir +// explicitly. Mirrors Task 4's resolveOutDir shape but slots under +// /build/ext/ instead of /build/php/: +// +// /build/ext/----/ +// +// Keying on the same tuple as the spec-hash makes the path +// deterministic: repeated runs with the same inputs overwrite the +// same dir instead of piling up random tempdirs. +func resolveExtOutDir(opts *extOpts) string { + if opts.OutDir != "" { + return opts.OutDir + } + slug := opts.Name + "-" + opts.Version + "-" + opts.PHPABI + "-" + opts.OS + "-" + opts.Arch + return filepath.Join(opts.Repo, "build", "ext", slug) +} + +// coreTagForFetch assembles the OCI tag fetch-core.sh constructs for +// the prerequisite php-core. The format — "---" — +// is baked into builders/common/fetch-core.sh and must match exactly +// or the sidecar pull inside the build container will miss. Example: +// PHPABI="8.4-nts", arch="x86_64" → "8.4-linux-x86_64-nts". +func coreTagForFetch(phpABI, arch string) string { + ts := tsFromPHPABI(phpABI) + ver := strings.TrimSuffix(phpABI, "-"+ts) + return ver + "-linux-" + arch + "-" + ts +} + +// tsFromPHPABI extracts the thread-safety suffix from a PHPABI string +// of the form "-". Returns "nts" as a safe default if the +// input is malformed — the spec-hash then diverges from a correct +// invocation, guaranteeing a cache miss rather than silent cross-use. +func tsFromPHPABI(phpABI string) string { + i := strings.LastIndex(phpABI, "-") + if i < 0 { + return "nts" + } + return phpABI[i+1:] +} + +// loadExtBuildDeps reads catalog/extensions/.yaml and returns the +// .build_deps.linux list joined by single spaces (the BUILD_DEPS env +// shape build-ext.sh expects). Mirrors the yq invocation in +// build-extension.yml: +// +// yq eval '.build_deps.linux // [] | join(" ")' catalog/extensions/.yaml +// +// Absent or empty returns "" — the builder treats that as a no-op. +// Reading YAML into map[string]any keeps us schema-agnostic so catalog +// additions don't require Go changes. +func loadExtBuildDeps(path string) (string, error) { + data, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + return "", fmt.Errorf("read extension catalog: %w", err) + } + var doc map[string]any + if err := yaml.Unmarshal(data, &doc); err != nil { + return "", fmt.Errorf("parse extension catalog: %w", err) + } + bd, ok := doc["build_deps"].(map[string]any) + if !ok { + return "", nil + } + linux, ok := bd["linux"].([]any) + if !ok { + return "", nil + } + pkgs := make([]string, 0, len(linux)) + for _, p := range linux { + if s, ok := p.(string); ok { + pkgs = append(pkgs, s) + } + } + return strings.Join(pkgs, " "), nil } // phpOpts is the parsed flag set for `phpup build php`. Repo is resolved diff --git a/internal/build/build_test.go b/internal/build/build_test.go index 108d2bf..829ae8c 100644 --- a/internal/build/build_test.go +++ b/internal/build/build_test.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -40,7 +41,11 @@ func writeRepoFixture(t *testing.T, dir string) { mustWrite("builders/common/fetch-core.sh", "#!/bin/bash\n") mustWrite("builders/common/builder-os.env", "BUILDER_OS=ubuntu-22.04\n") mustWrite("catalog/php.yaml", "versions:\n \"8.4\":\n sources:\n url: https://example.com/php-8.4.0.tar.xz\n") - mustWrite("catalog/extensions/redis.yaml", "name: redis\nversions:\n - \"6.2.0\"\n") + // Extension catalog with a real build_deps.linux list so + // loadExtBuildDeps has something to parse. The shape matches + // catalog/extensions/amqp.yaml's production form (see + // .github/workflows/build-extension.yml's yq invocation). + mustWrite("catalog/extensions/redis.yaml", "name: redis\nversions:\n - \"6.2.0\"\nbuild_deps:\n linux:\n - libssl-dev\n") } // seedLayout pushes a manifest into an oci-layout so BuildPHP's cache @@ -65,7 +70,16 @@ func seedLayout(t *testing.T, dir, bundleName, specHash string) string { // the output mount so BuildPHP's read-push step can proceed without a // real docker invocation. Looks for the mount at /tmp (matching the // production mount contract — see BuildPHP's Mounts comment for why). +// Defaults to kind=php-core; use fakeRunnerKind when the test needs a +// different meta.json kind (e.g. "php-ext" for BuildExt). func fakeRunner(bundleBytes []byte) RunnerFunc { + return fakeRunnerKind(bundleBytes, "php-core") +} + +// fakeRunnerKind is fakeRunner parameterised on the meta.json kind field +// so BuildExt tests can produce a synthetic php-ext bundle without +// tripping BuildPHP's schema-version assumption. +func fakeRunnerKind(bundleBytes []byte, kind string) RunnerFunc { return func(_ context.Context, opts *DockerRunOpts) error { var outHost string for _, m := range opts.Mounts { @@ -80,7 +94,7 @@ func fakeRunner(bundleBytes []byte) RunnerFunc { if err := os.WriteFile(filepath.Join(outHost, "bundle.tar.zst"), bundleBytes, 0o644); err != nil { return err } - meta := map[string]any{"schema_version": 3, "kind": "php-core"} + meta := map[string]any{"schema_version": 3, "kind": kind} mjson, err := json.Marshal(meta) if err != nil { return err @@ -89,6 +103,47 @@ func fakeRunner(bundleBytes []byte) RunnerFunc { } } +// fakeSidecar is a SidecarLifecycle that doesn't touch docker. Start +// returns a placeholder *Sidecar and a no-op stop; SeedCore records +// the call for assertions. All counts are atomic so tests can safely +// inspect them after concurrent dispatch (even though BuildExt itself +// is single-goroutine). +type fakeSidecar struct { + startCalls atomic.Int32 + seedCalls atomic.Int32 + stopCalls atomic.Int32 + recordedRef registry.Ref + recordedTag string + recordedOwn string +} + +// Start implements SidecarLifecycle; returns a synthetic sidecar with +// placeholder Network/hostnames that pass through DockerRunOpts without +// ever being dialled. +func (f *fakeSidecar) Start(_ context.Context) (*Sidecar, func(context.Context) error, error) { + f.startCalls.Add(1) + sc := &Sidecar{ + Name: "fake-sidecar", + Network: "fake-net", + InNetworkHost: "fake-sidecar:5000", + HostHost: "127.0.0.1:0", + } + return sc, func(context.Context) error { + f.stopCalls.Add(1) + return nil + }, nil +} + +// SeedCore implements SidecarLifecycle; records the ref/tag so tests +// can assert the right core was requested and swallows all I/O. +func (f *fakeSidecar) SeedCore(_ context.Context, _ *Sidecar, _ registry.Store, ref registry.Ref, owner, tag string) error { + f.seedCalls.Add(1) + f.recordedRef = ref + f.recordedTag = tag + f.recordedOwn = owner + return nil +} + func TestBuildPHP_CacheHit_ShortCircuitsWithoutRunning(t *testing.T) { repo := t.TempDir() writeRepoFixture(t, repo) @@ -384,10 +439,304 @@ func TestMain_EmptyArgs_Errors(t *testing.T) { } } -func TestMain_ExtDispatch_ReturnsStub(t *testing.T) { +// TestMain_ExtDispatch_RoutesToBuildExt verifies that `phpup build ext` +// routes through to BuildExt's flag parsing — calling with no flags +// trips the --ext required error, which is BuildExt's first validation +// step. Confirms the Main switch does not fall through to an unrelated +// branch. +func TestMain_ExtDispatch_RoutesToBuildExt(t *testing.T) { err := Main([]string{"ext"}) - if err == nil || !strings.Contains(err.Error(), "not yet supported") { - t.Errorf("Main([]string{\"ext\"}) err = %v, want containing \"not yet supported\"", err) + if err == nil || !strings.Contains(err.Error(), "--ext is required") { + t.Errorf("Main([]string{\"ext\"}) err = %v, want containing \"--ext is required\"", err) + } +} + +// TestBuildExt_CacheHit_ShortCircuits seeds the layout with a +// php-ext-redis manifest annotated with the expected spec-hash and +// verifies BuildExt returns "cache hit" without starting the sidecar +// or invoking the runner. +func TestBuildExt_CacheHit_ShortCircuits(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutDir := filepath.Join(t.TempDir(), "layout") + + hash, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "ext", Name: "redis", Version: "6.2.0", + OS: "linux", Arch: "x86_64", PHPABI: "8.4-nts", TS: "nts", + Repo: repo, + }) + if err != nil { + t.Fatalf("ComputeSpecHash: %v", err) + } + layoutURI := seedLayout(t, layoutDir, "php-ext-redis", hash) + + var runnerCalled bool + restoreRunner := SetRunner(func(_ context.Context, _ *DockerRunOpts) error { + runnerCalled = true + return errors.New("runner should not be called on cache hit") + }) + defer restoreRunner() + + fs := &fakeSidecar{} + restoreSidecar := SetSidecarLifecycle(fs) + defer restoreSidecar() + + out := captureStdout(t, func() { + err = BuildExt(context.Background(), []string{ + "--ext", "redis", + "--ext-version", "6.2.0", + "--php-abi", "8.4-nts", + "--php-core-digest", "sha256:" + strings.Repeat("0", 64), + "--registry", layoutURI, + "--repo", repo, + "--out-dir", t.TempDir(), + }) + }) + if err != nil { + t.Fatalf("BuildExt: %v", err) + } + if runnerCalled { + t.Error("runner was called on cache hit") + } + if fs.startCalls.Load() != 0 { + t.Errorf("sidecar.Start called %d times on cache hit, want 0", fs.startCalls.Load()) + } + if !strings.Contains(out, "cache hit") { + t.Errorf("stdout = %q, want contains \"cache hit\"", out) + } +} + +// TestBuildExt_CacheMiss_StartsSidecarSeedsCorePushes is the integration +// cousin of TestBuildPHP_CacheMiss_InvokesRunnerThenPushes: verifies +// that on an empty layout, BuildExt (a) starts the sidecar, (b) seeds +// the prerequisite core with the expected digest+tag, (c) runs the +// build, (d) pushes the resulting bundle. All via fakes — no real +// docker, no real registry. +func TestBuildExt_CacheMiss_StartsSidecarSeedsCorePushes(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutDir := filepath.Join(t.TempDir(), "layout") + layoutURI := "oci-layout:" + layoutDir + + restoreRunner := SetRunner(fakeRunnerKind([]byte("synthetic-ext-bundle"), "php-ext")) + defer restoreRunner() + + fs := &fakeSidecar{} + restoreSidecar := SetSidecarLifecycle(fs) + defer restoreSidecar() + + coreDigest := "sha256:" + strings.Repeat("a", 64) + err := BuildExt(context.Background(), []string{ + "--ext", "redis", + "--ext-version", "6.2.0", + "--php-abi", "8.4-nts", + "--php-core-digest", coreDigest, + "--registry", layoutURI, + "--repo", repo, + "--out-dir", t.TempDir(), + }) + if err != nil { + t.Fatalf("BuildExt: %v", err) + } + + // Sidecar was started, seeded once, and stopped. + if fs.startCalls.Load() != 1 { + t.Errorf("startCalls = %d, want 1", fs.startCalls.Load()) + } + if fs.seedCalls.Load() != 1 { + t.Errorf("seedCalls = %d, want 1", fs.seedCalls.Load()) + } + if fs.stopCalls.Load() != 1 { + t.Errorf("stopCalls = %d, want 1", fs.stopCalls.Load()) + } + // SeedCore received the right ref and tag. + if fs.recordedRef.Name != "php-core" || fs.recordedRef.Digest != coreDigest { + t.Errorf("recordedRef = %+v, want {Name:php-core Digest:%s}", fs.recordedRef, coreDigest) + } + wantTag := "8.4-linux-x86_64-nts" + if fs.recordedTag != wantTag { + t.Errorf("recordedTag = %q, want %q", fs.recordedTag, wantTag) + } + if fs.recordedOwn != "buildrush" { + t.Errorf("recordedOwn = %q, want buildrush", fs.recordedOwn) + } + + // Pushed bundle is discoverable via LookupBySpec. + hash, err := ComputeSpecHash(&SpecHashInputs{ + Kind: "ext", Name: "redis", Version: "6.2.0", + OS: "linux", Arch: "x86_64", PHPABI: "8.4-nts", TS: "nts", + Repo: repo, + }) + if err != nil { + t.Fatalf("ComputeSpecHash: %v", err) + } + s, _ := registry.Open(layoutURI) + ref, hit, err := s.LookupBySpec(context.Background(), "php-ext-redis", hash) + if err != nil || !hit { + t.Fatalf("LookupBySpec after build: hit=%v err=%v", hit, err) + } + if ref.Digest == "" { + t.Error("pushed ref has empty digest") + } +} + +// TestBuildExt_RunnerError_Propagates verifies the build-container +// exit code surfaces as an error with the expected prefix. Also +// doubles as a teardown probe: fakeSidecar.stopCalls MUST still be 1 +// after the error path exits, confirming the defer fires even on +// failure. +func TestBuildExt_RunnerError_Propagates(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + layoutURI := "oci-layout:" + filepath.Join(t.TempDir(), "layout") + + restoreRunner := SetRunner(func(_ context.Context, _ *DockerRunOpts) error { + return errors.New("boom") + }) + defer restoreRunner() + + fs := &fakeSidecar{} + restoreSidecar := SetSidecarLifecycle(fs) + defer restoreSidecar() + + err := BuildExt(context.Background(), []string{ + "--ext", "redis", + "--ext-version", "6.2.0", + "--php-abi", "8.4-nts", + "--php-core-digest", "sha256:" + strings.Repeat("b", 64), + "--registry", layoutURI, + "--repo", repo, + "--out-dir", t.TempDir(), + }) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Errorf("BuildExt err = %v, want containing \"boom\"", err) + } + if fs.stopCalls.Load() != 1 { + t.Errorf("stopCalls after error = %d, want 1 (teardown defer must run)", fs.stopCalls.Load()) + } +} + +// TestBuildExt_MissingFlags_Errors sweeps through each required flag +// and verifies parseExtFlags rejects the omission with a clear message. +func TestBuildExt_MissingFlags_Errors(t *testing.T) { + cases := []struct { + name string + args []string + wantMsg string + }{ + {"no ext", []string{"--ext-version", "1", "--php-abi", "8.4-nts", "--php-core-digest", "sha256:x"}, "--ext is required"}, + {"no ext-version", []string{"--ext", "redis", "--php-abi", "8.4-nts", "--php-core-digest", "sha256:x"}, "--ext-version is required"}, + {"no php-abi", []string{"--ext", "redis", "--ext-version", "1", "--php-core-digest", "sha256:x"}, "--php-abi is required"}, + {"no php-core-digest", []string{"--ext", "redis", "--ext-version", "1", "--php-abi", "8.4-nts"}, "--php-core-digest is required"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := BuildExt(context.Background(), tc.args) + if err == nil || !strings.Contains(err.Error(), tc.wantMsg) { + t.Errorf("BuildExt err = %v, want containing %q", err, tc.wantMsg) + } + }) + } +} + +// TestBuildExt_UnknownArch_Errors ensures the arch normalisation +// rejects bogus inputs before we ever touch the sidecar or runner. +func TestBuildExt_UnknownArch_Errors(t *testing.T) { + repo := t.TempDir() + writeRepoFixture(t, repo) + err := BuildExt(context.Background(), []string{ + "--ext", "redis", "--ext-version", "6.2.0", + "--php-abi", "8.4-nts", "--php-core-digest", "sha256:x", + "--arch", "bogus", + "--registry", "oci-layout:" + filepath.Join(t.TempDir(), "layout"), + "--repo", repo, + "--out-dir", t.TempDir(), + }) + if err == nil || !strings.Contains(err.Error(), "unknown arch") { + t.Errorf("BuildExt err = %v, want unknown arch", err) + } +} + +// TestResolveExtOutDir_DefaultShape asserts the derived default path +// when --out-dir is not supplied. Shape is load-bearing because it +// participates in the gitignore contract (build/ is ignored), mirroring +// resolveOutDir (php). +func TestResolveExtOutDir_DefaultShape(t *testing.T) { + repo := t.TempDir() + opts := &extOpts{ + Name: "redis", Version: "6.2.0", PHPABI: "8.4-nts", + OS: "jammy", Arch: "x86_64", + Repo: repo, + } + got := resolveExtOutDir(opts) + want := filepath.Join(repo, "build", "ext", "redis-6.2.0-8.4-nts-jammy-x86_64") + if got != want { + t.Errorf("resolveExtOutDir = %q, want %q", got, want) + } +} + +// TestResolveExtOutDir_ExplicitOverride asserts --out-dir is honored +// verbatim (pass-through, no derivation). +func TestResolveExtOutDir_ExplicitOverride(t *testing.T) { + opts := &extOpts{OutDir: "/custom/path"} + if got := resolveExtOutDir(opts); got != "/custom/path" { + t.Errorf("resolveExtOutDir with --out-dir = %q, want /custom/path", got) + } +} + +// TestCoreTagForFetch_MatchesFetchCoreShellLogic pins the tag shape +// phpup emits against the shell logic in builders/common/fetch-core.sh +// (which constructs TAG="${PHP_VER}-${OS}-${ARCH}-${PHP_TS}"). If this +// diverges, the build container's oras pull will miss the seeded image. +func TestCoreTagForFetch_MatchesFetchCoreShellLogic(t *testing.T) { + cases := []struct { + phpABI, arch, want string + }{ + {"8.4-nts", "x86_64", "8.4-linux-x86_64-nts"}, + {"8.3-nts", "aarch64", "8.3-linux-aarch64-nts"}, + {"8.2-nts", "x86_64", "8.2-linux-x86_64-nts"}, + } + for _, tc := range cases { + if got := coreTagForFetch(tc.phpABI, tc.arch); got != tc.want { + t.Errorf("coreTagForFetch(%q, %q) = %q, want %q", tc.phpABI, tc.arch, got, tc.want) + } + } +} + +// TestLoadExtBuildDeps_JoinsLinuxList verifies loadExtBuildDeps returns +// the space-joined list that build-ext.sh's BUILD_DEPS env expects — +// matching the yq invocation in build-extension.yml. +func TestLoadExtBuildDeps_JoinsLinuxList(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "amqp.yaml") + const body = "name: amqp\nbuild_deps:\n linux:\n - libfoo-dev\n - libbar-dev\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write yaml: %v", err) + } + got, err := loadExtBuildDeps(path) + if err != nil { + t.Fatalf("loadExtBuildDeps: %v", err) + } + if got != "libfoo-dev libbar-dev" { + t.Errorf("got %q, want %q", got, "libfoo-dev libbar-dev") + } +} + +// TestLoadExtBuildDeps_AbsentSection_ReturnsEmpty exercises the +// no-build-deps path (e.g. catalog/extensions/redis.yaml originally). +// Must return "" (not error) so build-ext.sh treats it as a no-op. +func TestLoadExtBuildDeps_AbsentSection_ReturnsEmpty(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "x.yaml") + if err := os.WriteFile(path, []byte("name: x\n"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + got, err := loadExtBuildDeps(path) + if err != nil { + t.Fatalf("loadExtBuildDeps: %v", err) + } + if got != "" { + t.Errorf("got %q, want empty", got) } } diff --git a/internal/build/sidecar.go b/internal/build/sidecar.go new file mode 100644 index 0000000..06573fb --- /dev/null +++ b/internal/build/sidecar.go @@ -0,0 +1,313 @@ +package build + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os/exec" + "strings" + "sync" + "time" + + "github.com/google/go-containerregistry/pkg/name" + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/empty" + "github.com/google/go-containerregistry/pkg/v1/mutate" + "github.com/google/go-containerregistry/pkg/v1/remote" + "github.com/google/go-containerregistry/pkg/v1/static" + "github.com/google/go-containerregistry/pkg/v1/types" + + "github.com/buildrush/setup-php/internal/registry" +) + +// Sidecar is an ephemeral distribution:3 OCI registry running on an +// isolated docker network. Used by phpup build ext to stage the +// prerequisite php-core artifact so the in-container fetch-core.sh +// pulls from it without any script change. +type Sidecar struct { + // Name is the docker container name assigned at start time. + Name string + // Network is the docker network the sidecar is attached to. Build + // containers MUST join this network to reach the sidecar by name. + Network string + // InNetworkHost is the hostname:port other containers on Network + // should dial to reach the sidecar (e.g. "phpup-sidecar-…:5000"). + // Not reachable from the host. + InNetworkHost string + // HostHost is the hostname:port reachable from the host process, + // used by go-containerregistry remote.Write to seed the sidecar. + // Typically "127.0.0.1:". + HostHost string +} + +// SidecarLifecycle is the package-level factory used by BuildExt. Tests +// override via SetSidecarLifecycle to inject a fake that doesn't touch +// real docker. Start returns the running sidecar and a stop function the +// caller MUST defer to tear down both the container and the network. +type SidecarLifecycle interface { + Start(ctx context.Context) (*Sidecar, func(context.Context) error, error) + SeedCore(ctx context.Context, sc *Sidecar, source registry.Store, ref registry.Ref, owner, tag string) error +} + +// sidecarLifecycleMu protects sidecarLifecycle during SetSidecarLifecycle +// swaps. Swaps are rare (test setup/teardown only) so the mutex cost is +// negligible, but it rules out a data race when two tests happen to swap +// concurrently. Tests that call SetSidecarLifecycle MUST NOT use +// t.Parallel() because the lifecycle is a package-level global. +var sidecarLifecycleMu sync.Mutex + +// sidecarLifecycle is the SidecarLifecycle BuildExt dispatches through +// when no test has overridden it via SetSidecarLifecycle. +var sidecarLifecycle SidecarLifecycle = defaultSidecarLifecycle{} + +// SetSidecarLifecycle swaps the package-level SidecarLifecycle and +// returns a restore function that callers MUST defer to revert. +// The typical test pattern is: +// +// restore := SetSidecarLifecycle(myFake) +// defer restore() +// +// The package-level state means tests that call SetSidecarLifecycle must +// not run in parallel — they share one global lifecycle. +func SetSidecarLifecycle(l SidecarLifecycle) func() { + sidecarLifecycleMu.Lock() + prev := sidecarLifecycle + sidecarLifecycle = l + sidecarLifecycleMu.Unlock() + return func() { + sidecarLifecycleMu.Lock() + sidecarLifecycle = prev + sidecarLifecycleMu.Unlock() + } +} + +// currentSidecarLifecycle returns the installed SidecarLifecycle under +// lock so callers observe a consistent value even if a swap is in +// progress. +func currentSidecarLifecycle() SidecarLifecycle { + sidecarLifecycleMu.Lock() + defer sidecarLifecycleMu.Unlock() + return sidecarLifecycle +} + +// defaultSidecarLifecycle is the production SidecarLifecycle. Its Start +// method shells out to `docker` to run distribution:3 on a fresh +// network; SeedCore pushes the prerequisite bundle via remote.Write. +type defaultSidecarLifecycle struct{} + +// Start spins up a distribution:3 container on a fresh network and +// waits for its /v2/ endpoint to become reachable. Returns the +// *Sidecar and a stop function the caller MUST defer to tear down +// both the container and the network. +func (defaultSidecarLifecycle) Start(ctx context.Context) (*Sidecar, func(context.Context) error, error) { + // Ephemeral unique names to avoid collision across concurrent + // runs. Timestamp in UTC so the name is deterministic at the + // nanosecond level; strip the "." from the fractional-second + // separator so the result fits docker's name constraints. + tag := strings.ReplaceAll(time.Now().UTC().Format("20060102T150405.000000000"), ".", "") + network := "phpup-build-" + tag + containerName := "phpup-sidecar-" + tag + + if err := dockerCmdCombined(ctx, "network", "create", network); err != nil { + return nil, nil, fmt.Errorf("sidecar: create network: %w", err) + } + + // --publish 127.0.0.1::5000 asks docker to pick a free host port + // bound to the loopback — the sidecar is intentionally not exposed + // on the public interface; it's only reachable by the host process + // and by build containers on the same docker network. + runOut, err := dockerCmdOutput(ctx, + "run", "-d", "--rm", + "--name", containerName, + "--network", network, + "--publish", "127.0.0.1::5000", + "distribution/distribution:3", + ) + if err != nil { + _ = dockerCmdCombined(context.Background(), "network", "rm", network) + return nil, nil, fmt.Errorf("sidecar: run registry: %w (output: %s)", err, runOut) + } + port, err := dockerPublishedPort(ctx, containerName, 5000) + if err != nil { + _ = dockerCmdCombined(context.Background(), "rm", "-f", containerName) + _ = dockerCmdCombined(context.Background(), "network", "rm", network) + return nil, nil, fmt.Errorf("sidecar: inspect port: %w", err) + } + + sc := &Sidecar{ + Name: containerName, + Network: network, + InNetworkHost: containerName + ":5000", + HostHost: "127.0.0.1:" + port, + } + + if err := waitForRegistry(ctx, sc.HostHost); err != nil { + _ = dockerCmdCombined(context.Background(), "rm", "-f", containerName) + _ = dockerCmdCombined(context.Background(), "network", "rm", network) + return nil, nil, fmt.Errorf("sidecar: wait: %w", err) + } + + stop := func(stopCtx context.Context) error { + var errs []error + if err := dockerCmdCombined(stopCtx, "rm", "-f", containerName); err != nil { + errs = append(errs, err) + } + if err := dockerCmdCombined(stopCtx, "network", "rm", network); err != nil { + errs = append(errs, err) + } + if len(errs) > 0 { + return fmt.Errorf("sidecar: stop: %v", errs) + } + return nil + } + return sc, stop, nil +} + +// SeedCore copies a php-core bundle from source into the sidecar at +// "//php-core:". Reads the source bundle via +// source.Fetch, builds a two-layer OCI image (matching layoutStore.Push's +// shape so oras-pulling the seeded image from the sidecar produces bytes +// identical to what the source store returns), and pushes via +// remote.Write with name.Insecure since distribution:3 serves HTTP by +// default. +func (defaultSidecarLifecycle) SeedCore(ctx context.Context, sc *Sidecar, source registry.Store, ref registry.Ref, owner, tag string) error { + rc, meta, err := source.Fetch(ctx, ref) + if err != nil { + return fmt.Errorf("sidecar.SeedCore: fetch source: %w", err) + } + defer func() { _ = rc.Close() }() + bundle, err := io.ReadAll(rc) + if err != nil { + return fmt.Errorf("sidecar.SeedCore: read source bundle: %w", err) + } + + img, err := buildTwoLayerImage(bundle, meta) + if err != nil { + return err + } + + // distribution:3 speaks HTTP by default, so name.Insecure is required; + // remote.Write falls back to HTTP when the target ref is parsed with + // name.Insecure. + target, err := name.ParseReference(sc.HostHost+"/"+owner+"/php-core:"+tag, name.Insecure) + if err != nil { + return fmt.Errorf("sidecar.SeedCore: parse ref: %w", err) + } + if err := remote.Write(target, img, remote.WithContext(ctx)); err != nil { + return fmt.Errorf("sidecar.SeedCore: push: %w", err) + } + return nil +} + +// buildTwoLayerImage assembles the OCI image shape that layoutStore.Push +// uses for bundles: layer 0 carries the raw bundle bytes; layer 1 +// (when meta is non-nil) carries the serialised meta sidecar. Keeping +// the shape in sync lets fetch-core.sh in the build container pull +// from the sidecar with oras and get byte-identical output to what +// registry.Store.Fetch would return directly. +func buildTwoLayerImage(bundle []byte, meta *registry.Meta) (v1.Image, error) { + img := emptyImage() + bundleLayer := static.NewLayer(bundle, types.OCILayer) + var err error + img, err = mutate.AppendLayers(img, bundleLayer) + if err != nil { + return nil, fmt.Errorf("sidecar: append bundle layer: %w", err) + } + if meta != nil { + metaBytes, err := marshalMeta(meta) + if err != nil { + return nil, fmt.Errorf("sidecar: marshal meta: %w", err) + } + metaLayer := static.NewLayer(metaBytes, types.OCILayer) + img, err = mutate.AppendLayers(img, metaLayer) + if err != nil { + return nil, fmt.Errorf("sidecar: append meta layer: %w", err) + } + } + return img, nil +} + +// execDocker runs docker with the given args and returns the combined +// stdout/stderr. G204 is a genuine false positive: exec.CommandContext +// passes argv directly to execve(2) (no shell), and all args come from +// typed sources within this package (static strings + lifecycle-state +// fields) — the wrapper's purpose is precisely to spawn docker with +// dynamic argv, so a fixed argv is impossible by design. +func execDocker(ctx context.Context, args ...string) ([]byte, error) { + return exec.CommandContext(ctx, "docker", args...).CombinedOutput() //nolint:gosec // G204 false positive: exec.CommandContext passes argv directly to execve(2) (no shell), and all args come from typed sources in this package. +} + +// dockerCmdCombined runs a docker command and returns its combined +// output folded into the error. Used for commands whose stdout is +// diagnostic rather than load-bearing. +func dockerCmdCombined(ctx context.Context, args ...string) error { + out, err := execDocker(ctx, args...) + if err != nil { + return fmt.Errorf("docker %s: %w (%s)", strings.Join(args, " "), err, string(out)) + } + return nil +} + +// dockerCmdOutput runs a docker command and returns its trimmed +// combined output. Used where the caller wants the stdout (e.g. the +// container id returned by `docker run -d`). +func dockerCmdOutput(ctx context.Context, args ...string) (string, error) { + out, err := execDocker(ctx, args...) + return strings.TrimSpace(string(out)), err +} + +// dockerPublishedPort inspects the container and returns the published +// host port that maps to containerPort. Uses docker's go-template +// formatter so we don't need to parse JSON. +func dockerPublishedPort(ctx context.Context, container string, containerPort int) (string, error) { + fmtFlag := fmt.Sprintf(`{{(index (index .NetworkSettings.Ports "%d/tcp") 0).HostPort}}`, containerPort) + out, err := execDocker(ctx, "inspect", "--format", fmtFlag, container) + if err != nil { + return "", fmt.Errorf("inspect: %w (%s)", err, string(out)) + } + return strings.TrimSpace(string(out)), nil +} + +// waitForRegistry polls the sidecar's /v2/ endpoint until it returns a +// non-5xx response or the 30-second deadline elapses. distribution:3 +// returns 200 for anonymous /v2/ as soon as it's listening; a 4xx +// would also mean "up and serving", so we only treat 5xx as "still +// starting". +func waitForRegistry(ctx context.Context, host string) error { + url := "http://" + host + "/v2/" + deadline := time.Now().Add(30 * time.Second) + for time.Now().Before(deadline) { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + req, err := http.NewRequestWithContext(ctx, http.MethodHead, url, http.NoBody) + if err != nil { + return fmt.Errorf("build request: %w", err) + } + resp, err := http.DefaultClient.Do(req) + if err == nil { + _ = resp.Body.Close() + if resp.StatusCode < 500 { + return nil + } + } + time.Sleep(200 * time.Millisecond) + } + return fmt.Errorf("registry at %s did not become healthy within 30s", host) +} + +// emptyImage returns a fresh v1.Image to append layers to. Kept as a +// function (not a constant) so tests can swap if needed and so the +// import of pkg/v1/empty lives at exactly one site. +func emptyImage() v1.Image { return empty.Image } + +// marshalMeta serialises a registry.Meta for embedding as the second +// layer of a seeded image. Kept separate from buildTwoLayerImage so +// the error path is localised and the marshal can be mocked if needed. +func marshalMeta(m *registry.Meta) ([]byte, error) { + return json.Marshal(m) +} diff --git a/internal/build/sidecar_test.go b/internal/build/sidecar_test.go new file mode 100644 index 0000000..934a2e4 --- /dev/null +++ b/internal/build/sidecar_test.go @@ -0,0 +1,239 @@ +package build + +import ( + "bytes" + "context" + "encoding/json" + "io" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/google/go-containerregistry/pkg/name" + "github.com/google/go-containerregistry/pkg/v1/remote" + + "github.com/buildrush/setup-php/internal/registry" +) + +// TestSetSidecarLifecycle_SwapAndRestore verifies the package-level +// swap primitive used by tests. Two calls stack LIFO: the inner +// restore returns the lifecycle to whatever was set before the inner +// SetSidecarLifecycle call, not all the way back to the original. +func TestSetSidecarLifecycle_SwapAndRestore(t *testing.T) { + original := currentSidecarLifecycle() + first := &fakeSidecar{} + second := &fakeSidecar{} + + restoreFirst := SetSidecarLifecycle(first) + if got := currentSidecarLifecycle(); got != first { + t.Errorf("after first swap, currentSidecarLifecycle = %T, want fakeSidecar (first)", got) + } + restoreSecond := SetSidecarLifecycle(second) + if got := currentSidecarLifecycle(); got != second { + t.Errorf("after second swap, currentSidecarLifecycle = %T, want fakeSidecar (second)", got) + } + restoreSecond() + if got := currentSidecarLifecycle(); got != first { + t.Errorf("after restoreSecond, currentSidecarLifecycle = %T, want first fake", got) + } + restoreFirst() + if got := currentSidecarLifecycle(); got != original { + t.Errorf("after restoreFirst, currentSidecarLifecycle = %T, want original", got) + } +} + +// TestDockerCmdCombined_BadBinary_ReturnsError pokes the thin +// error-wrapping path by invoking `docker` with an obviously-bogus +// subcommand. Works whether or not docker is installed because we +// just need the exec to fail — either "docker: executable not found" +// or "docker: 'bogus-subcommand' is not a docker command" both +// satisfy the assertion. +func TestDockerCmdCombined_BadBinary_ReturnsError(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + err := dockerCmdCombined(ctx, "totally-bogus-subcommand-"+strings.Repeat("x", 8)) + if err == nil { + t.Fatal("dockerCmdCombined on bogus args returned nil, want error") + } + if !strings.Contains(err.Error(), "docker") { + t.Errorf("err = %v, want containing \"docker\"", err) + } +} + +// TestWaitForRegistry_UnreachableHost_ExpiresPromptlyUnderCancel +// asserts that context cancellation short-circuits the 30-second +// polling window. Without cancellation this would block for 30s; we +// give the poll 500ms of head-start, then cancel and require the +// function to return within a second. +func TestWaitForRegistry_UnreachableHost_ExpiresPromptlyUnderCancel(t *testing.T) { + // A port that's never listening on loopback. 127.0.0.1:1 is the + // standard "you will not get a connection here" choice. + ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) + go func() { errCh <- waitForRegistry(ctx, "127.0.0.1:1") }() + time.Sleep(300 * time.Millisecond) + cancel() + select { + case err := <-errCh: + if err == nil { + t.Error("waitForRegistry returned nil after cancel, want error") + } + case <-time.After(2 * time.Second): + t.Fatal("waitForRegistry did not return within 2s of cancel") + } +} + +// TestBuildTwoLayerImage_IncludesMetaWhenSet asserts the two-layer +// shape buildTwoLayerImage produces: layer 0 is the bundle bytes, +// layer 1 is the marshalled meta. This matches layoutStore.Push's +// shape (see internal/registry/layout.go) so oras-pulling from a +// sidecar yields identical bytes to a direct source.Fetch. +func TestBuildTwoLayerImage_IncludesMetaWhenSet(t *testing.T) { + meta := ®istry.Meta{SchemaVersion: 3, Kind: "php-core"} + img, err := buildTwoLayerImage([]byte("bundle-bytes"), meta) + if err != nil { + t.Fatalf("buildTwoLayerImage: %v", err) + } + layers, err := img.Layers() + if err != nil { + t.Fatalf("Layers: %v", err) + } + if len(layers) != 2 { + t.Fatalf("len(layers) = %d, want 2", len(layers)) + } + rc0, err := layers[0].Compressed() + if err != nil { + t.Fatalf("Compressed[0]: %v", err) + } + defer func() { _ = rc0.Close() }() + got0, _ := io.ReadAll(rc0) + if !bytes.Equal(got0, []byte("bundle-bytes")) { + t.Errorf("layer[0] = %q, want %q", got0, "bundle-bytes") + } + rc1, err := layers[1].Compressed() + if err != nil { + t.Fatalf("Compressed[1]: %v", err) + } + defer func() { _ = rc1.Close() }() + got1, _ := io.ReadAll(rc1) + parsed := ®istry.Meta{} + if err := json.Unmarshal(got1, parsed); err != nil { + t.Fatalf("parse meta layer: %v", err) + } + if parsed.SchemaVersion != 3 || parsed.Kind != "php-core" { + t.Errorf("meta = %+v, want {3 php-core}", parsed) + } +} + +// TestBuildTwoLayerImage_NilMeta_OneLayer verifies that omitting meta +// (nil) produces a single-layer image. Matches layoutStore.Push's +// legacy-bundle shape so seeding from a store that returns nil Meta +// still round-trips. +func TestBuildTwoLayerImage_NilMeta_OneLayer(t *testing.T) { + img, err := buildTwoLayerImage([]byte("only-bundle"), nil) + if err != nil { + t.Fatalf("buildTwoLayerImage: %v", err) + } + layers, err := img.Layers() + if err != nil { + t.Fatalf("Layers: %v", err) + } + if len(layers) != 1 { + t.Fatalf("len(layers) = %d, want 1", len(layers)) + } +} + +// TestSidecar_LifecycleAndSeed_Real is the only test in this file that +// exercises real docker + a real distribution:3 container. Skipped +// under -short and when docker is absent, so CI without docker is +// unaffected. Pulls distribution:3 on first run (~50MB); subsequent +// runs hit the local image cache. +func TestSidecar_LifecycleAndSeed_Real(t *testing.T) { + if testing.Short() { + t.Skip("skipping real sidecar smoke under -short") + } + if _, err := exec.LookPath("docker"); err != nil { + t.Skipf("docker not found in PATH: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + sc, stop, err := defaultSidecarLifecycle{}.Start(ctx) + if err != nil { + t.Fatalf("Start: %v", err) + } + defer func() { + stopCtx, scancel := context.WithTimeout(context.Background(), 30*time.Second) + defer scancel() + _ = stop(stopCtx) + }() + + // Shape check on the sidecar fields. + if sc.Name == "" || sc.Network == "" || sc.InNetworkHost == "" || sc.HostHost == "" { + t.Fatalf("sidecar has empty fields: %+v", sc) + } + if !strings.HasPrefix(sc.HostHost, "127.0.0.1:") { + t.Errorf("HostHost = %q, want 127.0.0.1:", sc.HostHost) + } + + // Seed a fake bundle from a temp layout. + sourceDir := filepath.Join(t.TempDir(), "layout") + source, err := registry.Open("oci-layout:" + sourceDir) + if err != nil { + t.Fatalf("open source layout: %v", err) + } + bundlePayload := []byte("fake-core-bundle") + if err := source.Push(ctx, registry.Ref{Name: "php-core"}, + bytes.NewReader(bundlePayload), + ®istry.Meta{SchemaVersion: 3, Kind: "php-core"}, + registry.Annotations{BundleName: "php-core", SpecHash: "sha256:integration-test"}); err != nil { + t.Fatalf("seed source: %v", err) + } + // Resolve the source digest so SeedCore can Fetch it back. + fetchedRef, hit, err := source.LookupBySpec(ctx, "php-core", "sha256:integration-test") + if err != nil || !hit { + t.Fatalf("LookupBySpec: hit=%v err=%v", hit, err) + } + + if err := (defaultSidecarLifecycle{}).SeedCore(ctx, sc, source, fetchedRef, "buildrush", "test-tag"); err != nil { + t.Fatalf("SeedCore: %v", err) + } + + // Verify the seeded image is reachable on the sidecar's host port + // by pulling it back with go-containerregistry. name.Insecure + // because distribution:3 serves HTTP. + target, err := name.ParseReference(sc.HostHost+"/buildrush/php-core:test-tag", name.Insecure) + if err != nil { + t.Fatalf("parse ref: %v", err) + } + desc, err := remote.Get(target, remote.WithContext(ctx)) + if err != nil { + t.Fatalf("remote.Get: %v", err) + } + img, err := desc.Image() + if err != nil { + t.Fatalf("desc.Image: %v", err) + } + layers, err := img.Layers() + if err != nil { + t.Fatalf("Layers: %v", err) + } + if len(layers) < 1 { + t.Fatal("seeded image has no layers") + } + rc, err := layers[0].Compressed() + if err != nil { + t.Fatalf("Compressed: %v", err) + } + defer func() { _ = rc.Close() }() + got, err := io.ReadAll(rc) + if err != nil { + t.Fatalf("ReadAll: %v", err) + } + if !bytes.Equal(got, bundlePayload) { + t.Errorf("pulled bundle = %q, want %q", got, bundlePayload) + } +} From ebf196e28f28314293682b19be9b540d519b4367 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 16:40:29 +0200 Subject: [PATCH 10/11] chore(ci): rewire Makefile bundle-* + build-php-core.yml to phpup build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makefile's bundle-php / bundle-ext targets now invoke bin/phpup build instead of raw `docker run ubuntu:22.04 bash -c "... build-*.sh"`. Defaults match the hermetic dev loop (--registry oci-layout:./out/oci-layout); users override with REGISTRY=... for remote targets. A new bin/phpup file target depends on the embedded bundles.lock so make auto-rebuilds the binary when the lockfile changes. build-php-core.yml now runs the core build via `bin/phpup build php` (docker-wrapped builders/linux/build-php.sh, unchanged), writes into a project-relative output dir, then a thin "Stage bundle for publish" step copies bundle.tar.zst + .sha256 + meta.json to /tmp/ so the existing Push to GHCR, Sign bundle, Smoke test, and Upload bundle artifact steps read from the same paths they did before — preserving the `digest` job-output contract (sha256 of the bundle layer) byte- for-byte. build-extension.yml is intentionally not rewired in this commit: phpup build ext requires a workflow-supplied --php-core-digest and works against a store whose Push supports remote backends. Both gaps land in follow-up PRs (planner surfacing core-digest to the ext matrix, and internal/registry.remoteStore.Push). Left as a separate task so this wiring stays scoped. --- .github/workflows/build-php-core.yml | 39 +++++++++++++++-- Makefile | 64 ++++++++++++++++++++-------- 2 files changed, 82 insertions(+), 21 deletions(-) diff --git a/.github/workflows/build-php-core.yml b/.github/workflows/build-php-core.yml index 9ede6f1..993627d 100644 --- a/.github/workflows/build-php-core.yml +++ b/.github/workflows/build-php-core.yml @@ -49,12 +49,45 @@ jobs: curl -sSfLO "https://github.com/oras-project/oras/releases/download/v1.3.1/oras_1.3.1_linux_${ORAS_ARCH}.tar.gz" tar -xzf "oras_1.3.1_linux_${ORAS_ARCH}.tar.gz" -C /usr/local/bin oras - - name: Build PHP + - uses: actions/setup-go@v6 + with: + go-version: '1.26' + + - name: Build phpup + run: make bin/phpup + + # Run the PHP-core build via phpup. phpup docker-wraps + # builders/linux/build-php.sh unchanged and writes the resulting + # bundle.tar.zst + meta.json + bundle.tar.zst.sha256 into a + # project-relative output dir. The subsequent "Stage bundle for + # publish" step copies them to /tmp/ so "Push to GHCR", + # "Sign bundle", "Smoke test", and "Upload bundle artifact" keep + # reading from the same paths they did before this rewiring — + # preserving the digest job-output contract byte-for-byte. + - name: Build PHP core via phpup env: PHP_VERSION: ${{ inputs.version }} + OS: ${{ inputs.os }} ARCH: ${{ inputs.arch }} - WORKSPACE: ${{ github.workspace }} - run: ./builders/linux/build-php.sh + TS: ${{ inputs.ts }} + PHPUP_OUT_DIR: ${{ github.workspace }}/build/php/${{ inputs.version }}-${{ inputs.os }}-${{ inputs.arch }}-${{ inputs.ts }} + run: | + ./bin/phpup build php \ + --php "$PHP_VERSION" \ + --os "$OS" \ + --arch "$ARCH" \ + --ts "$TS" \ + --registry oci-layout:./out/oci-layout \ + --repo . \ + --out-dir "$PHPUP_OUT_DIR" + + - name: Stage bundle for publish + env: + PHPUP_OUT_DIR: ${{ github.workspace }}/build/php/${{ inputs.version }}-${{ inputs.os }}-${{ inputs.arch }}-${{ inputs.ts }} + run: | + cp "$PHPUP_OUT_DIR/bundle.tar.zst" /tmp/bundle.tar.zst + cp "$PHPUP_OUT_DIR/bundle.tar.zst.sha256" /tmp/bundle.tar.zst.sha256 + cp "$PHPUP_OUT_DIR/meta.json" /tmp/meta.json - name: Push to GHCR id: push diff --git a/Makefile b/Makefile index b63b096..90f889d 100644 --- a/Makefile +++ b/Makefile @@ -2,10 +2,23 @@ build-linux-amd64 build-linux-arm64 bundle-php bundle-ext gc-bundles-dry-run \ local-ci +# Path to the native phpup binary used by bundle-php / bundle-ext. Overridable +# so CI / power users can point at a pre-built binary. +PHPUP_BIN ?= bin/phpup + # Ensure the embedded lockfile is available for go vet/test/build cmd/phpup/bundles.lock: bundles.lock @cp bundles.lock cmd/phpup/bundles.lock +# Local native build of phpup used by bundle-php / bundle-ext. Kept as a +# file target so Make only rebuilds on demand. The embedded lockfile is the +# sole declared dependency because cmd/phpup's own source files change +# rarely enough that a `make clean` + `make bin/phpup` is acceptable; if +# this becomes a friction point, widen the deps to cmd/phpup/*.go. +$(PHPUP_BIN): cmd/phpup/bundles.lock + @mkdir -p $(dir $(PHPUP_BIN)) + go build -o $(PHPUP_BIN) ./cmd/phpup + # Full pre-push check: static analysis + tests + builds + a docker smoke that # exercises the published bundles on both jammy and noble runners. Takes ~5 # minutes when the bundle caches are cold. Use check-fast for rapid iteration @@ -83,24 +96,39 @@ build-linux-arm64: GOOS=linux GOARCH=arm64 CGO_ENABLED=0 go build -o bin/phpup-linux-arm64 ./cmd/phpup GOOS=linux GOARCH=arm64 CGO_ENABLED=0 go build -o bin/planner-linux-arm64 ./cmd/planner -# Build a PHP core bundle locally via Docker -bundle-php: - docker run --rm \ - -v $$(pwd):/workspace -w /workspace \ - -e PHP_VERSION=$(PHP_VERSION) \ - -e ARCH=$(or $(ARCH),x86_64) \ - ubuntu:22.04 \ - bash -c "apt-get update && apt-get install -y curl xz-utils && ./builders/linux/build-php.sh" - -# Build an extension bundle locally via Docker -bundle-ext: - docker run --rm \ - -v $$(pwd):/workspace -w /workspace \ - -e EXT_NAME=$(EXT_NAME) \ - -e EXT_VERSION=$(EXT_VERSION) \ - -e PHP_ABI=$(PHP_ABI) \ - ubuntu:22.04 \ - bash -c "apt-get update && apt-get install -y curl && ./builders/linux/build-ext.sh" +# Build a PHP core bundle locally via phpup (docker-wrapped under the hood). +# Invocation: +# make bundle-php PHP_VERSION=8.4 [OS=jammy] [ARCH=x86_64] [TS=nts] \ +# [REGISTRY=oci-layout:./out/oci-layout] +# phpup docker-wraps builders/linux/build-php.sh unchanged and writes the +# resulting OCI bundle into the target registry. +bundle-php: $(PHPUP_BIN) + $(PHPUP_BIN) build php \ + --php $(PHP_VERSION) \ + --os $(or $(OS),jammy) \ + --arch $(or $(ARCH),x86_64) \ + --ts $(or $(TS),nts) \ + --registry $(or $(REGISTRY),oci-layout:./out/oci-layout) \ + --repo . + +# Build an extension bundle locally via phpup (docker-wrapped under the hood). +# Invocation: +# make bundle-ext EXT_NAME=redis EXT_VERSION=6.2.0 PHP_ABI=8.4-nts \ +# PHP_CORE_DIGEST=sha256:… \ +# [OS=jammy] [ARCH=x86_64] \ +# [REGISTRY=oci-layout:./out/oci-layout] +# Requires the prerequisite php-core already in REGISTRY (run `make bundle-php` +# first, or point REGISTRY at a remote where the core is published). +bundle-ext: $(PHPUP_BIN) + $(PHPUP_BIN) build ext \ + --ext $(EXT_NAME) \ + --ext-version $(EXT_VERSION) \ + --php-abi $(PHP_ABI) \ + --arch $(or $(ARCH),x86_64) \ + --os $(or $(OS),jammy) \ + --php-core-digest $(PHP_CORE_DIGEST) \ + --registry $(or $(REGISTRY),oci-layout:./out/oci-layout) \ + --repo . # Clean build artifacts clean: From 054cec80c5b5cbb6240a1b866081383976049054 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 17:42:06 +0200 Subject: [PATCH 11/11] fix(build): chmod 1777 on docker bind-mount outDir (fixes GHA apt-key) TestBuildPHP_RealDockerSmoke failed on GitHub Actions linux runners with apt-key unable to create temporary config files. The outDir mounted into the build container at /tmp was mode 0o750, which the container-internal _apt user (dropped-privilege, different uid from host) couldn't write to. Docker Desktop on macOS transparently maps uids so the test passed locally; GHA enforces real mount semantics. Fix: prepareOutDir now chmods the directory to 1777 (world-writable + sticky) matching traditional /tmp semantics. This is exactly the intended use (the dir IS a /tmp replacement for a docker container); justification comment on the //nolint:gosec accompanies the chmod. --- internal/build/build.go | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/internal/build/build.go b/internal/build/build.go index 148fb6b..406b9d0 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -560,16 +560,35 @@ func resolveOutDir(opts *phpOpts) string { return filepath.Join(opts.Repo, "build", "php", slug) } -// prepareOutDir wipes any previous content at path (so stale files from an -// aborted earlier run don't masquerade as fresh output) and creates a -// clean directory. +// prepareOutDir wipes any previous content at path (so stale files from +// an aborted earlier run don't masquerade as fresh output) and creates +// a clean directory with mode 1777. The 1777 mode matches traditional +// /tmp semantics — necessary because this directory is bind-mounted +// into the build container at /tmp, and container-internal unprivileged +// users (notably apt-key, which drops to _apt) need write access. +// Without this, apt-key cannot create its temporary config files and +// `apt-get update` fails with "Couldn't create temporary file" on +// hosts where Docker doesn't transparently map uids (e.g., GitHub +// Actions linux runners, non-Docker-Desktop setups). func prepareOutDir(path string) error { if err := os.RemoveAll(path); err != nil { return fmt.Errorf("clean out dir: %w", err) } + // MkdirAll at a conservative 0o750 first (keeps gosec G301 quiet for + // the creation step), then Chmod up to 1777 below. MkdirAll also + // respects umask and never sets sticky/setuid/setgid bits, so a + // separate Chmod is required regardless. if err := os.MkdirAll(path, 0o750); err != nil { return fmt.Errorf("create out dir: %w", err) } + // 1777 is a genuine requirement of the docker bind-mount use case + // (see comment above); gosec G301 flags world-writable as suspicious + // but the /tmp-style bind mount is exactly the intended use — this + // directory IS a /tmp replacement for a container. + //nolint:gosec // G301: world-writable is required for /tmp-style docker bind mount; see func comment. + if err := os.Chmod(path, 0o1777); err != nil { + return fmt.Errorf("chmod out dir to 1777: %w", err) + } return nil }