fix(desktop): set MakerDeb/MakerRpm bin to lightfast#652
fix(desktop): set MakerDeb/MakerRpm bin to lightfast#652jeevanpillay wants to merge 1 commit intomainfrom
Conversation
The Linux makers default `options.bin` to `pkg.name` (via electron-installer-common's getDefaultsFromPackageJSON). After the release workflow runs `npm version`, that value is `@lightfast/desktop` — the package's npm name, not the executable name. The actual binary on disk is `lightfast`, set via `packagerConfig.executableName`. Without an explicit override, both makers fail at the binary-symlink step: could not find the Electron app binary at ".../Lightfast-linux-x64/@lightfast/desktop" Set `options.bin: "lightfast"` on both MakerDeb and MakerRpm. Surfaced by the @lightfast/desktop@0.1.0-rc.5 dry-run; mac legs were unaffected.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
📝 WalkthroughWalkthroughExplicit ChangesLinux Binary Packaging
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/forge.config.ts (1)
164-164: ⚡ Quick win[warning] Single-source the executable name to avoid future drift.
"lightfast"is now repeated across packager and makers (Line 116, Line 164, Line 181). Extract a constant so future renames don’t silently reintroduce this class of release break.Proposed diff
+const EXECUTABLE_NAME = "lightfast"; + const config: ForgeConfig = { packagerConfig: { name: "Lightfast", - executableName: "lightfast", + executableName: EXECUTABLE_NAME, @@ // .../Lightfast-linux-<arch>/@lightfast/desktop". - bin: "lightfast", + bin: EXECUTABLE_NAME, @@ // shares the same `getDefaultsFromPackageJSON` helper. - bin: "lightfast", + bin: EXECUTABLE_NAME,Also applies to: 181-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/forge.config.ts` at line 164, The executable name "lightfast" is hardcoded in multiple places (e.g., the bin field and packager/makers) which can drift; introduce a single constant (e.g., APP_NAME or EXECUTABLE_NAME) at the top of this module and replace all literal occurrences (the bin: "lightfast" entry and the repeated names referenced around the packager and makers blocks) with that constant so renames only require one change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/forge.config.ts`:
- Line 164: The executable name "lightfast" is hardcoded in multiple places
(e.g., the bin field and packager/makers) which can drift; introduce a single
constant (e.g., APP_NAME or EXECUTABLE_NAME) at the top of this module and
replace all literal occurrences (the bin: "lightfast" entry and the repeated
names referenced around the packager and makers blocks) with that constant so
renames only require one change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 539332c0-59ae-4a76-9b5e-72f4a4d86e58
📒 Files selected for processing (1)
apps/desktop/forge.config.ts
Summary
Surfaced by the
@lightfast/desktop@0.1.0-rc.5dry-run after PR #650 merged: bothBuild Linux x64andBuild Linux arm64legs failed identically at the maker step:Root cause
electron-installer-common'sgetDefaultsFromPackageJSONdefaultsoptions.bintopkg.name. After the release workflow runsnpm version, that field is@lightfast/desktop(the npm package name) — not the executable name. The actual binary on disk islightfast, set viapackagerConfig.executableNameinforge.config.ts:114. The makers don't readexecutableName— they have their ownbinoption which I didn't set in PR #650.MakerRpmshares the same default viaelectron-installer-redhat, so both makers fail the same way.Fix
Set
options.bin: "lightfast"on bothMakerDebandMakerRpm(one line each + a comment explaining the override). No other changes.Why local builds missed this
pnpm make --platform=darwin --arch=arm64(the Phase 1 verification) only exercises mac makers. Local Linux make wasn't in the verification path; the rc dry-run was the integration test for "Linux release path produces installable artifacts" — pattern fromfeedback_release_pipeline_dryrun.md(rc.1 → rc.4 surfaced 7 bugs invisible to local builds; this is bug #1 of the rc.5 round).Test plan
pnpm --filter @lightfast/desktop typecheckpasses locallymacos-14+ubuntu-22.04) goes green on this PR@lightfast/desktop@0.1.0-rc.6and confirm:build_linuxarch legs go greenlightfast_<v>_amd64.deb,lightfast_<v>_arm64.deb,lightfast-<v>.x86_64.rpm,lightfast-<v>.aarch64.rpmfinalizejob runs (was skipped in rc.5).dmgattestation glob extension still works (PR feat(desktop): linux makers + ci matrix; also extend mac attestation to .dmg #650 bundled change)Notes for reviewer
The mac legs in rc.5 succeeded — confirms PR #650's
build→build_macosrename and the.dmgattestation extension don't regress. The only failure was the Linux makerbindefault, which this PR fixes.Summary by CodeRabbit