FEA-1462: Add Windows distribution to electron-app - #267
mikeangstadt wants to merge 2 commits into
Conversation
- add Windows NSIS x64/arm64 packaging config and installer options to electron-builder\n- allow the electron-builder wrapper to honor explicit platform flags and add dist:mac/dist:win/dist:all scripts\n- add a generated Windows .ico asset and bump the desktop version to 0.15.105\n\nTesting:\n- node --check apps/desktop/scripts/run-electron-builder.mjs\n- git diff --check\n\nRisks:\n- Windows packaging is config-only here and has not been exercised on a Windows host in this environment\n- dist output still shares the existing dist-dmg directory naming, which is harmless but mac-centric
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a690e52a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thadeusb
left a comment
thadeusb
left a comment
There was a problem hiding this comment.
Config's clean and it's correctly scoped to config-only like you said. Codex already nailed the real gap (release never builds win). One heads-up from me on actually building the target, inline.
- split desktop release workflow into shared prepare, macOS publish, Windows publish, and finalization jobs so Windows artifacts are actually attached to releases on a Windows runner\n- document that Windows packaging requires a Windows host or wine-capable cross-build environment\n\nTesting:\n- git diff --check\n\nRisks:\n- Windows release path now depends on the windows-latest runner environment and has not been exercised in this local sandbox
|
I believe it goes without saying, but I'll say it anyway - all the interaction that this app does with the local filesystem, not to mention many different CLI apps, is unlikely to work on Windows. We could support UI-only flows, but the app is not designed to work that way. I am doubtful anyone could even onboard on a Windows machine. |
| pnpm -C apps/desktop clean:package | ||
| pnpm -C apps/desktop build | ||
| pnpm -C apps/desktop stage:package | ||
| node apps/desktop/scripts/run-electron-builder.mjs --win --publish always |
There was a problem hiding this comment.
This runs the wrapper with plain node from the repo root, but the wrapper spawns electron-builder by bare command name. electron-builder is only a desktop devDependency, so the package bin is available through the apps/desktop pnpm script/exec PATH, not guaranteed on the root Actions PATH. On windows-latest this can fail before any Windows installer is published. Please invoke it through pnpm -C apps/desktop exec node scripts/run-electron-builder.mjs --win --publish always or a dedicated desktop package script so the desktop bins are on PATH.
| pnpm -C apps/desktop clean:package | ||
| pnpm -C apps/desktop build | ||
| pnpm -C apps/desktop stage:package | ||
| node apps/desktop/scripts/run-electron-builder.mjs --win --publish always |
There was a problem hiding this comment.
This adds a second --publish always publisher while the macOS job also publishes the same version in parallel. For a new version, both jobs can pass prepare_release's no-release check and then race to create/upload v<version>; one job can fail or leave a partial draft that reruns then skip as already released. Please create the draft release once before the platform jobs, serialize the publishing step, or have platform jobs upload artifacts and publish everything from a single final job.
| entitlementsInherit: build/entitlements.mac.plist | ||
| notarize: true | ||
|
|
||
| win: |
There was a problem hiding this comment.
This adds a Windows target that CI publishes, but there is no Windows signing configuration or fail-closed signing guard, and the Windows job only supplies GITHUB_TOKEN. electron-builder skips signing when signing material is absent, so the released NSIS installer/update can be unsigned. Since packaged builds already auto-download and install release updates, please either configure and verify Windows signing with forceCodeSigning before publishing, or gate Windows publishing until that is ready.
| arch: | ||
| - x64 | ||
| - arm64 | ||
| artifactName: ${productName}-Setup-${version}-${arch}.${ext} |
There was a problem hiding this comment.
With x64 and arm64 configured, NSIS keeps buildUniversalInstaller enabled by default; because this artifact name includes ${arch}, electron-builder builds the universal installer plus per-arch installers. That makes the Windows release contract more than the advertised x64/arm64 assets and leaves the intended updater/download target ambiguous. Please choose one contract explicitly, e.g. set buildUniversalInstaller: false for separate x64/arm64 installers or publish a single universal installer and update the naming/artifact patterns accordingly.
| notarize: true | ||
|
|
||
| win: | ||
| icon: build/icon.ico |
There was a problem hiding this comment.
build/icon.ico is now a packaging input, but the icon source-of-truth flow still only documents/regenerates the PNG/ICNS and tray assets from app-icon.svg. Future icon regeneration will leave the Windows installer icon stale unless this path is reproducible. Please add .ico generation to the icon script or document the exact generation step alongside the existing icon workflow.
|
PR traceability issue: I fetched ClosedLoop |
4 participants
Summary
x64andarm64dist:mac,dist:win, anddist:alldesktop scripts--mac.icoasset and bump Desktop to0.15.105Testing
node --check apps/desktop/scripts/run-electron-builder.mjsgit diff --checkRisks
dist-dmgdirectory name, which is mac-centric but functionally harmless