From b20c38d1c18d0b00645e5baf627151cd3ab6be17 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 6 Oct 2026 12:33:59 +0200 Subject: [PATCH] fix: hold dist/ for the whole pipeline run --- scripts/artifacts.py | 28 ++++++++++++++++++++++++++ scripts/common.py | 1 + scripts/pipeline.py | 15 +++++++++----- tests/test_artifact_lock.py | 39 ++++++++++++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 6 deletions(-) diff --git a/scripts/artifacts.py b/scripts/artifacts.py index b901a8a9..7243904d 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -72,6 +72,11 @@ def _strip_timestamps(text: str) -> str: class ArtifactLockBusy(RuntimeError): """Raised when another process already holds the artifact directory.""" +# A run that holds an artifact directory for its whole duration names it +# here for the steps it spawns: they would otherwise be refused by it. +HELD_LOCK_ENV = "RETROBIOS_HELD_ARTIFACT_DIR" + + @contextlib.contextmanager def artifact_lock(directory: str, exclusive: bool = True): """Serialize access to a shared artifact directory across processes. @@ -88,6 +93,9 @@ def artifact_lock(directory: str, exclusive: bool = True): return os.makedirs(directory, exist_ok=True) + if os.environ.get(HELD_LOCK_ENV) == os.path.realpath(directory): + yield + return lock_path = os.path.join(directory, ".lock") mode = fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH with open(lock_path, "w") as handle: @@ -103,6 +111,26 @@ def artifact_lock(directory: str, exclusive: bool = True): finally: fcntl.flock(handle, fcntl.LOCK_UN) + +@contextlib.contextmanager +def hold_artifact_lock(directory: str): + """Hold a directory exclusively across several child steps. + + A lock taken step by step frees the directory between them: another + run can purge the packs one step built before the next step checks + them. The children this process spawns inherit the hold. + """ + with artifact_lock(directory): + previous = os.environ.get(HELD_LOCK_ENV) + os.environ[HELD_LOCK_ENV] = os.path.realpath(directory) + try: + yield + finally: + if previous is None: + os.environ.pop(HELD_LOCK_ENV, None) + else: + os.environ[HELD_LOCK_ENV] = previous + def _build_timestamp(db: dict | None = None) -> str: """Timestamp for generated artifacts. diff --git a/scripts/common.py b/scripts/common.py index f908706c..cb5a3348 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -1733,6 +1733,7 @@ from artifacts import ( # noqa: E402,F401 write_if_changed, ArtifactLockBusy, artifact_lock, + hold_artifact_lock, _TIMESTAMP_PATTERNS, _strip_timestamps, ) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 9c8357cb..d068389e 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -26,6 +26,7 @@ Usage: from __future__ import annotations import argparse +import contextlib import re import subprocess import sys @@ -33,7 +34,7 @@ import time from pathlib import Path sys.path.insert(0, str(Path(__file__).parent)) -from common import ArtifactLockBusy, artifact_lock +from common import ArtifactLockBusy, artifact_lock, hold_artifact_lock def run(cmd: list[str], label: str) -> tuple[bool, str]: @@ -340,11 +341,14 @@ def main(): # A second run on the same output directory is refused before any work: # the database rebuild alone takes minutes, and the reader holding the - # directory would otherwise see the answer only after all of it. - if not args.skip_packs and Path(args.output_dir).is_dir(): + # directory would otherwise see the answer only after all of it. The + # hold lasts until this process exits: taken step by step, it let another + # run purge the packs between the build and the integrity check, which + # then reported every platform SKIP and passed. + held = contextlib.ExitStack() + if not args.skip_packs: try: - with artifact_lock(args.output_dir): - pass + held.enter_context(hold_artifact_lock(args.output_dir)) except ArtifactLockBusy as exc: print(f"ERROR: {exc}") sys.exit(1) @@ -659,6 +663,7 @@ def main(): print(f"{'=' * 60}") print(f" Pipeline {'COMPLETE' if all_ok else 'FINISHED WITH ERRORS'}") print(f"{'=' * 60}") + held.close() sys.exit(0 if all_ok else 1) diff --git a/tests/test_artifact_lock.py b/tests/test_artifact_lock.py index 93d0945c..68b4d468 100644 --- a/tests/test_artifact_lock.py +++ b/tests/test_artifact_lock.py @@ -17,7 +17,7 @@ from pathlib import Path REPO_ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(REPO_ROOT / "scripts")) -from common import ArtifactLockBusy, artifact_lock # noqa: E402 +from common import ArtifactLockBusy, artifact_lock, hold_artifact_lock # noqa: E402 class ArtifactLockTest(unittest.TestCase): @@ -156,5 +156,42 @@ class PackLockCliTest(unittest.TestCase): self.assertIn("is in use by another run", proc.stdout) +class HeldLockTest(unittest.TestCase): + """pipeline.py took the lock step by step: between the pack build and the + integrity check dist/ was free, another run's purge emptied it, and the + check reported every platform SKIP and passed.""" + + def setUp(self): + self.dir = tempfile.mkdtemp() + + def _child_locks(self, env: dict) -> subprocess.CompletedProcess: + code = ( + "import sys; sys.path.insert(0, 'scripts')\n" + "from common import artifact_lock\n" + f"with artifact_lock({self.dir!r}):\n pass\n" + ) + return subprocess.run( + [sys.executable, "-c", code], cwd=str(REPO_ROOT), env=env, + capture_output=True, text=True, timeout=60, check=False, + ) + + def test_a_spawned_step_inherits_the_hold(self): + with hold_artifact_lock(self.dir): + self.assertEqual(self._child_locks(dict(os.environ)).returncode, 0) + + def test_any_other_run_is_refused_for_the_whole_hold(self): + with hold_artifact_lock(self.dir): + env = {k: v for k, v in os.environ.items() if not k.startswith("RETROBIOS_")} + proc = self._child_locks(env) + self.assertNotEqual(proc.returncode, 0) + self.assertIn("ArtifactLockBusy", proc.stderr) + self.assertNotIn("RETROBIOS_HELD_ARTIFACT_DIR", os.environ) + self.assertEqual(self._child_locks(dict(os.environ)).returncode, 0) + + def test_the_pipeline_holds_its_output_for_the_run(self): + source = (REPO_ROOT / "scripts" / "pipeline.py").read_text(encoding="utf-8") + self.assertIn("hold_artifact_lock(args.output_dir)", source) + + if __name__ == "__main__": unittest.main()