fix(install): a failed Node dependency install now fails the install instead of printing success (#85537)
* fix(install): fail when Node dependencies cannot install (#85297) The POSIX installer converted root and TUI npm failures into warnings, then printed a dependency-success message and reached the installation-complete banner with a zero exit status. This left consumers with no usable node_modules while reporting success. Treat both required npm installs as fatal: log an error, restore tracked lockfile churn, return status 1, and propagate the failure from the monolithic and node-deps stage callers. Successful installs, Termux and missing-Node skips, missing-manifest skips, and optional Playwright/Browser Use/Computer Use best-effort behavior remain unchanged. The fix is limited to the POSIX installer; the PowerShell installer is outside this issue's scope. Focused and adjacent installer tests passed (32), with bash syntax, py_compile, and diff checks clean. The broader installer family had 90 passes, one unrelated pre-existing failure, and two skips; the full suite was environment-limited by missing dependencies. CodeRabbit, iterative deep security/compatibility reviews, and final confidence security/compatibility reviews were clean against the final diff. Fixes #85297 * fix(install): require npm alongside node in check_node (#77003) A stray `node` symlink without a sibling `npm` (leftover from a node version manager) made check_node report "Node.js found"; every later npm install then failed and the desktop build died with an opaque "Node.js / npm unavailable". Node now only counts as found when npm resolves on the same PATH, with an explicit "stray node symlink?" branch that falls through to the Hermes-managed Node (which bundles npm). The overlapping success-log honesty half of the original PR is subsumed by the previous commit, which makes a failed npm install fatal rather than conditionally-logged; the behavioral tests there cover it, so this commit keeps only the check_node PATH-gate assertions. Fixes #77003. Co-authored-by: criptogus <criptogus@users.noreply.github.com> --------- Co-authored-by: Eugeniusz Gilewski <egilewski@egilewski.com> Co-authored-by: CriptoGus <128640021+criptogus@users.noreply.github.com> Co-authored-by: criptogus <criptogus@users.noreply.github.com>
This commit is contained in:
brooklyn!
GitHub
parent
d0bb377c96
commit
6a198f8a12
|
|
@ -835,8 +835,16 @@ check_node() {
|
|||
# enough for the desktop build AND an npm that can read our .npmrc. A
|
||||
# bad-band npm (see npm_supports_npmrc) fails `npm ci` outright, and the
|
||||
# managed Node we install instead bundles one that works.
|
||||
if command -v node &> /dev/null && node_satisfies_build "$(node --version)"; then
|
||||
if ! command -v npm &> /dev/null || npm_supports_npmrc "$(npm --version 2>/dev/null)"; then
|
||||
#
|
||||
# npm must actually be reachable, not just node: a stray `node` symlink
|
||||
# without a sibling npm (leftover from a node version manager) makes
|
||||
# `command -v node` succeed while every later `npm install` silently
|
||||
# fails and the desktop build dies with an opaque "Node.js / npm
|
||||
# unavailable" (#77003). Node only counts as found when npm resolves on
|
||||
# the same PATH.
|
||||
if command -v node &> /dev/null && command -v npm &> /dev/null \
|
||||
&& node_satisfies_build "$(node --version)"; then
|
||||
if npm_supports_npmrc "$(npm --version 2>/dev/null)"; then
|
||||
log_success "Node.js $(node --version) found"
|
||||
HAS_NODE=true
|
||||
return 0
|
||||
|
|
@ -848,14 +856,17 @@ check_node() {
|
|||
fi
|
||||
|
||||
# Prefer a Hermes-managed Node from a previous run over a too-old system one.
|
||||
if [ -x "$HERMES_HOME/node/bin/node" ] && node_satisfies_build "$("$HERMES_HOME/node/bin/node" --version)"; then
|
||||
if [ -x "$HERMES_HOME/node/bin/node" ] && [ -x "$HERMES_HOME/node/bin/npm" ] \
|
||||
&& node_satisfies_build "$("$HERMES_HOME/node/bin/node" --version)"; then
|
||||
export PATH="$HERMES_HOME/node/bin:$PATH"
|
||||
log_success "Node.js $("$HERMES_HOME/node/bin/node" --version) found (Hermes-managed)"
|
||||
HAS_NODE=true
|
||||
return 0
|
||||
fi
|
||||
|
||||
if command -v node &> /dev/null; then
|
||||
if command -v node &> /dev/null && ! command -v npm &> /dev/null; then
|
||||
log_warn "node found but npm is not on PATH (stray node symlink?) — installing Hermes-managed Node $NODE_VERSION LTS..."
|
||||
elif command -v node &> /dev/null; then
|
||||
log_warn "Node.js $(node --version) is too old (Hermes requires Node >=26) — installing Hermes-managed Node $NODE_VERSION..."
|
||||
elif [ "$DISTRO" = "termux" ]; then
|
||||
log_info "Node.js not found — installing Node.js via pkg..."
|
||||
|
|
@ -2294,9 +2305,14 @@ install_node_deps() {
|
|||
cd "$INSTALL_DIR"
|
||||
# Time-boxed: a stalled registry fetch would otherwise hang here with no
|
||||
# progress (same #39219 stall class as the desktop build below).
|
||||
run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent || {
|
||||
log_warn "npm install failed or timed out (browser tools may not work)"
|
||||
}
|
||||
# A failed npm install used to still print "✓ Node.js dependencies
|
||||
# installed", hiding the degradation from the user (#77003). Now it
|
||||
# fails the install outright instead of burying the warning (#85297).
|
||||
if ! run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent; then
|
||||
log_error "npm install failed or timed out; Node.js dependencies were not installed"
|
||||
restore_dirty_lockfiles "$INSTALL_DIR"
|
||||
return 1
|
||||
fi
|
||||
log_success "Node.js dependencies installed"
|
||||
|
||||
# Install Playwright browser + system dependencies.
|
||||
|
|
@ -2396,9 +2412,13 @@ install_node_deps() {
|
|||
log_info "Installing TUI dependencies..."
|
||||
cd "$INSTALL_DIR/ui-tui"
|
||||
# Time-boxed: a stalled registry fetch would otherwise hang here (#39219).
|
||||
run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent || {
|
||||
log_warn "TUI npm install failed or timed out (hermes --tui may not work)"
|
||||
}
|
||||
# Report success only on actual success, same as node-deps above
|
||||
# (#77003) — and fail the install outright (#85297).
|
||||
if ! run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent; then
|
||||
log_error "TUI npm install failed or timed out; TUI dependencies were not installed"
|
||||
restore_dirty_lockfiles "$INSTALL_DIR"
|
||||
return 1
|
||||
fi
|
||||
log_success "TUI dependencies installed"
|
||||
fi
|
||||
|
||||
|
|
@ -3292,7 +3312,7 @@ run_stage_body() {
|
|||
resolve_install_layout
|
||||
require_install_dir
|
||||
check_node
|
||||
install_node_deps
|
||||
install_node_deps || return
|
||||
install_uv
|
||||
install_browser_use_cli
|
||||
install_computer_use_driver
|
||||
|
|
@ -3410,7 +3430,7 @@ main() {
|
|||
clone_repo
|
||||
setup_venv
|
||||
install_deps
|
||||
install_node_deps
|
||||
install_node_deps || return
|
||||
install_browser_use_cli
|
||||
install_computer_use_driver
|
||||
setup_path
|
||||
|
|
|
|||
|
|
@ -0,0 +1,145 @@
|
|||
"""Behavioral coverage for required Node dependency installation."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
INSTALL_SH = REPO_ROOT / "scripts" / "install.sh"
|
||||
|
||||
|
||||
def _write_executable(path: Path, body: str) -> None:
|
||||
path.write_text(body, encoding="utf-8")
|
||||
path.chmod(0o755)
|
||||
|
||||
|
||||
def _run_node_deps_stage(
|
||||
tmp_path: Path,
|
||||
*,
|
||||
fail_directory: str | None,
|
||||
) -> tuple[subprocess.CompletedProcess[str], Path, list[str]]:
|
||||
install_dir = tmp_path / "install"
|
||||
tui_dir = install_dir / "ui-tui"
|
||||
bin_dir = tmp_path / "bin"
|
||||
hermes_home = tmp_path / "home"
|
||||
managed_bin = hermes_home / "bin"
|
||||
npm_calls = tmp_path / "npm-calls"
|
||||
|
||||
tui_dir.mkdir(parents=True)
|
||||
bin_dir.mkdir()
|
||||
managed_bin.mkdir(parents=True)
|
||||
(install_dir / "package.json").write_text(
|
||||
'{"name":"installer-regression-probe","private":true}\n',
|
||||
encoding="utf-8",
|
||||
)
|
||||
(tui_dir / "package.json").write_text(
|
||||
'{"name":"tui-regression-probe","private":true}\n',
|
||||
encoding="utf-8",
|
||||
)
|
||||
_write_executable(bin_dir / "node", "#!/bin/sh\necho v26.0.0\n")
|
||||
_write_executable(
|
||||
bin_dir / "npm",
|
||||
"""#!/bin/sh
|
||||
if [ "${1:-}" = "--version" ]; then
|
||||
echo 12.0.0
|
||||
exit 0
|
||||
fi
|
||||
printf '%s\\n' "$PWD" >> "$NPM_CALLS"
|
||||
if [ -n "${NPM_FAIL_DIRECTORY:-}" ] && [ "$PWD" = "$NPM_FAIL_DIRECTORY" ]; then
|
||||
echo "simulated npm lifecycle failure" >&2
|
||||
exit 37
|
||||
fi
|
||||
exit 0
|
||||
""",
|
||||
)
|
||||
_write_executable(managed_bin / "uv", "#!/bin/sh\necho 'uv probe'\n")
|
||||
|
||||
env = os.environ.copy()
|
||||
env.update(
|
||||
{
|
||||
"HERMES_HOME": str(hermes_home),
|
||||
"HERMES_INSTALL_DIR": str(install_dir),
|
||||
"NPM_CALLS": str(npm_calls),
|
||||
"NPM_FAIL_DIRECTORY": fail_directory or "",
|
||||
"PATH": f"{bin_dir}:{env['PATH']}",
|
||||
}
|
||||
)
|
||||
proc = subprocess.run(
|
||||
[
|
||||
"bash",
|
||||
str(INSTALL_SH),
|
||||
"--stage",
|
||||
"node-deps",
|
||||
"--json",
|
||||
"--skip-browser",
|
||||
"--skip-computer-use",
|
||||
],
|
||||
cwd=REPO_ROOT,
|
||||
env=env,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
)
|
||||
calls = npm_calls.read_text(encoding="utf-8").splitlines()
|
||||
return proc, install_dir, calls
|
||||
|
||||
|
||||
def _stage_result(proc: subprocess.CompletedProcess[str]) -> dict[str, object]:
|
||||
return json.loads(proc.stdout.splitlines()[-1])
|
||||
|
||||
|
||||
def test_root_node_dependency_failure_is_fatal(tmp_path: Path) -> None:
|
||||
install_dir = tmp_path / "install"
|
||||
proc, actual_install_dir, calls = _run_node_deps_stage(
|
||||
tmp_path,
|
||||
fail_directory=str(install_dir),
|
||||
)
|
||||
|
||||
assert actual_install_dir == install_dir
|
||||
assert proc.returncode != 0
|
||||
assert _stage_result(proc) == {
|
||||
"ok": False,
|
||||
"stage": "node-deps",
|
||||
"skipped": False,
|
||||
"reason": "exit code 1",
|
||||
}
|
||||
assert calls == [str(install_dir)]
|
||||
assert "Node.js dependencies installed" not in proc.stdout
|
||||
assert "TUI dependencies installed" not in proc.stdout
|
||||
assert not (install_dir / "node_modules").exists()
|
||||
|
||||
|
||||
def test_tui_node_dependency_failure_is_fatal(tmp_path: Path) -> None:
|
||||
install_dir = tmp_path / "install"
|
||||
tui_dir = install_dir / "ui-tui"
|
||||
proc, _, calls = _run_node_deps_stage(
|
||||
tmp_path,
|
||||
fail_directory=str(tui_dir),
|
||||
)
|
||||
|
||||
assert proc.returncode != 0
|
||||
assert _stage_result(proc)["ok"] is False
|
||||
assert calls == [str(install_dir), str(tui_dir)]
|
||||
assert "Node.js dependencies installed" in proc.stdout
|
||||
assert "TUI dependencies installed" not in proc.stdout
|
||||
|
||||
|
||||
def test_node_dependency_success_remains_successful(tmp_path: Path) -> None:
|
||||
proc, install_dir, calls = _run_node_deps_stage(
|
||||
tmp_path,
|
||||
fail_directory=None,
|
||||
)
|
||||
|
||||
assert proc.returncode == 0, proc.stderr
|
||||
assert _stage_result(proc) == {
|
||||
"ok": True,
|
||||
"stage": "node-deps",
|
||||
"skipped": False,
|
||||
}
|
||||
assert calls == [str(install_dir), str(install_dir / "ui-tui")]
|
||||
assert "Node.js dependencies installed" in proc.stdout
|
||||
assert "TUI dependencies installed" in proc.stdout
|
||||
|
|
@ -0,0 +1,42 @@
|
|||
"""Regression tests for install.sh Node/npm checks (#77003).
|
||||
|
||||
A stray `node` symlink without a sibling `npm` (leftover from a node
|
||||
version manager) made the installer report "✓ Node.js found" and then fail
|
||||
opaquely at the desktop stage. Node must only count as found when npm
|
||||
resolves on the same PATH, and npm install stages must not report success
|
||||
when the install actually failed.
|
||||
"""
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
INSTALL_SH = REPO_ROOT / "scripts" / "install.sh"
|
||||
|
||||
|
||||
def test_check_node_requires_npm_alongside_node() -> None:
|
||||
"""check_node must not report success when only `node` resolves.
|
||||
|
||||
Before the fix, `command -v node` succeeding was enough — a stray node
|
||||
symlink (no sibling npm) passed the check, every later `npm install`
|
||||
failed silently, and the desktop build died with an opaque
|
||||
"Node.js / npm unavailable" (#77003).
|
||||
"""
|
||||
text = INSTALL_SH.read_text()
|
||||
|
||||
# The system-toolchain branch now gates on BOTH node and npm.
|
||||
assert (
|
||||
"if command -v node &> /dev/null && command -v npm &> /dev/null \\" in text
|
||||
)
|
||||
# The "node found but npm missing" case has its own explicit branch that
|
||||
# falls through to installing the Hermes-managed Node (which bundles npm).
|
||||
assert "node found but npm is not on PATH (stray node symlink?)" in text
|
||||
|
||||
|
||||
def test_check_node_managed_requires_npm() -> None:
|
||||
"""The Hermes-managed Node fallback also requires its npm to exist."""
|
||||
text = INSTALL_SH.read_text()
|
||||
assert (
|
||||
'[ -x "$HERMES_HOME/node/bin/node" ] && [ -x "$HERMES_HOME/node/bin/npm" ] \\'
|
||||
in text
|
||||
)
|
||||
|
||||
Loading…
Reference in New Issue