-
Notifications
You must be signed in to change notification settings - Fork 52
Feat/windows arm64 support #63
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
base: main
Are you sure you want to change the base?
Changes from all commits
0d4dd3a
42083e7
cb115d3
e8f895e
f17a263
365652c
edc73fd
fc933b3
ec629d5
b315d0d
9005d67
b29f832
6e42f13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,29 +29,68 @@ concurrency: | |
|
|
||
| jobs: | ||
| build-windows: | ||
| name: Windows installer | ||
| name: Windows ${{ matrix.arch }} installer | ||
| runs-on: windows-latest | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| arch: [x64, arm64] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✓ Correct matrix shape — One nit: the |
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: ./.github/actions/setup | ||
|
|
||
| - name: Ensure MSVC ARM64 build tools | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two failure modes here that need guarding.
|
||
| if: matrix.arch == 'arm64' | ||
| shell: pwsh | ||
| run: | | ||
| $vswhere = "C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe" | ||
| $installPath = & $vswhere -latest -products * -property installationPath | ||
| $hasArm64 = & $vswhere -latest -products * ` | ||
| -requires Microsoft.VisualStudio.Component.VC.Tools.ARM64 ` | ||
| -property installationPath | ||
| if (-not $hasArm64) { | ||
| Write-Host "Installing MSVC ARM64 build tools component..." | ||
| $installer = "C:\Program Files (x86)\Microsoft Visual Studio\Installer\vs_installer.exe" | ||
| & $installer modify --installPath "$installPath" ` | ||
| --add Microsoft.VisualStudio.Component.VC.Tools.ARM64 ` | ||
| --quiet --norestart --wait | ||
| } else { | ||
| Write-Host "MSVC ARM64 build tools already present." | ||
| } | ||
|
|
||
| - name: Ensure win32-arm64 sharp binary | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dead-code step — Either delete both |
||
| # sharp (transitive via @xenova/transformers) only fetches a host-arch | ||
| # prebuilt at install time, so on the x64 runner the packaged arm64 app | ||
| # would otherwise ship an x64 sharp.node. Best-effort fetch of the | ||
| # win32-arm64 prebuilt; non-fatal so it never blocks the build. Verify | ||
| # the first arm64 release loads sharp if auto-captions use it. | ||
| if: matrix.arch == 'arm64' | ||
| continue-on-error: true | ||
| shell: bash | ||
| run: npm_config_platform=win32 npm_config_arch=arm64 npm rebuild sharp | ||
|
|
||
| - name: Cache caption assets | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: caption-assets | ||
| key: caption-assets-${{ runner.os }}-${{ hashFiles('scripts/fetch-caption-model.mjs') }} | ||
|
|
||
| - name: Build Windows app | ||
| - name: Build Windows app (x64) | ||
| if: matrix.arch == 'x64' | ||
| run: npm run build:win -- --publish never | ||
|
|
||
| - name: Build Windows app (arm64) | ||
| if: matrix.arch == 'arm64' | ||
| run: npm run build:win:arm64 -- --publish never | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✓ |
||
|
|
||
| - name: Upload Windows installer | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: openscreen-windows | ||
| path: release/**/Openscreen.Setup.*.exe | ||
| name: openscreen-windows-${{ matrix.arch }} | ||
| path: release/**/Openscreen.Setup.*-${{ matrix.arch }}.exe | ||
| if-no-files-found: error | ||
| retention-days: 30 | ||
|
|
||
|
|
@@ -330,10 +369,11 @@ jobs: | |
| echo "prerelease_flag=$PRERELEASE_FLAG" >> "$GITHUB_OUTPUT" | ||
| echo "notes_start_tag=$NOTES_START_TAG" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Download Windows installer | ||
| - name: Download Windows installers | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: openscreen-windows | ||
| pattern: openscreen-windows-* | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✓ |
||
| merge-multiple: true | ||
| path: artifacts/windows | ||
|
|
||
| - name: Download macOS arm64 DMG | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,7 +90,7 @@ | |
| "nsis" | ||
| ], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✓ Correct: the Adjacent issue (not changed in this PR): the |
||
| "icon": "icons/icons/win/icon.ico", | ||
| "artifactName": "${productName}.Setup.${version}.${ext}", | ||
| "artifactName": "${productName}.Setup.${version}-${arch}.${ext}", | ||
| "extraResources": [ | ||
| { | ||
| "from": "electron/native/bin", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,8 @@ | |
| "build:mac": "npm run build:native:mac && tsc && vite build && electron-builder --mac", | ||
| "build:native:win": "node scripts/build-windows-wgc-helper.mjs", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To prevent host-architecture compilation conflicts on Windows ARM64 developer machines (where "build:native:win": "node scripts/build-windows-wgc-helper.mjs --arch x64"
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
so the default script explicitly targets x64. The new If you reject this change, push back in the thread with reasoning before requesting re-review. |
||
| "build:win": "npm run build:native:win && tsc && vite build && electron-builder --win --config.npmRebuild=false", | ||
| "build:native:win:arm64": "node scripts/build-windows-wgc-helper.mjs --arch arm64", | ||
| "build:win:arm64": "npm run build:native:win:arm64 && tsc && vite build && electron-builder --win --arm64 --config.npmRebuild=false", | ||
| "build:linux": "tsc && vite build && electron-builder --linux AppImage deb pacman --config.npmRebuild=false", | ||
| "test": "vitest --run", | ||
| "test:watch": "vitest", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,37 @@ import os from "node:os"; | |
| import path from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| import { | ||
| resolveTargetArch, | ||
| resolveVcvarsArch, | ||
| winBinDirName, | ||
| } from "./windows-helper-arch.mjs"; | ||
|
|
||
| function parseArchFlag(argv) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Two fixes:
|
||
| const eq = argv.find((a) => a.startsWith("--arch=")); | ||
| if (eq) { | ||
| return eq.slice("--arch=".length); | ||
| } | ||
| const idx = argv.indexOf("--arch"); | ||
| if (idx !== -1) { | ||
| return argv[idx + 1]; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| const TARGET_ARCH = resolveTargetArch({ | ||
| cliArch: parseArchFlag(process.argv.slice(2)), | ||
| envArch: process.env.OPENSCREEN_WIN_HELPER_ARCH, | ||
| hostArch: process.arch, | ||
| }); | ||
| const VCVARS_ARCH = resolveVcvarsArch(process.arch, TARGET_ARCH); | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
| const ROOT = path.join(__dirname, ".."); | ||
| const SOURCE_DIR = path.join(ROOT, "electron", "native", "wgc-capture"); | ||
| const BUILD_DIR = path.join(SOURCE_DIR, "build"); | ||
| const COMPAT_LIB_DIR = path.join(BUILD_DIR, "compat-libs"); | ||
| const BIN_DIR = path.join(ROOT, "electron", "native", "bin", "win32-x64"); | ||
| const BIN_DIR = path.join(ROOT, "electron", "native", "bin", winBinDirName(TARGET_ARCH)); | ||
| const CMAKE = process.env.CMAKE_EXE ?? "cmake"; | ||
|
|
||
| function findVcVarsAll() { | ||
|
|
@@ -75,7 +100,7 @@ function findWindowsSdkUmLibDir() { | |
| return fs | ||
| .readdirSync(sdkLibRoot, { withFileTypes: true }) | ||
| .filter((entry) => entry.isDirectory()) | ||
| .map((entry) => path.join(sdkLibRoot, entry.name, "um", "x64")) | ||
| .map((entry) => path.join(sdkLibRoot, entry.name, "um", TARGET_ARCH)) | ||
| .filter((candidate) => fs.existsSync(path.join(candidate, "kernel32.lib"))) | ||
| .sort() | ||
| .at(-1); | ||
|
|
@@ -115,10 +140,10 @@ async function runInVsEnv(command) { | |
| cmdPath, | ||
| [ | ||
| "@echo off", | ||
| `call "${vcvarsAll}" x64`, | ||
| `call "${vcvarsAll}" ${VCVARS_ARCH}`, | ||
| "if errorlevel 1 exit /b %errorlevel%", | ||
| `if not exist "${COMPAT_LIB_DIR}" mkdir "${COMPAT_LIB_DIR}"`, | ||
| `for %%L in (gdi32.lib gdiplus.lib winspool.lib shell32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib) do if not exist "%WindowsSdkDir%Lib\\%WindowsSDKLibVersion%um\\x64\\%%L" copy /Y "%WindowsSdkDir%Lib\\%WindowsSDKLibVersion%um\\x64\\kernel32.Lib" "${COMPAT_LIB_DIR}\\%%L" >nul`, | ||
| `for %%L in (gdi32.lib gdiplus.lib winspool.lib shell32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib) do if not exist "%WindowsSdkDir%Lib\\%WindowsSDKLibVersion%um\\${TARGET_ARCH}\\%%L" copy /Y "%WindowsSdkDir%Lib\\%WindowsSDKLibVersion%um\\${TARGET_ARCH}\\kernel32.Lib" "${COMPAT_LIB_DIR}\\%%L" >nul`, | ||
| "if errorlevel 1 exit /b %errorlevel%", | ||
| `set "LIB=${sdkUmLibDir ? `${sdkUmLibDir};` : ""}%LIB%;${COMPAT_LIB_DIR}"`, | ||
| command, | ||
|
|
@@ -138,7 +163,23 @@ if (process.platform !== "win32") { | |
| process.exit(0); | ||
| } | ||
|
|
||
| console.log(`Building Windows WGC helper for target arch: ${TARGET_ARCH} (vcvars: ${VCVARS_ARCH})`); | ||
|
|
||
| // CMake caches the detected compiler in the build directory. Reusing a build | ||
| // dir that was configured for a different target arch silently produces | ||
| // wrong-arch binaries (the cached compiler wins over the new vcvars env). Wipe | ||
| // the build dir when the target arch changes; same-arch reruns stay incremental. | ||
| const ARCH_STAMP = path.join(BUILD_DIR, ".target-arch"); | ||
| if (fs.existsSync(BUILD_DIR)) { | ||
| const previousArch = fs.existsSync(ARCH_STAMP) | ||
| ? fs.readFileSync(ARCH_STAMP, "utf8").trim() | ||
| : ""; | ||
| if (previousArch !== TARGET_ARCH) { | ||
| fs.rmSync(BUILD_DIR, { recursive: true, force: true }); | ||
| } | ||
| } | ||
| fs.mkdirSync(BUILD_DIR, { recursive: true }); | ||
| fs.writeFileSync(ARCH_STAMP, TARGET_ARCH); | ||
|
|
||
| await runInVsEnv( | ||
| `"${CMAKE}" -S "${SOURCE_DIR}" -B "${BUILD_DIR}" -G Ninja -DCMAKE_BUILD_TYPE=Release`, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // Pure helpers that map a target CPU architecture to the parameters the Windows | ||
| // native-helper build needs. Kept side-effect free so it is unit-testable and | ||
| // importable from both the build script and Vitest. | ||
|
|
||
| export const SUPPORTED_ARCHES = ["x64", "arm64"]; | ||
|
|
||
| const ALIASES = new Map([ | ||
| ["x64", "x64"], | ||
| ["amd64", "x64"], | ||
| ["x86_64", "x64"], | ||
| ["arm64", "arm64"], | ||
| ["aarch64", "arm64"], | ||
| ]); | ||
|
|
||
| export function normalizeArch(value) { | ||
| if (value === undefined || value === null || value === "") { | ||
| return undefined; | ||
| } | ||
| return ALIASES.get(String(value).toLowerCase()); | ||
| } | ||
|
|
||
| export function resolveTargetArch({ cliArch, envArch, hostArch } = {}) { | ||
| for (const [label, raw] of [ | ||
| ["--arch", cliArch], | ||
| ["OPENSCREEN_WIN_HELPER_ARCH", envArch], | ||
| ]) { | ||
| if (raw !== undefined && raw !== null && raw !== "") { | ||
| const normalized = normalizeArch(raw); | ||
| if (!normalized) { | ||
| throw new Error( | ||
| `Invalid ${label} value "${raw}". Expected one of: ${SUPPORTED_ARCHES.join(", ")}.`, | ||
| ); | ||
| } | ||
| return normalized; | ||
| } | ||
| } | ||
|
|
||
| const host = normalizeArch(hostArch); | ||
| if (!host) { | ||
| throw new Error( | ||
| `Unsupported host architecture "${hostArch}". Pass --arch with one of: ${SUPPORTED_ARCHES.join(", ")}.`, | ||
| ); | ||
| } | ||
| return host; | ||
| } | ||
|
|
||
| export function resolveVcvarsArch(hostArch, targetArch) { | ||
| const host = normalizeArch(hostArch); | ||
| const target = normalizeArch(targetArch); | ||
| if (!host || !target) { | ||
| throw new Error(`Unsupported architecture pair host="${hostArch}" target="${targetArch}".`); | ||
| } | ||
| return host === target ? target : `${host}_${target}`; | ||
| } | ||
|
|
||
| export function winBinDirName(targetArch) { | ||
| const target = normalizeArch(targetArch); | ||
| if (!target) { | ||
| throw new Error(`Unsupported target architecture "${targetArch}".`); | ||
| } | ||
| return `win32-${target}`; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| SUPPORTED_ARCHES, | ||
| normalizeArch, | ||
| resolveTargetArch, | ||
| resolveVcvarsArch, | ||
| winBinDirName, | ||
| } from "./windows-helper-arch.mjs"; | ||
|
|
||
| describe("normalizeArch", () => { | ||
| it("maps known aliases to canonical arches", () => { | ||
| expect(normalizeArch("x64")).toBe("x64"); | ||
| expect(normalizeArch("amd64")).toBe("x64"); | ||
| expect(normalizeArch("x86_64")).toBe("x64"); | ||
| expect(normalizeArch("arm64")).toBe("arm64"); | ||
| expect(normalizeArch("aarch64")).toBe("arm64"); | ||
| expect(normalizeArch("ARM64")).toBe("arm64"); | ||
| }); | ||
|
|
||
| it("returns undefined for empty or unknown values", () => { | ||
| expect(normalizeArch(undefined)).toBeUndefined(); | ||
| expect(normalizeArch("")).toBeUndefined(); | ||
| expect(normalizeArch("mips")).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("resolveTargetArch", () => { | ||
| it("prefers the CLI arch over env and host", () => { | ||
| expect(resolveTargetArch({ cliArch: "arm64", envArch: "x64", hostArch: "x64" })).toBe("arm64"); | ||
| }); | ||
|
|
||
| it("falls back to env when no CLI arch", () => { | ||
| expect(resolveTargetArch({ envArch: "arm64", hostArch: "x64" })).toBe("arm64"); | ||
| }); | ||
|
|
||
| it("defaults to the host arch when nothing explicit is given", () => { | ||
| expect(resolveTargetArch({ hostArch: "arm64" })).toBe("arm64"); | ||
| expect(resolveTargetArch({ hostArch: "x64" })).toBe("x64"); | ||
| }); | ||
|
|
||
| it("throws on an invalid explicit arch", () => { | ||
| expect(() => resolveTargetArch({ cliArch: "mips", hostArch: "x64" })).toThrow(/Invalid/); | ||
| }); | ||
|
|
||
| it("throws on an unsupported host with no explicit arch", () => { | ||
| expect(() => resolveTargetArch({ hostArch: "ppc64" })).toThrow(/host/i); | ||
| }); | ||
| }); | ||
|
|
||
| describe("resolveVcvarsArch", () => { | ||
| it("returns native tokens when host equals target", () => { | ||
| expect(resolveVcvarsArch("x64", "x64")).toBe("x64"); | ||
| expect(resolveVcvarsArch("arm64", "arm64")).toBe("arm64"); | ||
| }); | ||
|
|
||
| it("returns cross tokens as <host>_<target>", () => { | ||
| expect(resolveVcvarsArch("x64", "arm64")).toBe("x64_arm64"); | ||
| expect(resolveVcvarsArch("arm64", "x64")).toBe("arm64_x64"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("winBinDirName", () => { | ||
| it("builds the packaged bin folder name", () => { | ||
| expect(winBinDirName("x64")).toBe("win32-x64"); | ||
| expect(winBinDirName("arm64")).toBe("win32-arm64"); | ||
| }); | ||
| }); | ||
|
|
||
| it("exposes the supported arch list", () => { | ||
| expect(SUPPORTED_ARCHES).toEqual(["x64", "arm64"]); | ||
| }); |
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.
Dispatch input
arch:is now misleading. Thisarch: [x64, arm64]matrix here is the right shape. But the dispatch inputarch:(around line 9, not in this diff) is consumed only by the macOS matrix expression at line 103. The Windows matrix is hardcoded toarch: [x64, arm64]regardless. So:arch=arm64will only build the macOS arm64 leg, leaving Windows builds untouched.Either rename to
macos_arch:and document the scope, or wire this input into the Windows matrix too (matching the macOS shape). Recommend the former — Windows builds are better off always running both arches on tag pushes.