diff --git a/docs/install-macos.md b/docs/install-macos.md index 0379211..efca4a0 100644 --- a/docs/install-macos.md +++ b/docs/install-macos.md @@ -113,10 +113,15 @@ which xhost # /usr/X11/bin/xhost or /opt/X11/bin/xhost With XQuartz installed, TIG does the rest of the X11 setup itself on each run: it sets `nolisten_tcp` to `false` so XQuartz accepts TCP connections, starts -XQuartz if it is not already running, and runs `xhost +localhost` to authorize -the connection. Under Podman the container is given -`DISPLAY=host.containers.internal:0`, which routes back out of the VM to -XQuartz on your Mac. +XQuartz if it is not already running, and runs +`xhost +localhost +inet:127.0.0.1` to authorize the connection. Under Podman +the container is given `DISPLAY=host.containers.internal:0`, which routes back +out of the VM to XQuartz on your Mac. + +An XQuartz update or a logout restarts the X server, which drops every `xhost` +authorization it was holding. TIG reauthorizes before every command, so running +`xhost + 127.0.0.1` by hand is not part of using `xvd`. Set `TIG_NO_X11=1` if +you would rather manage the display yourself. Check a window actually opens: @@ -128,7 +133,9 @@ XQuartz appears in the Dock and displays the 64×64 test image. Quit the window to return to the shell. > If you changed XQuartz's `Allow connections from network clients` setting by -> hand, restart XQuartz afterwards — the preference is read at startup. +> hand, restart XQuartz afterwards — the preference is read at startup. When +> TIG finds XQuartz already running with TCP off, it says so and asks you to +> quit it, rather than quitting it under your other X clients. ## 5. Apple Silicon: x86-64 emulation @@ -176,6 +183,7 @@ prefix, see `tig --shim` in the [tig-cli README](../tig-cli/README.md). | `Error: no container runtime found` | The machine is not running, or Podman is not on `PATH`. Run `podman machine start`, then `podman info`. | | GUI tool prints `Can't open display` or `Error: Can't open display: ` | XQuartz is missing, or you have not logged out and back in since installing it. Check `echo $DISPLAY` and `which xhost` (step 4). | | `xhost: unable to open display` | XQuartz is installed but `DISPLAY` points at a stale socket. Quit XQuartz, open a new terminal, and retry. | +| GUI tools stopped opening after an XQuartz update | XQuartz came back with TCP connections turned off. TIG rewrites the preference, but it is only read at startup: quit XQuartz and run the command again. | | A window opens but is blank or freezes | XQuartz was running before its `nolisten_tcp` setting changed. Quit XQuartz entirely and rerun the command; TIG restarts it. | | `exec format error`, `no matching manifest` | No x86-64 emulation in the VM on Apple Silicon (step 5). | | VICAR says a file does not exist, though it exists on your Mac | The path is outside your home directory and therefore not shared with the VM. Move the data under `$HOME` or recreate the machine with `-v` (step 2). | diff --git a/tig-cli/README.md b/tig-cli/README.md index 8676411..0bdeafd 100644 --- a/tig-cli/README.md +++ b/tig-cli/README.md @@ -330,8 +330,8 @@ same reason. ## GUI tools and X11 -GUI tools such as `xvd` and `marsmap` render on your host display. When tig -creates a container it first authorizes the display, so you do not have to: +GUI tools such as `xvd` and `marsmap` render on your host display. tig +authorizes the display for you, so you do not have to: - **Linux** — runs `xhost +local:`. The broad form is deliberate: with `label=disable` the container connects as the `LOCAL:` family, which @@ -339,13 +339,24 @@ creates a container it first authorizes the display, so you do not have to: and `/tmp/.X11-unix`, and `DISPLAY` is passed through per command. - **macOS** — makes XQuartz listen on TCP (`defaults write org.xquartz.X11 nolisten_tcp -bool false`), starts it if it - is not running, and runs `xhost +localhost`; the container uses - `DISPLAY=host.docker.internal:0` (`host.containers.internal` under Podman, - `host.lima.internal` under Finch). - -This happens only when a container is created, not on every command, and is -skipped silently when there is no `DISPLAY` or no `xhost` (a headless host has -no display to authorize). + is not running, and runs `xhost +localhost +inet:127.0.0.1`; the container + uses `DISPLAY=host.docker.internal:0` (`host.containers.internal` under + Podman, `host.lima.internal` under Finch). + +This happens before every command. An `xhost` authorization is the X server's +own state, and the container outlives the server — an XQuartz update, a logout, +an `xhost -` — so authorizing only at container creation left users running +`xhost` by hand afterwards. `xhost` is idempotent and costs milliseconds, so it +is simply repeated; the slower macOS work (starting XQuartz, reading its +preferences) is skipped while the X server is the one tig last saw, which it +remembers in `~/.cache/tig/x11-authorized`. Setting `TIG_NO_X11=1` leaves the +host display alone entirely. Everything here is skipped silently when there is +no `DISPLAY` or no `xhost` (a headless host has no display to authorize). + +XQuartz reads `nolisten_tcp` only when it starts. If it is already running with +TCP turned off — how it comes out of an update that reset the preference — tig +says so and asks you to quit XQuartz, rather than quitting it under your other +X clients. ## SELinux (RHEL, Fedora, CentOS) diff --git a/tig-cli/src/tig_cli/container.py b/tig-cli/src/tig_cli/container.py index 17eddd9..d856fef 100644 --- a/tig-cli/src/tig_cli/container.py +++ b/tig-cli/src/tig_cli/container.py @@ -597,6 +597,10 @@ def execute_vicar_command( Returns: Exit code from command execution """ + # Per command, not per container: the authorization belongs to the X + # server, which restarts far more often than the container does. + ensure_x11_ready() + if self.disable_path_translation: translated_args = args container_cwd = os.getcwd() diff --git a/tig-cli/src/tig_cli/fast.py b/tig-cli/src/tig_cli/fast.py index 5a2d0fe..8561d1d 100644 --- a/tig-cli/src/tig_cli/fast.py +++ b/tig-cli/src/tig_cli/fast.py @@ -31,6 +31,7 @@ build_run_spec, container_carries_builds, container_display, + ensure_x11_ready, forwarded_signals, get_calibration_path, get_container_image, @@ -80,6 +81,10 @@ def run(argv: list[str]) -> int | None: if not container_carries_builds(plan.name): return None + # The container is long-lived and the X server is not, so the display is + # authorized per command rather than when the container was created. + ensure_x11_ready() + claim = Claim() claim.acquire(plan.name) try: diff --git a/tig-cli/src/tig_cli/spec.py b/tig-cli/src/tig_cli/spec.py index 4911909..d5027df 100644 --- a/tig-cli/src/tig_cli/spec.py +++ b/tig-cli/src/tig_cli/spec.py @@ -101,6 +101,13 @@ def __exit__(self, *exc_info) -> bool: # How long to wait for XQuartz to come up on macOS before giving up. XQUARTZ_START_TIMEOUT = 5.0 +# Names the X server that was authorized last, so a command pays for xhost +# only when the server it has to reach is not that one any more. +X11_MARKER = "x11-authorized" + +# Set to leave the host display alone entirely. +DISABLE_X11_ENV = "TIG_NO_X11" + # Where calibration files (VISOR / mars_calibration_*) are mounted, matching # the layout VICAR's MARS programs expect. CALIBRATION_MOUNT = "/usr/local/vicar/mars_calib" @@ -214,6 +221,21 @@ def _run_quietly(command: list[str], timeout: float = 10.0) -> bool: return completed.returncode == 0 +def _output_of(command: list[str], timeout: float = 10.0) -> str | None: + """Run a host helper command; its output, or None if it failed.""" + import subprocess + + try: + completed = subprocess.run( + command, capture_output=True, text=True, timeout=timeout + ) + except (OSError, subprocess.SubprocessError): + return None + if completed.returncode != 0: + return None + return completed.stdout + + def selinux_enforcing() -> bool: """Whether this host is Linux with SELinux in Enforcing mode. @@ -243,13 +265,38 @@ def selinux_enforcing() -> bool: return completed.returncode == 0 and completed.stdout.strip() == "Enforcing" -def _ensure_xquartz() -> None: - """Make XQuartz running and listening on TCP, as the container needs.""" - import time +def _tcp_refused() -> bool: + """Whether XQuartz is set to refuse the TCP connections tig needs.""" + value = _output_of(["defaults", "read", "org.xquartz.X11", "nolisten_tcp"]) + return value is None or value.strip().lower() not in ("0", "false", "no") + +def _allow_tcp_connections() -> None: + """Have XQuartz listen on TCP, which the container's DISPLAY needs. + + The preference is only read at startup, so a running XQuartz that still + refuses TCP - how it comes out of an update that reset the preference - + is reported rather than quit out from under the user's other clients. + """ + if not _tcp_refused(): + return _run_quietly( ["defaults", "write", "org.xquartz.X11", "nolisten_tcp", "-bool", "false"] ) + if _run_quietly(["pgrep", "-x", "Xquartz"]): + print( + "tig: XQuartz is running with TCP connections turned off, which " + "GUI tools in the container need. Quit XQuartz and run the " + "command again.", + file=sys.stderr, + ) + + +def _ensure_xquartz() -> None: + """Make XQuartz running and listening on TCP, as the container needs.""" + import time + + _allow_tcp_connections() if _run_quietly(["pgrep", "-x", "Xquartz"]): return if not _run_quietly(["open", "-a", "XQuartz"]): @@ -261,26 +308,102 @@ def _ensure_xquartz() -> None: time.sleep(0.5) +def _x_socket(display: str) -> str | None: + """The socket file a local ``display`` names, if it is a local one.""" + host, separator, number = display.rpartition(":") + if not separator or host not in ("", "unix") or not number: + return None + return f"/tmp/.X11-unix/X{number.split('.')[0]}" + + +def x_server_identity() -> str | None: + """Something that differs whenever the X server is a different one. + + Recognizing the server is what lets the expensive XQuartz checks be + skipped while it stays the same. ``None`` means no server was + recognized, and the caller should do the work rather than assume. + """ + display = os.environ.get("DISPLAY", "") + if sys.platform == "darwin": + # The process, since the socket DISPLAY names belongs to launchd and + # outlives any one XQuartz. + running = _output_of(["pgrep", "-x", "Xquartz"]) or "" + pids = running.split() + return f"{display} xquartz={pids[0]}" if pids else None + + socket = _x_socket(display) + if socket is None: + # A display reached over the network: nothing local says whether it + # is still the server that was authorized. + return None + try: + info = os.stat(socket) + except OSError: + return None + return f"{display} socket={info.st_ino}:{int(info.st_mtime)}" + + +def _x11_marker() -> Path: + return Path(home_directory()) / ".cache" / "tig" / X11_MARKER + + +def _authorized_server() -> str | None: + """The server the last authorization was for, if it is still recorded.""" + try: + return _x11_marker().read_text() + except OSError: + return None + + +def _record_authorized_server(identity: str) -> None: + marker = _x11_marker() + try: + marker.parent.mkdir(mode=0o700, parents=True, exist_ok=True) + marker.write_text(identity) + except OSError: + pass + def ensure_x11_ready() -> None: """Authorize the host X server so GUI tools (xvd, marsmap) can connect. + Done before every command rather than only when a container is created: + the authorization is the X server's own state, which the container + outlives, so anything that drops it - a restart after an XQuartz update, + a logout, an ``xhost -`` - leaves the container unable to connect. That + is what has users running ``xhost`` by hand. ``xhost`` is idempotent and + costs milliseconds, so it is simply repeated; only the pricier XQuartz + checks are skipped while the server is the one already seen. + Silently does nothing when there is no display or no ``xhost``; those hosts simply have no GUI to authorize. """ import shutil + if os.environ.get(DISABLE_X11_ENV): + return if not os.environ.get("DISPLAY") or shutil.which("xhost") is None: return + identity = x_server_identity() if sys.platform == "darwin": - # The container reaches XQuartz over TCP via the host gateway name. - _ensure_xquartz() - _run_quietly(["xhost", "+localhost"]) + if identity is None or identity != _authorized_server(): + # The container reaches XQuartz over TCP via the host gateway + # name, so XQuartz has to be up and listening on it. + _ensure_xquartz() + # Asked again, since XQuartz may have only just been started. + identity = x_server_identity() + # The address explicitly as well as the name: the connection arrives + # from the runtime's gateway on IPv4 loopback however localhost + # happens to resolve. + authorized = _run_quietly(["xhost", "+localhost", "+inet:127.0.0.1"]) else: # The broad form is deliberate: with 'label=disable' the container # connects as the LOCAL: family, which '+local:docker' misses. - _run_quietly(["xhost", "+local:"]) + authorized = _run_quietly(["xhost", "+local:"]) + + if authorized and identity is not None: + _record_authorized_server(identity) def container_display(runtime: str = "docker") -> str: diff --git a/tig-cli/tests/test_container.py b/tig-cli/tests/test_container.py index a6bb323..773bf51 100644 --- a/tig-cli/tests/test_container.py +++ b/tig-cli/tests/test_container.py @@ -5,6 +5,7 @@ from datetime import datetime, timezone import pytest from unittest.mock import Mock, patch, MagicMock +from tig_cli import spec from tig_cli.config import Config from tig_cli.container import ( CALIBRATION_MOUNT, @@ -1100,9 +1101,18 @@ def test_host_root_mount_is_never_relabeled(home_dir, tmp_path): # --- X11 host setup --- -def test_x11_setup_authorizes_local_connections_on_linux(monkeypatch): +@pytest.fixture +def host_display(monkeypatch, tmp_path): + """A host with a display and no record of an authorized X server.""" + home = tmp_path / "x11-home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) monkeypatch.setenv("DISPLAY", ":0") + monkeypatch.delenv("TIG_NO_X11", raising=False) + return home + +def test_x11_setup_authorizes_local_connections_on_linux(host_display): with patch('sys.platform', 'linux'), \ patch('shutil.which', return_value="/usr/bin/xhost"), \ patch('subprocess.run') as run: @@ -1112,18 +1122,117 @@ def test_x11_setup_authorizes_local_connections_on_linux(monkeypatch): assert run.call_args[0][0] == ["xhost", "+local:"] -def test_x11_setup_skipped_without_a_display(monkeypatch): - monkeypatch.delenv("DISPLAY", raising=False) +def test_x11_setup_authorizes_again_for_every_command(host_display): + """Whatever dropped the authorization - a restart, a bare 'xhost -' - + the next command puts it back rather than trusting what it saw before.""" + with patch('sys.platform', 'linux'), \ + patch('shutil.which', return_value="/usr/bin/xhost"), \ + patch('tig_cli.spec.x_server_identity', return_value="server-1"), \ + patch('subprocess.run') as run: + run.return_value = subprocess.CompletedProcess([], 0) + ensure_x11_ready() + ensure_x11_ready() - with patch('subprocess.run') as run: + assert [call[0][0] for call in run.call_args_list] == [ + ["xhost", "+local:"], ["xhost", "+local:"] + ] + + +def test_xquartz_is_only_checked_for_a_server_not_seen_before(host_display): + """Starting XQuartz and reading its preferences is the slow part.""" + with patch('sys.platform', 'darwin'), \ + patch('shutil.which', return_value="/opt/X11/bin/xhost"), \ + patch('tig_cli.spec.x_server_identity', return_value="server-1"), \ + patch('tig_cli.spec._run_quietly', return_value=True), \ + patch('tig_cli.spec._ensure_xquartz') as xquartz: + ensure_x11_ready() + ensure_x11_ready() + + assert xquartz.call_count == 1 + + with patch('sys.platform', 'darwin'), \ + patch('shutil.which', return_value="/opt/X11/bin/xhost"), \ + patch('tig_cli.spec.x_server_identity', return_value="server-2"), \ + patch('tig_cli.spec._run_quietly', return_value=True), \ + patch('tig_cli.spec._ensure_xquartz') as restarted: + ensure_x11_ready() + + restarted.assert_called_once() + + +def test_x11_setup_can_be_turned_off(host_display, monkeypatch): + monkeypatch.setenv("TIG_NO_X11", "1") + + with patch('shutil.which', return_value="/usr/bin/xhost"), \ + patch('subprocess.run') as run: ensure_x11_ready() run.assert_not_called() -def test_x11_setup_skipped_without_xhost(monkeypatch): +def test_x_server_identity_follows_the_linux_socket(monkeypatch, tmp_path): + """A restarted server leaves a different socket behind.""" monkeypatch.setenv("DISPLAY", ":0") + socket = tmp_path / "X0" + socket.write_text("") + + with patch('sys.platform', 'linux'), \ + patch('tig_cli.spec._x_socket', return_value=str(socket)): + before = spec.x_server_identity() + socket.unlink() + socket.write_text("") + # A recreated socket can land on the same inode within the same + # second, which a restarted server minutes later would not. + replaced = os.stat(socket).st_mtime + 60 + os.utime(socket, (replaced, replaced)) + after = spec.x_server_identity() + + assert before is not None and before != after + +def test_a_network_display_is_never_taken_as_authorized(monkeypatch): + """Nothing local says whether a remote server is the one authorized.""" + monkeypatch.setenv("DISPLAY", "192.168.1.5:0") + + with patch('sys.platform', 'linux'): + assert spec.x_server_identity() is None + + +def test_failed_authorization_is_not_remembered(host_display): + """A skipped xhost is only safe after one that worked.""" + with patch('sys.platform', 'linux'), \ + patch('shutil.which', return_value="/usr/bin/xhost"), \ + patch('tig_cli.spec.x_server_identity', return_value="server-1"), \ + patch('tig_cli.spec._run_quietly', return_value=False): + ensure_x11_ready() + + assert spec._authorized_server() is None + + +def test_x_server_identity_follows_the_xquartz_process(monkeypatch): + monkeypatch.setenv("DISPLAY", "/private/tmp/com.apple.launchd.x/org.xquartz:0") + + with patch('sys.platform', 'darwin'), \ + patch('tig_cli.spec._output_of', side_effect=["501\n", "777\n", None]): + first = spec.x_server_identity() + second = spec.x_server_identity() + stopped = spec.x_server_identity() + + assert first != second + # No server running is no server to recognize, so nothing is skipped. + assert stopped is None + + +def test_x11_setup_skipped_without_a_display(host_display, monkeypatch): + monkeypatch.delenv("DISPLAY", raising=False) + + with patch('subprocess.run') as run: + ensure_x11_ready() + + run.assert_not_called() + + +def test_x11_setup_skipped_without_xhost(host_display): with patch('shutil.which', return_value=None), \ patch('subprocess.run') as run: ensure_x11_ready() @@ -1131,28 +1240,44 @@ def test_x11_setup_skipped_without_xhost(monkeypatch): run.assert_not_called() -def test_x11_setup_failure_is_not_fatal(monkeypatch): - monkeypatch.setenv("DISPLAY", ":0") - +def test_x11_setup_failure_is_not_fatal(host_display): with patch('sys.platform', 'linux'), \ patch('shutil.which', return_value="/usr/bin/xhost"), \ patch('subprocess.run', side_effect=OSError): ensure_x11_ready() # should not raise -def test_x11_setup_prepares_xquartz_on_macos(monkeypatch): +def test_x11_setup_prepares_xquartz_on_macos(host_display, monkeypatch): monkeypatch.setenv("DISPLAY", "/private/tmp/com.apple.launchd.x/org.xquartz:0") with patch('sys.platform', 'darwin'), \ patch('shutil.which', return_value="/opt/X11/bin/xhost"), \ patch('subprocess.run') as run: - run.return_value = subprocess.CompletedProcess([], 0) + run.return_value = subprocess.CompletedProcess( + [], 0, stdout="", stderr="" + ) ensure_x11_ready() commands = [call[0][0] for call in run.call_args_list] assert ["defaults", "write", "org.xquartz.X11", "nolisten_tcp", "-bool", "false"] in commands - assert ["xhost", "+localhost"] in commands + # Both families: the container's connection arrives on the loopback + # address, whichever way the name resolves. + assert ["xhost", "+localhost", "+inet:127.0.0.1"] in commands + + +def test_xquartz_left_refusing_tcp_says_so(host_display, capsys): + """An update resets the preference, and it is only read at startup.""" + with patch('sys.platform', 'darwin'), \ + patch('tig_cli.spec._tcp_refused', return_value=True), \ + patch('tig_cli.spec._run_quietly', return_value=True) as run: + spec._allow_tcp_connections() + + assert ["defaults", "write", "org.xquartz.X11", + "nolisten_tcp", "-bool", "false"] in [ + call[0][0] for call in run.call_args_list + ] + assert "Quit XQuartz" in capsys.readouterr().err def test_x11_setup_runs_when_a_container_is_created(home_dir): @@ -1163,16 +1288,16 @@ def test_x11_setup_runs_when_a_container_is_created(home_dir): ready.assert_called_once() -def test_x11_setup_skipped_when_a_container_is_reused(home_dir): - """Authorizing the display is container setup, not per-command work.""" - runtime = FakeRuntime() - manager = make_manager(home_dir, runtime=runtime) - runtime.containers[manager._run_spec([]).container_name()] = described() +@patch('tig_cli.container.subprocess.Popen') +def test_x11_setup_runs_for_every_command(mock_popen, home_dir): + """The container outlives the X server, so reusing one authorizes nobody.""" + mock_popen.return_value = Mock(wait=Mock(return_value=0)) + manager = make_manager(home_dir) with patch('tig_cli.container.ensure_x11_ready') as ready: - manager.ensure_container([]) + manager.execute_vicar_command("xvd", []) - ready.assert_not_called() + ready.assert_called_once() # --- execute_vicar_command --- @@ -1211,7 +1336,8 @@ def test_execute_vicar_command_passes_display(mock_popen, home_dir): """DISPLAY is passed per exec, so it is not part of container identity.""" mock_popen.return_value = Mock(wait=Mock(return_value=0)) - with patch.dict(os.environ, {"HOME": home_dir, "DISPLAY": ":7"}): + with patch.dict(os.environ, {"HOME": home_dir, "DISPLAY": ":7"}), \ + patch('tig_cli.container.ensure_x11_ready'): manager = make_manager(home_dir) with patch('sys.platform', 'linux'): manager.execute_vicar_command("xvd", []) diff --git a/tig-cli/tests/test_fast.py b/tig-cli/tests/test_fast.py index e79af39..40f6d81 100644 --- a/tig-cli/tests/test_fast.py +++ b/tig-cli/tests/test_fast.py @@ -98,6 +98,18 @@ def test_path_translation_can_be_disabled(host, engine, monkeypatch): assert command == ["vicar", "INP=/data/in.img"] +def test_authorizes_the_display_before_the_command(host, engine, monkeypatch): + """The warm path is where a GUI tool meets an X server it never told + about itself: the container it reuses was authorized for an older one.""" + order = [] + monkeypatch.setattr(fast, "ensure_x11_ready", lambda: order.append("xhost")) + engine.exec_command.side_effect = lambda *a, **k: order.append("exec") or 0 + + fast.run(["xvd", "in.img"]) + + assert order == ["xhost", "exec"] + + def test_claims_the_container_while_it_runs(host, engine): claimed = [] engine.exec_command.side_effect = lambda *a, **k: claimed.append(