Include Visual C++ DLLs with Windows PHP builds and add packageVersion field to identify PHP packages#4042
Conversation
fredrikekelund
changed the title
packageVersion field to identify PHP packages
📊 Performance Test ResultsComparing f7398ec 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) |
| $vswhere = Join-Path ${env:ProgramFiles(x86)} 'Microsoft Visual Studio\Installer\vswhere.exe' | ||
| $vsInstallPath = & $vswhere -latest -version '[17.0,18.0)' -products * -property installationPath | ||
| if ( -not $vsInstallPath ) { | ||
| throw 'Could not locate Visual Studio to package the Visual C++ runtime' | ||
| } | ||
|
|
||
| $vcRedistRoot = Join-Path $vsInstallPath 'VC\Redist\MSVC' | ||
| $vcRuntimeDir = Get-ChildItem -Path $vcRedistRoot -Directory | | ||
| ForEach-Object { Join-Path $_.FullName 'x64\Microsoft.VC143.CRT' } | | ||
| Where-Object { Test-Path (Join-Path $_ 'vcruntime140.dll') } | | ||
| Sort-Object { | ||
| [version]( (Get-Item (Join-Path $_ 'vcruntime140.dll')).VersionInfo.FileVersion ) | ||
| } -Descending | | ||
| Select-Object -First 1 | ||
| if ( -not $vcRuntimeDir ) { | ||
| throw "Could not locate the x64 app-local Visual C++ runtime under $vcRedistRoot" | ||
| } | ||
|
|
||
| $requiredVcRuntimeDlls = @('vcruntime140.dll', 'vcruntime140_1.dll') | ||
| foreach ( $dll in $requiredVcRuntimeDlls ) { | ||
| if ( -not (Test-Path (Join-Path $vcRuntimeDir $dll)) ) { | ||
| throw "Required Visual C++ runtime DLL not found: $dll" | ||
| } | ||
| } | ||
|
|
||
| Copy-Item -Path (Join-Path $vcRuntimeDir '*.dll') -Destination $stagingDir | ||
| $vcRuntimeVersion = (Get-Item (Join-Path $vcRuntimeDir 'vcruntime140.dll')).VersionInfo.FileVersion | ||
| Set-Content -Path (Join-Path $stagingDir 'vc-runtime.version') -Value $vcRuntimeVersion -NoNewline |
There was a problem hiding this comment.
Copying the DLLs from the CI Windows system was the method recommended by Codex for putting them next to the php.exe. AFAICT, Microsoft only offers an installation wizard for the Visual C++ Redistributables – not static DLLs that we can download and put where we want them.
|
Confirmed that #4045 fixes the issue on a Windows VM that has no system-wide Visual C++ Redistributable DLLs 👍 |
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
Code and fix LGTM 👍
The true test will happen when we distribute the new build, let me know if you want an extra validation then.
Suggestion:
- Let's add a migration to remove superseded PHP package dirs (older patch/package revisions of each minor, and minors we no longer support). Keeping them in
~/.studio/php-bin/just takes space for no purpose. It's ok to tackle as a follow-up.
Agreed that this is a good idea, although I'd prefer to do it in a follow-up. Not critical yet, either, as this is only the second version of each PHP package that we ship to users. |
3 participants
Related issues
How AI was used in this PR
Codex drove the implementation based on previous diagnosis.
Proposed Changes
Sentry shows that some users are running into
PHP command failed (code: 3221225781)errors. This is because of missing DLLs on their system – very likely the Visual C++ Redistributable ones, as PHP explicitly names that dependency. This PR updates the native PHP build workflow to include the necessary DLLs next tophp.exe.Because there aren't new PHP patch versions available yet for every minor PHP version we offer, I also had to find a way to rebuild PHP versions that already exist on appscdn. This cannot be done today without breaking local Studio installations, because they expect the appscdn package to have a predefined checksum. If the checksum of the appscdn package changes, the Studio client will refuse to install it.
The solution is to add a
packageVersionfield that identifies the package version separately from the PHP version. The workflow sets this tostudio-1by default – meaning a typical workflow run (that builds a new PHP version) can just use the default. Moreover, I've seterror_on_duplicateto always be true (same as in #4026, which I mistakenly merged to another branch than trunk)Testing Instructions
I'll manually verify that these builds do what we want on Windows.
Pre-merge Checklist