From 56655141200ec33698051d7616ad8f40174e0ffc Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 18:02:31 +0200 Subject: [PATCH 1/5] feat(registry): implement remoteStore.Push to GHCR remote.Push was ErrUnsupported in PR 1+2; this completes the remote backend so `phpup build ext --registry ghcr.io/` can publish ext bundles directly (PR 2's deferred follow-up). Design: Ref gains a Tag field used by remote push only. OCI remotes are tag-addressed for writes (the registry computes the digest from the manifest; callers cannot supply one), so remoteStore.Push needs a tag and errors out with a clear message if Ref.Tag is empty. layoutStore ignores Ref.Tag because its Push writes by index annotation, not by tag. Byte-identity with the existing `oras push` command in build-php-core.yml / build-extension.yml is a hard requirement: - layer 0: application/vnd.oci.image.layer.v1.tar+zstd (bundle) - layer 1: application/vnd.buildrush.meta.v1+json (meta sidecar) - manifest annotation org.opencontainers.artifact.type set to application/vnd.buildrush.php-core.v1 for php-core and application/vnd.buildrush.php-ext.v1 for php-ext-* Keeping these strings byte-identical with the CI path means cosign and downstream OCI clients see remoteStore-pushed bundles as indistinguishable from oras-pushed ones. Tests round-trip Push+ResolveDigest+Fetch through the in-process pkg/registry and assert both the artifact-type annotation and the layer media types match the CI contract. Known follow-up: internal/build/sidecar.go's SeedCore still builds its OCI image by hand via buildTwoLayerImage; it could now delegate to remoteStore.Push. Left as-is to keep this task's scope tight. --- internal/registry/media_types.go | 44 ++++++++ internal/registry/registry.go | 25 ++++- internal/registry/registry_test.go | 18 +++ internal/registry/remote.go | 78 +++++++++++-- internal/registry/remote_test.go | 174 ++++++++++++++++++++++++++++- 5 files changed, 321 insertions(+), 18 deletions(-) create mode 100644 internal/registry/media_types.go diff --git a/internal/registry/media_types.go b/internal/registry/media_types.go new file mode 100644 index 0000000..794d7bd --- /dev/null +++ b/internal/registry/media_types.go @@ -0,0 +1,44 @@ +package registry + +import "strings" + +// Media types and artifact types for bundles published to a remote registry. +// +// Keeping these byte-identical to what the existing `oras push` commands in +// .github/workflows/build-php-core.yml and .github/workflows/build-extension.yml +// emit is a hard requirement: a remoteStore-pushed artifact must be +// indistinguishable from an oras-pushed one, because downstream tooling +// (cosign signing, OCI clients, human operators running `oras discover`) +// keys off these strings. +const ( + // mediaTypePhpCoreArtifact is the oras --artifact-type for php-core + // bundles. Matches build-php-core.yml. + mediaTypePhpCoreArtifact = "application/vnd.buildrush.php-core.v1" + // mediaTypePhpExtArtifact is the oras --artifact-type for php-ext-* + // bundles. Matches build-extension.yml. + mediaTypePhpExtArtifact = "application/vnd.buildrush.php-ext.v1" + // mediaTypeBundleLayer is the OCI media type for the bundle tar.zst + // blob. Matches both build-*.yml files. + mediaTypeBundleLayer = "application/vnd.oci.image.layer.v1.tar+zstd" + // mediaTypeMetaSidecar is the media type for the meta.json sidecar. + // Matches both build-*.yml files. + mediaTypeMetaSidecar = "application/vnd.buildrush.meta.v1+json" + // annotationArtifactType is the OCI manifest annotation key `oras push` + // writes when invoked with --artifact-type. Replayed here so remote + // pushes carry the same annotation shape as the CI path. + annotationArtifactType = "org.opencontainers.artifact.type" +) + +// artifactTypeForBundle maps a bundle Name to the artifact-type annotation +// value that `oras push --artifact-type ` would set. +// +// The split mirrors the workflow files: php-ext- → phpExt, every +// other name → phpCore. The default keeps php-core + any future php-tool-* +// bundles on the phpCore artifact-type, matching pre-merge behavior in the +// existing CI path. +func artifactTypeForBundle(name string) string { + if strings.HasPrefix(name, "php-ext-") { + return mediaTypePhpExtArtifact + } + return mediaTypePhpCoreArtifact +} diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 052a55b..df6c2b5 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -26,20 +26,33 @@ import ( var ErrUnsupported = errors.New("registry: operation not supported by this backend") // Ref identifies a bundle within a Store by its logical Name and, optionally, -// its content-addressed Digest (in the usual "sha256:..." form). +// its content-addressed Digest (in the usual "sha256:..." form) or its +// human-readable Tag. +// +// Digest is used by Fetch/Has/LookupBySpec on every backend — those operations +// are content-addressed regardless of publication channel. Tag is a +// publication-time concern: the remote backend needs it for Push because OCI +// registries are tag-addressed for writes (the registry computes the digest +// from the manifest; callers cannot supply one). The layout backend ignores +// Tag because its Push writes by index annotation, not by tag. type Ref struct { Name string Digest string + Tag string } -// String renders the Ref as "name@digest" when a Digest is present, or just -// "name" otherwise. It is suitable for logs and error messages; it is not a -// canonical OCI reference. +// String renders the Ref as "name@digest" when a Digest is present, +// "name:tag" when only a Tag is present, or just "name" otherwise. It is +// suitable for logs and error messages; it is not a canonical OCI reference. func (r Ref) String() string { - if r.Digest == "" { + switch { + case r.Digest != "": + return r.Name + "@" + r.Digest + case r.Tag != "": + return r.Name + ":" + r.Tag + default: return r.Name } - return r.Name + "@" + r.Digest } // Meta describes bundle metadata persisted alongside the payload. Fields are diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go index 27bbed3..a0c1539 100644 --- a/internal/registry/registry_test.go +++ b/internal/registry/registry_test.go @@ -126,3 +126,21 @@ func TestRefString_EmptyDigest(t *testing.T) { t.Errorf("Ref.String() = %q, want %q", got, "php-core") } } + +func TestRefString_WithTag(t *testing.T) { + r := Ref{Name: "php-core", Tag: "8.4-linux-x86_64-nts"} + if got := r.String(); got != "php-core:8.4-linux-x86_64-nts" { + t.Errorf("Ref.String() = %q, want %q", got, "php-core:8.4-linux-x86_64-nts") + } +} + +// TestRefString_DigestPreferredOverTag documents the precedence: when both +// Digest and Tag are set (e.g. a Ref that was pushed by Tag and then had its +// digest resolved), String() renders by digest. Callers logging a Push target +// should log ref.Tag separately if they want to preserve "what we asked for". +func TestRefString_DigestPreferredOverTag(t *testing.T) { + r := Ref{Name: "php-core", Digest: "sha256:abc", Tag: "x.y.z"} + if got := r.String(); got != "php-core@sha256:abc" { + t.Errorf("Ref.String() = %q, want %q", got, "php-core@sha256:abc") + } +} diff --git a/internal/registry/remote.go b/internal/registry/remote.go index 4c82456..f6db3f0 100644 --- a/internal/registry/remote.go +++ b/internal/registry/remote.go @@ -10,14 +10,18 @@ import ( "github.com/google/go-containerregistry/pkg/authn" "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/remote/transport" + "github.com/google/go-containerregistry/pkg/v1/static" + "github.com/google/go-containerregistry/pkg/v1/types" ) // remoteStore is the HTTPS-registry backed Store. It wraps -// go-containerregistry/pkg/v1/remote for Fetch / Has / ResolveDigest; Push -// returns ErrUnsupported because remote pushes land with the `phpup build` -// subcommand in a later PR. +// go-containerregistry/pkg/v1/remote for Fetch / Has / ResolveDigest and +// Push; the only unsupported operation is LookupBySpec (see its comment). type remoteStore struct { base string auth authn.Authenticator @@ -134,10 +138,70 @@ func (s *remoteStore) Fetch(ctx context.Context, ref Ref) (io.ReadCloser, *Meta, return bundle, meta, nil } -// 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, _ Annotations) error { - return ErrUnsupported +// Push writes a bundle as a two-layer OCI image to "/:". +// +// Why Tag (and not Digest): OCI registries are tag-addressed for writes — +// the registry computes the manifest digest from the uploaded bytes; callers +// cannot supply one ahead of time. remote.Write therefore requires a tag. +// Digest-only Refs are a Fetch/Has concept; for Push the caller must provide +// a Tag. +// +// Media-type choices mirror the existing `oras push` command in +// .github/workflows/build-php-core.yml + build-extension.yml: layer 0 is the +// bundle at application/vnd.oci.image.layer.v1.tar+zstd, layer 1 (when meta +// is non-nil) is the meta sidecar at application/vnd.buildrush.meta.v1+json, +// and the manifest carries the OCI artifact-type annotation +// (org.opencontainers.artifact.type) that matches --artifact-type. Keeping +// this byte-identical with the CI path lets cosign + downstream OCI tooling +// treat remoteStore-pushed bundles exactly like oras-pushed ones. +func (s *remoteStore) Push(ctx context.Context, ref Ref, body io.Reader, meta *Meta, ann Annotations) error { + if ref.Name == "" { + return errors.New("remote.Push: ref.Name required") + } + if ref.Tag == "" { + return errors.New("remote.Push: ref.Tag required (remote registries are tag-addressed for writes)") + } + bundleBytes, err := io.ReadAll(body) + if err != nil { + return fmt.Errorf("remote.Push: read bundle: %w", err) + } + + layers := []v1.Layer{static.NewLayer(bundleBytes, types.MediaType(mediaTypeBundleLayer))} + if meta != nil { + metaBytes, err := json.Marshal(meta) + if err != nil { + return fmt.Errorf("remote.Push: marshal meta: %w", err) + } + layers = append(layers, static.NewLayer(metaBytes, types.MediaType(mediaTypeMetaSidecar))) + } + + img, err := mutate.AppendLayers(empty.Image, layers...) + if err != nil { + return fmt.Errorf("remote.Push: append layers: %w", err) + } + + // Mirror the layout backend's back-compat fallback: callers supply + // Annotations, but if they didn't set BundleName we fill it from + // ref.Name so round-trip Fetch via annotation-walk still works. + annotations := ann.asMap() + if annotations[annotationBundleName] == "" { + annotations[annotationBundleName] = ref.Name + } + // Replay what `oras push --artifact-type ` writes on the manifest. + annotations[annotationArtifactType] = artifactTypeForBundle(ref.Name) + annotated, ok := mutate.Annotations(img, annotations).(v1.Image) + if !ok { + return errors.New("remote.Push: mutate.Annotations did not return v1.Image") + } + + target, err := name.ParseReference(fmt.Sprintf("%s/%s:%s", s.base, ref.Name, ref.Tag)) + if err != nil { + return fmt.Errorf("remote.Push: parse target %q: %w", ref, err) + } + if err := remote.Write(target, annotated, remote.WithAuth(s.auth), remote.WithContext(ctx)); err != nil { + return fmt.Errorf("remote.Push %s: %w", ref, err) + } + return nil } // LookupBySpec is not supported on the remote backend: anonymous OCI diff --git a/internal/registry/remote_test.go b/internal/registry/remote_test.go index c9a0c49..73ac062 100644 --- a/internal/registry/remote_test.go +++ b/internal/registry/remote_test.go @@ -3,7 +3,6 @@ package registry import ( "bytes" "context" - "errors" "io" "net/http/httptest" "net/url" @@ -144,13 +143,178 @@ func TestRemoteStore_HasMissingRef_FalseNoError(t *testing.T) { } } -func TestRemoteStore_PushReturnsUnsupported(t *testing.T) { +// TestRemoteStore_Push_RoundTrip exercises the full Push→ResolveDigest→Fetch +// path against the in-process test registry. It asserts the bundle bytes and +// the Meta sidecar survive a round-trip, proving remote.Push emits an OCI +// image shape that the existing Fetch path can consume. +func TestRemoteStore_Push_RoundTrip(t *testing.T) { + ctx := context.Background() + host := startTestRegistry(t) + s, err := Open(host + "/buildrush") + if err != nil { + t.Fatalf("Open: %v", err) + } + + payload := []byte("roundtrip-payload") + meta := &Meta{SchemaVersion: 2, Kind: "php-core"} + if err := s.Push(ctx, + Ref{Name: "php-core", Tag: "roundtrip-tag"}, + bytes.NewReader(payload), meta, + Annotations{BundleName: "php-core", SpecHash: "sha256:abc"}); err != nil { + t.Fatalf("Push: %v", err) + } + + digest, err := s.ResolveDigest(ctx, host+"/buildrush/php-core:roundtrip-tag") + if err != nil { + t.Fatalf("ResolveDigest: %v", err) + } + + rc, gotMeta, err := s.Fetch(ctx, Ref{Name: "php-core", Digest: digest}) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + defer rc.Close() + gotBytes, err := io.ReadAll(rc) + if err != nil { + t.Fatalf("read: %v", err) + } + if !bytes.Equal(gotBytes, payload) { + t.Errorf("payload = %q, want %q", gotBytes, payload) + } + if gotMeta == nil || gotMeta.SchemaVersion != 2 || gotMeta.Kind != "php-core" { + t.Errorf("meta = %+v, want SchemaVersion:2 Kind:php-core", gotMeta) + } +} + +// TestRemoteStore_Push_RequiresTag guards the design invariant: remote +// registries are tag-addressed for writes, so Push must refuse a digest-only +// (or empty) Ref with an error mentioning Tag. +func TestRemoteStore_Push_RequiresTag(t *testing.T) { + ctx := context.Background() + host := startTestRegistry(t) + s, _ := Open(host + "/buildrush") + + err := s.Push(ctx, Ref{Name: "php-core"}, // intentionally no Tag + bytes.NewReader([]byte("x")), nil, Annotations{BundleName: "php-core"}) + if err == nil { + t.Fatal("Push with empty Tag err = nil, want error") + } + if !strings.Contains(err.Error(), "Tag") { + t.Errorf("Push err = %q, want mention of Tag", err) + } +} + +// TestRemoteStore_Push_ArtifactTypeAnnotation_PhpExt verifies the manifest +// annotation matches what `oras push --artifact-type application/vnd.buildrush.php-ext.v1` +// emits for php-ext-* bundles (see .github/workflows/build-extension.yml). +func TestRemoteStore_Push_ArtifactTypeAnnotation_PhpExt(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, Annotations{BundleName: "php-core"}) - if !errors.Is(err, ErrUnsupported) { - t.Fatalf("Push err = %v, want ErrUnsupported", err) + + ref := Ref{Name: "php-ext-redis", Tag: "6.2.0-8.4-nts-linux-x86_64"} + if err := s.Push(ctx, ref, + bytes.NewReader([]byte("x")), nil, + Annotations{BundleName: "php-ext-redis"}); err != nil { + t.Fatalf("Push: %v", err) + } + + target, err := name.ParseReference(host + "/buildrush/" + ref.Name + ":" + ref.Tag) + if err != nil { + t.Fatalf("parse: %v", err) + } + desc, err := remote.Get(target) + if err != nil { + t.Fatalf("remote.Get: %v", err) + } + img, err := desc.Image() + if err != nil { + t.Fatalf("image: %v", err) + } + mf, err := img.Manifest() + if err != nil { + t.Fatalf("manifest: %v", err) + } + if got := mf.Annotations["org.opencontainers.artifact.type"]; got != "application/vnd.buildrush.php-ext.v1" { + t.Errorf("artifact.type = %q, want %q", got, "application/vnd.buildrush.php-ext.v1") + } +} + +// TestRemoteStore_Push_ArtifactTypeAnnotation_PhpCore mirrors the php-ext +// assertion for the php-core artifact type (build-php-core.yml). +func TestRemoteStore_Push_ArtifactTypeAnnotation_PhpCore(t *testing.T) { + ctx := context.Background() + host := startTestRegistry(t) + s, _ := Open(host + "/buildrush") + + ref := Ref{Name: "php-core", Tag: "8.4-linux-x86_64-nts"} + if err := s.Push(ctx, ref, + bytes.NewReader([]byte("x")), nil, + Annotations{BundleName: "php-core"}); err != nil { + t.Fatalf("Push: %v", err) + } + + target, err := name.ParseReference(host + "/buildrush/" + ref.Name + ":" + ref.Tag) + if err != nil { + t.Fatalf("parse: %v", err) + } + desc, err := remote.Get(target) + if err != nil { + t.Fatalf("remote.Get: %v", err) + } + img, err := desc.Image() + if err != nil { + t.Fatalf("image: %v", err) + } + mf, err := img.Manifest() + if err != nil { + t.Fatalf("manifest: %v", err) + } + if got := mf.Annotations["org.opencontainers.artifact.type"]; got != "application/vnd.buildrush.php-core.v1" { + t.Errorf("artifact.type = %q, want %q", got, "application/vnd.buildrush.php-core.v1") + } +} + +// TestRemoteStore_Push_LayerMediaTypes verifies the bundle and meta layer +// media types are byte-identical to what `oras push` writes: the bundle at +// application/vnd.oci.image.layer.v1.tar+zstd and the meta sidecar at +// application/vnd.buildrush.meta.v1+json. +func TestRemoteStore_Push_LayerMediaTypes(t *testing.T) { + ctx := context.Background() + host := startTestRegistry(t) + s, _ := Open(host + "/buildrush") + + ref := Ref{Name: "php-core", Tag: "media-types"} + if err := s.Push(ctx, ref, + bytes.NewReader([]byte("bundle-bytes")), &Meta{SchemaVersion: 2, Kind: "php-core"}, + Annotations{BundleName: "php-core"}); err != nil { + t.Fatalf("Push: %v", err) + } + + target, err := name.ParseReference(host + "/buildrush/" + ref.Name + ":" + ref.Tag) + if err != nil { + t.Fatalf("parse: %v", err) + } + desc, err := remote.Get(target) + if err != nil { + t.Fatalf("remote.Get: %v", err) + } + img, err := desc.Image() + if err != nil { + t.Fatalf("image: %v", err) + } + mf, err := img.Manifest() + if err != nil { + t.Fatalf("manifest: %v", err) + } + if len(mf.Layers) != 2 { + t.Fatalf("len(layers) = %d, want 2", len(mf.Layers)) + } + if got := mf.Layers[0].MediaType; got != "application/vnd.oci.image.layer.v1.tar+zstd" { + t.Errorf("layers[0].MediaType = %q, want %q", got, "application/vnd.oci.image.layer.v1.tar+zstd") + } + if got := mf.Layers[1].MediaType; got != "application/vnd.buildrush.meta.v1+json" { + t.Errorf("layers[1].MediaType = %q, want %q", got, "application/vnd.buildrush.meta.v1+json") } } From cb7810d1285fe9244d18ffb3e3074766a32883c5 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 18:06:34 +0200 Subject: [PATCH 2/5] refactor(build): loadExtBuildDeps via catalog.LoadExtensionSpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up from PR 2 Task 5 code review. Drop the ad-hoc map[string]any parser in favour of the existing typed catalog API so the extension-schema shape lives in one place. Behavior is byte-identical — .build_deps.linux is joined with spaces exactly as before. --- internal/build/build.go | 31 ++++++------------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/internal/build/build.go b/internal/build/build.go index 406b9d0..02c3c59 100644 --- a/internal/build/build.go +++ b/internal/build/build.go @@ -11,8 +11,7 @@ import ( "strings" "time" - "gopkg.in/yaml.v3" - + "github.com/buildrush/setup-php/internal/catalog" "github.com/buildrush/setup-php/internal/registry" ) @@ -451,32 +450,14 @@ func tsFromPHPABI(phpABI string) string { // 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. +// Uses the typed catalog API so the extension-schema shape lives in one +// place (internal/catalog) instead of drifting across ad-hoc parsers. func loadExtBuildDeps(path string) (string, error) { - data, err := os.ReadFile(filepath.Clean(path)) + spec, err := catalog.LoadExtensionSpec(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 "", fmt.Errorf("load extension catalog: %w", err) } - return strings.Join(pkgs, " "), nil + return strings.Join(spec.BuildDeps["linux"], " "), nil } // phpOpts is the parsed flag set for `phpup build php`. Repo is resolved From 7408e5748998633f6e5c5c0e660d71b804524339 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 18:10:35 +0200 Subject: [PATCH 3/5] fix(build): sweep stale sidecars at Start to prevent zombie accumulation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flagged in PR 2 Task 5 code review as Important #1. If a sidecar lifecycle aborts before its deferred stop (panic, outer timeout, SIGKILL), the distribution:3 container and its docker network linger and can block future runs. Label all sidecar-created containers + networks with buildrush.phpup.sidecar=1, and at the top of Start, sweep anything matching that label. Opportunistic: errors from the sweep are ignored — either the zombies didn't collide with the new run (fine), or docker is broken enough that the subsequent Start will report its own clear error. Gated TestSidecar_SweepsZombiesOnStart_Real seeds a fake zombie container + network, calls Start, and asserts the zombie is gone. --- internal/build/sidecar.go | 38 ++++++++++++++++++- internal/build/sidecar_test.go | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/internal/build/sidecar.go b/internal/build/sidecar.go index 06573fb..e75c336 100644 --- a/internal/build/sidecar.go +++ b/internal/build/sidecar.go @@ -97,11 +97,46 @@ func currentSidecarLifecycle() SidecarLifecycle { // network; SeedCore pushes the prerequisite bundle via remote.Write. type defaultSidecarLifecycle struct{} +// sidecarLabel marks every container and network this lifecycle +// creates so sweepStaleSidecars can reliably clean up zombies from +// prior runs without touching unrelated docker state. +const sidecarLabel = "buildrush.phpup.sidecar=1" + +// sweepStaleSidecars removes any containers or networks from prior runs +// that didn't clean up after themselves (e.g. outer timeout killed the +// process before the defer). Scoped by label so unrelated docker state +// is untouched. Errors are ignored — if docker can't list or remove the +// resources, the subsequent Start will either succeed (the zombies didn't +// collide) or fail with its own clear error. +func sweepStaleSidecars(ctx context.Context) { + // Best-effort; failures are not actionable from the caller's + // perspective and would just add noise on first-ever-run (no + // prior label to match). + + // Containers first (they hold the network in use, so must go before the network). + if out, err := execDocker(ctx, "ps", "-aq", "--filter", "label="+sidecarLabel); err == nil { + for _, id := range strings.Fields(string(out)) { + _, _ = execDocker(ctx, "rm", "-f", id) + } + } + // Then networks. + if out, err := execDocker(ctx, "network", "ls", "-q", "--filter", "label="+sidecarLabel); err == nil { + for _, id := range strings.Fields(string(out)) { + _, _ = execDocker(ctx, "network", "rm", id) + } + } +} + // 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) { + // Opportunistic: clean up any zombie sidecars from prior runs that + // aborted before their deferred stop (panic, outer timeout, + // SIGKILL). Scoped by label so unrelated docker state is untouched. + sweepStaleSidecars(ctx) + // 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 @@ -110,7 +145,7 @@ func (defaultSidecarLifecycle) Start(ctx context.Context) (*Sidecar, func(contex network := "phpup-build-" + tag containerName := "phpup-sidecar-" + tag - if err := dockerCmdCombined(ctx, "network", "create", network); err != nil { + if err := dockerCmdCombined(ctx, "network", "create", "--label", sidecarLabel, network); err != nil { return nil, nil, fmt.Errorf("sidecar: create network: %w", err) } @@ -122,6 +157,7 @@ func (defaultSidecarLifecycle) Start(ctx context.Context) (*Sidecar, func(contex "run", "-d", "--rm", "--name", containerName, "--network", network, + "--label", sidecarLabel, "--publish", "127.0.0.1::5000", "distribution/distribution:3", ) diff --git a/internal/build/sidecar_test.go b/internal/build/sidecar_test.go index 934a2e4..dfe018e 100644 --- a/internal/build/sidecar_test.go +++ b/internal/build/sidecar_test.go @@ -237,3 +237,70 @@ func TestSidecar_LifecycleAndSeed_Real(t *testing.T) { t.Errorf("pulled bundle = %q, want %q", got, bundlePayload) } } + +// TestSidecar_SweepsZombiesOnStart_Real seeds a fake zombie container +// + network labeled as prior-run sidecars, then calls Start and asserts +// the zombie is swept as a side effect. Guards against leaked state +// from runs killed by an outer signal/timeout before defer stop(). +// Skipped under -short and when docker is absent. +func TestSidecar_SweepsZombiesOnStart_Real(t *testing.T) { + if testing.Short() { + t.Skip("skipping real docker test under -short") + } + if _, err := exec.LookPath("docker"); err != nil { + t.Skipf("docker not found: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + // Step 1: create a FAKE zombie: a container + network labeled as a + // sidecar but never properly torn down. + zombieName := "phpup-sidecar-zombie-" + strings.ReplaceAll(time.Now().UTC().Format("20060102T150405.000000000"), ".", "") + zombieNet := "phpup-build-zombie-" + strings.ReplaceAll(time.Now().UTC().Format("20060102T150405.000000000"), ".", "") + + if _, err := execDocker(ctx, "network", "create", "--label", "buildrush.phpup.sidecar=1", zombieNet); err != nil { + t.Fatalf("seed zombie network: %v", err) + } + // Use a tiny image; we don't care about the registry functionality here. + if _, err := execDocker(ctx, "run", "-d", "--name", zombieName, + "--network", zombieNet, + "--label", "buildrush.phpup.sidecar=1", + "alpine:3", "sleep", "300"); err != nil { + // Cleanup before failing. + _, _ = execDocker(ctx, "network", "rm", zombieNet) + t.Fatalf("seed zombie container: %v", err) + } + + // Step 2: verify zombie exists. + outBefore, _ := execDocker(ctx, "ps", "-aq", "--filter", "name="+zombieName) + if strings.TrimSpace(string(outBefore)) == "" { + _, _ = execDocker(ctx, "rm", "-f", zombieName) + _, _ = execDocker(ctx, "network", "rm", zombieNet) + t.Fatal("zombie container not created") + } + + // Step 3: Start a fresh sidecar — should sweep the zombie as a side effect. + sc, stop, err := defaultSidecarLifecycle{}.Start(ctx) + if err != nil { + // Cleanup any remaining zombies. + _, _ = execDocker(ctx, "rm", "-f", zombieName) + _, _ = execDocker(ctx, "network", "rm", zombieNet) + t.Fatalf("Start: %v", err) + } + defer func() { _ = stop(context.Background()) }() + + // Step 4: verify the zombie container is gone. + outAfter, _ := execDocker(ctx, "ps", "-aq", "--filter", "name="+zombieName) + if strings.TrimSpace(string(outAfter)) != "" { + // Force cleanup. + _, _ = execDocker(ctx, "rm", "-f", zombieName) + _, _ = execDocker(ctx, "network", "rm", zombieNet) + t.Errorf("zombie container %s was NOT swept by Start", zombieName) + } + + // Sanity: the new sidecar's name is different from the zombie. + if sc.Name == zombieName { + t.Errorf("new sidecar collided with zombie name: %s", sc.Name) + } +} From 9f66e0f3e1c393f0cf4811a1426bd3f6b569788a Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 18:17:58 +0200 Subject: [PATCH 4/5] feat(planner): thread php-core digest into ext matrix cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MatrixCell gains a CoreDigest string field populated only for Kind=="ext". ExpandExtMatrix reads the already-resolved core digests (by php_abi+os+arch key) and sets CoreDigest on each ext cell. Emitted in the matrix JSON as `core_digest` (omitempty so php/tool cells stay unchanged). plan-and-build.yml passes `matrix.core_digest` as `php_core_digest` input to build-extension.yml; the workflow declares the input but doesn't yet consume it (Task 5 does the phpup build ext rewire). No behavior change for the current CI pipeline — build-extension.yml still uses oras push unchanged. This commit is purely preparatory wiring for Task 5. --- .github/workflows/build-extension.yml | 5 + .github/workflows/plan-and-build.yml | 1 + cmd/lockfile-update/main.go | 9 +- cmd/planner/main.go | 17 ++- internal/planner/planner.go | 39 +++++-- internal/planner/planner_test.go | 142 +++++++++++++++++++++++++- 6 files changed, 203 insertions(+), 10 deletions(-) diff --git a/.github/workflows/build-extension.yml b/.github/workflows/build-extension.yml index 85f8288..4b24df7 100644 --- a/.github/workflows/build-extension.yml +++ b/.github/workflows/build-extension.yml @@ -21,6 +21,11 @@ on: required: false type: string default: '' + php_core_digest: + description: 'OCI digest of prerequisite php-core bundle (sha256:...); Task 5 wires the consumer.' + required: false + type: string + default: '' push: required: false type: boolean diff --git a/.github/workflows/plan-and-build.yml b/.github/workflows/plan-and-build.yml index f4bf3c5..4c81c34 100644 --- a/.github/workflows/plan-and-build.yml +++ b/.github/workflows/plan-and-build.yml @@ -58,6 +58,7 @@ jobs: os: ${{ matrix.os }} arch: ${{ matrix.arch }} spec_hash: ${{ matrix.spec_hash }} + php_core_digest: ${{ matrix.core_digest }} push: ${{ inputs.push }} update-lock: diff --git a/cmd/lockfile-update/main.go b/cmd/lockfile-update/main.go index ccd60c0..bf6c3f6 100644 --- a/cmd/lockfile-update/main.go +++ b/cmd/lockfile-update/main.go @@ -76,6 +76,12 @@ func main() { var resolved []resolvedEntry + // coreDigestByKey accumulates freshly-resolved PHP-core digests keyed by + // lockfile.PHPBundleKey so ExpandExtMatrix can stamp each ext cell's + // CoreDigest. lockfile-update is a digest-resolution pass — unresolved + // cores will warn during ext expansion. + coreDigestByKey := make(map[string]string) + // PHP core cells. phpCells := planner.ExpandPHPMatrix(cat.PHP) for i := range phpCells { @@ -95,6 +101,7 @@ func main() { } key := lockfile.PHPBundleKey(c.Version, c.OS, c.Arch, c.TS) resolved = append(resolved, resolvedEntry{Key: key, Digest: digest, SpecHash: c.SpecHash}) + coreDigestByKey[key] = digest } // Extensions. @@ -106,7 +113,7 @@ func main() { if err != nil { log.Fatalf("ext yaml %s: %v", ext.Name, err) } - cells := planner.ExpandExtMatrix(ext) + cells := planner.ExpandExtMatrix(ext, coreDigestByKey) for i := range cells { c := &cells[i] c.SpecHash = planner.ComputeSpecHash(c, extYAML, builderHashExt, builderOS) diff --git a/cmd/planner/main.go b/cmd/planner/main.go index 0a2f643..6c1d631 100644 --- a/cmd/planner/main.go +++ b/cmd/planner/main.go @@ -91,13 +91,28 @@ func main() { } result.PHP = planner.Matrix{Include: phpCells} + // Build a map of already-published php-core digests keyed by + // lockfile.PHPBundleKey (matches the key format ExpandExtMatrix builds + // internally). Used to populate ext cells' CoreDigest field so Task 5's + // build-extension job can pin the core by digest. Cells whose core is + // being rebuilt in the same run won't have an entry here yet — the + // workflow orchestrator (plan-and-build.yml) serializes build-ext + // after build-php so the lockfile can be refreshed before ext builds + // consume the value; this field is plumbing-only in Task 4. + coreDigestByKey := make(map[string]string, len(lf.Bundles)) + for key, entry := range lf.Bundles { + if strings.HasPrefix(key, "php:") { + coreDigestByKey[key] = entry.Digest + } + } + // Expand extension matrices var extCells []planner.MatrixCell for _, ext := range cat.Extensions { if ext.Kind == catalog.ExtensionKindBundled { continue } - cells := planner.ExpandExtMatrix(ext) + cells := planner.ExpandExtMatrix(ext, coreDigestByKey) extYAML, err := planner.ExtensionYAML(ext) if err != nil { log.Fatalf("ext yaml for %s: %v", ext.Name, err) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index f948305..29938ef 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/json" "fmt" + "log" "os" "path/filepath" @@ -23,6 +24,14 @@ type MatrixCell struct { Extension string `json:"extension,omitempty"` ExtVer string `json:"ext_version,omitempty"` PHPAbi string `json:"php_abi,omitempty"` + + // CoreDigest is the OCI manifest digest of the prerequisite php-core + // bundle for this ext cell (e.g., "sha256:abc..."). Populated ONLY for + // ext cells; zero for php/tool cells. Surfaced in the emitted matrix + // JSON as `core_digest` so build-extension.yml can pass it to + // `phpup build ext --php-core-digest`. omitempty keeps the JSON + // backward-compatible — php/tool cells don't gain a noisy empty field. + CoreDigest string `json:"core_digest,omitempty"` } // Matrix is the GitHub Actions matrix JSON format. @@ -60,7 +69,16 @@ func ExpandPHPMatrix(spec *catalog.PHPSpec) []MatrixCell { } // ExpandExtMatrix expands an extension's abi_matrix, applying excludes. -func ExpandExtMatrix(spec *catalog.ExtensionSpec) []MatrixCell { +// +// coreDigestByKey maps a canonical PHP bundle key (matching +// lockfile.PHPBundleKey — "php::::") to the resolved OCI +// digest of the prerequisite php-core bundle. The resolved digest (if any) is +// stored on each ext cell's CoreDigest field. Pass nil if no digest context +// is available; cells will have empty CoreDigest and a warning is logged per +// unresolved cell. The zero-value behavior intentionally matches the existing +// "missing ABI row" case (no silent skip) — so Task 5's consumer must treat +// empty CoreDigest as a hard error, not as "fall back to tag-form". +func ExpandExtMatrix(spec *catalog.ExtensionSpec, coreDigestByKey map[string]string) []MatrixCell { if spec.Kind == catalog.ExtensionKindBundled { return nil } @@ -74,13 +92,20 @@ func ExpandExtMatrix(spec *catalog.ExtensionSpec) []MatrixCell { if isExcluded(spec.Exclude, osName, arch, php) { continue } + coreKey := fmt.Sprintf("php:%s:%s:%s:%s", php, osName, arch, ts) + digest := coreDigestByKey[coreKey] + if digest == "" && coreDigestByKey != nil { + log.Printf("WARN: ExpandExtMatrix: no core digest for ext=%s ext_ver=%s php=%s os=%s arch=%s ts=%s (key=%s); cell will have empty CoreDigest", + spec.Name, ver, php, osName, arch, ts, coreKey) + } cells = append(cells, MatrixCell{ - Extension: spec.Name, - ExtVer: ver, - PHPAbi: fmt.Sprintf("%s-%s", php, ts), - OS: osName, - Arch: arch, - TS: ts, + Extension: spec.Name, + ExtVer: ver, + PHPAbi: fmt.Sprintf("%s-%s", php, ts), + OS: osName, + Arch: arch, + TS: ts, + CoreDigest: digest, }) } } diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index 2efde72..75545d7 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -3,6 +3,7 @@ package planner import ( "bytes" "encoding/json" + "log" "os" "path/filepath" "strings" @@ -111,7 +112,7 @@ func TestExpandExtMatrixWithExclude(t *testing.T) { // Without exclude: 1×2×2×1 = 4 per version = 4 // With exclude: -1 (windows+aarch64) = 3 - cells := ExpandExtMatrix(spec) + cells := ExpandExtMatrix(spec, nil) if len(cells) != 3 { t.Fatalf("len(cells) = %d, want 3", len(cells)) } @@ -123,6 +124,145 @@ func TestExpandExtMatrixWithExclude(t *testing.T) { } } +func TestExpandExtMatrix_PopulatesCoreDigest(t *testing.T) { + spec := &catalog.ExtensionSpec{ + Name: "redis", + Kind: catalog.ExtensionKindPECL, + Versions: []string{"6.2.0"}, + ABIMatrix: catalog.ABIMatrix{ + PHP: []string{"8.3", "8.4"}, + OS: []string{"linux"}, + Arch: []string{"x86_64", "aarch64"}, + TS: []string{"nts"}, + }, + } + // 2 PHP × 1 OS × 2 arch × 1 TS = 4 cells; synthetic digests for each. + coreDigests := map[string]string{ + "php:8.3:linux:x86_64:nts": "sha256:aaa83x", + "php:8.3:linux:aarch64:nts": "sha256:aaa83a", + "php:8.4:linux:x86_64:nts": "sha256:bbb84x", + "php:8.4:linux:aarch64:nts": "sha256:bbb84a", + } + + cells := ExpandExtMatrix(spec, coreDigests) + if len(cells) != 4 { + t.Fatalf("len(cells) = %d, want 4", len(cells)) + } + for _, c := range cells { + phpMinor := strings.TrimSuffix(c.PHPAbi, "-"+c.TS) + wantKey := "php:" + phpMinor + ":" + c.OS + ":" + c.Arch + ":" + c.TS + want := coreDigests[wantKey] + if c.CoreDigest != want { + t.Errorf("cell %+v: CoreDigest = %q, want %q (key %q)", c, c.CoreDigest, want, wantKey) + } + } +} + +func TestExpandExtMatrix_MissingCoreDigest_EmptyAndWarns(t *testing.T) { + spec := &catalog.ExtensionSpec{ + Name: "redis", + Kind: catalog.ExtensionKindPECL, + Versions: []string{"6.2.0"}, + ABIMatrix: catalog.ABIMatrix{ + PHP: []string{"8.4"}, + OS: []string{"linux"}, + Arch: []string{"x86_64"}, + TS: []string{"nts"}, + }, + } + + // Redirect log output to capture the warning. + var buf bytes.Buffer + oldOutput := log.Writer() + oldFlags := log.Flags() + log.SetOutput(&buf) + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(oldOutput) + log.SetFlags(oldFlags) + }) + + cells := ExpandExtMatrix(spec, map[string]string{}) // empty, non-nil + if len(cells) != 1 { + t.Fatalf("len(cells) = %d, want 1 (cell must not be silently skipped)", len(cells)) + } + if cells[0].CoreDigest != "" { + t.Errorf("cell CoreDigest = %q, want empty string when no digest is available", cells[0].CoreDigest) + } + if !strings.Contains(buf.String(), "WARN: ExpandExtMatrix: no core digest") { + t.Errorf("expected WARN log when core digest missing; got: %q", buf.String()) + } +} + +func TestExpandExtMatrix_NilDigestMap_NoWarn(t *testing.T) { + // Nil map = "no digest context available at all"; suppress warnings so + // callers that don't need digest resolution (e.g., isolated tests) don't + // spam logs. Cells still come out with empty CoreDigest. + spec := &catalog.ExtensionSpec{ + Name: "redis", + Kind: catalog.ExtensionKindPECL, + Versions: []string{"6.2.0"}, + ABIMatrix: catalog.ABIMatrix{ + PHP: []string{"8.4"}, + OS: []string{"linux"}, + Arch: []string{"x86_64"}, + TS: []string{"nts"}, + }, + } + var buf bytes.Buffer + oldOutput := log.Writer() + oldFlags := log.Flags() + log.SetOutput(&buf) + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(oldOutput) + log.SetFlags(oldFlags) + }) + cells := ExpandExtMatrix(spec, nil) + if len(cells) != 1 { + t.Fatalf("len(cells) = %d, want 1", len(cells)) + } + if cells[0].CoreDigest != "" { + t.Errorf("cell CoreDigest = %q, want empty string", cells[0].CoreDigest) + } + if buf.Len() != 0 { + t.Errorf("expected no warning with nil digest map; got: %q", buf.String()) + } +} + +func TestMatrixCell_JSON_OmitsEmptyCoreDigest(t *testing.T) { + // A php cell has CoreDigest == ""; JSON output must not include + // "core_digest" — keeps php/tool matrix bytes unchanged. + cell := MatrixCell{Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts"} + data, err := json.Marshal(cell) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if strings.Contains(string(data), "core_digest") { + t.Errorf("php cell JSON must not contain core_digest when empty; got: %s", data) + } +} + +func TestMatrixCell_JSON_IncludesCoreDigestForExt(t *testing.T) { + cell := MatrixCell{ + Extension: "redis", + ExtVer: "6.2.0", + PHPAbi: "8.4-nts", + OS: "linux", + Arch: "x86_64", + TS: "nts", + CoreDigest: "sha256:abcdef", + } + data, err := json.Marshal(cell) + if err != nil { + t.Fatalf("marshal: %v", err) + } + s := string(data) + if !strings.Contains(s, `"core_digest":"sha256:abcdef"`) { + t.Errorf("ext cell JSON must contain core_digest field; got: %s", s) + } +} + func TestComputeSpecHash(t *testing.T) { cell := MatrixCell{Version: "8.4", OS: "linux", Arch: "x86_64", TS: "nts"} h1 := ComputeSpecHash(&cell, []byte("catalog data"), "builder-hash-1", "ubuntu-22.04") From 768e56fd5ca981a7dd790a6953cd75ae9492c80c Mon Sep 17 00:00:00 2001 From: Maximilian Date: Thu, 23 Apr 2026 18:21:53 +0200 Subject: [PATCH 5/5] chore(ci): rewire build-extension.yml to phpup build ext Mirrors PR 2 Task 6's pattern for build-php-core.yml. Builder script stays unchanged; phpup build ext docker-wraps it and writes into ./build/ext//. A "Stage bundle for publish" step cp's the emitted bundle.tar.zst + .sha256 + meta.json to /tmp/ so the existing Push to GHCR, Sign bundle, and Upload artifact steps read from the same paths -- preserving the digest job-output contract byte-for-byte. inputs.php_core_digest now required (previously declared as optional stub by PR 2.5 Task 4). plan-and-build.yml already passes matrix.core_digest from the planner, so the CI wiring closes. Completes the deferred ext-workflow work from PR 2. --- .github/workflows/build-extension.yml | 53 ++++++++++++++++++++------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/.github/workflows/build-extension.yml b/.github/workflows/build-extension.yml index 4b24df7..f82affd 100644 --- a/.github/workflows/build-extension.yml +++ b/.github/workflows/build-extension.yml @@ -22,10 +22,9 @@ on: type: string default: '' php_core_digest: - description: 'OCI digest of prerequisite php-core bundle (sha256:...); Task 5 wires the consumer.' - required: false + description: 'OCI manifest digest of the prerequisite php-core bundle (sha256:...). Required for phpup build ext.' + required: true type: string - default: '' push: required: false type: boolean @@ -57,23 +56,49 @@ 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: Resolve build_deps from catalog - id: build_deps - run: | - deps=$(yq eval '.build_deps.linux // [] | join(" ")' catalog/extensions/${{ inputs.extension }}.yaml) - echo "deps=${deps}" >> "$GITHUB_OUTPUT" + - uses: actions/setup-go@v6 + with: + go-version: '1.26' + + - name: Build phpup + run: make bin/phpup - - name: Build extension + # Run the ext build via phpup. phpup docker-wraps + # builders/linux/build-ext.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", 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 extension via phpup env: EXT_NAME: ${{ inputs.extension }} EXT_VERSION: ${{ inputs.ext_version }} PHP_ABI: ${{ inputs.php_abi }} + OS: ${{ inputs.os }} ARCH: ${{ inputs.arch }} - GHCR_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REGISTRY: ghcr.io/${{ github.repository_owner }} - WORKSPACE: ${{ github.workspace }} - BUILD_DEPS: ${{ steps.build_deps.outputs.deps }} - run: ./builders/linux/build-ext.sh + PHP_CORE_DIGEST: ${{ inputs.php_core_digest }} + PHPUP_OUT_DIR: ${{ github.workspace }}/build/ext/${{ inputs.extension }}-${{ inputs.ext_version }}-${{ inputs.php_abi }}-${{ inputs.os }}-${{ inputs.arch }} + run: | + ./bin/phpup build ext \ + --ext "$EXT_NAME" \ + --ext-version "$EXT_VERSION" \ + --php-abi "$PHP_ABI" \ + --arch "$ARCH" \ + --os "$OS" \ + --php-core-digest "$PHP_CORE_DIGEST" \ + --registry oci-layout:./out/oci-layout \ + --repo . \ + --out-dir "$PHPUP_OUT_DIR" + + - name: Stage bundle for publish + env: + PHPUP_OUT_DIR: ${{ github.workspace }}/build/ext/${{ inputs.extension }}-${{ inputs.ext_version }}-${{ inputs.php_abi }}-${{ inputs.os }}-${{ inputs.arch }} + 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