diff --git a/AGENTS.md b/AGENTS.md index 2026440..23f048f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -274,8 +274,8 @@ R sub-dep data.json files live under `/tmp/r-sub-deps/source-{pkg}-latest/data.j | `internal/stack/stack.go` | `Stack` struct, `Load(stacksDir, name)` | | `internal/recipe/recipe.go` | `Recipe` interface, `Registry` | | `internal/recipe/php.go` | `PHPRecipe`; dispatches to `Load` or `LoadFromDir` based on `ExtensionsDir` | -| `internal/php/extensions.go` | `Load`, `LoadFromDir`, `applyPatch` — extension YAML loading + merge | -| `internal/php/assets/` | Extension YAML files (base + per-version patches) | +| `internal/php/extensions.go` | `LoadFromDir`, `applyPatch` — extension YAML loading + merge; `--extensions-dir` is required | +| `internal/php/testdata/` | Extension YAML files (base + per-version patches) — test fixtures only, not embedded | | `internal/archive/archive.go` | `StripTopLevelDir`, `StripFiles`, `InjectFile` | | `internal/portile/` | configure/make/install abstraction | | `internal/apt/` | apt-get install wrapper | diff --git a/internal/php/extensions.go b/internal/php/extensions.go index e898a7f..5b66d9e 100644 --- a/internal/php/extensions.go +++ b/internal/php/extensions.go @@ -2,114 +2,37 @@ // // # Extension data files // -// The YAML files that define which PHP extensions and native modules are built -// live in assets/ alongside the Go source: +// Extension YAML files define which PHP extensions and native modules are compiled +// into PHP binaries. In production, these files are owned by the php-buildpack +// repository (config/extensions/) and passed to binary-builder at build time via +// the --extensions-dir CLI flag. // -// internal/php/assets/ -// php8-base-extensions.yml — full list for all PHP 8.x (native modules + PECL extensions) -// php81-extensions-patch.yml — overrides/exclusions specific to PHP 8.1.x -// php82-extensions-patch.yml — overrides/exclusions specific to PHP 8.2.x -// php83-extensions-patch.yml — overrides/exclusions specific to PHP 8.3.x +// The testdata/ directory in this package contains a copy of the extension YAML +// files for use in unit tests. These are NOT compiled into the binary. // // # Loading extensions // -// There are two ways to load extensions: +// [LoadFromDir] is the primary API. It reads base + patch YAML files from a +// directory on the filesystem and returns a merged [ExtensionSet]. // -// - [Load] reads from the YAML files embedded in this package at compile time. -// This is the default behavior when binary-builder runs without --extensions-dir. +// [Load] is a convenience wrapper for tests that delegates to LoadFromDir +// using the testdata/ directory relative to the caller's working directory. +// It should NOT be used in production code paths. // -// - [LoadFromDir] reads from a directory on the filesystem, using the same -// naming convention as the embedded assets. This allows the extension -// configuration to be owned by a different repository (e.g. the php-buildpack) -// and passed to binary-builder at build time via the --extensions-dir CLI flag. -// -// Both functions return an [ExtensionSet] after applying base + patch merging. -// -// # Adding a new PHP minor version (e.g. 8.4) -// -// 1. Create assets/php84-extensions-patch.yml with any additions or exclusions -// relative to the PHP 8 base (an empty patch is valid: "---\nextensions:\n"). -// -// No code changes are required — the file is discovered automatically via the -// embedded FS glob. -// -// # Adding a new PHP major version (e.g. 9) -// -// 1. Create assets/php9-base-extensions.yml with the full extension list for PHP 9.x. -// 2. Create a patch file for each shipped minor version (e.g. assets/php90-extensions-patch.yml). -// -// Again, no code changes are required. -// -// # File naming convention (drives auto-discovery) +// # File naming convention // // - Base files: php{major}-base-extensions.yml (e.g. php8-base-extensions.yml) // - Patch files: php{major}{minor}-extensions-patch.yml (e.g. php84-extensions-patch.yml) -// -// Important: only .yml files are embedded (the glob is assets/*.yml). A file with a -// .yaml extension would be silently ignored. package php import ( - "embed" "fmt" "os" "path/filepath" - "regexp" - "strings" "gopkg.in/yaml.v3" ) -//go:embed assets/*.yml -var assetsFS embed.FS - -var ( - // baseFileRE matches e.g. "assets/php8-base-extensions.yml" and captures the major version ("8"). - baseFileRE = regexp.MustCompile(`^assets/php(\d+)-base-extensions\.yml$`) - // patchFileRE matches e.g. "assets/php83-extensions-patch.yml" and captures major+minor ("83"). - // The \d{2,} quantifier is intentional: patch filenames must encode both the major AND minor - // digit together (e.g. "php81", "php90"), never a bare major digit. This prevents a file named - // "php8-extensions-patch.yml" from matching (that would be a base file), and ensures a future - // PHP 9 patch is named "php90-extensions-patch.yml", not "php9-extensions-patch.yml" (which - // would be silently ignored). - patchFileRE = regexp.MustCompile(`^assets/php(\d{2,})-extensions-patch\.yml$`) -) - -// embeddedBases maps PHP major version (e.g. "8") → base YAML bytes. -// embeddedPatches maps PHP major+minor (e.g. "83") → patch YAML bytes. -// Both are populated once at init() from the embedded FS. -var ( - embeddedBases map[string][]byte - embeddedPatches map[string][]byte -) - -func init() { - embeddedBases = make(map[string][]byte) - embeddedPatches = make(map[string][]byte) - - entries, err := assetsFS.ReadDir("assets") - if err != nil { - panic("php/extensions: cannot read embedded assets: " + err.Error()) - } - - for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".yml") { - continue - } - path := "assets/" + e.Name() - data, err := assetsFS.ReadFile(path) - if err != nil { - panic("php/extensions: cannot read embedded file " + path + ": " + err.Error()) - } - - if m := baseFileRE.FindStringSubmatch(path); m != nil { - embeddedBases[m[1]] = data - } else if m := patchFileRE.FindStringSubmatch(path); m != nil { - embeddedPatches[m[1]] = data - } - } -} - // Extension represents a single PHP extension or native module. type Extension struct { Name string `yaml:"name"` @@ -136,11 +59,9 @@ type patchCategory struct { } // Load returns the ExtensionSet for the given PHP major+minor version by reading -// the embedded YAML data compiled into this package. -// -// It loads the base file for the major version (e.g. "8" → php8-base-extensions.yml), -// then applies the patch file for the specific minor version if one exists -// (e.g. "8"+"2" → php82-extensions-patch.yml). +// from the testdata/ directory in this package. This is a convenience function +// for tests only — production code should use [LoadFromDir] with the path provided +// via the --extensions-dir CLI flag. // // Merge rules (applied by applyPatch): // - Exclusions run first. If an exclusion has a matching addition, the addition @@ -149,29 +70,19 @@ type patchCategory struct { // - Remaining additions (no matching exclusion) override by name if present, // or are appended. func Load(phpMajor, phpMinor string) (*ExtensionSet, error) { - baseData, ok := embeddedBases[phpMajor] - if !ok { - return nil, fmt.Errorf("php/extensions: no base extensions file for PHP major version %q", phpMajor) - } - - var patchData []byte - if pd, ok := embeddedPatches[phpMajor+phpMinor]; ok { - patchData = pd - } - - return loadFromData(baseData, patchData, phpMajor, phpMinor) + return LoadFromDir("testdata", phpMajor, phpMinor) } // LoadFromDir returns the ExtensionSet for the given PHP major+minor version by // reading YAML files from a directory on the filesystem. The directory must use -// the same naming convention as the embedded assets: +// the following naming convention: // // - Base files: php{major}-base-extensions.yml (e.g. php8-base-extensions.yml) // - Patch files: php{major}{minor}-extensions-patch.yml (e.g. php84-extensions-patch.yml) // -// This allows the extension configuration to be owned by a different repository -// (e.g. the php-buildpack) and passed to binary-builder at build time via the -// --extensions-dir CLI flag. +// This is the primary API for loading extension configuration. In production, +// the directory is provided by the php-buildpack repo via the --extensions-dir +// CLI flag. func LoadFromDir(dir, phpMajor, phpMinor string) (*ExtensionSet, error) { basePath := filepath.Join(dir, fmt.Sprintf("php%s-base-extensions.yml", phpMajor)) baseData, err := os.ReadFile(basePath) @@ -193,8 +104,8 @@ func LoadFromDir(dir, phpMajor, phpMinor string) (*ExtensionSet, error) { return loadFromData(baseData, patchData, phpMajor, phpMinor) } -// loadFromData is the shared implementation for Load and LoadFromDir. It parses -// the base YAML, optionally applies a patch, and returns the merged ExtensionSet. +// loadFromData is the shared implementation. It parses the base YAML, optionally +// applies a patch, and returns the merged ExtensionSet. // When patchData is nil or empty, the base set is returned as-is. func loadFromData(baseData, patchData []byte, phpMajor, phpMinor string) (*ExtensionSet, error) { var set ExtensionSet diff --git a/internal/php/extensions_test.go b/internal/php/extensions_test.go index ad84edc..d94073e 100644 --- a/internal/php/extensions_test.go +++ b/internal/php/extensions_test.go @@ -264,24 +264,21 @@ extensions: assert.Equal(t, "redis", set.Extensions[0].Name) } -func TestLoadFromDir_MatchesEmbeddedLoad(t *testing.T) { - // LoadFromDir reading the real embedded assets directory should produce - // the same result as the embedded Load() function. - // - // This verifies that the two code paths are functionally equivalent. - embeddedSet, err := php.Load("8", "4") +func TestLoadFromDir_MatchesLoad(t *testing.T) { + // Load() delegates to LoadFromDir("testdata", ...), so both should + // produce identical results. This verifies the delegation works. + loadSet, err := php.Load("8", "4") require.NoError(t, err) - // The embedded assets live at internal/php/assets/ relative to the repo root. // In tests, the working directory is the package directory (internal/php/), - // so the assets dir is at "./assets". - dirSet, err := php.LoadFromDir("assets", "8", "4") + // so the testdata dir is at "./testdata". + dirSet, err := php.LoadFromDir("testdata", "8", "4") require.NoError(t, err) - assert.Equal(t, embeddedSet.NativeModules, dirSet.NativeModules, - "NativeModules should match between embedded and directory-based loading") - assert.Equal(t, embeddedSet.Extensions, dirSet.Extensions, - "Extensions should match between embedded and directory-based loading") + assert.Equal(t, loadSet.NativeModules, dirSet.NativeModules, + "NativeModules should match between Load and LoadFromDir") + assert.Equal(t, loadSet.Extensions, dirSet.Extensions, + "Extensions should match between Load and LoadFromDir") } func TestLoadFromDir_PatchFileReadError(t *testing.T) { diff --git a/internal/php/assets/php8-base-extensions.yml b/internal/php/testdata/php8-base-extensions.yml similarity index 100% rename from internal/php/assets/php8-base-extensions.yml rename to internal/php/testdata/php8-base-extensions.yml diff --git a/internal/php/assets/php81-extensions-patch.yml b/internal/php/testdata/php81-extensions-patch.yml similarity index 100% rename from internal/php/assets/php81-extensions-patch.yml rename to internal/php/testdata/php81-extensions-patch.yml diff --git a/internal/php/assets/php82-extensions-patch.yml b/internal/php/testdata/php82-extensions-patch.yml similarity index 100% rename from internal/php/assets/php82-extensions-patch.yml rename to internal/php/testdata/php82-extensions-patch.yml diff --git a/internal/php/assets/php83-extensions-patch.yml b/internal/php/testdata/php83-extensions-patch.yml similarity index 100% rename from internal/php/assets/php83-extensions-patch.yml rename to internal/php/testdata/php83-extensions-patch.yml diff --git a/internal/php/assets/php84-extensions-patch.yml b/internal/php/testdata/php84-extensions-patch.yml similarity index 100% rename from internal/php/assets/php84-extensions-patch.yml rename to internal/php/testdata/php84-extensions-patch.yml diff --git a/internal/php/assets/php85-extensions-patch.yml b/internal/php/testdata/php85-extensions-patch.yml similarity index 100% rename from internal/php/assets/php85-extensions-patch.yml rename to internal/php/testdata/php85-extensions-patch.yml diff --git a/internal/recipe/php.go b/internal/recipe/php.go index a58d649..547033b 100644 --- a/internal/recipe/php.go +++ b/internal/recipe/php.go @@ -65,13 +65,12 @@ var phpConfigureFlags = []string{ // PHPRecipe builds PHP and all of its extensions. // -// By default, extension data (which PECL extensions and native modules to build) -// is loaded from YAML files embedded in the internal/php package. When ExtensionsDir -// is set, extension data is loaded from that directory instead, allowing the -// configuration to be owned by a different repository (e.g. the php-buildpack). +// ExtensionsDir is required and must point to a directory containing the extension +// YAML files (base + per-version patches). In production, this is provided by the +// php-buildpack repo via the --extensions-dir CLI flag. type PHPRecipe struct { Fetcher fetch.Fetcher - ExtensionsDir string // optional: filesystem directory containing extension YAML files + ExtensionsDir string // required: filesystem directory containing extension YAML files } func (p *PHPRecipe) Name() string { return "php" } @@ -89,18 +88,13 @@ func (p *PHPRecipe) Build(ctx context.Context, s *stack.Stack, src *source.Input phpMajor := parts[0] phpMinor := parts[1] - // Load extension set (base + patch). - // If ExtensionsDir is set, load from the filesystem; otherwise use the - // embedded YAML data compiled into internal/php/. - var ( - extSet *php.ExtensionSet - err error - ) - if p.ExtensionsDir != "" { - extSet, err = php.LoadFromDir(p.ExtensionsDir, phpMajor, phpMinor) - } else { - extSet, err = php.Load(phpMajor, phpMinor) + // Load extension set (base + patch) from the extensions directory. + // ExtensionsDir is required — it is provided by the php-buildpack repo via + // the --extensions-dir CLI flag in production, or by test fixtures in tests. + if p.ExtensionsDir == "" { + return fmt.Errorf("php: --extensions-dir is required for PHP builds (extension config has moved to the php-buildpack repo)") } + extSet, err := php.LoadFromDir(p.ExtensionsDir, phpMajor, phpMinor) if err != nil { return fmt.Errorf("php: loading extensions: %w", err) } diff --git a/internal/recipe/php_test.go b/internal/recipe/php_test.go index 80f6d78..0fd50cf 100644 --- a/internal/recipe/php_test.go +++ b/internal/recipe/php_test.go @@ -3,6 +3,7 @@ package recipe_test import ( "context" "os" + "path/filepath" "strings" "testing" @@ -15,6 +16,19 @@ import ( "github.com/stretchr/testify/require" ) +// phpTestdataDir is the absolute path to the extension YAML testdata files. +// Must be resolved at init time before any test calls useTempWorkDir() which +// changes the working directory. +var phpTestdataDir string + +func init() { + abs, err := filepath.Abs("../php/testdata") + if err != nil { + panic("php_test: cannot resolve testdata path: " + err.Error()) + } + phpTestdataDir = abs +} + func TestPHPRecipeName(t *testing.T) { r := &recipe.PHPRecipe{} assert.Equal(t, "php", r.Name()) @@ -41,7 +55,7 @@ func TestPHPRecipeBuildInstallsAptPackages(t *testing.T) { src := &source.Input{Version: "8.3.2"} outData := &output.OutData{} - r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()} + r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir} _ = r.Build(context.Background(), s, src, fakeRun, outData) assert.True(t, hasCallMatching(fakeRun.Calls, "apt-get", "libssl-dev"), "should apt-get install libssl-dev") @@ -65,7 +79,7 @@ func TestPHPRecipeBuildCreatesSymlinks(t *testing.T) { src := &source.Input{Version: "8.3.2"} outData := &output.OutData{} - r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()} + r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir} _ = r.Build(context.Background(), s, src, fakeRun, outData) assert.True(t, hasCallMatching(fakeRun.Calls, "ln", "/usr/local/include/curl"), "should create curl symlink") @@ -84,7 +98,7 @@ func TestPHPRecipeBuildConfigureFlags(t *testing.T) { src := &source.Input{Version: "8.3.2"} outData := &output.OutData{} - r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()} + r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir} _ = r.Build(context.Background(), s, src, fakeRun, outData) // The configure command is run via bash -c with LIBS=-lz prefix. @@ -113,15 +127,15 @@ func TestPHPRecipeBuildPopulatesSubDependencies(t *testing.T) { src := &source.Input{Version: "8.3.2"} outData := &output.OutData{} - r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()} + r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir} _ = r.Build(context.Background(), s, src, fakeRun, outData) - // SubDependencies should be populated from the embedded extension YAML. + // SubDependencies should be populated from the extension YAML testdata. // Check a representative sample of well-known extensions. require.NotNil(t, outData.SubDependencies) assert.Contains(t, outData.SubDependencies, "apcu", "apcu should be in sub-dependencies") assert.Contains(t, outData.SubDependencies, "rabbitmq", "rabbitmq should be in sub-dependencies") - assert.Greater(t, len(outData.SubDependencies), 20, "should have many sub-dependencies from the embedded YAML") + assert.Greater(t, len(outData.SubDependencies), 20, "should have many sub-dependencies from the testdata YAML") } func TestPHPRecipeBuildDownloadsSource(t *testing.T) { @@ -141,7 +155,7 @@ func TestPHPRecipeBuildDownloadsSource(t *testing.T) { outData := &output.OutData{} f := newFakeFetcher() - r := &recipe.PHPRecipe{Fetcher: f} + r := &recipe.PHPRecipe{Fetcher: f, ExtensionsDir: phpTestdataDir} _ = r.Build(context.Background(), s, src, fakeRun, outData) // Should download PHP source via Fetcher (not wget). @@ -160,7 +174,7 @@ func TestPHPRecipeBuildInvalidVersion(t *testing.T) { src := &source.Input{Version: "invalid"} outData := &output.OutData{} - r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()} + r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir} err := r.Build(context.Background(), s, src, fakeRun, outData) require.Error(t, err) assert.Contains(t, err.Error(), "invalid version") @@ -221,3 +235,23 @@ func TestPHPRecipeBuildExtensionsDirMissingBaseFile(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "extensions") } + +func TestPHPRecipeBuildRequiresExtensionsDir(t *testing.T) { + // ExtensionsDir is required. Build should fail with a clear error when + // it is not set (empty string). + useTempWorkDir(t) + fakeRun := runner.NewFakeRunner() + + s := &stack.Stack{ + Name: "cflinuxfs4", + AptPackages: map[string][]string{"php_build": {}}, + PHPSymlinks: []stack.Symlink{}, + } + src := &source.Input{Version: "8.3.2"} + outData := &output.OutData{} + + r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: ""} + err := r.Build(context.Background(), s, src, fakeRun, outData) + require.Error(t, err) + assert.Contains(t, err.Error(), "--extensions-dir is required") +}