Conversation
dcf22ae to
02bc232
Compare
fredrikekelund
left a comment
There was a problem hiding this comment.
I know this isn't ready yet, but I left a few comments with preliminary observations
| php bin/spc download --with-php="$PHP_MINOR" --for-extensions="$EXTENSIONS" --retry=2 | ||
|
|
||
| BUILD_ROOT="$SPC_DIR/buildroot-arm64" | ||
| SOURCE_PATH="$SPC_DIR/source-arm64" | ||
| PKG_ROOT="$SPC_DIR/pkgroot/aarch64-darwin" | ||
| SPC_ENV=( | ||
| "BUILD_ROOT_PATH=$BUILD_ROOT" | ||
| "SOURCE_PATH=$SOURCE_PATH" | ||
| "PKG_ROOT_PATH=$PKG_ROOT" | ||
| ) | ||
|
|
||
| rm -rf "$BUILD_ROOT" "$SOURCE_PATH" | ||
|
|
||
| env "${SPC_ENV[@]}" php bin/spc install-pkg pkg-config | ||
| env "${SPC_ENV[@]}" php bin/spc doctor --auto-fix=never | ||
| env "${SPC_ENV[@]}" php bin/spc build "$EXTENSIONS" --build-cli --with-suggested-libs |
There was a problem hiding this comment.
The spc craft command, with its more declarative approach, seems like an appealing option. AFAICT, it wraps spc download, spc install-pkg, and spc build into a single command.
| python3 - "$SPC_DIR" <<'PY' | ||
| from pathlib import Path | ||
| import sys | ||
|
|
||
| root = Path(sys.argv[1]) | ||
|
|
||
| def replace_once(path, old, new): | ||
| file = root / path | ||
| text = file.read_text() | ||
| if new in text: | ||
| return | ||
| if old not in text: | ||
| raise SystemExit(f"Could not patch {path}") | ||
| file.write_text(text.replace(old, new, 1)) | ||
|
|
||
| def ensure_patch(path, marker, replacements): | ||
| file = root / path | ||
| text = file.read_text() | ||
| if marker in text: | ||
| return | ||
| for old, new in replacements: | ||
| if old in text: | ||
| file.write_text(text.replace(old, new, 1)) | ||
| return | ||
| raise SystemExit(f"Could not patch {path}") | ||
|
|
||
| replace_once( | ||
| "src/SPC/util/executor/UnixCMakeExecutor.php", | ||
| """ $target_arch = arch2gnu(php_uname('m')); | ||
| $cflags = getenv('SPC_DEFAULT_C_FLAGS'); | ||
| """, | ||
| """ $target_arch = arch2gnu(php_uname('m')); | ||
| $target_mac_arch = match ($target_arch) { | ||
| 'aarch64' => 'arm64', | ||
| default => $target_arch, | ||
| }; | ||
| $cflags = getenv('SPC_DEFAULT_C_FLAGS'); | ||
| """, | ||
| ) | ||
| ensure_patch( | ||
| "src/SPC/util/executor/UnixCMakeExecutor.php", | ||
| 'CMAKE_OSX_ARCHITECTURES \\"{$target_mac_arch}\\"', | ||
| [ | ||
| ( | ||
| ' $toolchain .= "\\nset(CMAKE_OSX_ARCHITECTURES \\"{$target_arch}\\" CACHE STRING \\"\\" FORCE)";', | ||
| ' $toolchain .= "\\nset(CMAKE_OSX_ARCHITECTURES \\"{$target_mac_arch}\\" CACHE STRING \\"\\" FORCE)";', | ||
| ), | ||
| ( | ||
| """CMAKE; | ||
| // Whoops, linux may need CMAKE_AR sometimes | ||
| """, | ||
| """CMAKE; | ||
| if (PHP_OS_FAMILY === 'Darwin') { | ||
| $toolchain .= "\\nset(CMAKE_OSX_ARCHITECTURES \\"{$target_mac_arch}\\" CACHE STRING \\"\\" FORCE)"; | ||
| } | ||
| // Whoops, linux may need CMAKE_AR sometimes | ||
| """, | ||
| ), | ||
| ], | ||
| ) | ||
| ensure_patch( | ||
| "src/SPC/util/SPCConfigUtil.php", | ||
| "$libs = str_replace('-lstdc++', '', $libs);", | ||
| [ | ||
| ( | ||
| """ if ($this->hasCpp($extensions, $libraries)) { | ||
| $libcpp = SPCTarget::getTargetOS() === 'Darwin' ? '-lc++' : '-lstdc++'; | ||
| $libs = str_replace($libcpp, '', $libs) . " {$libcpp}"; | ||
| } | ||
| """, | ||
| """ if ($this->hasCpp($extensions, $libraries)) { | ||
| $isDarwin = SPCTarget::getTargetOS() === 'Darwin'; | ||
| $libcpp = $isDarwin ? '-lc++' : '-lstdc++'; | ||
| $libs = str_replace($libcpp, '', $libs); | ||
| if ($isDarwin) { | ||
| $libs = str_replace('-lstdc++', '', $libs); | ||
| } | ||
| $libs .= " {$libcpp}"; | ||
| } | ||
| """, | ||
| ), | ||
| ], | ||
| ) | ||
| PY |
There was a problem hiding this comment.
Two things:
- Can we add some comments to explain what's going on here?
- Why Python?
| file "$PHP_BIN" | ||
| "$PHP_BIN" --version | grep -q "PHP $PHP_VERSION " | ||
|
|
||
| modules="$("$PHP_BIN" -m)" | ||
| expected_modules=( | ||
| ctype curl dom exif fileinfo filter gd iconv imagick intl mbstring mysqli mysqlnd | ||
| "Zend OPcache" openssl PDO pdo_sqlite Phar session SimpleXML sodium sqlite3 | ||
| tokenizer xml xmlreader xmlwriter zip zlib | ||
| ) | ||
| excluded_modules=(redis apcu bcmath pdo_mysql pgsql imap soap sockets ftp) | ||
|
|
||
| for module in "${expected_modules[@]}"; do | ||
| if ! grep -Fxq "$module" <<<"$modules"; then | ||
| echo "Expected PHP module missing: $module" >&2 | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| for module in "${excluded_modules[@]}"; do | ||
| if grep -Fxq "$module" <<<"$modules"; then | ||
| echo "Unexpected PHP module present: $module" >&2 | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Not bad, but strikes me as somewhat redundant…
| PHP_MINOR="${PHP_MINOR:-"${PHP_VERSION%.*}"}" | ||
| OUTPUT_DIR="${OUTPUT_DIR:-"$ROOT_DIR/out/php-binaries"}" | ||
| ARTIFACT_BASENAME="php-${PHP_VERSION}-cli-macos-aarch64" | ||
| EXTENSIONS="ctype,curl,dom,exif,fileinfo,filter,gd,iconv,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pdo,pdo_sqlite,phar,session,simplexml,sodium,sqlite3,tokenizer,xml,xmlreader,xmlwriter,zip,zlib" |
There was a problem hiding this comment.
| EXTENSIONS="ctype,curl,dom,exif,fileinfo,filter,gd,iconv,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pdo,pdo_sqlite,phar,session,simplexml,sodium,sqlite3,tokenizer,xml,xmlreader,xmlwriter,zip,zlib" | |
| EXTENSIONS="apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,gettext,iconv,igbinary,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_sqlite,phar,posix,readline,redis,session,shmop,simplexml,sockets,sodium,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,xsl,yaml,zip,zlib" |
Adds the following extensions: apcu, bcmath, calendar, dba, ftp, gettext, igbinary, pcntl, pdo_mysql, posix, readline, redis, shmop, sockets, ssh2, xsl, yaml.
This is quite a generous list, but I don't think that hurts. It's based on the official WordPress recommendation and static-php's "common extensions" list.
Moreover, per my other comment, it'd be nice to use the declarative config file craft.yml for clarity. I assume it also helps with cross-platform portability.
📊 Performance Test ResultsComparing 637658f vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
…cli-binaries' into rsm-1650-codex-build-static-php-cli-binaries
| artifact_arch: x86_64 | ||
| archive_extension: zip | ||
| binary_name: php.exe | ||
| extensions: apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,iconv,igbinary,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pdo,pdo_mysql,pdo_sqlite,phar,redis,session,shmop,simplexml,sockets,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,yaml,zip,zlib |
There was a problem hiding this comment.
No gettext, intl, sodium, or xsl on Windows?
| extensions: apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,gettext,iconv,igbinary,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_sqlite,phar,posix,readline,redis,session,shmop,simplexml,sockets,sodium,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,xsl,yaml,zip,zlib | ||
| - target: macos-x86_64 | ||
| runner: macos-15-intel | ||
| artifact_platform: macos | ||
| artifact_arch: x86_64 | ||
| archive_extension: tar.gz | ||
| binary_name: php | ||
| extensions: apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,gettext,iconv,igbinary,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_sqlite,phar,posix,readline,redis,session,shmop,simplexml,sockets,sodium,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,xsl,yaml,zip,zlib |
There was a problem hiding this comment.
| extensions: apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,gettext,iconv,igbinary,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_sqlite,phar,posix,readline,redis,session,shmop,simplexml,sockets,sodium,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,xsl,yaml,zip,zlib | |
| - target: macos-x86_64 | |
| runner: macos-15-intel | |
| artifact_platform: macos | |
| artifact_arch: x86_64 | |
| archive_extension: tar.gz | |
| binary_name: php | |
| extensions: apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,gettext,iconv,igbinary,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_sqlite,phar,posix,readline,redis,session,shmop,simplexml,sockets,sodium,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,xsl,yaml,zip,zlib | |
| extensions: &php_extensions | |
| apcu,bcmath,calendar,ctype,curl,dba,dom,exif,fileinfo,filter,ftp,gd,gettext,iconv,igbinary,imagick,intl,mbregex,mbstring,mysqli,mysqlnd,opcache,openssl,pcntl,pdo,pdo_mysql,pdo_sqlite,phar,posix,readline,redis,session,shmop,simplexml,sockets,sodium,sqlite3,ssh2,tokenizer,xml,xmlreader,xmlwriter,xsl,yaml,zip,zlib | |
| - target: macos-x86_64 | |
| runner: macos-15-intel | |
| artifact_platform: macos | |
| artifact_arch: x86_64 | |
| archive_extension: tar.gz | |
| binary_name: php | |
| extensions: *php_extensions |
We could explicitly reuse the extensions definition for both macOS architectures like so.
|
|
||
| steps: | ||
| - name: Checkout Studio | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v6 |
I'm not GitHub Actions wiz, but v6 is the latest version. Seems like we could use that.
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Checkout static-php-cli | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v6 |
| chmod +x "$RUNNER_TEMP/php" | ||
| xattr -cr "$RUNNER_TEMP/php" | ||
| tar -czf "$out_dir/$artifact" -C "$RUNNER_TEMP" php | ||
| shasum -a 256 "$out_dir/$artifact" | awk '{print $1}' > "$out_dir/$artifact.sha256" |
There was a problem hiding this comment.
If we build and distribute the binaries ourselves, what do we do with the checksums?
There was a problem hiding this comment.
When we upload the binaries to the CDN and update the binary metadata, we can store the checksum alongside each CDN URL. It’s a good safety measure to verify the archive after download and before extraction.
| Studio currently downloads native PHP binaries on demand from the upstream | ||
| static-php-cli CDN. Custom Studio-built binaries are not bundled in the repo or | ||
| uploaded by PR CI. | ||
|
|
There was a problem hiding this comment.
I say we can drop this part and just focus on how it'll work once this PR lands.
| Do not patch `static-php-cli` by default. If the upstream build fails, use the | ||
| workflow logs to add the smallest targeted patch and document the exact upstream | ||
| failure that requires it. |
There was a problem hiding this comment.
This feel like it's written for AI. Why did we have to patch it before?
There was a problem hiding this comment.
It was one of the attempts to build Mac Intel, but it no longer applies.
|
Looking good! We're still missing |

Related issues
How AI was used in this PR
AI helped compare the Buildkite and GitHub Actions approaches, simplify the static-php-cli build, and validate the generated artifacts. I reviewed the changes and tested the resulting binaries locally.
Proposed Changes
Build PHP CLI BinariesGitHub Actions workflow.static-php-cliinstead of custom Buildkite scripts.8.4.20and static-php-cli2.8.5, with workflow inputs to override them.macos-aarch64,macos-x86_64, andwindows-x86_64..sha256sidecars as GitHub Actions artifacts.Testing Instructions
macos-aarch64,macos-x86_64, andwindows-x86_64jobs pass..sha256file.php -vreports PHP8.4.20..studiofolder -~/.studio/php-bin/8.4for examplenpm run cli:buildSTUDIO_RUNTIME=native-php node apps/cli/dist/cli/main.mjs site createPre-merge Checklist