Bundle default PHP binary#3554
Conversation
📊 Performance Test ResultsComparing 87dfc0c 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) |
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
LGTM 👍 I left a few comments that would be nice to address, but the fundamentals look solid
There was a problem hiding this comment.
I know this script was there before this PR, but it strikes me now that we should maybe say that it downloads a PHP package rather than a binary, because it's not just that single binary on Windows.
| const binaryName = isWindows ? 'php.exe' : 'php'; | ||
| const platformKey = `${ args.platform }-${ effectiveArch }`; | ||
| const phpBinaryRoot = | ||
| process.env.STUDIO_PHP_BINARY_INSTALL_ROOT ?? path.join( getConfigDirectory(), 'php-bin' ); |
There was a problem hiding this comment.
STUDIO_PHP_BINARY_INSTALL_ROOT could be an option passed to the script instead of an environment variable.
| destination patch folder does not exist. For non-bundled PHP versions, Studio downloads the | ||
| tracked patch for the current platform and architecture, then verifies the | ||
| checked-in SHA-256 before extracting the archive. If metadata is missing for the | ||
| requested device, native PHP install fails for that version. |
There was a problem hiding this comment.
| destination patch folder does not exist. For non-bundled PHP versions, Studio downloads the | |
| tracked patch for the current platform and architecture, then verifies the | |
| checked-in SHA-256 before extracting the archive. If metadata is missing for the | |
| requested device, native PHP install fails for that version. | |
| destination patch folder does not exist. Studio downloads other PHP versions | |
| on demand from manifest URLs, then verifies the checked-in SHA-256 before | |
| extracting the archive. If metadata is missing for the requested device, native | |
| PHP install fails for that version. |
Doesn't have to be exactly like this, but I think we can make it clearer that we ship with one PHP version and the other ones are downloaded on demand.
| return fs.existsSync( getBundledDefaultPhpPath() ); | ||
| } | ||
|
|
||
| export const installBundledDefaultPhp: Migration = { |
There was a problem hiding this comment.
Let's add a comment that explains what this migration does.
| if ( ! bundledDefaultPhpExists() || fs.existsSync( getDefaultPhpDestinationDir() ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
It looks like we're repeating needsToRun() here
3 participants
Related issues
How AI was used in this PR
Codex helped implement and review the packaging/runtime changes. I reviewed the diff and manually smoke-tested the packaged app behavior.
Proposed Changes
~/.studio/php-bin/<patch>before falling back to CDN download.php.ini,ca-bundle.crt, and runtime files are managed in the user config directory.Testing Instructions
macOS packaged-app smoke test:
npm run make:macos-arm64npm run make:macos-x64find apps/studio/out -path '*/Studio.app/Contents/Resources/php-bin/*/php' -type f -lsapps/studio/out/Studio-darwin-arm64/Studio.app/Contents/Resources/php-bin/8.4.21/php.export DEV_CONFIG_DIR="$(mktemp -d)"export STUDIO_RUNTIME=native-phpapps/studio/out/Studio-darwin-arm64/Studio.app/Contents/MacOS/Studiofind "$DEV_CONFIG_DIR/php-bin" -maxdepth 3 -type f | sortphp-bin/8.4.21/php,php-bin/8.4.21/php.ini,php-bin/8.4.21/ca-bundle.crt,php-bin/8.4.21/runtime.json, andphp-bin/8.4.21/ext/xdebug.so."$DEV_CONFIG_DIR/php-bin/8.4.21/php" -vWindows packaged-app smoke test:
npm run make:windows-x64npm run make:windows-arm64Get-ChildItem .\apps\studio\out -Recurse -Filter php.exe | Where-Object { $_.FullName -like '*\resources\php-bin\*\php.exe' } | Select-Object -ExpandProperty FullNameapps\studio\out\Studio-win32-x64\resources\php-bin\8.4.21\php.exe.$env:DEV_CONFIG_DIR = Join-Path $env:TEMP ("studio-test-" + [guid]::NewGuid())New-Item -ItemType Directory -Path $env:DEV_CONFIG_DIR | Out-Null$env:STUDIO_RUNTIME = "native-php".\apps\studio\out\Studio-win32-x64\Studio.exeGet-ChildItem "$env:DEV_CONFIG_DIR\php-bin" -Recurse -File | Sort-Object FullName | Select-Object -ExpandProperty FullNamephp-bin\8.4.21\php.exe,php-bin\8.4.21\php.ini,php-bin\8.4.21\ca-bundle.crt,php-bin\8.4.21\runtime.json, and extension DLLs underphp-bin\8.4.21\ext\.& (Get-ChildItem "$env:DEV_CONFIG_DIR\php-bin" -Recurse -Filter php.exe | Select-Object -First 1).FullName -vPre-merge Checklist