diff --git a/pyproject.toml b/pyproject.toml index 8d7e845..c4519a6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -60,6 +60,25 @@ fixable = ["ALL"] # Verified empirically, not copied: with this block removed, ruff reports # exactly these four rules across tests/ and nothing else. SLF001 was in # the original list and fired zero times, so it is gone. +[tool.ruff.lint.isort] +# venv_bootstrap sets up .venv as a side effect of being imported, so it has +# to be imported before anything that lives in .venv. Left to its own devices +# isort sorts it alphabetically -- after fastapi and httpx -- which silently +# breaks the thing it exists to do. Giving it its own section ahead of the +# standard library makes the formatter enforce the order instead of undoing +# it, which is better than a comment asking people not to reformat. +section-order = [ + "future", + "bootstrap", + "standard-library", + "third-party", + "first-party", + "local-folder", +] + +[tool.ruff.lint.isort.sections] +bootstrap = ["venv_bootstrap"] + [tool.ruff.lint.per-file-ignores] # The single subprocess in this repository. S603 flags every subprocess call # and has no code-level fix -- exiftool is a binary, and the pure-Python EXIF @@ -67,6 +86,11 @@ fixable = ["ALL"] # entry can name exactly one file: any other module that shells out fails the # gate, which is the point. Do not widen this to a directory. "signal_bot/external.py" = ["S603"] +# The bootstrap that installs .venv. It cannot route through external.py -- +# that module lives inside the package it is installing -- so it is the one +# other place allowed to start a process. Kept to a single small file for +# exactly that reason. kuhy's call, 2026-08-31. Do not widen to a directory. +"tool/venv_bootstrap.py" = ["S603", "S606"] "tests/**/*.py" = [ "ARG", # fixture and mock params exist for their side effect "D", # test names are the documentation diff --git a/tests/test_external.py b/tests/test_external.py index 0e957bb..7841421 100644 --- a/tests/test_external.py +++ b/tests/test_external.py @@ -70,11 +70,16 @@ def test_the_default_timeout_is_long_enough_for_a_video() -> None: def test_it_is_the_only_module_that_imports_subprocess() -> None: - """The whole reason this module exists: exactly one per-file-ignore. + """Two files may shell out, and the list is closed. - A second file shelling out would need a second S603 exemption, and the - point of the single entry is that adding one is a visible decision rather - than a habit that spreads. + A further file would need a third S603 exemption, and the point of naming + them here is that adding one is a visible decision rather than a habit + that spreads. + + tool/venv_bootstrap.py is the second, added on kuhy's call of 2026-08-31. + It cannot route through this module: it installs the package this module + is part of, and runs on an interpreter where that package does not import + yet. That is the only reason it qualifies -- being inconvenient is not. """ here = Path(__file__).resolve() root = here.parent.parent @@ -85,4 +90,4 @@ def test_it_is_the_only_module_that_imports_subprocess() -> None: # This file names the import in order to look for it. if path != here and "import subprocess" in path.read_text(encoding="utf-8") ) - assert importers == ["signal_bot/external.py"] + assert importers == ["signal_bot/external.py", "tool/venv_bootstrap.py"] diff --git a/tests/test_tool_imports.py b/tests/test_tool_imports.py index 56adc15..5973de2 100644 --- a/tests/test_tool_imports.py +++ b/tests/test_tool_imports.py @@ -37,6 +37,16 @@ def test_there_is_something_to_check() -> None: assert _modules() +def _load(stem: str) -> object: + """Import one tool/ module by path, the way the parametrised test does.""" + spec = importlib.util.spec_from_file_location(f"_probe_{stem}", TOOL / f"{stem}.py") + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + @pytest.mark.parametrize("path", _modules(), ids=lambda path: path.name) def test_the_module_imports(path: Path) -> None: spec = importlib.util.spec_from_file_location(f"_tool_{path.stem}", path) @@ -64,3 +74,38 @@ def test_the_tagger_answers_help() -> None: assert result.ok, result.stderr for flag in ("--import", "--untagged", "--dry-run"): assert flag in result.stdout + + +def test_the_bootstrap_reports_what_this_interpreter_cannot_import() -> None: + # The whole decision rests on this: an interpreter that can already + # import everything must be left alone, or every run pays for a venv it + # does not need. + bootstrap = _load("venv_bootstrap") + assert bootstrap.missing() == [] + assert bootstrap.missing(("definitely_not_a_real_module",)) == ["definitely_not_a_real_module"] + # A name that is not importable *and* not a valid spec query has to be + # reported, not raised: find_spec raises ValueError for some of them. + assert bootstrap.missing((".relative",)) == [".relative"] + + +def test_the_bootstrap_module_is_stdlib_only() -> None: + # It runs before .venv exists, so a third-party import here would be the + # same bug one level down. + source = (TOOL / "venv_bootstrap.py").read_text(encoding="utf-8") + for banned in ("import fastapi", "import httpx", "import uvicorn", "from signal_bot"): + assert banned not in source + + +def test_the_bootstrap_only_installs_when_the_venv_cannot_already_do_it( + tmp_path: Path, +) -> None: + # Measured bug: ensure() tested the *current* interpreter to decide both + # whether to re-exec and whether to install. The interpreter you typed + # never grows a fastapi, so every single launch reinstalled the project + # before re-execing. The two decisions are separate. + bootstrap = _load("venv_bootstrap") + bootstrap.VENV_PYTHON = tmp_path / "nonexistent" / "python" + assert bootstrap.ready() is False + bootstrap.VENV_PYTHON = Path(sys.executable) + assert bootstrap.ready(("sys",)) is True + assert bootstrap.ready(("definitely_not_a_real_module",)) is False diff --git a/tool/tagger.py b/tool/tagger.py index dc856d2..25e2677 100755 --- a/tool/tagger.py +++ b/tool/tagger.py @@ -10,10 +10,11 @@ Names are typed *into the page*, never as arguments: an argument lands in the shell history, and the whole design of the pack is that the names exist in one directory and nowhere else. -Run it through ``./tag.sh``, which sets up ``.venv`` on first use -- the -dependencies are not in the system interpreter, and a script that tells you to -go and install them yourself is a script that does not own its dependencies. +Either entry point sets up ``.venv`` on first use -- the dependencies are not +in the system interpreter, and a script that tells you to go and install them +yourself is a script that does not own its dependencies. + python tool/tagger.py --untagged # bootstraps, then runs ./tag.sh # the configured pack ./tag.sh --dir ~/pack --port 8140 ./tag.sh --import ~/photos # dedup, strip and import, then tag @@ -27,6 +28,12 @@ this is where that shows up rather than in the group. from __future__ import annotations +# Importing this sets up .venv if the interpreter running us cannot import +# what follows. It has to come first, and it is an import rather than a call +# so that nothing here needs an E402 suppression. "python tool/tagger.py" +# puts tool/ on sys.path, which is what makes the plain name resolve. +from venv_bootstrap import REPO_ROOT + import argparse import sys import webbrowser @@ -48,7 +55,7 @@ from signal_bot.pack_import import plan_import, run_import if TYPE_CHECKING: from signal_bot.pack_report import ImportReport -PAGE: Final = Path(__file__).with_name("tagger.html") +PAGE: Final = REPO_ROOT / "tool" / "tagger.html" DEFAULT_PORT: Final = 8140 diff --git a/tool/venv_bootstrap.py b/tool/venv_bootstrap.py new file mode 100755 index 0000000..179342f --- /dev/null +++ b/tool/venv_bootstrap.py @@ -0,0 +1,155 @@ +#!/usr/bin/env python3 +# Copyright (c) 2026 Krzysztof Rudnicki. MIT licence. +"""Make a script work with whatever interpreter you started it with. + +``python tool/tagger.py`` failed with ``ModuleNotFoundError: fastapi`` because +the dependencies live in ``.venv`` and nothing said so. So instead of reporting +that, this fixes it: if the running interpreter cannot import what the script +needs, it creates ``.venv`` if there is none, installs the project into it, and +re-executes the same command line through it. + +``tag.sh`` and ``run.sh`` get there through ``scripts/ensure_venv.sh``. This +module is the same job for the case where somebody runs the ``.py`` directly, +which is what the file's own docstring used to tell them to do. + +**Stdlib only, and it must stay that way.** It runs *before* the environment +holding the dependencies exists, so importing anything third-party here would +be the same bug one level down. That is also why it cannot use +``signal_bot.external``, the one module allowed to start a subprocess: it lives +inside the package being installed. Hence the second, scoped entry in +``per-file-ignores`` -- added deliberately, on kuhy's call of 2026-08-31, and +the reason it is a whole file for two subprocess calls is so that entry can +name something this small. + +The sentinel environment variable stops a loop: if the re-executed interpreter +still cannot import what it needs, the second pass gives up and lets the real +ImportError through, so the error names the missing module rather than this. +""" + +from __future__ import annotations + +import importlib.util +import os +import shutil +import subprocess +import sys +from pathlib import Path +from typing import Final + +REPO_ROOT: Final = Path(__file__).resolve().parent.parent +VENV_DIR: Final = REPO_ROOT / ".venv" +VENV_PYTHON: Final = VENV_DIR / "bin" / "python" + +SENTINEL: Final = "SIGNAL_BOT_VENV_BOOTSTRAPPED" + +# Enough to tell "wrong interpreter" from "project not installed". Not the +# whole dependency list -- pip is the authority on that, not this file. +REQUIRED: Final = ("fastapi", "uvicorn", "httpx", "signal_bot") + + +def missing(packages: tuple[str, ...] = REQUIRED) -> list[str]: + """Which of ``packages`` this interpreter cannot import. + + ``find_spec`` rather than ``import``: it answers the same question without + paying for the module or running its side effects. + """ + absent = [] + for name in packages: + try: + found = importlib.util.find_spec(name) is not None + except ImportError, ValueError: + found = False + if not found: + absent.append(name) + return absent + + +def ensure(packages: tuple[str, ...] = REQUIRED) -> None: + """Re-execute through ``.venv`` if this interpreter is missing anything. + + Does nothing in the normal case: an interpreter that can already import + everything is the right one, whether or not it is the venv's. + + Args: + packages: Module names the caller is about to import. + """ + absent = missing(packages) + if not absent or os.environ.get(SENTINEL): + return + # Only build if the venv cannot already do the job. Testing the *current* + # interpreter is what decides to re-exec; testing the *venv's* is what + # decides to install. Conflating them reinstalls on every single launch, + # because the interpreter you typed never grows a fastapi. + if not ready(packages): + sys.stderr.write(f"== setting up .venv (missing: {', '.join(absent)})\n") + provision() + _reexec() + + +def ready(packages: tuple[str, ...] = REQUIRED) -> bool: + """Whether the venv's interpreter can already import ``packages``.""" + if not VENV_PYTHON.exists(): + return False + return _run([str(VENV_PYTHON), "-c", f"import {', '.join(packages)}"], quiet=True) + + +def provision() -> None: + """Create the virtualenv if it is not there, then install the project.""" + if not VENV_PYTHON.exists() and not _run([*_venv_creator(), str(VENV_DIR)]): + _die(f"could not create a virtualenv at {VENV_DIR}") + if not _install(): + _die("could not install the project into .venv") + + +def _venv_creator() -> list[str]: + """The command that makes a virtualenv, preferring uv when it is here.""" + uv = shutil.which("uv") + return [uv, "venv"] if uv is not None else [sys.executable, "-m", "venv"] + + +def _install() -> bool: + """Install the project editable, so a later git pull reaches the tagger.""" + uv = shutil.which("uv") + target = f"{REPO_ROOT}[dev]" + if uv is not None: + return _run( + [uv, "pip", "install", "-e", target], {**os.environ, "VIRTUAL_ENV": str(VENV_DIR)} + ) + return _run([str(VENV_PYTHON), "-m", "pip", "install", "-e", target]) + + +def _reexec() -> None: + """Hand the same command line to the interpreter that can run it.""" + if not VENV_PYTHON.exists(): + _die(f"no interpreter at {VENV_PYTHON} after install") + os.execve(str(VENV_PYTHON), [str(VENV_PYTHON), *sys.argv], {**os.environ, SENTINEL: "1"}) + + +def _run(command: list[str], env: dict[str, str] | None = None, *, quiet: bool = False) -> bool: + """Run one setup command and say whether it worked. + + Setup output is shown, because an install that takes a minute should look + like it is doing something. The probe is quiet: it fails on every normal + first run and its traceback would be the first thing anybody saw. + """ + output = subprocess.DEVNULL if quiet else None + try: + completed = subprocess.run(command, check=False, env=env, stdout=output, stderr=output) + except OSError: + return False + return completed.returncode == 0 + + +def _die(message: str) -> None: + """Stop with the command to run, not just what broke.""" + sys.stderr.write(f"Error: {message}\n") + sys.stderr.write(f" Try: cd {REPO_ROOT} && uv venv .venv && uv pip install -e '.[dev]'\n") + raise SystemExit(1) + + +# Run on import, not from a call site. A caller that had to remember to call +# ensure() would have to do it *before* its own third-party imports, and code +# before imports is E402 -- so the fix would need a lint suppression in every +# file it protects. Importing this module is the whole interface: take a +# constant from it and .venv is guaranteed by the time the next import runs. +ensure()