From cca439a90328d777d5c5bca17901bb883e710367 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:13:30 +0100 Subject: [PATCH] fix: run refused writers in a copy of the tree --- scripts/generate_site.py | 5 +++ tests/scratchtree.py | 78 ++++++++++++++++++++++++++++++++++ tests/test_artifact_lock.py | 17 +++++--- tests/test_system_selection.py | 44 +++++++++++++++---- 4 files changed, 129 insertions(+), 15 deletions(-) create mode 100644 tests/scratchtree.py diff --git a/scripts/generate_site.py b/scripts/generate_site.py index a49eb98ea..3da470085 100644 --- a/scripts/generate_site.py +++ b/scripts/generate_site.py @@ -56,6 +56,7 @@ from common import ( load_platform_config, load_provenance_snapshots, native_layer, + require_directory, require_yaml, resolution_is_hash_exact, resolve_local_file, @@ -12390,6 +12391,10 @@ def main() -> None: args = parser.parse_args() if args.docs_dir is None: args.docs_dir = str(Path(args.site_dir) / STARLIGHT_CONTENT) + # Before any write: the public assets and the bootstraps went out first, + # and a mistyped folder stopped the run only after them. + require_directory(args.emulators_dir, "--emulators-dir") + require_directory(args.platforms_dir, "--platforms-dir") try: # Two runs over one docs/ delete each other's pages and scratch # files in the sweep, so the second is refused instead. diff --git a/tests/scratchtree.py b/tests/scratchtree.py new file mode 100644 index 000000000..93ec3fdae --- /dev/null +++ b/tests/scratchtree.py @@ -0,0 +1,78 @@ +"""A disposable copy of the checkout, for tests that run a writer and expect +it to refuse. + +Run in the checkout, a regression made the very write the test guards +against: generate_site rewrote website/src/data/published-parts.json and the +FAQ page whatever --docs-dir said, and a pipeline whose lock probe failed +would regenerate database.json and install/. A file another session had +already changed listed the same name in `git diff --name-only` before and +after, so its edits were lost without a signal. + +The copy holds every file git tracks or would track, as the working tree has +it. bios/ and data/ (17 GB and 600 MB of inputs no such test writes) are +links to the checkout's. +""" + +from __future__ import annotations + +import atexit +import hashlib +import os +import shutil +import subprocess +import tempfile +from functools import cache +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +LINKED = ("bios", "data") + + +@cache +def scratch_checkout() -> Path: + """The copy, built once per test process and removed at its exit.""" + root = Path(tempfile.mkdtemp(prefix="checkout-", dir=REPO_ROOT / "tmp" / "tests")) + # A detached worktree with nothing checked out: the scripts that read + # HEAD (the site's bootstrap pin) find this commit. + subprocess.run( + ["git", "worktree", "add", "--quiet", "--detach", "--no-checkout", str(root), "HEAD"], + cwd=REPO_ROOT, capture_output=True, check=True, + ) + atexit.register(_remove, root) + listed = subprocess.run( + ["git", "ls-files", "-z", "--cached", "--others", "--exclude-standard"], + cwd=REPO_ROOT, capture_output=True, check=True, + ).stdout.decode("utf-8") + for name in filter(None, listed.split("\0")): + source = REPO_ROOT / name + if name.split("/", 1)[0] in LINKED or not source.is_file() or source.is_symlink(): + continue + target = root / name + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(source, target) + for name in LINKED: + if (REPO_ROOT / name).exists(): + (root / name).symlink_to(REPO_ROOT / name, target_is_directory=True) + return root + + +def _remove(root: Path) -> None: + subprocess.run(["git", "worktree", "remove", "--force", str(root)], + cwd=REPO_ROOT, capture_output=True, check=False) + shutil.rmtree(root, ignore_errors=True) + + +def snapshot(root: Path) -> dict[str, str]: + """The SHA-1 of every file under root, links not followed. A file + written with the bytes it held is no change: what is caught is a file + whose content a run replaced. Scratch space, caches, bytecode and lock + files are no output.""" + seen = {} + for directory, dirs, files in os.walk(root): + dirs[:] = [d for d in dirs if d != "__pycache__" + and Path(directory, d) not in (root / "tmp", root / ".cache")] + for name in files: + path = Path(directory) / name + if name != ".lock" and not path.is_symlink(): + seen[str(path.relative_to(root))] = hashlib.sha1(path.read_bytes()).hexdigest() + return seen diff --git a/tests/test_artifact_lock.py b/tests/test_artifact_lock.py index 72804b1cb..a75d1b667 100644 --- a/tests/test_artifact_lock.py +++ b/tests/test_artifact_lock.py @@ -25,6 +25,7 @@ from common import ( # noqa: E402 hold_repository_outputs, publish_hold, ) +from tests.scratchtree import scratch_checkout # noqa: E402 def _tempdir(test: unittest.TestCase) -> str: @@ -115,11 +116,13 @@ class PackLockCliTest(unittest.TestCase): self.dir = _tempdir(self) def _run(self, argv: list[str]) -> subprocess.CompletedProcess: + # In a copy: a pipeline that missed the lock regenerated + # database.json and install/ wherever it ran. return subprocess.run( [sys.executable, *argv], capture_output=True, text=True, - cwd=str(REPO_ROOT), + cwd=str(scratch_checkout()), timeout=300, ) @@ -234,11 +237,12 @@ class PipelineHoldsItsFixedOutputs(unittest.TestCase): def test_a_second_pipeline_is_refused_whatever_its_output_dir(self): scratch = _tempdir(self) - with hold_repository_outputs(str(REPO_ROOT)): + checkout = scratch_checkout() + with hold_repository_outputs(str(checkout)): proc = subprocess.run( [sys.executable, "scripts/pipeline.py", "--offline", "--skip-docs", "--output-dir", scratch], - capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=120, + capture_output=True, text=True, cwd=str(checkout), timeout=120, ) self.assertEqual(proc.returncode, 1, proc.stdout + proc.stderr) self.assertIn("another run is writing database.json", proc.stderr) @@ -260,11 +264,12 @@ class StandaloneWritersHonourTheRepositoryHold(unittest.TestCase): env = {k: v for k, v in os.environ.items() if k != "RETROBIOS_HELD_ARTIFACT_DIR"} return subprocess.run( [sys.executable, *argv], capture_output=True, text=True, - cwd=str(REPO_ROOT), timeout=300, env=env, check=False, + cwd=str(scratch_checkout()), timeout=300, env=env, check=False, ) def test_a_writer_of_a_fixed_output_is_refused(self): - with hold_repository_outputs(str(REPO_ROOT)): + # The writers run in a copy, where a missed lock writes nothing of ours. + with hold_repository_outputs(str(scratch_checkout())): for argv in ( ["scripts/generate_db.py", "--bios-dir", "bios", "--output", "database.json"], ["scripts/generate_pack.py", "--all", "--manifest", "--offline", @@ -369,7 +374,7 @@ class SiteGenerationHoldsDocs(unittest.TestCase): with artifact_lock(docs): result = subprocess.run( [sys.executable, "scripts/generate_site.py", "--docs-dir", docs], - cwd=REPO_ROOT, capture_output=True, text=True, timeout=600, + cwd=scratch_checkout(), capture_output=True, text=True, timeout=600, ) self.assertEqual(result.returncode, 1, result.stdout + result.stderr) self.assertIn("in use by another run", result.stderr) diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index c7016c539..44d405f03 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -23,6 +23,7 @@ sys.path.insert(0, str(REPO_ROOT / "scripts")) import common # noqa: E402 import generate_pack as builder # noqa: E402 +from tests.scratchtree import scratch_checkout, snapshot # noqa: E402 class SpellingsAgree(unittest.TestCase): @@ -219,7 +220,7 @@ class AMissingDatabaseIsNamed(unittest.TestCase): with self.subTest(script=script): proc = subprocess.run( [sys.executable, f"scripts/{script}", "--db", "nosuch.json", *extra], - capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=600, + capture_output=True, text=True, cwd=str(scratch_checkout()), timeout=600, check=False, ) self.assertNotEqual(proc.returncode, 0, proc.stdout[-500:]) @@ -254,7 +255,7 @@ class AMissingBiosDirIsRefused(unittest.TestCase): with self.subTest(script=script): proc = subprocess.run( [sys.executable, f"scripts/{script}", "--bios-dir", "nosuch", *extra], - capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=600, + capture_output=True, text=True, cwd=str(scratch_checkout()), timeout=600, check=False, ) self.assertNotEqual(proc.returncode, 0) @@ -307,14 +308,15 @@ class AMissingProfileDirIsRefused(unittest.TestCase): "dat": str(root / "empty.dat")} taking = _scripts_taking("--emulators-dir") self.assertGreater(len(taking), 15) - tracked = ["git", "diff", "--name-only"] - before = subprocess.run(tracked, cwd=REPO_ROOT, capture_output=True, text=True).stdout + # In a copy: run in the checkout, a regression wrote there first. + checkout = scratch_checkout() + before = snapshot(checkout) for path in taking: extra = [arg.format(**values) for arg in self.EXTRA.get(path.stem, [])] with self.subTest(script=path.stem): proc = subprocess.run( [*_command(path), "--emulators-dir", "nosuch", *extra], - capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=600, + capture_output=True, text=True, cwd=str(checkout), timeout=600, check=False, env={**os.environ, "RETROBIOS_OFFLINE": "1"}, ) self.assertNotEqual(proc.returncode, 0, proc.stdout[-500:]) @@ -326,9 +328,11 @@ class AMissingProfileDirIsRefused(unittest.TestCase): ] self.assertEqual(written, []) # Read as empty, generate_site rewrote published-parts.json and the - # FAQ, and sitesources source-paths.json. - after = subprocess.run(tracked, cwd=REPO_ROOT, capture_output=True, text=True).stdout - self.assertEqual(after, before) + # FAQ, and sitesources source-paths.json. The copy compares the + # files themselves: a name `git diff` already listed hid a rewrite. + after = snapshot(checkout) + self.assertEqual(sorted(k for k in before.keys() | after.keys() + if before.get(k) != after.get(k)), []) class AMissingPlatformsDirIsRefused(unittest.TestCase): @@ -367,7 +371,7 @@ class AMissingPlatformsDirIsRefused(unittest.TestCase): with self.subTest(script=path.stem): proc = subprocess.run( [*_command(path), "--platforms-dir", "nosuch", *extra], - capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=600, + capture_output=True, text=True, cwd=str(scratch_checkout()), timeout=600, check=False, env={**os.environ, "RETROBIOS_OFFLINE": "1"}, ) self.assertNotEqual(proc.returncode, 0, proc.stdout[-500:]) @@ -375,6 +379,28 @@ class AMissingPlatformsDirIsRefused(unittest.TestCase): self.assertRegex(proc.stderr, r"ERROR: .*nosuch") +class TheCopyKeepsTheCheckoutOutOfReach(unittest.TestCase): + """The refusal tests above run their writers in a copy.""" + + def test_a_script_in_the_copy_writes_to_the_copy(self): + checkout = scratch_checkout() + proc = subprocess.run( + [sys.executable, "-c", "import sys; sys.path.insert(0, 'scripts'); " + "import generate_site; print(generate_site._REPO_ROOT)"], + capture_output=True, text=True, cwd=str(checkout), timeout=300, check=True, + ) + self.assertEqual(Path(proc.stdout.strip()), checkout.resolve()) + + def test_a_rewrite_of_a_file_is_seen(self): + checkout = scratch_checkout() + page = checkout / "wiki" / "index.md" + before = snapshot(checkout) + original = page.read_bytes() + self.addCleanup(page.write_bytes, original) + page.write_bytes(original + b"\n") + self.assertNotEqual(snapshot(checkout), before) + + class AMissingInputFileIsNamed(unittest.TestCase): """The documented `--from-md5-file missing.txt` ended on a FileNotFoundError at the bottom of a stack trace."""