From 2d206a3a42429465f6bb99c9425e3f7316273782 Mon Sep 17 00:00:00 2001 From: emozilla Date: Sun, 28 Jun 2026 01:07:54 -0400 Subject: [PATCH] fix(desktop): stop hermes desktop from clobbering tracked main.cjs (#52735) `npm run build` ended with `bundle-electron-main.mjs`, which esbuild-bundled electron/main.cjs and renamed the bundle on top of the tracked source file. Because every `hermes desktop` runs `npm run build`, each launch rewrote a checked-in source file (~7.5k-line source -> ~14.8k-line bundle), dirtying the working tree with a build artifact that `git restore` couldn't keep (the next launch re-clobbered it) and forcing autostash/restore conflicts on update. The bundle only existed to inline `simple-git` so the packaged app.asar (which ships no node_modules) wouldn't crash at launch with "Cannot find module 'simple-git'". Replace it with the mechanism the repo already uses for the other hoisted runtime dep (node-pty): stage the dependency closure and resolve it from process.resourcesPath at runtime. - stage-native-deps.cjs: resolve simple-git's runtime closure (walking dependencies + optionalDependencies, so a version bump that adds a transitive dep can't silently reintroduce the crash) and stage it under build/native-deps/vendor/node_modules/. The `vendor/` nesting is load-bearing: electron-builder drops a node_modules dir at the ROOT of an extraResources copy but keeps a nested one. - git-review-ops.cjs: fall back to the staged native-deps/vendor/node_modules/simple-git when the hoisted require() fails; dev runs resolve the hoisted copy and never hit the fallback. - package.json: drop the bundler from the `build` script so main.cjs is never a build target again. - nix/desktop.nix: drop the direct bundler call (the closure rides the existing `cp -rn native-deps` into $out) and patch process.resourcesPath in git-review-ops.cjs alongside main.cjs. - delete scripts/bundle-electron-main.mjs. Verified: electron-builder's own file filter keeps the full staged closure (0 dropped), and a packaged win-unpacked build launches with the git-review pane resolving simple-git from the staged vendor path. --- apps/desktop/electron/git-review-ops.cjs | 21 ++- apps/desktop/package.json | 2 +- apps/desktop/scripts/bundle-electron-main.mjs | 33 ----- apps/desktop/scripts/stage-native-deps.cjs | 124 ++++++++++++++++++ nix/desktop.nix | 23 +++- 5 files changed, 161 insertions(+), 42 deletions(-) delete mode 100644 apps/desktop/scripts/bundle-electron-main.mjs diff --git a/apps/desktop/electron/git-review-ops.cjs b/apps/desktop/electron/git-review-ops.cjs index 28f5fc7f955..a7df19880eb 100644 --- a/apps/desktop/electron/git-review-ops.cjs +++ b/apps/desktop/electron/git-review-ops.cjs @@ -10,7 +10,26 @@ const { execFile } = require('node:child_process') const fs = require('node:fs/promises') const path = require('node:path') -const simpleGit = require('simple-git') +// `simple-git` is a pure-JS runtime dep that workspace dedup hoists into the +// repo-root node_modules. Packaged builds set `files:` in package.json, which +// excludes node_modules from the asar, so the normal require() fails at launch +// (issue #52735: "Cannot find module 'simple-git'"). We ship the dep's +// closure under resources/native-deps/vendor/node_modules/ via extraResources +// + scripts/stage-native-deps.cjs, and resolve from there when the hoisted +// require() isn't reachable. The `vendor/` nesting matters: electron-builder +// drops a node_modules dir at the root of an extraResources copy but keeps a +// nested one. Dev mode never hits the fallback -- Node's normal lookup finds +// the hoisted copy. +let simpleGit +try { + simpleGit = require('simple-git') +} catch { + const resourcesPath = process.resourcesPath + if (!resourcesPath) { + throw new Error("git-review IPC: 'simple-git' not found and no resourcesPath to fall back to") + } + simpleGit = require(path.join(resourcesPath, 'native-deps', 'vendor', 'node_modules', 'simple-git')) +} const { resolveRequestedPathForIpc } = require('./hardening.cjs') diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 6d0ca01f33f..6ec73647b7b 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -18,7 +18,7 @@ "profile:main": "wait-on http://127.0.0.1:5174 && cross-env XCURSOR_SIZE=24 HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron --inspect=9229 .", "profile:main:cpu": "wait-on http://127.0.0.1:5174 && cross-env XCURSOR_SIZE=24 NODE_OPTIONS=--cpu-prof HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron .", "start": "npm run build && electron .", - "build": "node scripts/assert-root-install.cjs && node scripts/write-build-stamp.cjs && node scripts/stage-native-deps.cjs && tsc -b && vite build && node scripts/bundle-electron-main.mjs && npm run postbuild", + "build": "node scripts/assert-root-install.cjs && node scripts/write-build-stamp.cjs && node scripts/stage-native-deps.cjs && tsc -b && vite build && npm run postbuild", "postbuild": "node scripts/assert-dist-built.cjs", "prebuilder": "node scripts/patch-electron-builder-mac-binary.cjs", "builder": "cross-env NODE_OPTIONS=--max-old-space-size=16384 node scripts/run-electron-builder.cjs", diff --git a/apps/desktop/scripts/bundle-electron-main.mjs b/apps/desktop/scripts/bundle-electron-main.mjs deleted file mode 100644 index bb5b0ad061b..00000000000 --- a/apps/desktop/scripts/bundle-electron-main.mjs +++ /dev/null @@ -1,33 +0,0 @@ -#!/usr/bin/env node -// bundle-electron-main.mjs — bundles electron/main.cjs into a single -// self-contained file so the nix build doesn't need to ship node_modules/. -// -// `electron` is provided by the runtime; `node-pty` is staged separately -// via stage-native-deps.cjs. `preload.cjs` is NOT require()'d by main — -// Electron loads it via path.join(__dirname, 'preload.cjs') — so it stays -// as a separate file and doesn't need bundling. -import { build } from 'esbuild' -import { resolve, dirname } from 'node:path' -import { fileURLToPath } from 'node:url' -import { renameSync } from 'node:fs' - -const here = dirname(fileURLToPath(import.meta.url)) -const root = resolve(here, '..') -const entry = resolve(root, 'electron/main.cjs') -const tmp = resolve(root, 'electron/main.bundled.cjs') - -await build({ - entryPoints: [entry], - bundle: true, - platform: 'node', - format: 'cjs', - target: 'node20', - outfile: tmp, - external: ['electron', 'node-pty'], - logLevel: 'info' -}) - -// Overwrite the original with the bundled version. -renameSync(tmp, entry) - -console.log(`bundled ${entry}`) diff --git a/apps/desktop/scripts/stage-native-deps.cjs b/apps/desktop/scripts/stage-native-deps.cjs index d84ae2cf51f..ef68368dee7 100644 --- a/apps/desktop/scripts/stage-native-deps.cjs +++ b/apps/desktop/scripts/stage-native-deps.cjs @@ -66,6 +66,31 @@ const NATIVE_DEPS = [ } ] +// Pure-JS runtime dependencies that the packaged electron main require()s but +// that workspace dedup hoists into the repo-root node_modules -- out of reach +// of electron-builder's file collector, exactly like node-pty above. Unlike +// node-pty there is no native binary to select; we stage each package's whole +// directory into build/native-deps/vendor/node_modules/ so the dep's own +// internal require()s resolve against a real node_modules tree, and the +// requiring file (electron/git-review-ops.cjs) falls back to that path via +// process.resourcesPath when the normal require() fails. See issue #52735 +// (packaged app crashed at launch on `Cannot find module 'simple-git'`). +// +// The closure is resolved at stage time by walking dependencies + +// optionalDependencies, so a simple-git version bump that pulls in a new +// transitive dep can't silently re-introduce the crash. +// +// Layout note: the closure lands in build/native-deps/vendor/node_modules/, +// NOT build/native-deps/node_modules/. electron-builder's file collector +// hard-drops a `node_modules` directory that sits at the ROOT of an +// extraResources copy (app-builder-lib/out/util/filter.js: `if (relative === +// "node_modules") return false`), but keeps a NESTED one. Nesting under +// `vendor/` makes node_modules a subdirectory so it survives packing; the +// require() fallback in git-review-ops.cjs resolves the matching +// vendor/node_modules path. +const JS_DEP_ROOTS = ['simple-git'] +const JS_DEP_STAGE_ROOT = path.join(STAGE_ROOT, 'vendor', 'node_modules') + function rmrf(target) { fs.rmSync(target, { recursive: true, force: true }) } @@ -148,12 +173,111 @@ function stageOne(spec) { console.log(`[stage-native-deps] ${path.relative(APP_ROOT, spec.to)}: ${copied} files`) } +// Resolve a package's directory by name, searching the repo-root node_modules +// first (where workspace dedup hoists everything) and then the requiring +// package's own node_modules for any non-hoisted nested copy. +// +// We deliberately do NOT use require.resolve(`${name}/package.json`): packages +// with an "exports" map that doesn't list "./package.json" (e.g. simple-git +// 3.x) make that subpath unresolvable under Node's exports enforcement +// (ERR_PACKAGE_PATH_NOT_EXPORTED), which fails on CI even though it happened to +// work locally. Instead resolve the package's main entry (exports-aware) and +// walk up to the directory whose package.json's "name" matches. +function resolvePkgDir(name, fromDir) { + const searchPaths = [fromDir, REPO_ROOT, path.join(REPO_ROOT, 'node_modules')] + let entry + try { + entry = require.resolve(name, { paths: searchPaths }) + } catch { + return null + } + // Walk up from the resolved entry file to the package root: the first + // ancestor dir whose package.json declares this package's name. + let dir = path.dirname(entry) + while (true) { + const pjPath = path.join(dir, 'package.json') + try { + const pj = JSON.parse(fs.readFileSync(pjPath, 'utf8')) + if (pj.name === name) { + return dir + } + } catch { + // no package.json here (or unreadable) — keep walking up + } + const parent = path.dirname(dir) + if (parent === dir) { + return null + } + dir = parent + } +} + +// Walk dependencies + optionalDependencies from each root package and return +// the set of resolved package directories in the runtime closure. Keyed by +// package name so a dep reached via two paths is staged once. +function resolveJsClosure(roots) { + const closure = new Map() // name -> absolute package dir + const stack = roots.map(name => ({ name, fromDir: REPO_ROOT })) + while (stack.length) { + const { name, fromDir } = stack.pop() + if (closure.has(name)) continue + const dir = resolvePkgDir(name, fromDir) + if (!dir) { + throw new Error( + `stage-native-deps: could not resolve '${name}' for the simple-git ` + + `closure. Run \`npm install\` at the workspace root first.` + ) + } + closure.set(name, dir) + let pj + try { + pj = JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf8')) + } catch { + continue + } + const deps = { ...(pj.dependencies || {}), ...(pj.optionalDependencies || {}) } + for (const depName of Object.keys(deps)) { + stack.push({ name: depName, fromDir: dir }) + } + } + return closure +} + +// Stage the resolved JS dependency closure into build/native-deps/vendor/node_modules/ +// so the packaged app (and the nix output) can require() it from +// process.resourcesPath when the hoisted-root require() isn't reachable. Each +// package is copied whole (minus node_modules/ — the closure is flattened so +// every dep already has its own top-level entry) into a real node_modules +// layout, which keeps the deps' own internal require()s working unchanged. +function stageJsClosure(roots) { + const closure = resolveJsClosure(roots) + rmrf(JS_DEP_STAGE_ROOT) + ensureDir(JS_DEP_STAGE_ROOT) + let staged = 0 + for (const [name, fromDir] of closure) { + const dest = path.join(JS_DEP_STAGE_ROOT, name) + ensureDir(path.dirname(dest)) + // Copy the package directory but skip any nested node_modules/ — the + // closure is flattened, so nested copies would just bloat the bundle. + fs.cpSync(fromDir, dest, { + recursive: true, + filter: src => path.basename(src) !== 'node_modules' + }) + staged += 1 + } + console.log( + `[stage-native-deps] vendor/node_modules/: ${staged} package(s) ` + + `(${[...closure.keys()].sort().join(', ')})` + ) +} + function main() { rmrf(STAGE_ROOT) ensureDir(STAGE_ROOT) for (const spec of NATIVE_DEPS) { stageOne(spec) } + stageJsClosure(JS_DEP_ROOTS) } main() diff --git a/nix/desktop.nix b/nix/desktop.nix index 544895096c0..a912ef0745f 100644 --- a/nix/desktop.nix +++ b/nix/desktop.nix @@ -55,13 +55,15 @@ let npm exec tsc -b npm exec vite build - # Bundle the electron main into a single self-contained file so - # the nix output doesn't need node_modules/. simple-git (the only - # external runtime dep of the electron main) gets inlined; electron - # and node-pty are external (provided by the runtime / native-deps). - # preload.cjs stays separate — Electron loads it via __dirname, not - # require(), so it must remain a standalone file. - node scripts/bundle-electron-main.mjs + # simple-git is the electron main's external runtime dep. It is not + # bundled into main.cjs; instead the stage-native-deps.cjs call above + # copies its closure to apps/desktop/build/native-deps/vendor/node_modules/, + # which installPhase ships into $out/native-deps/ — the same path the + # packaged app uses. electron/git-review-ops.cjs resolves it from + # process.resourcesPath when the hoisted require() isn't reachable + # (see issue #52735). node-pty's prebuilt is staged the same way; + # electron is provided by the runtime. preload.cjs stays separate — + # Electron loads it via __dirname, not require(). popd runHook postBuild @@ -131,6 +133,13 @@ stdenv.mkDerivation { substituteInPlace $out/share/hermes-desktop/electron/main.cjs \ --replace-fail "process.resourcesPath" "'$out/share/hermes-desktop'" + # git-review-ops.cjs has the same process.resourcesPath fallback for its + # staged simple-git dep (native-deps/vendor/node_modules/), so it needs the same + # rewrite — otherwise the require() fallback resolves against the electron + # dist's resources path and fails to load simple-git (issue #52735). + substituteInPlace $out/share/hermes-desktop/electron/git-review-ops.cjs \ + --replace-fail "process.resourcesPath" "'$out/share/hermes-desktop'" + # Wrap the nixpkgs electron binary to launch our app. Set # HERMES_DESKTOP_HERMES to the absolute path of the nix-built `hermes` # binary so the desktop's resolver step 4 ("existing Hermes CLI on