diff --git a/bot_bottle/cli/commands/doctor.py b/bot_bottle/cli/commands/doctor.py index 1cc218da..0144694d 100644 --- a/bot_bottle/cli/commands/doctor.py +++ b/bot_bottle/cli/commands/doctor.py @@ -2,7 +2,8 @@ bot-bottle and report what's ready. Fails (non-zero exit) only on the two hard requirements: a new-enough -Python and at least one usable backend. The config directory is a soft +Python and at least one backend that is *ready* (passes its full status +checks, so `start` can actually work). The config directory is a soft check — `install.sh` creates it, but a missing one only warrants a note, not a failure, since `start` provisions what it needs on first run. """ @@ -13,7 +14,7 @@ import argparse import sys from pathlib import Path -from ...backend import is_backend_available, known_backend_names +from ...backend import is_backend_ready, known_backend_names from ..constants import PROG MIN_PYTHON = (3, 11) @@ -43,19 +44,25 @@ def _check_python() -> bool: def _check_backends() -> bool: - """At least one backend must be available on this host. Report each - known backend so the operator sees why a missing one is missing.""" - available = [] + """At least one backend must be *ready* to run a bottle — i.e. pass its + full status() checks (daemon reachable, network pool present, KVM usable), + not merely have a binary on PATH. A binary-only check would report `ok` + on a host with a stopped Docker daemon or a half-configured Firecracker, + where `start` still can't work. Each not-ready backend prints its own + diagnostics (quiet=False) so the operator sees exactly what's missing.""" + ready = [] for name in known_backend_names(): - if is_backend_available(name): - available.append(name) - if available: - _ok("backend", f"available: {', '.join(available)}") + if is_backend_ready(name, quiet=False): + _ok("backend", f"{name}: ready") + ready.append(name) + else: + _warn("backend", f"{name}: not ready (see diagnostics above)") + if ready: return True _fail( "backend", - "no backend available; install Apple Container (macOS), " - "Firecracker (Linux), or Docker", + "no backend is ready to run a bottle; start Docker, or finish " + "Apple Container (macOS) / Firecracker (Linux) setup", ) return False diff --git a/docs/prds/prd-new-install-script.md b/docs/prds/prd-new-install-script.md index 87cff452..1009a111 100644 --- a/docs/prds/prd-new-install-script.md +++ b/docs/prds/prd-new-install-script.md @@ -33,7 +33,7 @@ user whether their host is actually ready to run a bottle. `bot-bottle doctor`. It never installs Docker or a VM backend silently and never uses `sudo`. - `install.sh` is idempotent — safe to re-run. -- `bot-bottle doctor` reports Python version, backend availability, and +- `bot-bottle doctor` reports Python version, backend *readiness*, and config-dir presence, exiting non-zero when a hard prerequisite is unmet. - The package keeps **zero runtime pip dependencies** (stdlib-only, matching the existing constraint in `AGENTS.md`). @@ -129,8 +129,11 @@ A POSIX `sh` bootstrapper that: 4. Installs via `pipx` if available, else `python3 -m pip install --user`. The spec defaults to the git URL and is overridable via `BOT_BOTTLE_INSTALL_SPEC` (used by tests / local installs). -4. Locates the `bot-bottle` entry point (PATH or `~/.local/bin`). -5. Runs `bot-bottle doctor` and reports the result. +5. Locates the `bot-bottle` entry point: PATH first, else the + interpreter's own user-scheme scripts dir resolved via `sysconfig` + (`~/.local/bin` on Linux, `~/Library/Python//bin` on a python.org + macOS interpreter — not hardcoded). +6. Runs `bot-bottle doctor` and reports the result. It is idempotent and never calls `sudo`. @@ -140,10 +143,12 @@ A new store-free subcommand (no DB migration required) that checks and reports: - **python** — interpreter version (hard requirement: ≥ 3.11). -- **backend** — at least one backend available on this host - (macos-container / firecracker / docker), reusing - `is_backend_available()` rather than hardcoding Docker, since the - default backend is now a VM backend. Hard requirement. +- **backend** — at least one backend *ready* on this host + (macos-container / firecracker / docker), via `is_backend_ready()` — a + full backend `status()` probe (daemon reachable, network pool present, + KVM usable), not a PATH-only check: a stopped daemon or half-configured + backend must not report `ok` when `start` can't work. Each not-ready + backend prints its own diagnostics. Hard requirement. - **config** — whether `~/.bot-bottle/` exists (advisory only; `start` provisions on first run). @@ -152,7 +157,8 @@ Exits 0 when both hard requirements pass, non-zero otherwise. ## Testing strategy - Unit test `bot-bottle doctor` success/failure paths with backend - availability and Python version mocked. + readiness (`is_backend_ready`) and Python version mocked, including the + available-but-not-ready → fail case. - Unit test that `pyproject.toml` parses, declares the entry point and an empty `dependencies` list, and that every `package-data` glob resolves to a file that exists on disk (guards against drift). diff --git a/install.sh b/install.sh index 0af73145..058c2109 100755 --- a/install.sh +++ b/install.sh @@ -94,13 +94,22 @@ fi # --- locate the entry point -------------------------------------------------- +# The pip --user scripts directory is platform-specific: ~/.local/bin on Linux, +# but ~/Library/Python//bin on a python.org macOS interpreter. Ask the +# interpreter for its own user-scheme scripts dir instead of hardcoding. +USER_SCRIPTS="$(python3 - <<'PY' +import sysconfig +print(sysconfig.get_path("scripts", sysconfig.get_preferred_scheme("user"))) +PY +)" + if command -v bot-bottle >/dev/null 2>&1; then BOT_BOTTLE_BIN="bot-bottle" -elif [ -x "${HOME}/.local/bin/bot-bottle" ]; then - BOT_BOTTLE_BIN="${HOME}/.local/bin/bot-bottle" - say "note: add ${HOME}/.local/bin to your PATH to run 'bot-bottle' directly" +elif [ -n "${USER_SCRIPTS}" ] && [ -x "${USER_SCRIPTS}/bot-bottle" ]; then + BOT_BOTTLE_BIN="${USER_SCRIPTS}/bot-bottle" + say "note: add ${USER_SCRIPTS} to your PATH to run 'bot-bottle' directly" else - die "bot-bottle was installed but is not on PATH; add ~/.local/bin to PATH and re-run" + die "bot-bottle was installed but is not on PATH; add ${USER_SCRIPTS:-your user scripts dir} to PATH and re-run" fi # --- verify ------------------------------------------------------------------ diff --git a/tests/unit/test_cli_doctor.py b/tests/unit/test_cli_doctor.py index 871398ed..7b82d0e3 100644 --- a/tests/unit/test_cli_doctor.py +++ b/tests/unit/test_cli_doctor.py @@ -2,8 +2,12 @@ `doctor` is a store-free diagnostic — it must run on a fresh install before any DB migration, and its exit code gates only the two hard -prerequisites (Python and an available backend). The config-dir check is -advisory and never affects the exit code. +prerequisites (Python and at least one *ready* backend). The config-dir +check is advisory and never affects the exit code. + +Backend readiness is probed with `is_backend_ready()` (a full status() +check), not the cheap PATH-only `is_backend_available()` — a host with a +stopped daemon or half-configured backend must not report `ok`. """ from __future__ import annotations @@ -26,26 +30,43 @@ def _run(argv: list[str] | None = None) -> tuple[int, str]: class TestDoctor(unittest.TestCase): - def test_passes_when_python_and_backend_ok(self): + def test_passes_when_python_and_backend_ready(self): with patch.object(doctor, "known_backend_names", return_value=("docker",)), \ - patch.object(doctor, "is_backend_available", return_value=True): + patch.object(doctor, "is_backend_ready", return_value=True): code, out = _run() self.assertEqual(0, code) self.assertIn("ok: python", out) - self.assertIn("ok: backend: available: docker", out) + self.assertIn("ok: backend: docker: ready", out) - def test_fails_when_no_backend_available(self): + def test_fails_when_no_backend_ready(self): + # The regression the reviewer flagged: a backend whose binary is on PATH + # but whose daemon/pool isn't ready must NOT pass. is_backend_ready is + # the full status() check, so returning False here means "not ready". with patch.object(doctor, "known_backend_names", return_value=("docker", "firecracker")), \ - patch.object(doctor, "is_backend_available", return_value=False): + patch.object(doctor, "is_backend_ready", return_value=False): code, out = _run() self.assertEqual(1, code) self.assertIn("fail: backend", out) + self.assertIn("warn: backend: docker: not ready", out) + + def test_passes_when_at_least_one_backend_ready(self): + # docker not ready, firecracker ready → overall pass, mixed report. + def ready(name: str, *, quiet: bool = False) -> bool: + del quiet + return name == "firecracker" + + with patch.object(doctor, "known_backend_names", return_value=("docker", "firecracker")), \ + patch.object(doctor, "is_backend_ready", side_effect=ready): + code, out = _run() + self.assertEqual(0, code) + self.assertIn("warn: backend: docker: not ready", out) + self.assertIn("ok: backend: firecracker: ready", out) def test_fails_when_python_too_old(self): # Force the version gate to fail without touching the interpreter. with patch.object(doctor, "MIN_PYTHON", (99, 0)), \ patch.object(doctor, "known_backend_names", return_value=("docker",)), \ - patch.object(doctor, "is_backend_available", return_value=True): + patch.object(doctor, "is_backend_ready", return_value=True): code, out = _run() self.assertEqual(1, code) self.assertIn("fail: python", out) @@ -57,7 +78,7 @@ class TestDoctor(unittest.TestCase): with tempfile.TemporaryDirectory() as tmp, \ patch.object(doctor.Path, "home", return_value=Path(tmp)), \ patch.object(doctor, "known_backend_names", return_value=("docker",)), \ - patch.object(doctor, "is_backend_available", return_value=True): + patch.object(doctor, "is_backend_ready", return_value=True): code, out = _run() self.assertEqual(0, code) self.assertIn("warn: config", out) @@ -66,7 +87,7 @@ class TestDoctor(unittest.TestCase): with tempfile.TemporaryDirectory() as tmp, \ patch.object(doctor.Path, "home", return_value=Path(tmp)), \ patch.object(doctor, "known_backend_names", return_value=("docker",)), \ - patch.object(doctor, "is_backend_available", return_value=True): + patch.object(doctor, "is_backend_ready", return_value=True): (Path(tmp) / ".bot-bottle").mkdir() code, out = _run() self.assertEqual(0, code) diff --git a/tests/unit/test_install_script.py b/tests/unit/test_install_script.py index 3186a5cf..a0a27905 100644 --- a/tests/unit/test_install_script.py +++ b/tests/unit/test_install_script.py @@ -9,6 +9,7 @@ create the config tree, install the package, and verify with `doctor`. from __future__ import annotations import os +import sysconfig import unittest from pathlib import Path @@ -68,6 +69,33 @@ class TestInstallScript(unittest.TestCase): self.assertIn("EXTERNALLY-MANAGED", self.text) self.assertIn("pipx", self.text) + def test_resolves_user_scripts_dir_not_hardcoded(self): + # The pip --user scripts dir differs by platform; the script must ask + # the interpreter (sysconfig + the preferred *user* scheme) rather than + # hardcoding Linux's ~/.local/bin (which is wrong on macOS python.org). + self.assertIn("get_preferred_scheme", self.text) + self.assertIn("sysconfig", self.text) + # No hardcoded Linux path in executable lines (a comment may mention it). + code = "\n".join( + ln for ln in self.text.splitlines() + if ln.strip() and not ln.lstrip().startswith("#") + ) + self.assertNotIn(".local/bin", code) + + def test_macos_user_scheme_is_not_dot_local_bin(self): + # The case the fix exists for: a python.org macOS interpreter uses the + # osx_framework_user scheme, whose scripts land under + # ~/Library/Python//bin — NOT ~/.local/bin. Drive the same + # sysconfig lookup install.sh uses, with a mac-like userbase, to prove + # it resolves a non-~/.local/bin directory. + self.assertIn("osx_framework_user", sysconfig.get_scheme_names()) + scripts = sysconfig.get_path( + "scripts", "osx_framework_user", + vars={"userbase": "/Users/dev/Library/Python/3.11"}, + ) + self.assertEqual("/Users/dev/Library/Python/3.11/bin", scripts) + self.assertNotIn("/.local/bin", scripts) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_resources.py b/tests/unit/test_resources.py index 639b2e8a..110353aa 100644 --- a/tests/unit/test_resources.py +++ b/tests/unit/test_resources.py @@ -109,6 +109,15 @@ class TestWheelMode(unittest.TestCase): (root2 / "bot_bottle" / "cli" / "__init__.py").read_text(), ) + def test_failed_stage_cleans_up_temp_dir(self): + # A failure mid-stage must not leave a half-written temp dir behind. + with self._wheel(): + base = resources.bot_bottle_root() / "build-root" + with patch.object(resources.shutil, "copytree", side_effect=OSError("boom")): + with self.assertRaises(OSError): + resources.build_root() + self.assertEqual([], list(base.glob(".staging-*"))) + def test_rebuilds_when_stage_incomplete(self): # A crash mid-stage can leave a dir without its `.complete` marker; the # next call must rebuild it rather than trust the partial tree.