From f8098c6b6fe5b764d1970c928884ff3fa1a04e2f Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 17 Jun 2026 16:08:01 -0700 Subject: [PATCH] fix(desktop): resolve electronDist to the actual electron install location (#48081) After the June lockfile regeneration (#46652) floated electron and reshuffled npm workspace hoisting, the desktop pack fails with "The specified electronDist does not exist". apps/desktop/package.json pointed electronDist at the repo root (../../node_modules/electron/dist) while npm now installs electron nested under apps/desktop/node_modules/electron. The two contradict, so a clean install can never package the app (Windows + macOS). - electronDist -> node_modules/electron/dist (resolved relative to apps/desktop, i.e. the workspace-local install npm actually produces). - hermes_cli/main.py, scripts/install.sh, scripts/install.ps1: add a runtime electron-dir resolver that prefers apps/desktop/node_modules/electron and falls back to the root hoist, so dist checks + the mirror re-download work under either npm layout. - patch-electron-builder-mac-binary.cjs: try the workspace-local Electron.app before the root hoist in the macOS binary-restore fallback (sibling site no PR touched). - test: assert build.electronDist resolves to where the lockfile installs electron, so a future hoist change (root <-> nested) can't silently break it. Salvages the overlapping work in #48003 (sitkarev), #48012 (omegazheng), and #48033 (james47kjv). Co-authored-by: sitkarev <59806492+sitkarev@users.noreply.github.com> Co-authored-by: omegazheng Co-authored-by: james47kjv <220877172+james47kjv@users.noreply.github.com> --- apps/desktop/package.json | 2 +- .../patch-electron-builder-mac-binary.cjs | 5 ++ hermes_cli/main.py | 39 ++++++++++---- scripts/install.ps1 | 47 ++++++++++------- scripts/install.sh | 51 ++++++++++++------- scripts/release.py | 3 ++ tests/hermes_cli/test_gui_command.py | 27 ++++++++++ tests/test_desktop_electron_pin.py | 39 ++++++++++++++ 8 files changed, 167 insertions(+), 46 deletions(-) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 5a3df77e91d..7935e2611fb 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -135,7 +135,7 @@ }, "build": { "electronVersion": "40.10.2", - "electronDist": "../../node_modules/electron/dist", + "electronDist": "node_modules/electron/dist", "appId": "com.nousresearch.hermes", "productName": "Hermes", "executableName": "Hermes", diff --git a/apps/desktop/scripts/patch-electron-builder-mac-binary.cjs b/apps/desktop/scripts/patch-electron-builder-mac-binary.cjs index 38315b9c65c..b88c281219f 100644 --- a/apps/desktop/scripts/patch-electron-builder-mac-binary.cjs +++ b/apps/desktop/scripts/patch-electron-builder-mac-binary.cjs @@ -24,6 +24,11 @@ const replacement = ` // ${marker}: electron-builder 26.8.x can sometimes cop if (!fs.existsSync(bundledElectronBinary)) { const candidates = [ path.join(packager.info.framework.distMacOsAppName, "Contents", "MacOS", electronBranding.productName), + // npm may nest the workspace-only electron devDep under + // apps/desktop/node_modules (process.cwd() during pack), or hoist + // it to the repo root. Try the workspace-local install first, then + // the root hoist, so the fallback works under either layout. + path.join(process.cwd(), "node_modules", "electron", "dist", "Electron.app", "Contents", "MacOS", electronBranding.productName), path.join(process.cwd(), "..", "..", "node_modules", "electron", "dist", "Electron.app", "Contents", "MacOS", electronBranding.productName), ]; const sourceBinary = candidates.find(candidate => fs.existsSync(candidate)); diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 1e5ef8fc3ea..e9f77945f1e 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5110,16 +5110,33 @@ def _purge_electron_build_cache(desktop_dir: Path) -> list[Path]: return removed -def _electron_dist_binary(project_root: Path) -> Path: - """Return the path to the Electron main binary inside ``node_modules``. +def _electron_dir(project_root: Path) -> Path: + """Return the Electron package directory the desktop workspace installs. - electron-builder reads the binary from ``build.electronDist`` - (``node_modules/electron/dist``) since #38673, so this is the exact file - whose absence makes a pack fail with "The specified electronDist does not - exist". The basename differs per OS (the platform Electron is named for the - host the build runs on). + npm may keep workspace-only dev dependencies under + ``apps/desktop/node_modules`` instead of hoisting them to the repo root. + Which layout you get depends on the npm version and what else is installed, + so a build path that assumes one or the other breaks intermittently across + machines. ``apps/desktop/package.json`` points electron-builder's + ``electronDist`` at ``node_modules/electron/dist`` relative to the desktop + project, so prefer the workspace-local package and fall back to the root + hoist when that's where npm landed it. """ - dist = project_root / "node_modules" / "electron" / "dist" + desktop_local = project_root / "apps" / "desktop" / "node_modules" / "electron" + if desktop_local.exists(): + return desktop_local + return project_root / "node_modules" / "electron" + + +def _electron_dist_binary(project_root: Path) -> Path: + """Return the path to the Electron main binary inside the installed package. + + electron-builder reads the binary from ``build.electronDist`` since #38673, + so this is the exact file whose absence makes a pack fail with "The + specified electronDist does not exist". The basename differs per OS (the + platform Electron is named for the host the build runs on). + """ + dist = _electron_dir(project_root) / "dist" if sys.platform == "darwin": return dist / "Electron.app" / "Contents" / "MacOS" / "Electron" if sys.platform == "win32": @@ -5169,7 +5186,7 @@ def _redownload_electron_dist( if _electron_dist_ok(project_root): return True - electron_dir = project_root / "node_modules" / "electron" + electron_dir = _electron_dir(project_root) installer = electron_dir / "install.js" if not installer.is_file(): return False @@ -5387,8 +5404,8 @@ def cmd_gui(args: argparse.Namespace): print(" Pre-build first: cd apps/desktop && npm run build") print(" Or drop --skip-build to install dependencies and build automatically.") sys.exit(1) - if not (PROJECT_ROOT / "node_modules" / "electron" / "package.json").exists(): - print("✗ --skip-build --source requires existing workspace dependencies.") + if not (_electron_dir(PROJECT_ROOT) / "package.json").exists(): + print("✗ --skip-build --source requires existing desktop workspace dependencies.") print(f" Install first: cd {PROJECT_ROOT} && npm ci") print(" Or drop --skip-build to install dependencies and build automatically.") sys.exit(1) diff --git a/scripts/install.ps1 b/scripts/install.ps1 index b4ee5796bad..0f04f2826cc 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -2161,28 +2161,41 @@ function Clear-ElectronBuildCache { return $removed } -# True when node_modules\electron\dist holds a usable Electron binary. -# electron-builder reads the binary from build.electronDist -# (node_modules\electron\dist) since #38673, so this is the exact file whose -# absence makes a pack fail with "The specified electronDist does not exist". A -# dist dir that exists but is missing electron.exe (partial extraction / aborted -# postinstall) is NOT ok. +# Return the Electron package directory the desktop workspace installs. npm may +# nest workspace-only dev dependencies under apps\desktop\node_modules instead +# of hoisting them to the repo root; which layout you get depends on the npm +# version and what else is installed. apps\desktop\package.json points +# electron-builder's electronDist there, so prefer the workspace-local package +# and fall back to the root hoist. +function Get-ElectronDir { + param([string]$InstallDir) + $desktopLocal = Join-Path $InstallDir 'apps\desktop\node_modules\electron' + if (Test-Path -LiteralPath $desktopLocal) { return $desktopLocal } + return (Join-Path $InstallDir 'node_modules\electron') +} + +# True when the desktop workspace electronDist holds a usable Electron binary. +# electron-builder reads the binary from build.electronDist since #38673, so +# this is the exact file whose absence makes a pack fail with "The specified +# electronDist does not exist". A dist dir that exists but is missing +# electron.exe (partial extraction / aborted postinstall) is NOT ok. function Test-ElectronDist { param([string]$InstallDir) - $distExe = Join-Path $InstallDir 'node_modules\electron\dist\electron.exe' + $electronDir = Get-ElectronDir -InstallDir $InstallDir + $distExe = Join-Path $electronDir 'dist\electron.exe' return (Test-Path -LiteralPath $distExe) } -# (Re)populate node_modules\electron\dist via electron's own downloader. +# (Re)populate the desktop Electron dist via electron's own downloader. # -# Since #38673 the desktop build pins build.electronDist to -# node_modules\electron\dist, so electron-builder reads the Electron binary -# straight from there and never downloads it during `npm run pack`. That dist -# tree is produced by the electron package's postinstall (install.js) during -# `npm ci`. When that download is blocked/throttled (GitHub's release host is -# unreachable in some regions - #47266), dist is missing and re-running pack only -# re-throws "The specified electronDist does not exist". The mirror fallback -# therefore has to drive THIS downloader, not another pack. +# Since #38673 the desktop build pins build.electronDist, so electron-builder +# reads the Electron binary straight from there and never downloads it during +# `npm run pack`. That dist tree is produced by the electron package's +# postinstall (install.js) during `npm ci`. When that download is +# blocked/throttled (GitHub's release host is unreachable in some regions - +# #47266), dist is missing and re-running pack only re-throws "The specified +# electronDist does not exist". The mirror fallback therefore has to drive THIS +# downloader, not another pack. # # No-op (returns $true) when the dist binary is already present. Otherwise drops a # partial dist + version marker (electron's install.js short-circuits when @@ -2193,7 +2206,7 @@ function Restore-ElectronDist { param([string]$InstallDir, [string]$Mirror) if (Test-ElectronDist -InstallDir $InstallDir) { return $true } - $electronDir = Join-Path $InstallDir 'node_modules\electron' + $electronDir = Get-ElectronDir -InstallDir $InstallDir $distExe = Join-Path $electronDir 'dist\electron.exe' $installer = Join-Path $electronDir 'install.js' if (-not (Test-Path -LiteralPath $installer)) { return $false } diff --git a/scripts/install.sh b/scripts/install.sh index b3387fc9c02..0703d245144 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -2407,15 +2407,31 @@ _desktop_pack() { # failed, and we never override a user-pinned ELECTRON_MIRROR. DESKTOP_ELECTRON_FALLBACK_MIRROR="https://npmmirror.com/mirrors/electron/" -# True (returns 0) when node_modules/electron/dist holds a usable Electron -# binary. electron-builder reads the binary from build.electronDist -# (node_modules/electron/dist) since #38673, so this is the exact file whose -# absence makes a pack fail with "The specified electronDist does not exist". A -# dist dir that exists but is missing the binary (partial extraction / aborted -# postinstall) is NOT ok. $1 = the workspace root holding node_modules. +# Return the Electron package directory the desktop workspace installs. npm may +# nest workspace-only dev dependencies under apps/desktop/node_modules instead +# of hoisting them to the repo root; which layout you get depends on the npm +# version and what else is installed. apps/desktop/package.json points +# electron-builder's electronDist there, so prefer the workspace-local package +# and fall back to the root hoist. $1 = the workspace root holding node_modules. +_electron_dir() { + local install_dir="$1" + if [ -d "$install_dir/apps/desktop/node_modules/electron" ]; then + printf '%s\n' "$install_dir/apps/desktop/node_modules/electron" + else + printf '%s\n' "$install_dir/node_modules/electron" + fi +} + +# True (returns 0) when the desktop workspace electronDist holds a usable +# Electron binary. electron-builder reads the binary from build.electronDist +# since #38673, so this is the exact file whose absence makes a pack fail with +# "The specified electronDist does not exist". A dist dir that exists but is +# missing the binary (partial extraction / aborted postinstall) is NOT ok. +# $1 = the workspace root holding node_modules. _electron_dist_ok() { local install_dir="$1" - local electron_dir="$install_dir/node_modules/electron" + local electron_dir + electron_dir="$(_electron_dir "$install_dir")" if [ "$OS" = "macos" ]; then [ -e "$electron_dir/dist/Electron.app/Contents/MacOS/Electron" ] else @@ -2423,16 +2439,16 @@ _electron_dist_ok() { fi } -# (Re)populate node_modules/electron/dist via electron's own downloader. +# (Re)populate the desktop Electron dist via electron's own downloader. # -# Since #38673 the desktop build pins build.electronDist to -# node_modules/electron/dist, so electron-builder reads the Electron binary -# straight from there and never downloads it during `npm run pack`. That dist -# tree is produced by the electron package's postinstall (install.js) during -# `npm ci`. When that download is blocked/throttled (GitHub's release host is -# unreachable in some regions - #47266), dist is missing and re-running pack only -# re-throws "The specified electronDist does not exist". The mirror fallback -# therefore has to drive THIS downloader, not another pack. +# Since #38673 the desktop build pins build.electronDist, so electron-builder +# reads the Electron binary straight from there and never downloads it during +# `npm run pack`. That dist tree is produced by the electron package's +# postinstall (install.js) during `npm ci`. When that download is +# blocked/throttled (GitHub's release host is unreachable in some regions - +# #47266), dist is missing and re-running pack only re-throws "The specified +# electronDist does not exist". The mirror fallback therefore has to drive THIS +# downloader, not another pack. # # No-op (returns 0) when the dist binary is already present. Otherwise drops a # partial dist + version marker (electron's install.js short-circuits when @@ -2442,7 +2458,8 @@ _electron_dist_ok() { _restore_electron_dist() { local install_dir="$1" local mirror="${2:-}" - local electron_dir="$install_dir/node_modules/electron" + local electron_dir + electron_dir="$(_electron_dir "$install_dir")" _electron_dist_ok "$install_dir" && return 0 [ -f "$electron_dir/install.js" ] || return 1 diff --git a/scripts/release.py b/scripts/release.py index 94756753ca2..2a74b301507 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -45,6 +45,9 @@ ACP_REGISTRY_MANIFEST = REPO_ROOT / "acp_registry" / "agent.json" # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { + "59806492+sitkarev@users.noreply.github.com": "sitkarev", + "zheng@omegasys.eu": "omegazheng", + "220877172+james47kjv@users.noreply.github.com": "james47kjv", "yuhanglin@YuhangdeMac-mini.local": "1960697431", "despitemeguru@gmail.com": "definitelynotguru", "chaslui@outlook.com": "ChasLui", diff --git a/tests/hermes_cli/test_gui_command.py b/tests/hermes_cli/test_gui_command.py index 6bdd95a3551..9f7805e02ec 100644 --- a/tests/hermes_cli/test_gui_command.py +++ b/tests/hermes_cli/test_gui_command.py @@ -616,6 +616,33 @@ def test_electron_dist_ok_per_platform(tmp_path, monkeypatch, platform, rel): assert cli_main._electron_dist_ok(tmp_path) is True +def test_electron_dir_prefers_workspace_local_package(tmp_path): + """npm may nest electron under apps/desktop; resolve there over the root hoist.""" + root_electron = tmp_path / "node_modules" / "electron" + local_electron = tmp_path / "apps" / "desktop" / "node_modules" / "electron" + root_electron.mkdir(parents=True) + local_electron.mkdir(parents=True) + + assert cli_main._electron_dir(tmp_path) == local_electron + + +def test_electron_dir_falls_back_to_root_hoist(tmp_path): + """When npm hoists electron to the repo root, resolve there.""" + root_electron = tmp_path / "node_modules" / "electron" + root_electron.mkdir(parents=True) + + assert cli_main._electron_dir(tmp_path) == root_electron + + +def test_electron_dist_ok_finds_workspace_local_binary(tmp_path, monkeypatch): + """A nested apps/desktop electron with a valid binary counts as ok.""" + monkeypatch.setattr(cli_main.sys, "platform", "linux") + binp = tmp_path / "apps" / "desktop" / "node_modules" / "electron" / "dist" / "electron" + binp.parent.mkdir(parents=True) + binp.write_text("", encoding="utf-8") + assert cli_main._electron_dist_ok(tmp_path) is True + + def test_redownload_electron_dist_noop_when_present(tmp_path, monkeypatch): """Already-healthy dist → no download, so an unrelated build failure can't trigger a needless ~200 MB refetch.""" diff --git a/tests/test_desktop_electron_pin.py b/tests/test_desktop_electron_pin.py index e1a0d2e88e7..ee29dfd0861 100644 --- a/tests/test_desktop_electron_pin.py +++ b/tests/test_desktop_electron_pin.py @@ -94,3 +94,42 @@ def test_lockfile_resolves_the_pinned_electron(): f"but the pin is {spec!r}; run `npm install --package-lock-only` so " "`npm ci` stays consistent." ) + + +def test_electron_dist_matches_lockfile_install_location(): + """build.electronDist must point at where the lockfile installs Electron. + + electron-builder copies the unpacked Electron from ``build.electronDist`` + (resolved relative to ``apps/desktop``). npm workspace hoisting is not + deterministic across machines/npm versions: it may nest Electron under + ``apps/desktop/node_modules/electron`` or hoist it to the repo root. If + electronDist points at one location while the lockfile installs at the + other, packaging fails with ``The specified electronDist does not exist`` — + the "Building desktop app" failure reported after the June lockfile + regeneration floated Electron and reshuffled the hoist. Lock the two + together so a hoist change (root <-> nested) can't silently break the path + again. + """ + if not ROOT_LOCK.is_file(): + pytest.skip("root package-lock.json not present") + electron_dist = _desktop_pkg().get("build", {}).get("electronDist") + assert electron_dist, "build.electronDist is missing" + + lock = json.loads(ROOT_LOCK.read_text(encoding="utf-8")) + electron_paths = [ + path + for path in lock.get("packages", {}) + if path.endswith("node_modules/electron") + ] + assert electron_paths, "no electron entry found in package-lock.json" + + desktop_dir = REPO_ROOT / "apps" / "desktop" + # electronDist is resolved relative to the apps/desktop project dir. + configured = (desktop_dir / electron_dist).resolve() + # Where the lockfile actually places Electron's unpacked dist. + installed = {(REPO_ROOT / p / "dist").resolve() for p in electron_paths} + assert configured in installed, ( + f"build.electronDist={electron_dist!r} resolves to {configured}, but the " + f"lockfile installs Electron at {sorted(str(p) for p in installed)}. " + "electron-builder will fail with 'electronDist does not exist'." + )