From d3d89c890e8d8a883ebbfe45a4782d1d90dddc9f Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 31 Aug 2026 13:34:27 +0200 Subject: [PATCH] feat: python tool/tagger.py sets up .venv too, not just tag.sh The shell entry points got this in 99aa194; running the .py directly still died with ModuleNotFoundError, which is the command the file's own docstring had been telling people to use for months. tool/venv_bootstrap.py creates .venv, installs the project and re-executes the same command line through it. It is stdlib-only by necessity: it runs on an interpreter where this package does not import yet, so it cannot route through signal_bot.external, the one module allowed to start a process. That is why it gets the second scoped per-file-ignore rather than the rule being bent -- kuhy's call. test_external.py now asserts a closed list of two, so a third file still fails the gate. Two things that are less obvious than they look: It runs on import rather than exposing a function to call. A call would have to happen before the caller's own third-party imports, and code before imports is E402 -- so a call-based design needs a lint suppression in every file it protects. Importing the module is the whole interface. And isort would sort "venv_bootstrap" after "fastapi", silently breaking the ordering the module depends on. It gets its own isort section ahead of the standard library, so the formatter enforces the order instead of undoing it. Caught while verifying: ensure() used one check to decide both whether to re-exec and whether to install. The interpreter you typed never grows a fastapi, so every launch reinstalled the project first. Splitting them (ready() probes the venv, missing() probes the caller) takes a warm run from a full pip install to 0.47s. Pinned by a test. Verified on a clone with no .venv: first run installs and prints one line, second and third print nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WBrE1b9HvFw3xvG9bJiDFt --- pyproject.toml | 24 ++++++ tests/test_external.py | 15 ++-- tests/test_tool_imports.py | 45 +++++++++++ tool/tagger.py | 15 +++- tool/venv_bootstrap.py | 155 +++++++++++++++++++++++++++++++++++++ 5 files changed, 245 insertions(+), 9 deletions(-) create mode 100755 tool/venv_bootstrap.py 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()