Comfy-Org / Comfy-Org/Comfy-Desktop

CI: compile the NSIS installer script to catch installer.nsh regressions

Open
#1,191 1 comment 1 reaction 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.