diff --git a/contributors/emails/gercamjr.dev@gmail.com b/contributors/emails/gercamjr.dev@gmail.com new file mode 100644 index 000000000000..6ac577a3590c --- /dev/null +++ b/contributors/emails/gercamjr.dev@gmail.com @@ -0,0 +1,2 @@ +gercamjr +# PR #68945 salvage (update: recover the web UI build when npm leaves no tsc/vite) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 3f3d5680647d..f8ae45fdf2b0 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5226,6 +5226,62 @@ def _run_npm_install_deterministic( ) +def _npm_bin_exists(bin_dir: Path, name: str) -> bool: + """True when an npm bin shim for *name* exists (POSIX or Windows).""" + return any( + (bin_dir / candidate).exists() + for candidate in (name, f"{name}.cmd", f"{name}.ps1", f"{name}.exe") + ) + + +def _web_build_toolchain_ready(*roots: Path) -> bool: + """True when ``tsc`` and ``vite`` shims are reachable from any of *roots*. + + Callers must pass every root the build would search; checking only one + reports a healthy tree as broken. + """ + bin_dirs = [ + bin_dir + for bin_dir in (root / "node_modules" / ".bin" for root in roots) + if bin_dir.is_dir() + ] + return bool(bin_dirs) and all( + any(_npm_bin_exists(bin_dir, tool) for bin_dir in bin_dirs) + for tool in ("tsc", "vite") + ) + + +def _web_toolchain_roots(web_dir: Path) -> tuple[Path, ...]: + """Roots whose ``node_modules/.bin`` can satisfy the web build. + + ``npm run build`` prepends ``node_modules/.bin`` for the package and each + of its ancestors, so shims hoisted to the workspace root and shims nested + under a package that owns its lockfile (#42973) are equally valid. + """ + return (web_dir, web_dir.parent) + + +def _missing_web_build_tool(output: str) -> str | None: + """Return the build tool a failed ``npm run build`` could not resolve. + + Each shell words this differently: ``sh: 1: tsc: not found`` (dash), + ``vite: command not found`` (bash/zsh), and ``'tsc' is not recognized as + an internal or external command`` (cmd.exe). + """ + lowered = output.lower() + for tool in ("tsc", "vite"): + if any( + phrase in lowered + for phrase in ( + f"{tool}: not found", + f"{tool}: command not found", + f"'{tool}' is not recognized", + ) + ): + return tool + return None + + def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: """Build the web UI frontend if npm is available, serializing across processes. @@ -5333,12 +5389,16 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: npm_workspace_args: tuple[str, ...] = () if npm_cwd == web_dir else ("--workspace", "web") if _is_termux_startup_environment(): npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir) - r1 = _run_npm_install_deterministic( - npm, - npm_cwd, - extra_args=(*npm_workspace_args, "--silent"), - env=build_env, - ) + + def _install_web_deps(*, silent: bool) -> "subprocess.CompletedProcess": + return _run_npm_install_deterministic( + npm, + npm_cwd, + extra_args=(*npm_workspace_args, "--silent") if silent else npm_workspace_args, + env=build_env, + ) + + r1 = _install_web_deps(silent=True) if r1.returncode != 0: _say( f" {'✗' if fatal else '⚠'} Web UI npm install failed" @@ -5355,11 +5415,22 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: # recoverable (the stale-dist fallback below handles the kill path). r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: - # Retry once after a short delay — covers boot-time races on Windows - # (antivirus scanning Node.js binaries, npm cache not ready, transient - # I/O when launched via Scheduled Task at logon). See issue #23817. - _time.sleep(3) - r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + # The install above can exit 0 while leaving the tree without a build + # toolchain — a lockfile-hash skip over a half-installed tree, or an + # interrupted link step. The generic retry below just reruns the same + # command, so `tsc: not found` survives it and the stale dist is + # served forever. Reinstall (non-silent, so the user sees it) first. + missing_tool = _missing_web_build_tool((r2.stdout or "") + (r2.stderr or "")) + if missing_tool: + _say(f" ⚠ Build could not resolve {missing_tool} — reinstalling web dependencies...") + _install_web_deps(silent=False) + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + if r2.returncode != 0: + # Retry once after a short delay — covers boot-time races on Windows + # (antivirus scanning Node.js binaries, npm cache not ready, transient + # I/O when launched via Scheduled Task at logon). See issue #23817. + _time.sleep(3) + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: # _run_with_idle_timeout merges stderr into stdout; older callers @@ -10140,6 +10211,15 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True + # A matching lockfile hash over a tree whose web build toolchain never + # landed must NOT skip the reinstall — otherwise every later `hermes + # update` keeps rebuilding against a half-installed tree and serving a + # stale dist. + web_dir = PROJECT_ROOT / "web" + if (web_dir / "package.json").is_file() and not _web_build_toolchain_ready( + *_web_toolchain_roots(web_dir) + ): + return True try: # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12] diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index 5a1914bc76bb..d550a2297230 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -149,6 +149,41 @@ class TestCmdUpdateNpmLockfileCache: ) assert hm._npm_lockfile_changed(tmp_path) is True + def test_missing_web_build_toolchain_defeats_skip(self, tmp_path, monkeypatch): + """A hash recorded over a tree that never got tsc/vite must not skip. + + Otherwise the half-installed tree is permanent: every later update + trusts the hash, the build keeps failing, and the stale dist is served + forever. + """ + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text('{"workspaces": ["web"]}') + (tmp_path / "web").mkdir() + (tmp_path / "web" / "package.json").write_text("{}") + bin_dir = tmp_path / "node_modules" / ".bin" + bin_dir.mkdir(parents=True) + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is True + + (bin_dir / "tsc").touch() + (bin_dir / "vite").touch() + assert hm._npm_lockfile_changed(tmp_path) is False + + def test_toolchain_check_skipped_without_a_web_package(self, tmp_path, monkeypatch): + """Prebuilt bundles ship no web/ source — they must still skip.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is False + def test_workspace_package_json_edit_defeats_skip(self, tmp_path, monkeypatch): """The manifest list comes from the root package.json `workspaces` globs (npm's source of truth), so ANY workspace (desktop included) diff --git a/tests/hermes_cli/test_web_ui_build.py b/tests/hermes_cli/test_web_ui_build.py index 95c0960de41a..df273054d8e3 100644 --- a/tests/hermes_cli/test_web_ui_build.py +++ b/tests/hermes_cli/test_web_ui_build.py @@ -21,8 +21,11 @@ import pytest from hermes_cli.main import ( _web_ui_build_needed, _build_web_ui, - _run_npm_install_deterministic, _compute_web_ui_content_hash, + _missing_web_build_tool, + _run_npm_install_deterministic, + _web_build_toolchain_ready, + _web_toolchain_roots, _web_ui_stamp_path, _write_web_ui_build_stamp, ) @@ -563,3 +566,136 @@ class TestBuildWebUIFlock: def test_lock_file_is_gitignored(self): gitignore = Path(__file__).resolve().parents[2] / ".gitignore" assert ".web_ui_build.lock" in gitignore.read_text(encoding="utf-8") + + +def _link_shims(bin_dir: Path, *names: str) -> None: + bin_dir.mkdir(parents=True, exist_ok=True) + for name in names: + (bin_dir / name).touch() + + +class TestWebBuildToolchainReady: + """A tree is ready when the build can resolve tsc AND vite from any root. + + ``npm run build`` searches ``node_modules/.bin`` from the script's own + package up through every ancestor, so a shim in either place counts. + """ + + def test_missing_toolchain_is_not_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + + def test_partial_toolchain_is_not_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + + def test_hoisted_shims_at_workspace_root_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + def test_shims_nested_under_the_package_are_ready(self, tmp_path): + """#42973 layout: web/ owns its lockfile, so npm links shims there.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "node_modules").mkdir() + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + def test_shims_split_across_roots_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + _link_shims(web_dir / "node_modules" / ".bin", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + @pytest.mark.parametrize("shim", ["tsc.cmd", "tsc.ps1", "tsc.exe"]) + def test_windows_shim_extensions_count(self, tmp_path, shim): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", shim, "vite.cmd") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + +class TestWebToolchainRoots: + def test_searches_the_package_and_its_workspace_root(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + assert _web_toolchain_roots(web_dir) == (web_dir, tmp_path) + + +class TestMissingWebBuildTool: + """Every shell words an unresolvable binary differently.""" + + @pytest.mark.parametrize( + "output,expected", + [ + ("sh: 1: tsc: not found\nnpm error code 127", "tsc"), + ("bash: line 1: vite: command not found", "vite"), + ("'tsc' is not recognized as an internal or external command", "tsc"), + ("error TS2307: Cannot find module './x'", None), + ("", None), + ], + ) + def test_detects_the_unresolvable_tool(self, output, expected): + assert _missing_web_build_tool(output) == expected + + +class TestBuildRecoversFromMissingToolchain: + def test_reinstalls_and_retries_when_the_build_cannot_resolve_tsc(self, tmp_path): + """The generic retry reruns the same command, so it can't fix this alone.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_fail = __import__("subprocess").CompletedProcess( + [], 127, stdout="sh: 1: tsc: not found\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[build_fail, build_ok]) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 2 + assert mock_build.call_count == 2 + + def test_healthy_tree_builds_without_an_extra_install(self, tmp_path): + """No pre-build probing: a build that works is never second-guessed.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 + assert mock_build.call_count == 1 + + def test_unrelated_build_failure_takes_the_generic_retry_only(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + type_error = __import__("subprocess").CompletedProcess( + [], 2, stdout="src/app.tsx(3,1): error TS2307: Cannot find module\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[type_error, build_ok]), \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1