diff --git a/docs/RUNBOOK-PhaseC.md b/docs/RUNBOOK-PhaseC.md index bdecf26..547b0dc 100644 --- a/docs/RUNBOOK-PhaseC.md +++ b/docs/RUNBOOK-PhaseC.md @@ -216,22 +216,24 @@ ci vm cleanup --max-age-hours 4 --lock-file /var/lib/ci/vm-start.lock Launches the **interactive** VMware Workstation GUI (not the headless vmrun path) so you can watch/poke a clone by hand. +**Required once per desktop session** — authorise `ci-runner` on your X server +(run as YOUR logged-in user, not ci-runner): ```bash -# authorise the service user on your desktop X session first (run as YOUR user): xhost +SI:localuser:ci-runner - -# then open the VMX as ci-runner, forcing the display: +``` +Then open the GUI as ci-runner, forcing the display: +```bash ci vmgui --vmx /var/lib/ci/build-vms/Clone_dbg-1_*/Clone_dbg-1_*.vmx --display :0 ci vmgui --vmx --display :0 --power-on # also power it on (-x) ci vmgui --vmx --display :0 --fullscreen # power on + fullscreen (-X) - -# bare GUI, no VM loaded (just open Workstation): -ci vmgui --display :0 +ci vmgui --display :0 # bare GUI, no VM loaded ``` -The GUI needs an X display; `sudo -u ci-runner` strips `$DISPLAY`, so pass -`--display :0` (or export `DISPLAY`). Other flags: `--new-window` (`-n`), -`--vmware-path` to override the binary. The command spawns the GUI detached and -returns immediately (prints the child PID). +**If nothing opens**: you skipped the `xhost` step. `vmgui` now pre-flights the +display (`xdpyinfo`) and fails with the exact `xhost` command instead of dying +silently; it also drops an unreadable inherited `XAUTHORITY` and logs the GUI's +output to `/tmp/vmgui-*.log`. The GUI needs an X display — `sudo -u ci-runner` +loses your cookie, so `xhost` + `--display :0` is the reliable combo. Other +flags: `--new-window` (`-n`), `--vmware-path`. Spawns detached, prints PID + log. --- diff --git a/src/ci_orchestrator/commands/vm.py b/src/ci_orchestrator/commands/vm.py index e952a66..79a5a98 100644 --- a/src/ci_orchestrator/commands/vm.py +++ b/src/ci_orchestrator/commands/vm.py @@ -15,6 +15,7 @@ from __future__ import annotations import os import shutil import subprocess +import tempfile import time from datetime import UTC, datetime, timedelta from pathlib import Path @@ -488,22 +489,49 @@ def vm_new( # ────────────────────────────────────────────────────────────────────────── open -def _launch_gui(argv: list[str], env: dict[str, str]) -> int: - """Spawn the VMware Workstation GUI detached. Returns the child PID. +def _x_display_reachable(env: dict[str, str]) -> tuple[bool, str]: + """Probe whether the X display in ``env`` is usable. + + Uses ``xdpyinfo`` when available (it exits non-zero and prints the reason + if the display cannot be opened). Returns ``(ok, detail)``; if the probe + tool is missing we optimistically return ``(True, "")`` so the launch is + still attempted. + """ + probe = shutil.which("xdpyinfo") + if probe is None: + return True, "" + try: + res = subprocess.run( # fixed argv, no shell + [probe], + env=env, + stdout=subprocess.DEVNULL, + stderr=subprocess.PIPE, + timeout=10, + check=False, + ) + except (OSError, subprocess.TimeoutExpired) as exc: + return False, str(exc) + if res.returncode == 0: + return True, "" + return False, (res.stderr or b"").decode(errors="replace").strip() + + +def _launch_gui(argv: list[str], env: dict[str, str], log_path: Path) -> subprocess.Popen[bytes]: + """Spawn the VMware Workstation GUI detached, logging stdio to ``log_path``. Isolated so tests can monkeypatch it without spawning a real process. The - GUI must outlive the CLI, so the child is started in its own session with - its stdio detached. + GUI must outlive the CLI, so the child runs in its own session; its output + goes to a log file (not /dev/null) so failures are diagnosable. """ - proc = subprocess.Popen( # argv is a fixed list, never a shell string + log_fh = log_path.open("wb") + return subprocess.Popen( # argv is a fixed list, never a shell string argv, env=env, stdin=subprocess.DEVNULL, - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, + stdout=log_fh, + stderr=subprocess.STDOUT, start_new_session=True, ) - return proc.pid @click.command("vmgui") @@ -575,14 +603,35 @@ def vmgui( env = dict(os.environ) if display: env["DISPLAY"] = display - if not env.get("DISPLAY"): + disp = env.get("DISPLAY", "") + if not disp: + raise click.ClickException( + "no X display set ($DISPLAY empty and --display omitted). Pass " + "--display :0 (and authorise this user on the desktop session: " + "run `xhost +SI:localuser:ci-runner` as the logged-in user)." + ) + + # An inherited XAUTHORITY pointing at another user's cookie (e.g. the + # desktop user's ~/.Xauthority) is unreadable by ci-runner and blocks the + # connection with "Authorization required". Drop it so the launch relies on + # host-based auth (xhost) instead. + xauth = env.get("XAUTHORITY") + if xauth and not os.access(xauth, os.R_OK): + env.pop("XAUTHORITY", None) click.echo( - "[vmgui] WARNING: no X display set ($DISPLAY empty and --display " - "omitted). The GUI will fail to start. Pass --display :0 and run " - "`xhost +SI:localuser:ci-runner` from a desktop session first.", + f"[vmgui] dropping unreadable XAUTHORITY ({xauth}); relying on xhost.", err=True, ) + # Pre-flight: fail loudly with the cause instead of silently spawning a + # GUI that immediately dies on "cannot open display". + ok, detail = _x_display_reachable(env) + if not ok: + raise click.ClickException( + f"cannot open X display {disp!r}: {detail or 'unknown error'}. " + "From the desktop session run: xhost +SI:localuser:ci-runner" + ) + argv = [gui] if new_window: argv.append("-n") @@ -599,12 +648,32 @@ def vmgui( err=True, ) + ts = datetime.now(tz=UTC).strftime("%Y%m%d-%H%M%S") + log_path = Path(tempfile.gettempdir()) / f"vmgui-{os.getpid()}-{ts}.log" + click.echo(f"[vmgui] launching VMware GUI: {' '.join(argv)}") try: - pid = _launch_gui(argv, env) + proc = _launch_gui(argv, env, log_path) except (OSError, ValueError) as exc: raise click.ClickException(f"failed to launch VMware GUI: {exc}") from exc - click.echo(f"[vmgui] VMware GUI started (pid {pid}); display={env.get('DISPLAY', '')}") + + # Brief grace: if the GUI dies right away, surface its log instead of + # claiming success. + time.sleep(1.5) + rc = proc.poll() + if rc is not None and rc != 0: + try: + tail = log_path.read_text(errors="replace").strip().splitlines()[-5:] + except OSError: + tail = [] + detail = "\n ".join(tail) if tail else "(no output captured)" + raise click.ClickException( + f"VMware GUI exited immediately (code {rc}). Log {log_path}:\n {detail}" + ) + + click.echo( + f"[vmgui] VMware GUI started (pid {proc.pid}); display={disp}; log={log_path}" + ) __all__ = ["cleanup_orphans", "vm", "vmgui"] diff --git a/tests/python/test_commands_vm.py b/tests/python/test_commands_vm.py index c820ea9..3164471 100644 --- a/tests/python/test_commands_vm.py +++ b/tests/python/test_commands_vm.py @@ -502,26 +502,44 @@ def test_vm_cleanup_backend_unavailable(monkeypatch: pytest.MonkeyPatch, tmp_pat # ── vmgui ─────────────────────────────────────────────────────────────────── -def _capture_launch(monkeypatch: pytest.MonkeyPatch) -> dict[str, Any]: - """Patch _launch_gui to record argv/env instead of spawning a process.""" +class _FakeProc: + """Minimal subprocess.Popen stand-in for vmgui tests.""" + + def __init__(self, pid: int = 4242, rc: int | None = None) -> None: + self.pid = pid + self._rc = rc + + def poll(self) -> int | None: + return self._rc + + +def _capture_launch( + monkeypatch: pytest.MonkeyPatch, *, rc: int | None = None +) -> dict[str, Any]: + """Patch _launch_gui + preflight + sleep so no real process is spawned.""" captured: dict[str, Any] = {} - def _fake(argv: list[str], env: dict[str, str]) -> int: + def _fake(argv: list[str], env: dict[str, str], log_path: Path) -> _FakeProc: captured["argv"] = argv captured["env"] = env - return 4242 + captured["log_path"] = log_path + if rc is not None and rc != 0: + log_path.write_text("cannot open display\n") + return _FakeProc(rc=rc) monkeypatch.setattr(vm_module, "_launch_gui", _fake) + monkeypatch.setattr(vm_module, "_x_display_reachable", lambda _env: (True, "")) + monkeypatch.setattr(vm_module.time, "sleep", lambda _s: None) return captured -def test_vm_open_missing_vmx_errors(tmp_path: Path) -> None: +def test_vmgui_missing_vmx_errors(tmp_path: Path) -> None: result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(tmp_path / "nope.vmx")]) assert result.exit_code != 0 assert "VMX file not found" in result.output -def test_vm_open_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_vmgui_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: vmx = tmp_path / "clone.vmx" vmx.write_text("config") cap = _capture_launch(monkeypatch) @@ -533,7 +551,7 @@ def test_vm_open_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> assert "pid 4242" in result.output -def test_vm_open_power_on_adds_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_vmgui_power_on_adds_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: vmx = tmp_path / "clone.vmx" vmx.write_text("config") cap = _capture_launch(monkeypatch) @@ -543,7 +561,7 @@ def test_vm_open_power_on_adds_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path assert "-x" in cap["argv"] -def test_vm_open_fullscreen_adds_capital_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_vmgui_fullscreen_adds_capital_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: vmx = tmp_path / "clone.vmx" vmx.write_text("config") cap = _capture_launch(monkeypatch) @@ -555,7 +573,7 @@ def test_vm_open_fullscreen_adds_capital_x(monkeypatch: pytest.MonkeyPatch, tmp_ assert "-X" in cap["argv"] and "-n" in cap["argv"] -def test_vm_open_display_option_overrides_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_vmgui_display_option_overrides_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: vmx = tmp_path / "clone.vmx" vmx.write_text("config") cap = _capture_launch(monkeypatch) @@ -566,42 +584,80 @@ def test_vm_open_display_option_overrides_env(monkeypatch: pytest.MonkeyPatch, t assert "display=:1" in result.output -def test_vm_open_warns_without_display(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_vmgui_errors_without_display(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: vmx = tmp_path / "clone.vmx" vmx.write_text("config") _capture_launch(monkeypatch) monkeypatch.delenv("DISPLAY", raising=False) result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)]) - assert result.exit_code == 0, result.output + assert result.exit_code != 0 assert "no X display" in result.output -def test_vm_open_launch_failure_is_reported(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_vmgui_preflight_failure_reports_xhost(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + vmx = tmp_path / "clone.vmx" + vmx.write_text("config") + _capture_launch(monkeypatch) + # Override the optimistic preflight with a failing probe. + monkeypatch.setattr( + vm_module, "_x_display_reachable", lambda _env: (False, "cannot open display :0") + ) + monkeypatch.setenv("DISPLAY", ":0") + result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)]) + assert result.exit_code != 0 + assert "xhost +SI:localuser:ci-runner" in result.output + + +def test_vmgui_drops_unreadable_xauthority(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + vmx = tmp_path / "clone.vmx" + vmx.write_text("config") + cap = _capture_launch(monkeypatch) + monkeypatch.setenv("DISPLAY", ":0") + monkeypatch.setenv("XAUTHORITY", str(tmp_path / "missing.Xauthority")) + result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)]) + assert result.exit_code == 0, result.output + assert "XAUTHORITY" not in cap["env"] + assert "dropping unreadable XAUTHORITY" in result.output + + +def test_vmgui_immediate_exit_surfaces_log(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + vmx = tmp_path / "clone.vmx" + vmx.write_text("config") + _capture_launch(monkeypatch, rc=1) + monkeypatch.setenv("DISPLAY", ":0") + result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)]) + assert result.exit_code != 0 + assert "exited immediately" in result.output + assert "cannot open display" in result.output + + +def test_vmgui_launch_oserror_is_reported(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: vmx = tmp_path / "clone.vmx" vmx.write_text("config") - def _boom(argv: list[str], env: dict[str, str]) -> int: + def _boom(argv: list[str], env: dict[str, str], log_path: Path) -> Any: raise OSError("vmware not found") monkeypatch.setattr(vm_module, "_launch_gui", _boom) + monkeypatch.setattr(vm_module, "_x_display_reachable", lambda _env: (True, "")) + monkeypatch.setattr(vm_module.time, "sleep", lambda _s: None) monkeypatch.setenv("DISPLAY", ":0") result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)]) assert result.exit_code != 0 assert "failed to launch VMware GUI" in result.output -def test_vm_open_without_vmx_launches_bare_gui(monkeypatch: pytest.MonkeyPatch) -> None: +def test_vmgui_without_vmx_launches_bare_gui(monkeypatch: pytest.MonkeyPatch) -> None: cap = _capture_launch(monkeypatch) monkeypatch.setenv("DISPLAY", ":0") result = CliRunner().invoke(cli, ["vmgui"]) assert result.exit_code == 0, result.output - # Only the binary (no vmx path, no power-on flags). assert cap["argv"][1:] == [] or cap["argv"][1:] == ["-n"] assert "-x" not in cap["argv"] and "-X" not in cap["argv"] assert "pid 4242" in result.output -def test_vm_open_power_on_ignored_without_vmx(monkeypatch: pytest.MonkeyPatch) -> None: +def test_vmgui_power_on_ignored_without_vmx(monkeypatch: pytest.MonkeyPatch) -> None: cap = _capture_launch(monkeypatch) monkeypatch.setenv("DISPLAY", ":0") result = CliRunner().invoke(cli, ["vmgui", "--power-on"])