From c51666b9cd310500ad78f8482f2216b4b7fc4008 Mon Sep 17 00:00:00 2001 From: Duncan McKinnon Date: Thu, 13 Aug 2026 11:19:00 -0700 Subject: [PATCH] Reuse the venv on upgrade instead of recreating it `brew upgrade` bumps VALCORE_VERSION, the stamp goes stale, and the launcher ran `uv venv` against a path that already held a venv. uv refuses that: it prompts to replace when a TTY is attached, and errors out when one is not. Under `set -e` the launcher then died before reaching the install, so every post-upgrade run from a script, cron job, or CI step failed until someone ran valcore by hand and answered the prompt. Answering it destroyed the venv and reinstalled ~115 packages to change one of them. Create the venv only when there is no usable interpreter at $venv/bin/python, and otherwise install the pinned version straight into the existing venv. A real 0.0.8 -> 0.0.9 upgrade now prepares and installs 1 package rather than 115, with no prompt and nothing interactive. `uv venv` gains --clear on the paths that do create one, so a half-written venv from an interrupted first run is replaced rather than aborting the same way. If installing into a reused venv fails -- most likely its interpreter no longer satisfies the new release's requires-python -- rebuild once and retry rather than leaving the user stuck. The "(first run)" message now says "upgrading" when that is what is happening. The stub uv in the launcher tests grew a `mkdir -p` where the real one refuses an existing venv, which is why the upgrade path looked healthy in CI while failing for real users. It now models the refusal, and can simulate an install that fails until the venv is rebuilt. Co-Authored-By: Claude Opus 5 --- packaging/valcore.sh | 48 +++++++++++++++++----- tests/test_launcher.py | 93 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 127 insertions(+), 14 deletions(-) diff --git a/packaging/valcore.sh b/packaging/valcore.sh index 0f6deae..7057de5 100755 --- a/packaging/valcore.sh +++ b/packaging/valcore.sh @@ -5,18 +5,46 @@ venv="$home/venv" stamp="$venv/.version" version="${VALCORE_VERSION:?VALCORE_VERSION not set}" +# --refresh-package: $version is a release published moments before the formula bump, +# so uv's cached index listing for valcore predates it and resolution fails with +# "there is no version of valcore==$version". Scoped to this one package so the cache +# still serves the dependency tree, which is what keeps provisioning fast. +install_pinned() { + uv pip install --python "$venv/bin/python" --refresh-package valcore "valcore==$version" >&2 +} + +# Match the package's requires-python rather than pinning one release: uv then reuses an +# interpreter the user already has instead of downloading a toolchain, and brew installs +# keep working after 3.12 goes end-of-life. --clear because a half-written venv from an +# interrupted run would otherwise make `uv venv` abort; on a fresh path it is a no-op. +create_venv() { + uv venv --clear --python ">=3.11" "$venv" >&2 +} + if [ ! -x "$venv/bin/valcore" ] || [ "$(cat "$stamp" 2>/dev/null)" != "$version" ]; then mkdir -p "$home" && chmod 700 "$home" - echo "valcore: provisioning environment (first run)…" >&2 - # Match the package's requires-python rather than pinning one release: uv - # then reuses an interpreter the user already has instead of downloading a - # toolchain, and brew installs keep working after 3.12 goes end-of-life. - uv venv --python ">=3.11" "$venv" >&2 - # --refresh-package: $version is a release published moments before the formula bump, - # so uv's cached index listing for valcore predates it and resolution fails with - # "there is no version of valcore==$version". Scoped to this one package so the cache - # still serves the dependency tree, which is what keeps provisioning fast. - uv pip install --python "$venv/bin/python" --refresh-package valcore "valcore==$version" >&2 + + # Upgrades reuse the existing venv and let uv install just what changed. Recreating it + # would be both slow (~115 packages) and, more importantly, broken: `uv venv` refuses a + # path that already holds a venv, prompting at a TTY and failing outright without one -- + # so under `set -e` every post-upgrade run from a script or CI job died here. + if [ -x "$venv/bin/python" ]; then + # Braces are required: the ellipsis is a word character to bash's parser, so an + # unbraced "$version…" is read as the variable "version…" and set -u aborts. + echo "valcore: upgrading environment to ${version}…" >&2 + else + echo "valcore: provisioning environment (first run)…" >&2 + create_venv + fi + + # A reused venv can still be unusable -- most likely its interpreter no longer satisfies + # the new release's requires-python. Rebuild once rather than leaving the user stuck. + if ! install_pinned; then + echo "valcore: environment is stale, rebuilding…" >&2 + create_venv + install_pinned + fi + printf '%s' "$version" > "$stamp" fi diff --git a/tests/test_launcher.py b/tests/test_launcher.py index 34063ed..ee07b72 100644 --- a/tests/test_launcher.py +++ b/tests/test_launcher.py @@ -16,19 +16,40 @@ LAUNCHER = Path(__file__).resolve().parent.parent / "packaging" / "valcore.sh" # A fake ``uv`` that logs every invocation and materializes the files the launcher -# expects: ``uv venv`` creates the venv bin dir, ``uv pip install`` drops in an -# executable ``valcore`` entrypoint that echoes a marker so we can prove the exec +# expects: ``uv venv`` creates the venv bin dir and interpreter, ``uv pip install`` drops +# in an executable ``valcore`` entrypoint that echoes a marker so we can prove the exec # passthrough happened. +# +# The ``venv`` case refuses a path that already holds a venv unless ``--clear`` is passed, +# because that is what the real uv does. The original stub used a bare ``mkdir -p``, which +# succeeded either way -- so the upgrade path looked healthy in tests while failing for +# real users the moment no TTY was attached to answer uv's replace prompt. +# +# ``UV_FAIL_INSTALL_MARKER``, when it points at an existing file, makes ``pip install`` +# fail; ``venv --clear`` removes it. That models a reused venv whose interpreter no longer +# satisfies the new release's requires-python, and recovering by rebuilding. FAKE_UV = """#!/bin/bash echo "$@" >> "$UV_LOG" case "$1" in venv) venv="${@: -1}" + if [ -d "$venv" ] && [[ "$*" != *--clear* ]]; then + echo "error: A virtual environment already exists at: $venv" >&2 + exit 1 + fi + rm -f "${UV_FAIL_INSTALL_MARKER:-/nonexistent}" + rm -rf "$venv" mkdir -p "$venv/bin" + printf '#!/bin/bash\\n' > "$venv/bin/python" + chmod +x "$venv/bin/python" ;; pip) python="$4" bin="$(dirname "$python")" + if [ -f "${UV_FAIL_INSTALL_MARKER:-/nonexistent}" ]; then + echo "error: no interpreter satisfies requires-python" >&2 + exit 1 + fi mkdir -p "$bin" printf '#!/bin/bash\\necho "VALCORE_RAN $@"\\n' > "$bin/valcore" chmod +x "$bin/valcore" @@ -73,6 +94,11 @@ def _log(env: dict[str, str]) -> str: return log.read_text() if log.exists() else "" +def _called_venv(log: str) -> bool: + """Whether `uv venv` was invoked, ignoring the venv path in `pip install` lines.""" + return any(line.startswith("venv ") for line in log.splitlines()) + + def test_first_run_provisions_venv(env: dict[str, str]) -> None: result = _run(env, "serve") assert result.returncode == 0, result.stderr @@ -82,7 +108,7 @@ def test_first_run_provisions_venv(env: dict[str, str]) -> None: assert oct(home.stat().st_mode & 0o777) == "0o700" log = _log(env) - assert "venv --python >=3.11" in log + assert "venv --clear --python >=3.11" in log # --refresh-package valcore: the pinned version is published moments before the # formula bump, so uv's cached index listing for valcore predates it and resolution # fails with "there is no version of valcore==X". Scoped to the one package so the @@ -111,6 +137,13 @@ def test_second_run_skips_provisioning(env: dict[str, str]) -> None: def test_stale_stamp_triggers_reprovision(env: dict[str, str]) -> None: + """A `brew upgrade` reinstalls the pinned version without recreating the venv. + + The venv is reused deliberately. Recreating it would be slow (~115 packages for a + version bump) and, worse, `uv venv` refuses a path that already holds a venv: it + prompts at a TTY and fails outright without one, so under `set -e` every post-upgrade + run from a script or CI job used to die before reaching the install. + """ first = _run(env, "serve") assert first.returncode == 0, first.stderr @@ -123,9 +156,61 @@ def test_stale_stamp_triggers_reprovision(env: dict[str, str]) -> None: assert second.returncode == 0, second.stderr log = _log(env) - assert "venv --python >=3.11" in log + # Match the *command*, not the substring: every `pip install` line contains the venv + # path, so a bare `"venv" not in log` would never hold. + assert not _called_venv(log), "upgrade must reuse the existing venv, not recreate it" assert "pip install" in log assert stamp.read_text() == "0.1.0" + assert "VALCORE_RAN serve" in second.stdout + + +def test_upgrade_reports_upgrading_not_first_run(env: dict[str, str]) -> None: + _run(env, "serve") + (Path(env["VALCORE_HOME"]) / "venv" / ".version").write_text("0.0.9") + + result = _run(env, "serve") + + assert "upgrading environment to 0.1.0" in result.stderr + assert "first run" not in result.stderr + + +def test_upgrade_rebuilds_when_the_reused_venv_cannot_take_the_install( + env: dict[str, str], tmp_path: Path +) -> None: + """A venv whose interpreter no longer satisfies requires-python is rebuilt, not fatal.""" + _run(env, "serve") + + stamp = Path(env["VALCORE_HOME"]) / "venv" / ".version" + stamp.write_text("0.0.9") + + # Poison installs into the existing venv; only `venv --clear` clears the marker. + marker = tmp_path / "install-fails" + marker.write_text("") + env = {**env, "UV_FAIL_INSTALL_MARKER": str(marker)} + Path(env["UV_LOG"]).write_text("") + + result = _run(env, "serve") + assert result.returncode == 0, result.stderr + + log = _log(env) + assert "venv --clear" in log, "must rebuild after the in-place install fails" + assert "rebuilding" in result.stderr + assert stamp.read_text() == "0.1.0" + assert "VALCORE_RAN serve" in result.stdout + + +def test_partial_venv_from_an_interrupted_run_is_cleared(env: dict[str, str]) -> None: + """A venv dir with no interpreter is a half-written first run, not something to reuse.""" + venv = Path(env["VALCORE_HOME"]) / "venv" + venv.mkdir(parents=True) + (venv / "lib").mkdir() + + result = _run(env, "serve") + assert result.returncode == 0, result.stderr + + assert "first run" in result.stderr + assert "venv --clear" in _log(env) + assert "VALCORE_RAN serve" in result.stdout def test_missing_version_exits_nonzero(env: dict[str, str]) -> None: