fix(update): repair failed Node deps on an already-current checkout (#85539)
A failed npm install during `hermes update` prints "Fix npm and re-run `hermes update`" -- but re-running on a current checkout hit the "Already up to date!" early return before the Node refresh, so the repair advice could never work and node_modules stayed stale forever (#77211). The commit_count == 0 path now runs the Node refresh through _repair_node_deps_on_current_checkout. _update_node_dependencies self-gates on the lockfile hash, which is only recorded after a SUCCESSFUL npm install (and re-trips when node_modules is missing or the web toolchain never landed), so healthy installs pay one hash check and nothing else; a previously failed install actually repairs. A clean refresh pairs with the web build like every other call site; a failed one surfaces the fix-npm hint instead of "Already up to date!". Fixes #77211. Co-authored-by: RelaxJonh <RelaxJonh@users.noreply.github.com> Co-authored-by: JonthanaHanh <JonthanaHanh@users.noreply.github.com>
This commit is contained in:
parent
cd344a280f
commit
266b2b3611
|
|
@ -2102,6 +2102,34 @@ def _record_npm_lockfile_hash(hermes_root: Path) -> None:
|
|||
except OSError:
|
||||
logger.debug("Could not write npm lockfile hash cache")
|
||||
|
||||
def _repair_node_deps_on_current_checkout(print_completion) -> None:
|
||||
"""Repair Node deps on the ``commit_count == 0`` path (#77211).
|
||||
|
||||
A current checkout does not imply healthy Node deps: a previous npm
|
||||
install may have failed (EBADENGINE from a node/npm mismatch, network
|
||||
timeout, interrupted install) and its error message says to "re-run
|
||||
hermes update" — but the early return never reached the Node refresh,
|
||||
so that repair advice could never work. ``_update_node_dependencies``
|
||||
self-gates on the lockfile hash, which is only recorded after a
|
||||
SUCCESSFUL npm install (and re-trips when node_modules is missing or
|
||||
the web toolchain never landed), so this is a cheap no-op on healthy
|
||||
installs and a real repair after a failed one.
|
||||
"""
|
||||
node_failures = _update_node_dependencies()
|
||||
if node_failures:
|
||||
print(f" ⚠ Node.js refresh failed for: {', '.join(node_failures)}")
|
||||
print(" Fix npm and re-run `hermes update`.")
|
||||
print_completion(
|
||||
"⚠ Checkout is current, but Node.js dependencies could not be repaired."
|
||||
)
|
||||
return
|
||||
# Pair the refresh with the web build like every other
|
||||
# _update_node_dependencies call site; it staleness-checks internally,
|
||||
# so this is a no-op when nothing changed.
|
||||
_m()._build_web_ui(_m().PROJECT_ROOT / "web")
|
||||
print_completion("✓ Already up to date!")
|
||||
|
||||
|
||||
def _update_node_dependencies() -> list[str]:
|
||||
"""Refresh Node deps for the ui-tui and web workspaces.
|
||||
|
||||
|
|
@ -4228,7 +4256,7 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
|||
print(f"⚠ Venv still unhealthy after repair: {detail_after}")
|
||||
print(" Close all Hermes windows/gateways and re-run: hermes update")
|
||||
else:
|
||||
_print_update_completion("✓ Already up to date!")
|
||||
_repair_node_deps_on_current_checkout(_print_update_completion)
|
||||
if runtime_repaired is not None and not _m()._is_windows():
|
||||
print()
|
||||
print(
|
||||
|
|
|
|||
|
|
@ -0,0 +1,44 @@
|
|||
"""The commit_count == 0 path must repair Node deps, not just Python (#77211).
|
||||
|
||||
A previous ``hermes update`` whose npm install failed printed "Fix npm and
|
||||
re-run `hermes update`" — but re-running hit the "Already up to date!" early
|
||||
return before the Node refresh, so the advice could never work. The repair
|
||||
now runs through ``_repair_node_deps_on_current_checkout``, which delegates
|
||||
to ``_update_node_dependencies`` (self-gating on the lockfile hash, recorded
|
||||
only after a successful install, so healthy installs stay a cheap no-op).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from hermes_cli import update_cmd
|
||||
|
||||
|
||||
def test_current_checkout_repairs_failed_node_deps(capsys):
|
||||
"""A recorded failure surfaces the fix-npm hint, not 'Already up to date!'."""
|
||||
completion = MagicMock()
|
||||
with patch.object(
|
||||
update_cmd, "_update_node_dependencies", return_value=["ui-tui, web workspaces"]
|
||||
), patch.object(update_cmd, "_m") as m:
|
||||
update_cmd._repair_node_deps_on_current_checkout(completion)
|
||||
|
||||
m.return_value._build_web_ui.assert_not_called()
|
||||
completion.assert_called_once()
|
||||
assert "could not be repaired" in completion.call_args[0][0]
|
||||
out = capsys.readouterr().out
|
||||
assert "Node.js refresh failed for: ui-tui, web workspaces" in out
|
||||
assert "Fix npm and re-run `hermes update`." in out
|
||||
|
||||
|
||||
def test_current_checkout_healthy_node_deps_reports_up_to_date():
|
||||
"""A clean refresh (or lockfile-hash no-op) still says 'Already up to date!'."""
|
||||
completion = MagicMock()
|
||||
with patch.object(
|
||||
update_cmd, "_update_node_dependencies", return_value=[]
|
||||
), patch.object(update_cmd, "_m") as m:
|
||||
update_cmd._repair_node_deps_on_current_checkout(completion)
|
||||
|
||||
# The refresh pairs with the web build like every other call site.
|
||||
m.return_value._build_web_ui.assert_called_once()
|
||||
completion.assert_called_once_with("✓ Already up to date!")
|
||||
Loading…
Reference in New Issue