fix(desktop): resolve electronDist dynamically + self-heal blocked installs (supersedes #48081/#48082) (#48091)
* fix(desktop): resolve electronDist dynamically + self-heal blocked installs Supersedes the static-path approach (#48081) and the install-step self-heal (#48082) with a fix that removes the whole failure class instead of chasing each symptom. Three distinct faults converged into the June desktop-build outage; this closes all three. Root cause (the part #48081 left open — "Gap B"): build.electronDist was a static relative path in apps/desktop/package.json, but npm workspace hoisting is NOT deterministic — depending on the npm version and what else is installed, npm nests the workspace-only electron devDep under apps/desktop/node_modules/electron OR hoists it to the repo root. A static path matches only one layout, so a clean install intermittently fails with "The specified electronDist does not exist". #48081 re-pointed the path at the nested layout (correct today) but electron-builder reads electronDist STATICALLY, so any future hoist change silently breaks it again — only caught by a CI invariant, never self-corrected. Fix: - scripts/run-electron-builder.cjs: resolve electron the way Node's runtime does — require.resolve("electron/package.json") walks node_modules from the desktop project upward and finds electron wherever npm actually put it. The path can never drift out of sync with the install layout again, on any OS/npm version. * dist present -> pass -c.electronDist=<abs>/dist so electron-builder reuses the unpacked runtime (keeps the #38673 fast path that dodges the 26.8.x missing-binary re-unpack bug). * dist absent -> omit electronDist; electron-builder fetches Electron itself via @electron/get honoring electronVersion + ELECTRON_MIRROR. package.json: builder script now runs the wrapper; the static build.electronDist is removed (the resolver owns it). - main.py / install.sh / install.ps1: on a dependency-install failure where the electron package staged but its dist is missing (electron's install.js process.exit(1) on a blocked/throttled binary download — #47266/#47917/#48021), repopulate the dist via electron's downloader (canonical, then npmmirror.com) and CONTINUE to the build instead of aborting. npm runs postinstall LAST, so the only casualty is electron/dist; bailing here is what made the pack-time mirror self-heal unreachable on a blocked network. Hard-fail only when electron never staged at all (a genuine dependency error). - The pack-time mirror fallback now retries the build even when the pre-fetch can't populate the dist: the wrapper lets electron-builder download Electron itself via the mirror, so the retry is no longer a no-op (it was, when electronDist was a static path). The exact 40.10.2 pin (already on main) keeps the third mode — the native @electron-internal/extract-zip win32 binding that 40.10.3/40.10.4 ship without a published prebuild — from recurring. Tests: - test_desktop_electron_pin.py: replace the static-path-matches-lockfile invariant with contracts that there is no hardcoded electronDist to drift, the builder script routes through the resolver, and the resolver uses Node module resolution + injects -c.electronDist. - test_gui_command.py: install-failure self-heal continues to build; genuine (electron-never-staged) install failure still hard-fails; pack retries under the mirror even when the pre-fetch is blocked. Salvages/supersedes the overlapping community work in #48003 (sitkarev), #48012 (omegazheng), #48033 (james47kjv), and #48082. Co-authored-by: sitkarev <59806492+sitkarev@users.noreply.github.com> Co-authored-by: omegazheng <zheng@omegasys.eu> Co-authored-by: james47kjv <220877172+james47kjv@users.noreply.github.com> * fix(desktop): narrow Electron self-heal to real missing-dist failures Follow-up on #48091 to remove the remaining misdiagnosis risk from the installer/build fallback path (#46785 concern): only take the Electron repair/retry path when Electron's package files are staged and dist is actually missing/corrupt. - main.py: add _electron_pkg_staged_missing_dist() and use it to gate install failure recovery; fail fast for unrelated npm install errors. - main.py/install.sh/install.ps1: run cache purge + retry only when dist is missing; do not retry unrelated tsc/vite/build failures under an Electron-specific narrative. - install.sh/install.ps1: tighten install-stage self-heal guard to require both package.json + install.js and missing dist. - tests: add coverage that install failure hard-fails when Electron dist already exists, and update retry test to reflect the tightened recovery condition. Validation: - Python tests: 64 passed - install.sh-related tests included in the run - Real mac build on this machine: - npm ci at repo root: success - cd apps/desktop && npm run pack: success - electron-builder packaged darwin arm64 and used custom unpacked Electron dist * refactor(desktop): trim electron self-heal helpers and comments Deduplicate mirror-retry into _try_redownload_electron_dist / shell counterparts; shorten wrapper and install-script commentary without changing recovery semantics. --------- Co-authored-by: sitkarev <59806492+sitkarev@users.noreply.github.com> Co-authored-by: omegazheng <zheng@omegasys.eu> Co-authored-by: james47kjv <220877172+james47kjv@users.noreply.github.com>
This commit is contained in:
brooklyn!
co-authored by
sitkarev
omegazheng
james47kjv
sitkarev
omegazheng
james47kjv
parent
acc8916ac7
commit
c1f9eb0ec4
@@ -485,13 +485,15 @@ def test_gui_retries_pack_once_after_purging_build_cache(tmp_path, monkeypatch):
|
||||
patch("hermes_cli.main._desktop_linux_sandbox_fixup", return_value=True), \
|
||||
patch("hermes_cli.main._write_desktop_build_stamp"), \
|
||||
patch("hermes_cli.main._purge_electron_build_cache", return_value=[Path("/c/electron.zip")]) as mock_purge, \
|
||||
patch("hermes_cli.main._electron_dist_ok", return_value=False), \
|
||||
patch("hermes_cli.main._redownload_electron_dist", return_value=True), \
|
||||
patch("hermes_cli.main.subprocess.run", side_effect=[pack_fail, pack_ok, launch_ok]) as mock_run, \
|
||||
pytest.raises(SystemExit) as exc:
|
||||
cli_main.cmd_gui(_ns())
|
||||
|
||||
assert exc.value.code == 0
|
||||
mock_purge.assert_called_once()
|
||||
# pack(fail) → purge → pack(ok) → launch = 3 subprocess.run calls
|
||||
# pack(fail) → repair succeeds → pack(ok) → launch = 3 subprocess.run calls
|
||||
assert mock_run.call_count == 3
|
||||
assert mock_run.call_args_list[0].args[0] == ["/usr/bin/npm", "run", "pack"]
|
||||
assert mock_run.call_args_list[1].args[0] == ["/usr/bin/npm", "run", "pack"]
|
||||
@@ -535,10 +537,12 @@ def test_gui_redownloads_electron_via_mirror_then_repacks(tmp_path, monkeypatch,
|
||||
assert "Desktop GUI build failed" in capsys.readouterr().out
|
||||
|
||||
|
||||
def test_gui_skips_pack_when_electron_redownload_unrecoverable(tmp_path, monkeypatch, capsys):
|
||||
"""When the Electron binary can't be fetched at all (mirror also blocked),
|
||||
skip the pointless final pack — it would just re-throw the same missing
|
||||
electronDist — and fail with a clear message instead."""
|
||||
def test_gui_retries_pack_under_mirror_even_when_prefetch_blocked(tmp_path, monkeypatch, capsys):
|
||||
"""When electron's own downloader can't fetch the binary (even via the
|
||||
mirror), still retry pack under ELECTRON_MIRROR: the build resolves
|
||||
electronDist dynamically and lets electron-builder fetch Electron itself
|
||||
via @electron/get, which honors the mirror. That retry is no longer
|
||||
pointless (it was, back when electronDist was a static path)."""
|
||||
root = _make_desktop_tree(tmp_path)
|
||||
monkeypatch.setattr(cli_main, "PROJECT_ROOT", root)
|
||||
_make_packaged_executable(root, monkeypatch, platform="linux")
|
||||
@@ -553,17 +557,96 @@ def test_gui_skips_pack_when_electron_redownload_unrecoverable(tmp_path, monkeyp
|
||||
patch("hermes_cli.main._purge_electron_build_cache", return_value=[]), \
|
||||
patch("hermes_cli.main._electron_dist_ok", return_value=False), \
|
||||
patch("hermes_cli.main._redownload_electron_dist", return_value=False), \
|
||||
patch("hermes_cli.main.subprocess.run", side_effect=[pack_fail]) as mock_run, \
|
||||
patch("hermes_cli.main.subprocess.run", side_effect=[pack_fail, pack_fail]) as mock_run, \
|
||||
pytest.raises(SystemExit) as exc:
|
||||
cli_main.cmd_gui(_ns())
|
||||
|
||||
assert exc.value.code == 1
|
||||
# Only the initial pack ran; both retries were skipped because no binary
|
||||
# could be produced.
|
||||
assert mock_run.call_count == 1
|
||||
out = capsys.readouterr().out
|
||||
assert "Could not re-download Electron from the mirror" in out
|
||||
assert "Desktop GUI build failed" in out
|
||||
# Initial pack + mirror-driven pack = 2; the mirror retry runs even though
|
||||
# the pre-fetch failed, so electron-builder gets a shot at downloading.
|
||||
assert mock_run.call_count == 2
|
||||
assert "ELECTRON_MIRROR" not in (mock_run.call_args_list[0].kwargs.get("env") or {})
|
||||
assert mock_run.call_args_list[1].kwargs["env"]["ELECTRON_MIRROR"]
|
||||
assert "Desktop GUI build failed" in capsys.readouterr().out
|
||||
|
||||
|
||||
def test_gui_install_failure_self_heals_electron_and_continues(tmp_path, monkeypatch, capsys):
|
||||
"""npm ci failing on electron's blocked binary download must NOT abort the
|
||||
install: with the electron package staged, repopulate its dist and continue
|
||||
to the build instead of sys.exit-ing before pack ever runs (#47266/#48021)."""
|
||||
root = _make_desktop_tree(tmp_path)
|
||||
monkeypatch.setattr(cli_main, "PROJECT_ROOT", root)
|
||||
packaged_exe = _make_packaged_executable(root, monkeypatch, platform="linux")
|
||||
# electron package staged on disk (postinstall download was the casualty).
|
||||
(root / "apps" / "desktop" / "node_modules" / "electron").mkdir(parents=True)
|
||||
(root / "apps" / "desktop" / "node_modules" / "electron" / "package.json").write_text("{}", encoding="utf-8")
|
||||
(root / "apps" / "desktop" / "node_modules" / "electron" / "install.js").write_text("", encoding="utf-8")
|
||||
|
||||
install_fail = subprocess.CompletedProcess(["npm", "ci"], 1)
|
||||
pack_ok = subprocess.CompletedProcess(["npm", "run", "pack"], 0)
|
||||
launch_ok = subprocess.CompletedProcess([str(packaged_exe)], 0)
|
||||
|
||||
with patch("hermes_cli.main.shutil.which", return_value="/usr/bin/npm"), \
|
||||
patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_fail), \
|
||||
patch("hermes_cli.main._desktop_linux_sandbox_fixup", return_value=True), \
|
||||
patch("hermes_cli.main._write_desktop_build_stamp"), \
|
||||
patch("hermes_cli.main._electron_dist_ok", return_value=False), \
|
||||
patch("hermes_cli.main._try_redownload_electron_dist", return_value=True) as mock_dl, \
|
||||
patch("hermes_cli.main.subprocess.run", side_effect=[pack_ok, launch_ok]) as mock_run, \
|
||||
pytest.raises(SystemExit) as exc:
|
||||
cli_main.cmd_gui(_ns())
|
||||
|
||||
assert exc.value.code == 0
|
||||
mock_dl.assert_called() # tried to repopulate the dist
|
||||
# pack + launch ran — the install failure did NOT abort the build.
|
||||
assert mock_run.call_count == 2
|
||||
assert "repopulated" in capsys.readouterr().out.lower()
|
||||
|
||||
|
||||
def test_gui_install_failure_hard_fails_when_electron_not_staged(tmp_path, monkeypatch, capsys):
|
||||
"""A dependency-install failure where electron never even staged is a genuine
|
||||
error (not a blocked binary download) — hard-fail with guidance, don't try to
|
||||
self-heal a tree that isn't there."""
|
||||
root = _make_desktop_tree(tmp_path)
|
||||
monkeypatch.setattr(cli_main, "PROJECT_ROOT", root)
|
||||
_make_packaged_executable(root, monkeypatch, platform="linux")
|
||||
|
||||
install_fail = subprocess.CompletedProcess(["npm", "ci"], 1)
|
||||
|
||||
with patch("hermes_cli.main.shutil.which", return_value="/usr/bin/npm"), \
|
||||
patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_fail), \
|
||||
patch("hermes_cli.main.subprocess.run") as mock_run, \
|
||||
pytest.raises(SystemExit) as exc:
|
||||
cli_main.cmd_gui(_ns())
|
||||
|
||||
assert exc.value.code == 1
|
||||
mock_run.assert_not_called() # build never started
|
||||
assert "Desktop dependency install failed" in capsys.readouterr().out
|
||||
|
||||
|
||||
def test_gui_install_failure_hard_fails_when_electron_dist_exists(tmp_path, monkeypatch, capsys):
|
||||
"""If npm install fails but Electron dist is already present, don't classify
|
||||
it as the blocked-download shape; fail fast as a generic install error."""
|
||||
root = _make_desktop_tree(tmp_path)
|
||||
monkeypatch.setattr(cli_main, "PROJECT_ROOT", root)
|
||||
_make_packaged_executable(root, monkeypatch, platform="linux")
|
||||
electron_dir = root / "apps" / "desktop" / "node_modules" / "electron"
|
||||
electron_dir.mkdir(parents=True)
|
||||
(electron_dir / "package.json").write_text("{}", encoding="utf-8")
|
||||
(electron_dir / "install.js").write_text("", encoding="utf-8")
|
||||
|
||||
install_fail = subprocess.CompletedProcess(["npm", "ci"], 1)
|
||||
|
||||
with patch("hermes_cli.main.shutil.which", return_value="/usr/bin/npm"), \
|
||||
patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_fail), \
|
||||
patch("hermes_cli.main._electron_dist_ok", return_value=True), \
|
||||
patch("hermes_cli.main.subprocess.run") as mock_run, \
|
||||
pytest.raises(SystemExit) as exc:
|
||||
cli_main.cmd_gui(_ns())
|
||||
|
||||
assert exc.value.code == 1
|
||||
mock_run.assert_not_called()
|
||||
assert "Desktop dependency install failed" in capsys.readouterr().out
|
||||
|
||||
|
||||
def test_gui_does_not_override_user_electron_mirror(tmp_path, monkeypatch, capsys):
|
||||
|
||||
@@ -96,40 +96,40 @@ def test_lockfile_resolves_the_pinned_electron():
|
||||
)
|
||||
|
||||
|
||||
def test_electron_dist_matches_lockfile_install_location():
|
||||
"""build.electronDist must point at where the lockfile installs Electron.
|
||||
DESKTOP_DIR = REPO_ROOT / "apps" / "desktop"
|
||||
ELECTRON_BUILDER_WRAPPER = DESKTOP_DIR / "scripts" / "run-electron-builder.cjs"
|
||||
|
||||
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"
|
||||
def test_no_static_electron_dist_that_can_drift():
|
||||
"""build.electronDist must not be a static path — hoisting is non-deterministic."""
|
||||
assert "electronDist" not in _desktop_pkg().get("build", {}), (
|
||||
"build.electronDist is hardcoded again. npm hoisting is non-deterministic, "
|
||||
"so a static path silently breaks packaging when the layout changes. Let "
|
||||
"scripts/run-electron-builder.cjs resolve it dynamically instead."
|
||||
)
|
||||
|
||||
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'."
|
||||
|
||||
def test_builder_script_routes_through_dynamic_resolver():
|
||||
"""npm run builder must invoke run-electron-builder.cjs, not bare electron-builder."""
|
||||
builder = _desktop_pkg().get("scripts", {}).get("builder", "")
|
||||
assert "run-electron-builder.cjs" in builder, (
|
||||
f"the 'builder' script must run scripts/run-electron-builder.cjs, got "
|
||||
f"{builder!r}"
|
||||
)
|
||||
assert ELECTRON_BUILDER_WRAPPER.is_file(), (
|
||||
f"missing dynamic-resolver wrapper at {ELECTRON_BUILDER_WRAPPER}"
|
||||
)
|
||||
|
||||
|
||||
def test_resolver_uses_node_module_resolution():
|
||||
"""Wrapper must resolve electron via require.resolve and pass -c.electronDist."""
|
||||
src = ELECTRON_BUILDER_WRAPPER.read_text(encoding="utf-8")
|
||||
assert 'require.resolve("electron/package.json")' in src, (
|
||||
"run-electron-builder.cjs must resolve electron via "
|
||||
"require.resolve('electron/package.json') to stay hoist-proof."
|
||||
)
|
||||
# And it must hand the resolved dist to electron-builder as an override.
|
||||
assert "-c.electronDist=" in src, (
|
||||
"run-electron-builder.cjs must pass the resolved dist to electron-builder "
|
||||
"via -c.electronDist."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user