-
Couldn't load subscription status.
- Fork 51
Update build scripts to support Windows ARM64 binaries #1916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📊 Performance Test ResultsComparing a74b5f8 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
| sha: windowsReleaseInfo.sha1, | ||
| url: `${ cdnURL }/${ baseName }-win32-v${ version }-full.nupkg`, | ||
| size: windowsReleaseInfo.size, | ||
| releasesData[ 'dev' ][ 'win32' ] = releasesData[ 'dev' ][ 'win32' ] ?? {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to update the API endpoint that handles app updates to account for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 194541-ghe-Automattic/wpcom
64146fc to
5c47b7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
| If ($LastExitCode -ne 0) { Exit $LastExitCode } | ||
|
|
||
| Write-Host "--- :node: Building App for Windows ($BuildType)" | ||
| Write-Host "--- :node: Building App for Windows ($BuildType) - $Architecture" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting nitpick:
| Write-Host "--- :node: Building App for Windows ($BuildType) - $Architecture" | |
| Write-Host "--- :node: Building App for Windows ($BuildType - $Architecture)" |
| "make:windows-release": "powershell -File .buildkite/commands/build-for-windows.ps1 -BuildType release", | ||
| "make:windows-dev": "powershell -File .buildkite/commands/build-for-windows.ps1 -BuildType dev", | ||
| "make:windows-x64": "electron-vite build --outDir=dist && electron-forge make --arch=x64 --platform=win32", | ||
| "make:windows-arm64": "electron-vite build --outDir=dist && electron-forge make --arch=arm64 --platform=win32", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, we look the npm run make:windows... command for dev vs release builds, exchanging it for x64 vs arm64.
Given the following check in the PS1 to build the app
studio/.buildkite/commands/build-for-windows.ps1
Lines 31 to 41 in b952849
| # Run appropriate script based on build type | |
| if ($BuildType -eq $BUILD_TYPE_DEV) { | |
| Write-Host "Preparing dev build..." | |
| node ./scripts/prepare-dev-build-version.mjs | |
| If ($LastExitCode -ne 0) { Exit $LastExitCode } | |
| $env:IS_DEV_BUILD="true" | |
| } else { | |
| Write-Host "Preparing release build..." | |
| node ./scripts/confirm-tag-matches-version.mjs | |
| If ($LastExitCode -ne 0) { Exit $LastExitCode } | |
| } |
I think this will result in calls to these commands building for release by default. This seems consistent with the other make:... and I think it's okay, but thought I'd to call it out just in case.
b2980c1 to
6883410
Compare
|
Note that currently our So if you have any npm native dependencies that require native compilation (typically via node-gyb), it'll likely fail for arm64 due to not finding the compiler for that architecture. @mokagio is currently working on pre-provisioning our Windows CI machines (custom AMI) so that the Windows SDK and MSVC tools would soon be pre-installed in those windows CI instances, not only to avoid having to install them from scratch on every job but also so that we pre-install the ARM64 compiler component too in addition to the x86 one. |
|
@AliSoftware, thanks for sharing those details. As far as I understand, The build completes, and the resulting binary runs on ARM. Do you see anything that can still break? |
even if the MSVC ARM64 compiler is not installed on the x64 machine doing the compilation?! Color me surprised 🤔 I wonder if the compilation passed for ARM64 this time just because of something like that dependency being available as a pre-compiled binary and not needing node-gyb to compile it locally (which would explain why it didn't need to have the ARM64 compiler installed)? 🤔 |
I checked it more - downloaded the arm64 build from this branch, installed the app on ARM (Parallels on Mac), located fs_ext.node in app.asar.unpack, and it looks like it's x64: It would mean that build and compilation works just fine without further changes, but the arm64 build compiles native dependencies for x64, and it works well on Windows on arm64, thanks to x64 emulation? (It works already for the Intel binary, which works on arm64). |

Related issues
Proposed Changes
Adds support for building Windows ARM64 binaries alongside x64, following the same architecture matrix pattern used for macOS builds.
Changes
Build Scripts:
make:windows-x64andmake:windows-arm64npm scripts.buildkite/commands/build-for-windows.ps1to accept architecture parameterscripts/package-appx.mjsarchitecture-awareCI Pipeline:
Distribution:
scripts/generate-releases-manifest.mjsto generate architecture-specific manifest entriesstudio-win32-{arch}-v{version}.exeCleanup:
make:windows-devandmake:windows-releasescriptsBreaking Change⚠️
The
releases.jsonmanifest structure for Windows has changed from:To:
The backend update API (
wpcom/v2/studio-app/updates) will need to be updated to look upreleases[version][platform][arch]instead ofreleases[version][platform]for Windows. The app already sendsprocess.archin the update request.Testing Instructions
I tested:
npm run make:windows-x64andnpm run make:windows-arm64Pre-merge Checklist