Comfy-Org / Comfy-Org/Comfy-Desktop
CI: compile the NSIS installer script to catch installer.nsh regressions
- Dominant language
- TypeScript
- Stars
- 458
- Forks
- 59
- Avg merge
- 22h 18m
- Merged PRs (30d)
- 45
Description
## Problem
No CI job compiles the NSIS installer. CI's `pnpm run build` is only `electron-vite build` (+ Playwright on the unpacked app); `makensis` runs **only** in the ToDesktop release pipeline. As a result, a broken `scripts/installer.nsh` passes all PR checks and only fails at release time.
This bit us in #1149 → #1190: a StrFunc `\` usage compiled fine locally (ANSI) and passed all CI checks, but broke the production ToDesktop build with:
```
warning 6010: install function "StrStr" not referenced - zeroing code (0-33) out
Error: warning treated as error
```
electron-builder compiles the script twice (the uninstaller pass does not emit the `customInstall` hook), and ToDesktop builds with Unicode + `-WX`, so any NSIS warning is fatal.
## Goal
Add a fast CI check (Windows runner) that compiles `installer.nsh` the way the release builder does — **Unicode + `-WX`**, covering both the installer and uninstaller passes — so installer-script regressions are caught on the PR.
## Approaches considered (decide in implementation)
- **A) Real build via `electron-builder --win`** — most faithful (compiles the actual generated `installer.nsi` through electron-builder's template). Downsides: needs `bootstrap-python/win-x64` fetched first (ToDesktop `beforeBuild` hook, not electron-builder), downloads Electron x64+arm64, runs afterPack, may need signing disabled; ~minutes.
- **B) Standalone harness `.nsi`** that `!include`s `scripts/installer.nsh` and runs `makensis -WX` in `Unicode true` in two variants:
- *uninstaller pass*: include the file but do **not** insert the hook macros → reproduces the exact #1190 6010 scenario. Cheap (~tens of ms), low-maintenance, needs only a stub `vc_redist_version.nsh`.
- *installer pass*: also `!insertmacro customHeader/customInstallMode/customInstall/customFinishPage` → catches macro-body errors, but requires stubbing electron-builder's environment (StdUtils + WinShell plugins, MUI finish-page, nsDialogs, vars like `\`/`\`, defines like `\`). Brittle; can drift from electron-builder's real env.
makensis itself is fast (~56 ms for a small script); the cost in the real pipeline is Electron download + signing + packaging, not makensis.
## Notes / gotchas
- Must use `Unicode true` and `-WX` to match ToDesktop; an ANSI compile without `-WX` does **not** reproduce the failure.
- `\` (FileFunc) is emitted lazily on use, so it does not trip 6010; StrFunc-style top-level function declarations do.
- The cheap harness (B/uninstaller-pass) precisely guards the regression class we actually hit; the installer-pass variant adds coverage at the cost of maintenance — weigh before committing to it.
Follow-up to #1190.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with scripts/installer.nsh and the ToDesktop release pipeline to compare how makensis is invoked, then review the existing pnpm run build checks. Add a Windows CI check that compiles the installer script with Unicode true and -WX, covering both installer and uninstaller passes. Done means the check catches NSIS warnings and script regressions on pull requests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- electron, github-actions
- Domain
- build-system, ci-cd
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100