From b274e2303d5c0e38ee91978ec81a5b3cfa30b788 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:12:15 +0100 Subject: [PATCH 01/23] fix: annotate the five affinity sort keys --- scripts/common.py | 2 +- tests/test_audit_regressions.py | 42 +++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/scripts/common.py b/scripts/common.py index a1f057f23..21ef30930 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -1317,7 +1317,7 @@ def _by_affinity(paths: list[str], file_entry: dict, dest_hint: str) -> list[str owners = _owner_tokens(file_entry) sized = any(file_entry.get(k) for k in ("size", "min_size", "max_size")) - def score(path: str) -> tuple[int, bool, bool, int]: + def score(path: str) -> tuple[int, bool, bool, bool, int]: fits = 0 if sized and os.path.exists(path): fits = int(size_fits(file_entry, os.path.getsize(path))) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index d697062c4..267e4b28c 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -7138,5 +7138,47 @@ class OpenMsxSystemRomsAreFoundBySha1(unittest.TestCase): self.assertEqual(named, []) +class TupleAnnotationsMatchTheirReturns(unittest.TestCase): + """`_by_affinity.score` gained a fifth sort key while its annotation kept + four, so the signature misread the order that sends C-Dogs' + gamecontrollerdb.txt behind a neutral copy.""" + + @staticmethod + def _own_returns(fn: ast.AST): + stack = list(ast.iter_child_nodes(fn)) + while stack: + node = stack.pop() + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + continue + if isinstance(node, ast.Return): + yield node + stack.extend(ast.iter_child_nodes(node)) + + def test_a_fixed_tuple_annotation_counts_its_returned_values(self): + sources = sorted((ROOT / "scripts").rglob("*.py")) + [ROOT / "install.py"] + mismatched = [] + for path in sources: + for fn in ast.walk(ast.parse(path.read_text(encoding="utf-8"))): + if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + hint = fn.returns + if not (isinstance(hint, ast.Subscript) and isinstance(hint.value, ast.Name) + and hint.value.id == "tuple"): + continue + declared = hint.slice.elts if isinstance(hint.slice, ast.Tuple) else [hint.slice] + if any(isinstance(e, ast.Constant) and e.value is Ellipsis for e in declared): + continue + for ret in self._own_returns(fn): + value = ret.value + if (isinstance(value, ast.Tuple) + and not any(isinstance(e, ast.Starred) for e in value.elts) + and len(value.elts) != len(declared)): + mismatched.append( + f"{path.relative_to(ROOT)}:{ret.lineno} " + f"{len(value.elts)} returned, {len(declared)} declared" + ) + self.assertEqual(mismatched, []) + + if __name__ == "__main__": unittest.main() From 1c3462e7fd7fc292de4b5dd432a9704b728e6811 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:13:13 +0100 Subject: [PATCH 02/23] fix: name the python floor in generate_pack --- scripts/generate_pack.py | 14 +++++++++----- scripts/pipeline.py | 4 ++-- tests/test_audit_regressions.py | 18 ++++++++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 4039b1655..ec84947a7 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -12,6 +12,13 @@ and 3-tier storage (embedded/external/user_provided). from __future__ import annotations +import sys + +# The README's first command. Under 3.8 it stopped on a SyntaxError that read +# like a damaged file. The build needs 3.9, as pipeline.py says. +if sys.version_info < (3, 9): + sys.exit(f"generate_pack.py needs Python 3.9 or newer, this is {sys.version.split()[0]}") + from collections.abc import Callable, Iterable, Iterator from artifacts import _build_timestamp, set_plain_mode @@ -23,7 +30,6 @@ import json import os import re import shutil -import sys import tempfile import zipfile from pathlib import Path @@ -5079,10 +5085,8 @@ def inject_manifest(zip_path: str, manifest: dict) -> None: with zipfile.ZipFile(scratch, "a", zipfile.ZIP_DEFLATED) as zf: _write_generated_member(zf, "manifest.json", manifest_json) else: - with ( - zipfile.ZipFile(zip_path, "r") as src, - zipfile.ZipFile(scratch, "w", zipfile.ZIP_DEFLATED) as dst, - ): + with zipfile.ZipFile(zip_path, "r") as src, \ + zipfile.ZipFile(scratch, "w", zipfile.ZIP_DEFLATED) as dst: for item in src.infolist(): if item.filename == "manifest.json": continue diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 798f6b861..1ee445c71 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -57,8 +57,8 @@ import tempfile import time from pathlib import Path -# generate_pack's parenthesized context managers do not compile before -# CPython 3.9, which builds the same database, manifests and packs as 3.12. +# The build scripts need CPython 3.9, which builds the same database, +# manifests and packs as 3.12. # On an older interpreter the run got through the first steps and failed in # the packs, so it stops before step 0. A test compiles every script under # this floor. diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 267e4b28c..d22888989 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -118,6 +118,24 @@ class ReadmeRegressions(unittest.TestCase): self.assertEqual(proc.returncode, 1) self.assertIn("needs Python 3.9", proc.stderr) + def test_the_readme_build_command_names_the_floor(self): + """The README's first command is generate_pack.py. Under 3.8 it + stopped on a raw SyntaxError at a parenthesized `with`.""" + import shutil # noqa: PLC0415 + + python38 = shutil.which("python3.8") + if python38 is None: + if os.environ.get("CI"): + self.fail("CI must provide python3.8") + self.skipTest("no python3.8 on PATH") + proc = subprocess.run( + [python38, "-I", "scripts/generate_pack.py", "--help"], + cwd=ROOT, capture_output=True, text=True, timeout=60, check=False, + ) + self.assertEqual(proc.returncode, 1) + self.assertNotIn("SyntaxError", proc.stderr) + self.assertIn("generate_pack.py needs Python 3.9", proc.stderr) + def test_the_floor_interpreter_imports_every_script(self): """Every module under scripts/ is loaded by a real interpreter of the floor version, as the installer's 3.8 floor is proven by running it. From 6445e5d7d61ecf070a29fe263947b475ca607cef Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:21:53 +0100 Subject: [PATCH 03/23] fix: refuse a missing emulator profile directory --- scripts/common.py | 20 ++++--- scripts/fileless_audit.py | 3 +- scripts/mame_ref_audit.py | 2 + scripts/scraper/arcade_parent_importer.py | 4 +- scripts/scraper/romset_dat_importer.py | 3 +- tests/test_audit_regressions.py | 11 +--- tests/test_system_selection.py | 69 +++++++++++++++++++++++ 7 files changed, 91 insertions(+), 21 deletions(-) diff --git a/scripts/common.py b/scripts/common.py index 21ef30930..7e78ead96 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -43,7 +43,12 @@ def require_directory(path: str, flag: str = "--bios-dir") -> None: raise SystemExit(f"ERROR: {flag} {path} is not a directory") -class DatabaseUnreadable(FileNotFoundError): +class InputMissing(FileNotFoundError): + """An input a command was pointed at does not exist. run_cli prints the + message on one line instead of a stack trace.""" + + +class DatabaseUnreadable(InputMissing): """The database a command was pointed at does not exist or is not JSON.""" @@ -2181,12 +2186,11 @@ def load_emulator_profiles( return {} profiles = {} emu_path = Path(emulators_dir) - if not emu_path.exists(): - print( - f"warning: no emulator profile directory at {emulators_dir}", - file=sys.stderr, - ) - return profiles + if not emu_path.is_dir(): + # Read as "no emulator", a mistyped --emulators-dir built a pack + # without any core file marked OK, and sitesources wrote {} over + # source-paths.json, both exiting 0. + raise InputMissing(f"no emulator profile directory at {emulators_dir}") for f in source_yaml_paths(emu_path): with open(f) as fh: profile = yaml_load(fh) or {} @@ -3571,7 +3575,7 @@ def run_cli(main: Callable[[], int | None]) -> None: try: code = main() sys.stdout.flush() - except DatabaseUnreadable as exc: + except InputMissing as exc: print(f"ERROR: {exc}", file=sys.stderr) sys.exit(1) except BrokenPipeError: diff --git a/scripts/fileless_audit.py b/scripts/fileless_audit.py index deea6b65b..cbb5b1cb1 100644 --- a/scripts/fileless_audit.py +++ b/scripts/fileless_audit.py @@ -25,7 +25,7 @@ from pathlib import Path sys.path.insert(0, os.path.dirname(__file__)) import upstream -from common import suggest_names +from common import require_directory, suggest_names from profile_sync import collect_citations, ref_role, select_views, split_source_ref from safeparse import yaml_load @@ -122,6 +122,7 @@ def main() -> None: args = parser.parse_args() if args.all == bool(args.emulators): parser.error("name profiles or pass --all, not both") + require_directory(args.emulators_dir, "--emulators-dir") known = sorted(path.stem for path in Path(args.emulators_dir).glob("*.yml")) unknown = [name for name in args.emulators if name not in known] diff --git a/scripts/mame_ref_audit.py b/scripts/mame_ref_audit.py index c770bdbaf..387e42fd3 100644 --- a/scripts/mame_ref_audit.py +++ b/scripts/mame_ref_audit.py @@ -27,6 +27,7 @@ from dataclasses import dataclass sys.path.insert(0, os.path.dirname(__file__)) import upstream +from common import require_directory from safeparse import yaml_load from scraper.mame_parser import _MACHINE_MACROS, strip_comments @@ -145,6 +146,7 @@ def main() -> None: help="rewrite the refs the audit can settle", ) args = parser.parse_args() + require_directory(args.emulators_dir, "--emulators-dir") total = 0 for name in args.emulators: diff --git a/scripts/scraper/arcade_parent_importer.py b/scripts/scraper/arcade_parent_importer.py index de14eaf19..ee1eca55a 100644 --- a/scripts/scraper/arcade_parent_importer.py +++ b/scripts/scraper/arcade_parent_importer.py @@ -50,7 +50,7 @@ from dataclasses import dataclass, field from pathlib import Path from ..artifacts import write_text_atomic -from ..common import load_emulator_profiles +from ..common import load_emulator_profiles, run_cli from .romset_dat_importer import ( LISTXML_CHUNK, MAME_LISTXML_URL, @@ -1203,4 +1203,4 @@ def main() -> int: if __name__ == "__main__": - sys.exit(main()) + run_cli(main) diff --git a/scripts/scraper/romset_dat_importer.py b/scripts/scraper/romset_dat_importer.py index 55a081af8..57cb22bfe 100644 --- a/scripts/scraper/romset_dat_importer.py +++ b/scripts/scraper/romset_dat_importer.py @@ -45,6 +45,7 @@ from ..common import ( list_registered_platforms, load_emulator_profiles, load_platform_config, + run_cli, ) from ..dumpcatalog import build_snapshot, load_provenance_snapshots, write_snapshot from .logiqx_parser import LogiqxDat, parse_logiqx @@ -665,4 +666,4 @@ def main() -> int: if __name__ == "__main__": - sys.exit(main()) + run_cli(main) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index d22888989..b1e24bdf8 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -2836,17 +2836,10 @@ class ACheckThatCannotAnswerDoesNotPass(unittest.TestCase): ) def test_a_missing_profile_directory_says_so(self): - import io as _io - from scripts import common as _common - stderr = _io.StringIO() - with contextlib.redirect_stderr(stderr): - profiles = _common.load_emulator_profiles( - str(ROOT / "no-such-emulator-dir") - ) - self.assertEqual(profiles, {}) - self.assertIn("no emulator profile directory", stderr.getvalue()) + with self.assertRaisesRegex(_common.InputMissing, "no emulator profile directory"): + _common.load_emulator_profiles(str(ROOT / "no-such-emulator-dir")) class TestEntryPointsRunEveryClass(unittest.TestCase): diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index 8ab0fe7cd..56c16cba1 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -262,6 +262,75 @@ class AMissingBiosDirIsRefused(unittest.TestCase): self.assertFalse(Path(out).exists()) +def _scripts_taking(flag: str) -> list[Path]: + pattern = re.compile(r'add_argument\(\s*"%s"' % re.escape(flag)) + return [ + path for path in sorted((REPO_ROOT / "scripts").glob("*.py")) + + sorted((REPO_ROOT / "scripts" / "scraper").glob("*.py")) + if pattern.search(path.read_text(encoding="utf-8")) + ] + + +def _command(path: Path) -> list[str]: + if path.parent.name == "scraper": + return [sys.executable, "-m", f"scripts.scraper.{path.stem}"] + return [sys.executable, f"scripts/{path.name}"] + + +class AMissingProfileDirIsRefused(unittest.TestCase): + """A mistyped --emulators-dir read as "no emulator": generate_pack built + a pack without its core files marked OK, verify --strict passed, and + sitesources wrote {} over source-paths.json, every one exiting 0.""" + + EXTRA = { + "arcade_parent_importer": ["--all", "--offline"], + "fileless_audit": ["--all"], + "generate_pack": ["--platform", "retroarch", "--system", "atari-lynx", + "--offline", "--output-dir", "{out}"], + "generate_site": ["--docs-dir", "{out}"], + "generate_truth": ["--all", "--output-dir", "{out}"], + "mame_ref_audit": ["mame"], + "profile_sync": ["--all", "--triage"], + "romset_dat_importer": ["--source", "mame", "--pack", "{dat}", "--output", "{out}/r.json"], + "sitesources": ["--cache-dir", "{cache}", "--output", "{out}/source-paths.json"], + "validate_pr": ["bios/Sony/PlayStation/scph5501.bin"], + "verify": ["--platform", "retroarch", "--strict"], + } + + def test_every_script_taking_an_emulators_dir_refuses_a_missing_one(self): + scratch = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(scratch.cleanup) + root = Path(scratch.name) + (root / "cache").mkdir() + (root / "empty.dat").write_bytes(b"") + values = {"out": str(root / "out"), "cache": str(root / "cache"), + "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 + 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, + check=False, env={**os.environ, "RETROBIOS_OFFLINE": "1"}, + ) + self.assertNotEqual(proc.returncode, 0, proc.stdout[-500:]) + self.assertNotIn("Traceback", proc.stderr, proc.stderr[-1500:]) + self.assertRegex(proc.stderr, r"ERROR: .*nosuch") + written = [ + str(p.relative_to(root)) for p in (root / "out").rglob("*") + if p.is_file() and p.name != ".lock" + ] + 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) + + class StrictVerifyFailsOnACriticalMiss(unittest.TestCase): """verify exited 0 with a required file missing on an md5 platform, so the pipeline printed "verify OK" over a pack the frontend rejects.""" From 27991c84ba329d15dc1d75817c38f97ccb915ac7 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:25:20 +0100 Subject: [PATCH 04/23] fix: refuse a missing platforms directory --- scripts/check_freshness.py | 2 ++ scripts/check_release.py | 3 ++- scripts/common.py | 2 +- scripts/generate_truth.py | 2 ++ scripts/release_notes.py | 2 ++ scripts/validate_pr.py | 2 ++ scripts/validate_site.py | 3 +++ tests/test_system_selection.py | 44 ++++++++++++++++++++++++++++++++++ 8 files changed, 58 insertions(+), 2 deletions(-) diff --git a/scripts/check_freshness.py b/scripts/check_freshness.py index fac97bc71..55fa8a67e 100644 --- a/scripts/check_freshness.py +++ b/scripts/check_freshness.py @@ -58,6 +58,7 @@ from common import ( # noqa: E402 list_registered_platforms, load_emulator_profiles, load_platform_registry, + require_directory, upstream_profile_index, write_text_atomic, yaml_load, @@ -1789,6 +1790,7 @@ def main() -> int: if unknown: parser.error(unknown_areas_message(unknown)) + require_directory(args.platforms_dir, "--platforms-dir") platforms_dir = Path(args.platforms_dir) profiles = load_emulator_profiles(args.emulators_dir, skip_aliases=False) findings: list[Finding] = [] diff --git a/scripts/check_release.py b/scripts/check_release.py index e53e5b560..4fc8c1932 100644 --- a/scripts/check_release.py +++ b/scripts/check_release.py @@ -33,7 +33,7 @@ from pathlib import Path sys.path.insert(0, os.path.dirname(__file__)) from build_port import ARCHIVES from check_release_assets import digests_of -from common import list_release_pack_platforms +from common import list_release_pack_platforms, require_directory from largefiles import LARGE_FILES_REPO from release_record import RECORD, load_record @@ -208,6 +208,7 @@ def main() -> int: ) parser.add_argument("--json", action="store_true", help="JSON output") args = parser.parse_args() + require_directory(args.platforms_dir, "--platforms-dir") record = load_record(args.record) tag = args.tag or record.get("tag") diff --git a/scripts/common.py b/scripts/common.py index 7e78ead96..a8833cecf 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -508,7 +508,7 @@ def list_registered_platforms( """ registry_path = os.path.join(platforms_dir, "_registry.yml") if not os.path.exists(registry_path): - raise FileNotFoundError(f"no platform registry at {registry_path}") + raise InputMissing(f"no platform registry at {registry_path}") with open(registry_path) as f: registry = yaml_load(f) or {} platforms = [] diff --git a/scripts/generate_truth.py b/scripts/generate_truth.py index 37e2728fc..ac5856667 100644 --- a/scripts/generate_truth.py +++ b/scripts/generate_truth.py @@ -21,6 +21,7 @@ from common import ( load_emulator_profiles, load_platform_config, load_target_config, + require_directory, require_platform, require_yaml, truth_target_dir, @@ -72,6 +73,7 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: def main(argv: list[str] | None = None) -> None: args = parse_args(argv) + require_directory(args.platforms_dir, "--platforms-dir") registry_path = os.path.join(args.platforms_dir, "_registry.yml") with open(registry_path) as f: diff --git a/scripts/release_notes.py b/scripts/release_notes.py index 3ff155eee..cf3903cae 100644 --- a/scripts/release_notes.py +++ b/scripts/release_notes.py @@ -26,6 +26,7 @@ from common import ( list_release_pack_platforms, load_platform_config, load_platform_registry, + require_directory, write_text_atomic, ) from siteguides import install_command @@ -162,6 +163,7 @@ def main() -> int: parser.add_argument("--output", required=True, type=Path, help="notes file to write, passed to gh release create --notes-file") args = parser.parse_args() + require_directory(args.platforms_dir, "--platforms-dir") record = release_record.load_record(args.record) if not record.get("packs"): diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index cbc6fa1bd..cae324e65 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -34,6 +34,7 @@ from common import ( load_emulator_profiles, load_platform_config, parse_md5_list, + require_directory, ) from hashing import parse_hash_list from nativemode import digest_algorithm @@ -798,6 +799,7 @@ def main() -> None: if os.path.realpath(os.getcwd()) != os.path.realpath(REPO_ROOT): args.files = [os.path.abspath(f) for f in args.files] work_from_repo_root(args, parser, ["db", "platforms_dir", "emulators_dir"]) + require_directory(args.platforms_dir, "--platforms-dir") files = _files_to_validate(args, parser) if not files: diff --git a/scripts/validate_site.py b/scripts/validate_site.py index c922ad414..6db66bc09 100644 --- a/scripts/validate_site.py +++ b/scripts/validate_site.py @@ -499,6 +499,9 @@ def main() -> None: help="stop listing after this many errors (default: %(default)s)", ) args = parser.parse_args() + from common import require_directory # noqa: PLC0415 + + require_directory(str(args.platforms_dir), "--platforms-dir") try: issues = validate_site(Path(args.site_dir), args.base, args.platforms_dir, args.record) diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index 56c16cba1..3d554abf3 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -331,6 +331,50 @@ class AMissingProfileDirIsRefused(unittest.TestCase): self.assertEqual(after, before) +class AMissingPlatformsDirIsRefused(unittest.TestCase): + """`verify.py --platforms-dir noplat` ended on a FileNotFoundError at the + bottom of a stack trace, and validate_pr read it as no platform at all + and exited 0.""" + + EXTRA = { + "auto_fetch": ["--all", "--dry-run"], + "check_release": ["--tag", "v1"], + "diff_truth": ["--all"], + "export_native": ["--all", "--output-dir", "{out}"], + "generate_pack": ["--platform", "retroarch", "--offline", "--output-dir", "{out}"], + "generate_site": ["--docs-dir", "{out}"], + "generate_truth": ["--all", "--output-dir", "{out}"], + "release_notes": ["--whats-new", "{notes}", "--output", "{out}/notes.md"], + "release_record": ["{out}", "--tag", "v1"], + "romset_dat_importer": ["--source", "mame", "--pack", "{dat}", "--output", "{out}/r.json"], + "validate_pr": ["bios/Sony/PlayStation/scph5501.bin"], + "validate_site": ["--site-dir", "{out}"], + "verify": ["--platform", "retroarch"], + } + + def test_every_script_taking_a_platforms_dir_refuses_a_missing_one(self): + scratch = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(scratch.cleanup) + root = Path(scratch.name) + (root / "empty.dat").write_bytes(b"") + (root / "notes.md").write_text("Notes.\n") + values = {"out": str(root / "out"), "dat": str(root / "empty.dat"), + "notes": str(root / "notes.md")} + taking = _scripts_taking("--platforms-dir") + self.assertGreater(len(taking), 15) + 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), "--platforms-dir", "nosuch", *extra], + capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=600, + check=False, env={**os.environ, "RETROBIOS_OFFLINE": "1"}, + ) + self.assertNotEqual(proc.returncode, 0, proc.stdout[-500:]) + self.assertNotIn("Traceback", proc.stderr, proc.stderr[-1500:]) + self.assertRegex(proc.stderr, r"ERROR: .*nosuch") + + class StrictVerifyFailsOnACriticalMiss(unittest.TestCase): """verify exited 0 with a required file missing on an md5 platform, so the pipeline printed "verify OK" over a pack the frontend rejects.""" From 7d33bd6e8e8e14c0d0703c9ca497c045850b9508 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:26:50 +0100 Subject: [PATCH 05/23] fix: count core data trees apart from baseline --- scripts/generate_pack.py | 16 +++++++++++++--- tests/test_core_data_trees.py | 25 +++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index ec84947a7..87e5cadb6 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -608,6 +608,7 @@ def _report_pack_counts( core_count: int, source: str, verification_mode: str, + tree_count: int = 0, ) -> None: """Print the line the pipeline's consistency check reads. @@ -639,10 +640,11 @@ def _report_pack_counts( f"{core_count} from emulator profiles [{verification_mode}]" ) else: - baseline = total_files - core_count + baseline = total_files - core_count - tree_count + trees = f"{tree_count} in core data trees + " if tree_count else "" print( f" {zip_path}: {total_files} files packed ({baseline} baseline + " - f"{core_count} from cores), {', '.join(parts)} [{verification_mode}]" + f"{trees}{core_count} from cores), {', '.join(parts)} [{verification_mode}]" ) def _select_variants( @@ -1314,11 +1316,19 @@ def generate_pack( core_trees = [t for t in core_trees if not _extras_for_systems([t], owned)] elif system_filter: core_trees = _extras_for_systems(core_trees, system_filter) + # Counted apart: the trees a core reads are not on the platform's + # list. MyMinUI declares nothing and its line read "6944 baseline". total_files += _pack_data_directories( zf, pack_systems, data_registry, platform_name, base_dest, flatten, case_insensitive, seen_destinations, seen_lower, + seen_parents, required_only=required_only, + ) + tree_count = _pack_data_directories( + zf, {}, data_registry, platform_name, base_dest, + flatten, case_insensitive, seen_destinations, seen_lower, seen_parents, required_only=required_only, core_trees=core_trees, ) + total_files += tree_count # README.txt for users: personalized step-by-step per platform num_systems = len(pack_systems) @@ -1357,7 +1367,7 @@ def generate_pack( _report_pack_counts( zip_path, file_status, total_files, core_count, source, - verification_mode, + verification_mode, tree_count=tree_count, ) if cut is not None: _report_left_out( diff --git a/tests/test_core_data_trees.py b/tests/test_core_data_trees.py index 6cc8c8ca0..b5681263c 100644 --- a/tests/test_core_data_trees.py +++ b/tests/test_core_data_trees.py @@ -165,5 +165,30 @@ class AmberelecCarriesTheMachines(unittest.TestCase): ) +class TheCountLineKeepsTreesOutOfTheBaseline(unittest.TestCase): + """MyMinUI declares no file, and its pack line read "6944 baseline": the + fbneo trees its cores read were counted as the platform's list.""" + + def test_a_platform_declaring_nothing_has_no_baseline(self) -> None: + import contextlib + import io + + previous = os.getcwd() + os.chdir(REPO_ROOT) + self.addCleanup(os.chdir, previous) + db = common.load_database("database.json") + record = builder.PackRecord() + out = io.StringIO() + with contextlib.redirect_stdout(out): + builder.generate_pack( + "myminui", "platforms", db, "bios", "tmp", offline=True, record=record, + data_registry=common.load_data_dir_registry("platforms"), + ) + line = next(l for l in out.getvalue().splitlines() if "files packed" in l) + self.assertIn("(0 baseline + ", line) + if record.trees: + self.assertIn(f"+ {len(record.trees)} in core data trees + ", line) + + if __name__ == "__main__": unittest.main() From f96e7c10f400e934fead8375a28a80a987f2b079 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:27:29 +0100 Subject: [PATCH 06/23] fix: name a missing --from-md5-file --- scripts/generate_pack.py | 3 +++ tests/test_system_selection.py | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 87e5cadb6..196bda7ec 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -3573,6 +3573,9 @@ def _main(held: contextlib.ExitStack) -> None: except ValueError as exc: print(f"ERROR: {exc}", file=sys.stderr) sys.exit(1) + except OSError as exc: + print(f"ERROR: --from-md5-file {args.from_md5_file}: {exc.strerror}", file=sys.stderr) + sys.exit(1) if not hashes: print( f"ERROR: no hash in the input. Accepted: {ACCEPTED_HASHES}", diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index 3d554abf3..4811bcf25 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -375,6 +375,29 @@ class AMissingPlatformsDirIsRefused(unittest.TestCase): self.assertRegex(proc.stderr, r"ERROR: .*nosuch") +class AMissingInputFileIsNamed(unittest.TestCase): + """The documented `--from-md5-file missing.txt` ended on a + FileNotFoundError at the bottom of a stack trace.""" + + def _refused(self, argv: list[str], message: str) -> None: + proc = subprocess.run( + [sys.executable, *argv], capture_output=True, text=True, + cwd=str(REPO_ROOT), timeout=600, check=False, + env={**os.environ, "RETROBIOS_OFFLINE": "1"}, + ) + self.assertNotEqual(proc.returncode, 0, proc.stdout[-500:]) + self.assertNotIn("Traceback", proc.stderr, proc.stderr[-1500:]) + self.assertIn(message, proc.stderr) + + def test_a_missing_hash_list_is_named(self): + with tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") as tmp: + self._refused( + ["scripts/generate_pack.py", "--platform", "batocera", "--from-md5-file", + "nosuch.txt", "--offline", "--output-dir", tmp], + "ERROR: --from-md5-file nosuch.txt: No such file or directory", + ) + + class StrictVerifyFailsOnACriticalMiss(unittest.TestCase): """verify exited 0 with a required file missing on an md5 platform, so the pipeline printed "verify OK" over a pack the frontend rejects.""" From 74055a9f31b371b32d52e70fc92d9b5baec53d8f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:27:36 +0100 Subject: [PATCH 07/23] fix: name a missing data directory registry --- scripts/check_buildbot_system.py | 6 +++++- scripts/refresh_data_dirs.py | 4 ++-- tests/test_system_selection.py | 6 ++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/scripts/check_buildbot_system.py b/scripts/check_buildbot_system.py index 9035aeda0..d25acadf3 100644 --- a/scripts/check_buildbot_system.py +++ b/scripts/check_buildbot_system.py @@ -15,13 +15,14 @@ from __future__ import annotations import argparse import json import logging +import os import subprocess import sys import urllib.error import urllib.parse import urllib.request from pathlib import Path -from common import yaml_load +from common import InputMissing, yaml_load log = logging.getLogger(__name__) @@ -194,6 +195,9 @@ def main() -> None: help="the data directory registry to check (default: %(default)s)", ) args = parser.parse_args() + # Read after the buildbot answered, a mistyped path ended on a trace. + if not os.path.isfile(args.registry): + raise InputMissing(f"Registry not found: {args.registry}") report = check(args.registry) diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index 64568a753..25d820f08 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -27,7 +27,7 @@ import zipfile from collections.abc import Iterator from pathlib import Path from artifacts import file_lock as _file_lock, set_plain_mode -from common import github_token, platform_arg, require_platform, suggest_names, yaml_load +from common import InputMissing, github_token, platform_arg, require_platform, suggest_names, yaml_load try: import yaml @@ -53,7 +53,7 @@ def load_registry(registry_path: str = DEFAULT_REGISTRY) -> dict[str, dict]: raise ImportError("PyYAML required: pip install pyyaml") path = Path(registry_path) if not path.exists(): - raise FileNotFoundError(f"Registry not found: {registry_path}") + raise InputMissing(f"Registry not found: {registry_path}") with open(path) as f: data = yaml_load(f) or {} return data.get("data_directories", {}) diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index 4811bcf25..48ad9eefa 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -397,6 +397,12 @@ class AMissingInputFileIsNamed(unittest.TestCase): "ERROR: --from-md5-file nosuch.txt: No such file or directory", ) + def test_a_missing_data_directory_registry_is_named(self): + for argv in (["scripts/refresh_data_dirs.py", "--registry", "nosuch.yml", "--dry-run"], + ["scripts/check_buildbot_system.py", "--registry", "nosuch.yml"]): + with self.subTest(script=argv[0]): + self._refused(argv, "ERROR: Registry not found: nosuch.yml") + class StrictVerifyFailsOnACriticalMiss(unittest.TestCase): """verify exited 0 with a required file missing on an md5 platform, so From 2cb9a5a495000714eaa2dbf607eccbec274199ab Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:28:14 +0100 Subject: [PATCH 08/23] fix: an unanswered buildbot check exits 1 --- scripts/check_buildbot_system.py | 15 +++++++++++---- tests/test_audit_regressions.py | 31 +++++++++++++++++++++++-------- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/scripts/check_buildbot_system.py b/scripts/check_buildbot_system.py index d25acadf3..f81f47c2f 100644 --- a/scripts/check_buildbot_system.py +++ b/scripts/check_buildbot_system.py @@ -213,10 +213,12 @@ def main() -> None: sys.exit(1) # Unreachable upstream means the freshness question was not answered, and - # a zero exit says it was answered "fresh". A new asset nobody tracks, or - # a tracked one the buildbot dropped, is a change to act on: a zero exit - # there let the online pipeline call the system directory current. - if report.get("error") or pending(report): + # a zero exit says it was answered "fresh". An asset whose HEAD went + # unanswered is the same question left open, as check_freshness counts + # UNKNOWN. A new asset nobody tracks, or a tracked one the buildbot + # dropped, is a change to act on: a zero exit there let the online + # pipeline call the system directory current. + if report.get("error") or pending(report) or unanswered(report): raise SystemExit(1) @@ -225,6 +227,11 @@ def pending(report: dict) -> list[dict]: return [e for e in report.get("entries", []) if e["status"] in ("NEW", "STALE")] +def unanswered(report: dict) -> list[dict]: + """Tracked entries whose remote version could not be read.""" + return [e for e in report.get("entries", []) if e["status"] == "UNKNOWN"] + + if __name__ == "__main__": from common import run_cli diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index b1e24bdf8..600cdf19b 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -2826,14 +2826,29 @@ class ACheckThatCannotAnswerDoesNotPass(unittest.TestCase): self.assertEqual(unsatisfied, ["bios/absent.bin"]) def test_an_unreachable_buildbot_is_not_a_fresh_verdict(self): - source = (ROOT / "scripts" / "check_buildbot_system.py").read_text( - encoding="utf-8" - ) - self.assertIn( - 'if report.get("error") or pending(report):', - source.split("def main(")[-1], - "main() ignores the error the report carries", - ) + """Every HEAD unanswered printed "16 unknown" and exited 0, so the + online pipeline printed check_buildbot OK.""" + sys.path.insert(0, str(ROOT / "scripts")) + import check_buildbot_system as cbs # noqa: PLC0415 + + registry = str(ROOT / "platforms" / "_data_dirs.yml") + tracked = cbs.load_tracked_entries(registry) + self.assertTrue(tracked) + cases = { + "index unreachable": mock.patch.object( + cbs, "fetch_index", side_effect=OSError("down")), + "every HEAD unanswered": mock.patch.object( + cbs, "fetch_index", return_value=set(tracked)), + } + for label, index in cases.items(): + with self.subTest(label), index, \ + mock.patch.object(cbs, "get_remote_etag", return_value=None), \ + mock.patch.object(sys, "argv", ["check_buildbot_system.py", + "--registry", registry]), \ + contextlib.redirect_stdout(io.StringIO()), \ + self.assertRaises(SystemExit) as stop: + cbs.main() + self.assertEqual(stop.exception.code, 1) def test_a_missing_profile_directory_says_so(self): from scripts import common as _common From 46c651f0b024391d84a5f6bf3f94b1463f21fd81 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:29:12 +0100 Subject: [PATCH 09/23] fix: check every variant pack in step 6 --- scripts/pipeline.py | 36 ++++++++++++++++++--------------- tests/test_audit_regressions.py | 14 +++++++++++++ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 1ee445c71..0b020810e 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -790,6 +790,25 @@ class _Skipped: SKIPPED = _Skipped() +def integrity_command(args: argparse.Namespace) -> list[str]: + """Step 6: check the packs step 4 built, under the same expectation.""" + cmd = [ + sys.executable, "scripts/generate_pack.py", "--all", "--verify-packs", + "--output-dir", args.output_dir, + ] + if args.include_archived: + cmd.append("--include-archived") + # Without the target a targeted run reported every system the target + # removed as missing from a pack that was exactly right. Without + # --all-variants the five narrowed packs were never opened, and an empty + # _Platform pack passed. + if args.target: + cmd.extend(["--target", args.target]) + if args.all_variants: + cmd.append("--all-variants") + return cmd + + def _refuse_unapplied_flags( parser: argparse.ArgumentParser, args: argparse.Namespace ) -> None: @@ -1330,22 +1349,7 @@ def main() -> None: skip_notice("pack_integrity", "no full pack built") results["pack_integrity"] = SKIPPED elif not args.skip_packs: - integrity_cmd = [ - sys.executable, - "scripts/generate_pack.py", - "--all", - "--verify-packs", - "--output-dir", - args.output_dir, - ] - if args.include_archived: - integrity_cmd.append("--include-archived") - # Step 4 built with the target, so step 6 has to check against the - # same expectation. Without it a targeted run reported every system - # the target removed as missing from a pack that was exactly right. - if args.target: - integrity_cmd.extend(["--target", args.target]) - ok, _ = run(integrity_cmd, STEP_LABELS["pack_integrity"]) + ok, _ = run(integrity_command(args), STEP_LABELS["pack_integrity"]) results["pack_integrity"] = ok all_ok = all_ok and ok else: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 600cdf19b..5d59c03e5 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -966,6 +966,20 @@ class CatalogRatioRegressions(unittest.TestCase): class PipelineRegressions(unittest.TestCase): + def test_integrity_checks_the_variants_step_4_built(self): + """Step 6 ran --verify-packs without --all-variants: the five + narrowed packs were never opened and an empty _Platform passed.""" + import argparse # noqa: PLC0415 + + def command(**flags) -> list[str]: + base = {"output_dir": "dist", "include_archived": False, + "target": None, "all_variants": False} + return pipeline.integrity_command(argparse.Namespace(**{**base, **flags})) + + self.assertIn("--all-variants", command(all_variants=True)) + self.assertNotIn("--all-variants", command()) + self.assertEqual(command(target="switch")[-2:], ["--target", "switch"]) + def test_pack_parser_removes_source_metadata(self): output = "\n".join( [ From a6d7c92900819ca6f8802863b972f45484f448e6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:29:41 +0100 Subject: [PATCH 10/23] fix: refuse --full-narrowings without full pack --- scripts/pipeline.py | 6 +++++- tests/test_audit_regressions.py | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 0b020810e..b8593c307 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -816,7 +816,7 @@ def _refuse_unapplied_flags( --all-variants builds every source itself, so a --source beside it is dropped; --skip-packs builds none, so both are, and step 5 does not run - for --full-narrowings to widen. + for --full-narrowings to widen, nor does it without a full pack. """ # --source defaults to None so a typed `--source full` is seen: compared # with its default value, it went through beside the modes that refuse it. @@ -829,6 +829,10 @@ def _refuse_unapplied_flags( ("--full-narrowings", args.full_narrowings)): if on: parser.error(f"{flag} is incompatible with --skip-packs") + # Step 5 compares the full pack, and --source platform or truth builds + # none: the comparison is skipped and the flag had no effect. + if args.full_narrowings and source_given and args.source != "full": + parser.error(f"--full-narrowings is incompatible with --source {args.source}") if args.source is None: args.source = "full" diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 5d59c03e5..fdcf30854 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -4185,6 +4185,8 @@ class EveryToolRefusesAFlagItWouldDrop(unittest.TestCase): ["--offline", "--all-variants", "--source", "platform"], ["--offline", "--all-variants", "--source", "full"], ["--offline", "--skip-packs", "--source", "full"], + ["--offline", "--source", "truth", "--full-narrowings"], + ["--offline", "--source", "platform", "--full-narrowings"], ], } From 9b08925dd05d8d511da6ebb12e7103dfbc33feff Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:30:47 +0100 Subject: [PATCH 11/23] fix: finalize only the packs this run built --- scripts/generate_pack.py | 27 ++++++++++++++++++++++++--- tests/test_finalize_pack_names.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 196bda7ec..af2306a24 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -3037,6 +3037,8 @@ def _run_platform_packs( # Per pack, the systems each of its files is for, written into its # manifest.json once the pack is verified (pack_systems). systems_for: dict[str, dict] = {} + # The packs this run writes, the only ones it verifies and rewrites. + built: set[str] = set() failed: list[str] = [] for source, required_only in variants: @@ -3137,7 +3139,12 @@ def _run_platform_packs( # the output directory already held and exit 0. if not _produced(args.split, zip_path, zip_paths): failed.append(representative) - elif zip_path and not system_filter: + continue + built.update( + os.path.relpath(path, args.output_dir) + for path in ([zip_path] if zip_path else zip_paths) + ) + if zip_path and not system_filter: # The install list of the same choices names each file's # systems: the pack records them, so a pack alone (a # handheld without Wi-Fi) can be narrowed by system. @@ -3160,6 +3167,7 @@ def _run_platform_packs( db, platforms_dir=args.platforms_dir, emulators_dir=args.emulators_dir, + built=built, pack_systems_by_name=systems_for, skip_conformance=skip_conf, data_registry=data_registry, @@ -3176,6 +3184,10 @@ def _run_platform_packs( db, platforms_dir=args.platforms_dir, emulators_dir=args.emulators_dir, + built={ + os.path.basename(name) for name in built + if os.path.dirname(name) == entry + }, skip_conformance=True, data_registry=data_registry, regions=getattr(args, "regions", None), @@ -5294,12 +5306,18 @@ def verify_and_finalize_packs( emulators_dir: str = "emulators", no_game_data: bool = False, pack_systems_by_name: dict[str, dict] | None = None, + built: Iterable[str] | None = None, ) -> bool: """Verify all packs, inject manifests, generate SHA256SUMS. pack_systems_by_name gives, per pack file name, the systems its manifest.json records (pack_systems). + ``built`` names the packs this run wrote, and only those are verified + and get a manifest. A pack another run left in the directory was + reopened, recompressed and given a manifest without its pack_systems, + which made the installer refuse to narrow it. None takes every pack. + Two-stage verification: 1. Hash check against database.json (integrity) 2. Extract + verify against platform config (conformance) @@ -5316,14 +5334,17 @@ def verify_and_finalize_packs( expected = _expected_pack_names( registered, platforms_dir, regions, target_name, no_game_data=no_game_data ) + packs = _pack_archives(output_dir) + if built is not None: + packs = [name for name in packs if name in set(built)] pack_to_platform: dict[str, list[str]] = {} - for name in _pack_archives(output_dir): + for name in packs: for pname in registered: if name in expected[pname]: pack_to_platform.setdefault(name, []).append(pname) failing: list[str] = [] - for name in _pack_archives(output_dir): + for name in packs: zip_path = os.path.join(output_dir, name) # Stage 1: database integrity diff --git a/tests/test_finalize_pack_names.py b/tests/test_finalize_pack_names.py index c443f4345..fe7c8e686 100644 --- a/tests/test_finalize_pack_names.py +++ b/tests/test_finalize_pack_names.py @@ -31,5 +31,35 @@ class FinalizeByExactName(unittest.TestCase): self.assertTrue(ok) +class APackFromAnotherRunIsLeftAlone(unittest.TestCase): + """A one-platform build verified, recompressed and gave a new manifest to + every pack in the directory: MiSTer's lost its pack_systems fields after a + MinUI run, and a bizhawk run rewrote 52 GB of packs it never built.""" + + def test_a_second_platform_does_not_rewrite_the_first(self): + import json + import subprocess + + with tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") as tmp: + def build(platform: str) -> str: + proc = subprocess.run( + [sys.executable, "scripts/generate_pack.py", "--platform", platform, + "--offline", "--output-dir", tmp], + cwd=REPO_ROOT, capture_output=True, text=True, timeout=600, check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stdout[-2000:] + proc.stderr[-2000:]) + return proc.stdout + + build("misterfpga") + (first,) = Path(tmp).glob("MiSTer*_BIOS_Pack.zip") + before = first.read_bytes() + output = build("minui") + self.assertNotIn(first.name, output) + self.assertEqual(first.read_bytes(), before) + with zipfile.ZipFile(first) as zf: + manifest = json.loads(zf.read("manifest.json")) + self.assertIn("install_list", manifest) + + if __name__ == "__main__": unittest.main() From af3c90e4483336d5b452205003e02229198ca7c6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:31:28 +0100 Subject: [PATCH 12/23] fix: unwind a terminated run to drop its scratch --- scripts/common.py | 13 ++++++++++++ tests/test_atomic_artifacts.py | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/scripts/common.py b/scripts/common.py index a8833cecf..104c7f951 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -15,6 +15,7 @@ import functools import json import os import re +import signal import sys import zipfile from pathlib import Path @@ -3564,6 +3565,10 @@ from dumpcatalog import ( # noqa: E402,F401 ) +def _exit_on_signal(signum: int, _frame: object) -> None: + sys.exit(128 + signum) + + def run_cli(main: Callable[[], int | None]) -> None: """Run a command-line entry point to its exit status. @@ -3571,7 +3576,15 @@ def run_cli(main: Callable[[], int | None]) -> None: and the next write died on a BrokenPipeError traceback. Output nobody reads any more ends the run quietly, with the status a shell gives a writer whose reader left. + + SIGTERM (timeout, kill) and SIGHUP (a closed terminal) end the process + without unwinding by default: a pack build left its multi-GB hidden + .partial behind. Raised as SystemExit, they run the cleanup a failed + write runs, and the status stays the one a shell gives the signal. """ + for name in ("SIGTERM", "SIGHUP"): + if hasattr(signal, name): + signal.signal(getattr(signal, name), _exit_on_signal) try: code = main() sys.stdout.flush() diff --git a/tests/test_atomic_artifacts.py b/tests/test_atomic_artifacts.py index ad3868035..f506b7573 100644 --- a/tests/test_atomic_artifacts.py +++ b/tests/test_atomic_artifacts.py @@ -296,5 +296,43 @@ class RestoreSkipsDownloadsInProgress(unittest.TestCase): self.assertIn(str(cache / "done.bin"), index.values()) +class ATerminatedBuildLeavesNoScratch(unittest.TestCase): + """`timeout 120 generate_pack.py --platform RetroArch` left a 3.4 GB + hidden .partial in dist/: SIGTERM ended the process without running the + cleanup a failed write runs.""" + + CHILD = ( + "import sys, time\n" + "sys.path.insert(0, 'scripts')\n" + "from common import run_cli\n" + "from generate_pack import _writing_pack\n" + "def main():\n" + " with _writing_pack(sys.argv[1]) as zf:\n" + " zf.writestr('a.bin', b'x')\n" + " print('writing', flush=True)\n" + " time.sleep(60)\n" + "run_cli(main)\n" + ) + + def test_sigterm_removes_the_partial_pack(self): + import signal # noqa: PLC0415 + import subprocess # noqa: PLC0415 + + if not hasattr(signal, "SIGTERM") or os.name != "posix": + self.skipTest("POSIX signals") + with tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") as tmp: + pack = Path(tmp) / "Demo_BIOS_Pack.zip" + child = subprocess.Popen( + [sys.executable, "-c", self.CHILD, str(pack)], cwd=REPO_ROOT, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + ) + self.assertEqual(child.stdout.readline().strip(), "writing") + self.assertTrue(list(Path(tmp).glob(".*.partial"))) + child.send_signal(signal.SIGTERM) + child.communicate(timeout=60) + self.assertEqual(child.returncode, 128 + signal.SIGTERM) + self.assertEqual(sorted(p.name for p in Path(tmp).iterdir()), []) + + if __name__ == "__main__": unittest.main() From cb17145665ab6345e2104edeab0951be9dbc6f2f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:32:26 +0100 Subject: [PATCH 13/23] fix: install setuptools for the packaging guard --- .github/workflows/validate.yml | 3 ++- CONTRIBUTING.md | 2 +- pyproject.toml | 4 ++++ scripts/generate_readme.py | 2 +- tests/test_audit_regressions.py | 4 ++++ 5 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index eca666728..c1e18a9e4 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -223,8 +223,9 @@ jobs: # heading guard's premise, and skipped on every run without it. pyte # reads the installer's full screen back in its end-to-end tests, # hypothesis fuzzes its key decoder, and vermin holds install.py to 3.8. + # setuptools reads pyproject.toml for the packaging guard. - name: Install dependencies - run: pip install pyyaml "jsonschema[format-nongpl]==4.26.0" "markdown==3.10.2" "pyte==0.8.2" "hypothesis==6.170.8" "vermin==1.9.2" + run: pip install pyyaml "jsonschema[format-nongpl]==4.26.0" "markdown==3.10.2" "pyte==0.8.2" "hypothesis==6.170.8" "vermin==1.9.2" "setuptools==80.9.0" # The full screen also runs under the 3.8 floor: the end-to-end tests # start install.py with the interpreter that runs them. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ae2c25459..9fe1b0e7c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -68,7 +68,7 @@ validation above is its check. For code, profiles or platform files, these are the checks CI runs on a pull request (Python 3.12 or newer): ```bash -pip install pyyaml "jsonschema[format-nongpl]" markdown pyte hypothesis vermin +pip install pyyaml "jsonschema[format-nongpl]" markdown pyte hypothesis vermin setuptools python scripts/validate_pr.py --changed # the BIOS files the branch adds python scripts/validate_schemas.py --source-only # profiles and platform files python scripts/profile_sync.py --added --base upstream/main --strict # each profile the branch adds diff --git a/pyproject.toml b/pyproject.toml index bfb24f9c8..508eecc34 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,6 +33,10 @@ dev = [ "pyte>=0.8.2", "hypothesis>=6.100", "vermin>=1.9", + # The packaging guard reads this file through setuptools, which pip + # installs only in its isolated build environment and 3.12 no longer + # bundles: without it the guard skipped on every CI run. + "setuptools>=61", ] # The site is built by Astro Starlight from website/package.json and its # lockfile (npm ci --prefix website): no Python package builds it. diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index 006aab519..2d02f93da 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -974,7 +974,7 @@ validation above is its check. For code, profiles or platform files, these are the checks CI runs on a pull request (Python 3.12 or newer): ```bash -pip install pyyaml "jsonschema[format-nongpl]" markdown pyte hypothesis vermin +pip install pyyaml "jsonschema[format-nongpl]" markdown pyte hypothesis vermin setuptools python scripts/validate_pr.py --changed # the BIOS files the branch adds python scripts/validate_schemas.py --source-only # profiles and platform files python scripts/profile_sync.py --added --base upstream/main --strict # each profile the branch adds diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index fdcf30854..647510864 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -2766,6 +2766,10 @@ class TheProjectInstallsWithPip(unittest.TestCase): import importlib.util # noqa: PLC0415 if importlib.util.find_spec("setuptools") is None: + # The dev extra and CI install it: skipped there, the guard held + # nothing on any run. + if os.environ.get("CI"): + self.fail("CI must install setuptools") self.skipTest("setuptools is not installed") program = ( "from setuptools.dist import Distribution\n" From 96f100ac225e36a34b4de932ea79add9de35e897 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:33:17 +0100 Subject: [PATCH 14/23] fix: stop coverage on a missing input directory --- scripts/generate_readme.py | 5 +++++ tests/test_coverage_cache.py | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index 2d02f93da..c6a3b2143 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -20,6 +20,7 @@ from pathlib import Path sys.path.insert(0, os.path.dirname(__file__)) from common import ( + InputMissing, compute_composition, count_catalog_matched, declared_root, @@ -211,6 +212,10 @@ def platform_coverages( coverages[name] = compute_coverage( name, platforms_dir, db, data_registry, emulators_dir=emulators_dir, ) + except InputMissing: + # A missing profile or platform directory is no platform + # left out: it emptied every one and cached the result. + raise except FileNotFoundError as exc: print(f" {name}: skipped ({exc})", file=sys.stderr) COVERAGE_CACHE_DIR.mkdir(parents=True, exist_ok=True) diff --git a/tests/test_coverage_cache.py b/tests/test_coverage_cache.py index b9b383980..95f7a99d8 100644 --- a/tests/test_coverage_cache.py +++ b/tests/test_coverage_cache.py @@ -95,6 +95,21 @@ class CoverageCache(unittest.TestCase): self._run() self.assertEqual(len(list((self.work / "cache").glob("*.pickle"))), 1) + def test_a_missing_input_directory_stops_the_run(self): + """A missing --emulators-dir skipped every platform as if it had no + YAML, then replaced the cache with the empty result.""" + self._run() + kept = sorted((self.work / "cache").glob("*.pickle")) + + def missing(*_args, **_kwargs): + raise generate_readme.InputMissing("no emulator profile directory at nosuch") + + with mock.patch.object(generate_readme, "compute_coverage", missing), \ + mock.patch.object(generate_readme, "_coverage_inputs", return_value="other"), \ + self.assertRaises(generate_readme.InputMissing): + self._run() + self.assertEqual(sorted((self.work / "cache").glob("*.pickle")), kept) + if __name__ == "__main__": unittest.main() From 286e39d199ff7af6157b7465de9cf1436e1d5d4a Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:34:17 +0100 Subject: [PATCH 15/23] fix: let two generators purge the coverage cache --- scripts/generate_readme.py | 7 +++++-- tests/test_coverage_cache.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index c6a3b2143..0f5476db2 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -219,13 +219,16 @@ def platform_coverages( except FileNotFoundError as exc: print(f" {name}: skipped ({exc})", file=sys.stderr) COVERAGE_CACHE_DIR.mkdir(parents=True, exist_ok=True) - for stale in COVERAGE_CACHE_DIR.glob("*.pickle"): - stale.unlink() scratch = cache.with_suffix(f".{os.getpid()}.tmp") with scratch.open("wb") as handle: pickle.dump({"platforms": names, "coverages": coverages}, handle, protocol=pickle.HIGHEST_PROTOCOL) scratch.replace(cache) + # generate_site and generate_readme both purge here. One unlinking + # what the other had just listed ended it on FileNotFoundError. + for stale in COVERAGE_CACHE_DIR.glob("*.pickle"): + if stale != cache: + stale.unlink(missing_ok=True) ordered = {name: coverages[name] for name in platform_names if name in coverages} if report: for name in names: diff --git a/tests/test_coverage_cache.py b/tests/test_coverage_cache.py index 95f7a99d8..8c2b56175 100644 --- a/tests/test_coverage_cache.py +++ b/tests/test_coverage_cache.py @@ -95,6 +95,23 @@ class CoverageCache(unittest.TestCase): self._run() self.assertEqual(len(list((self.work / "cache").glob("*.pickle"))), 1) + def test_a_purge_racing_another_generator_does_not_fail(self): + """generate_site and generate_readme purge the same directory: one + unlinking a pickle the other had listed ended on FileNotFoundError.""" + cache_dir = self.work / "cache" + cache_dir.mkdir() + ghost = cache_dir / "gone.pickle" + real_glob = Path.glob + + def listing_a_vanished_file(path, pattern): + found = list(real_glob(path, pattern)) + return [*found, ghost] if path == cache_dir else found + + with mock.patch.object(Path, "glob", listing_a_vanished_file): + coverages = self._run() + self.assertEqual(sorted(coverages), ["alpha", "beta"]) + self.assertEqual(len(list(cache_dir.glob("*.pickle"))), 1) + def test_a_missing_input_directory_stops_the_run(self): """A missing --emulators-dir skipped every platform as if it had no YAML, then replaced the cache with the empty result.""" From 639204712bc9774bb690f9b12ec38ce995bc8e37 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:35:53 +0100 Subject: [PATCH 16/23] fix: check a tree before waiting to replace it --- scripts/refresh_data_dirs.py | 52 ++++++++++++++++++++++----------- tests/test_refresh_data_dirs.py | 25 ++++++++++++++++ 2 files changed, 60 insertions(+), 17 deletions(-) diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index 25d820f08..7b74f7be1 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -442,13 +442,45 @@ def refresh_entry( The decision and the swap run under one lock per directory: a run that waited for another session re-reads the version it recorded and finds nothing left to do. + + The exclusive lock waits for every reader, and a pipeline reads the + trees for its whole run: a second build sat on "waiting" for a tree the + remote check would have found current. The check is made first without + it, and only a tree to replace waits. """ if dry_run: return _refresh_entry(key, entry, force, dry_run, versions_path) + if not force: + needs, _remote, cached_tag = _decide(key, entry, False, versions_path) + if needs is None: + return None + if not needs: + log.info("[%s] up to date (tag: %s)", key, _short_tag(cached_tag)) + return False with cache_lock(entry["local_cache"]): return _refresh_entry(key, entry, force, dry_run, versions_path) +def _decide( + key: str, entry: dict, force: bool, versions_path: str +) -> tuple[bool | None, str | None, str | None]: + """(whether the tree needs a refresh, the remote tag read, the recorded + tag). None when the remote could not be read.""" + version = entry.get("version", "master") + cached = _load_versions(versions_path).get(key, {}) + cached_tag = cached.get("sha") or cached.get("etag") + if force or not Path(entry["local_cache"]).exists(): + return True, None, cached_tag + if entry.get("source_type", "tarball") == "zip": + remote_tag = _get_remote_etag(entry["source_url"].format(version=version)) + else: + remote_tag = get_remote_sha(entry["source_url"], version) + if remote_tag is None: + log.warning("[%s] could not check remote, skipping", key) + return None, None, cached_tag + return remote_tag != cached_tag, remote_tag, cached_tag + + def _refresh_entry( key: str, entry: dict, force: bool, dry_run: bool, versions_path: str ) -> bool | None: @@ -458,23 +490,9 @@ def _refresh_entry( local_cache = entry["local_cache"] exclude = entry.get("exclude", []) - versions = _load_versions(versions_path) - cached = versions.get(key, {}) - cached_tag = cached.get("sha") or cached.get("etag") - - needs_refresh = force or not Path(local_cache).exists() - - remote_tag: str | None = None - if not needs_refresh: - if source_type == "zip": - remote_tag = _get_remote_etag(source_url) - else: - remote_tag = get_remote_sha(entry["source_url"], version) - if remote_tag is None: - log.warning("[%s] could not check remote, skipping", key) - return None - needs_refresh = remote_tag != cached_tag - + needs_refresh, remote_tag, cached_tag = _decide(key, entry, force, versions_path) + if needs_refresh is None: + return None if not needs_refresh: log.info("[%s] up to date (tag: %s)", key, _short_tag(cached_tag)) return False diff --git a/tests/test_refresh_data_dirs.py b/tests/test_refresh_data_dirs.py index 22f31c59c..42f21d122 100644 --- a/tests/test_refresh_data_dirs.py +++ b/tests/test_refresh_data_dirs.py @@ -93,6 +93,31 @@ class RefreshConcurrency(unittest.TestCase): ) self.assertEqual(seen, [True]) + def test_a_current_tree_is_not_waited_for(self): + """A pipeline reads every tree for its whole run. A build beside it + waited on the exclusive lock before asking the remote, which would + have answered that nothing changed.""" + import fcntl # noqa: PLC0415 + + self.cache.mkdir(parents=True) + rdd._record_version("pak", {"sha": "e1"}, self.versions) + entry = {"source_type": "zip", "source_url": "https://x/pak.zip", + "local_cache": str(self.cache)} + result: list = [] + with open(self.cache.with_name(".pak.lock"), "a") as reader, \ + mock.patch.object(rdd, "_get_remote_etag", return_value="e1"): + fcntl.flock(reader, fcntl.LOCK_SH) + worker = threading.Thread(target=lambda: result.append( + rdd.refresh_entry("pak", entry, versions_path=self.versions) + ), daemon=True) + worker.start() + worker.join(timeout=20) + finished = not worker.is_alive() + fcntl.flock(reader, fcntl.LOCK_UN) + worker.join(timeout=20) + self.assertTrue(finished, "the refresh waited for a reader") + self.assertEqual(result, [False]) + def test_version_file_is_never_left_truncated(self): Path(self.versions).parent.mkdir(parents=True) Path(self.versions).write_text(json.dumps({"keep": {"sha": "1"}})) From 1ed605969df79e3a3624a12955a30aa891381c2b Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:42:09 +0100 Subject: [PATCH 17/23] fix: standalone writers take the repository lock --- scripts/artifacts.py | 63 +++++++++++++++++++++++++-------- scripts/common.py | 4 ++- scripts/generate_db.py | 9 ++++- scripts/generate_games_index.py | 7 ++++ scripts/generate_pack.py | 7 ++-- scripts/generate_readme.py | 5 +++ scripts/generate_site.py | 7 ++-- scripts/pipeline.py | 9 +++-- scripts/sitesources.py | 15 +++++--- tests/test_artifact_lock.py | 56 ++++++++++++++++++++++++++++- 10 files changed, 152 insertions(+), 30 deletions(-) diff --git a/scripts/artifacts.py b/scripts/artifacts.py index 7cbfebc17..66b752837 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -208,11 +208,16 @@ 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. +# A run that holds artifact directories for its whole duration names them +# here, os.pathsep apart, for the steps it spawns: they would otherwise be +# refused by it. A pipeline holds the repository root and its --output-dir. HELD_LOCK_ENV = "RETROBIOS_HELD_ARTIFACT_DIR" +def _held_directories() -> list[str]: + return [d for d in os.environ.get(HELD_LOCK_ENV, "").split(os.pathsep) if d] + + @contextlib.contextmanager def artifact_lock(directory: str, exclusive: bool = True) -> Iterator[None]: """Serialize access to a shared artifact directory across processes. @@ -229,7 +234,7 @@ def artifact_lock(directory: str, exclusive: bool = True) -> Iterator[None]: return os.makedirs(directory, exist_ok=True) - if os.environ.get(HELD_LOCK_ENV) == os.path.realpath(directory): + if os.path.realpath(directory) in _held_directories(): yield return lock_path = os.path.join(directory, ".lock") @@ -256,16 +261,24 @@ def hold_artifact_lock(directory: str) -> Iterator[None]: 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 + with artifact_lock(directory), publish_hold(directory): + yield + + +@contextlib.contextmanager +def publish_hold(directory: str) -> Iterator[None]: + """Name a directory this process holds to the steps it spawns.""" + previous = os.environ.get(HELD_LOCK_ENV) + os.environ[HELD_LOCK_ENV] = os.pathsep.join( + [*_held_directories(), 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 # What a pipeline run writes at fixed paths, whatever its --output-dir. @@ -280,20 +293,40 @@ def hold_repository_outputs(root: str = ".") -> Iterator[None]: on dist/: both rewrote database.json, and the second to reach the manifest step found install/ held after minutes of work, or, when the steps did not overlap, rewrote the committed manifests from its own - state. The lock file is the repository root's .lock. + state. The lock file is the repository root's .lock. A pipeline passes + it to its steps with publish_hold. """ stack = contextlib.ExitStack() try: stack.enter_context(artifact_lock(root)) except ArtifactLockBusy as exc: raise ArtifactLockBusy( - f"another pipeline run is writing {', '.join(REPOSITORY_OUTPUTS)} " + f"another run is writing {', '.join(REPOSITORY_OUTPUTS)} " f"(lock: {os.path.join(root, '.lock')}). Wait for it to finish." ) from exc with stack: yield +_REPO_ROOT = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) + + +def repository_output_lock(path: str) -> contextlib.AbstractContextManager[None]: + """The repository hold, when *path* is one of its fixed outputs. + + Only a pipeline took it: generate_db.py run beside a pipeline replaced + database.json between its verify and its packs, and a manifest run on + install/ made the pipeline's own manifest step fail after minutes of + work. A path elsewhere (a scratch --output) takes nothing. + """ + target = os.path.realpath(path) + for output in REPOSITORY_OUTPUTS: + fixed = os.path.join(_REPO_ROOT, output.rstrip("/")) + if target == fixed or target.startswith(fixed + os.sep): + return hold_repository_outputs(_REPO_ROOT) + return contextlib.nullcontext() + + def _build_timestamp(db: dict | None = None) -> str: """Timestamp for generated artifacts. diff --git a/scripts/common.py b/scripts/common.py index 104c7f951..1c28b625a 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -3547,6 +3547,8 @@ from artifacts import ( # noqa: E402,F401 artifact_lock, hold_artifact_lock, hold_repository_outputs, + publish_hold, + repository_output_lock, _TIMESTAMP_PATTERNS, _strip_timestamps, ) @@ -3588,7 +3590,7 @@ def run_cli(main: Callable[[], int | None]) -> None: try: code = main() sys.stdout.flush() - except InputMissing as exc: + except (InputMissing, ArtifactLockBusy) as exc: print(f"ERROR: {exc}", file=sys.stderr) sys.exit(1) except BrokenPipeError: diff --git a/scripts/generate_db.py b/scripts/generate_db.py index 9d81396ef..f1fda2283 100644 --- a/scripts/generate_db.py +++ b/scripts/generate_db.py @@ -26,6 +26,7 @@ from common import ( DEFAULT_PROVENANCE_DIR, list_registered_platforms, load_provenance_snapshots, + repository_output_lock, require_yaml, set_plain_mode, source_yaml_paths, @@ -430,7 +431,11 @@ def main() -> int: help="Directory with dump-catalog snapshots", ) args = parser.parse_args() + with repository_output_lock(args.output): + return _generate(args) + +def _generate(args: argparse.Namespace) -> int: bios_dir = Path(args.bios_dir) if not bios_dir.is_dir(): print(f"Error: BIOS directory '{bios_dir}' not found", file=sys.stderr) @@ -742,4 +747,6 @@ def _collect_all_aliases( if __name__ == "__main__": - sys.exit(main() or 0) + from common import run_cli + + run_cli(main) diff --git a/scripts/generate_games_index.py b/scripts/generate_games_index.py index de991a60a..73473392f 100644 --- a/scripts/generate_games_index.py +++ b/scripts/generate_games_index.py @@ -49,6 +49,7 @@ from common import ( # noqa: E402 load_emulator_profiles, load_platform_config, native_layer, + repository_output_lock, run_cli, ) from region import normalize_declared # noqa: E402 @@ -661,7 +662,13 @@ def main() -> int: help="exit 1 when a file would change, naming what differs (implies --quiet)") parser.add_argument("--quiet", action="store_true", help="print only failures") args = parser.parse_args() + if args.check: + return _index(args) + with repository_output_lock(str(args.output_dir)): + return _index(args) + +def _index(args: argparse.Namespace) -> int: files, coverage = render(args.output_dir) if not (args.quiet or args.check): print_coverage(coverage, files) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index af2306a24..fa91eb16f 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -48,6 +48,7 @@ from common import ( profiles_for_systems, write_text_atomic, artifact_lock, + repository_output_lock, ArtifactLockBusy, build_target_cores_cache, build_zip_contents_index, @@ -2809,9 +2810,11 @@ def _run_manifest_mode( @contextlib.contextmanager def _pack_output_lock(output_dir: str, exclusive: bool = True) -> Iterator[None]: - """Hold the output directory for the duration of a pack run.""" + """Hold the output directory for the duration of a pack run, and the + repository's fixed outputs when it is one of them (install/).""" try: - with artifact_lock(output_dir, exclusive=exclusive): + with artifact_lock(output_dir, exclusive=exclusive), \ + repository_output_lock(output_dir) if exclusive else contextlib.nullcontext(): yield except ArtifactLockBusy as exc: print(f"ERROR: {exc}", file=sys.stderr) diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index 0f5476db2..556caa839 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -30,6 +30,7 @@ from common import ( load_emulator_profiles, load_platform_config, load_platform_registry, + repository_output_lock, resolve_platform_cores, system_index, unique_emulator_profiles, @@ -1034,7 +1035,11 @@ def main() -> None: help="platform YAML directory (default: %(default)s)", ) args = parser.parse_args() + with repository_output_lock("README.md"): + _write(args) + +def _write(args: argparse.Namespace) -> None: import contextlib from common import load_data_dir_registry diff --git a/scripts/generate_site.py b/scripts/generate_site.py index 6fe7b38ca..2f71f9262 100644 --- a/scripts/generate_site.py +++ b/scripts/generate_site.py @@ -12080,11 +12080,14 @@ def main() -> None: # files in the sweep, so the second is refused instead. import contextlib - from common import load_data_dir_registry + from common import load_data_dir_registry, repository_output_lock from refresh_data_dirs import hold_data_dirs # The coverage pages resolve against data/, held while they are read. - with artifact_lock(args.docs_dir), contextlib.ExitStack() as held: + # Whatever --docs-dir says, website/src/data and wiki/ are written. + with artifact_lock(args.docs_dir), \ + repository_output_lock(str(PUBLISHED_PARTS_FILE)), \ + contextlib.ExitStack() as held: hold_data_dirs(held, load_data_dir_registry(args.platforms_dir)) _generate(args) except ArtifactLockBusy as exc: diff --git a/scripts/pipeline.py b/scripts/pipeline.py index b8593c307..a7614af82 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -73,6 +73,7 @@ from common import ( hold_artifact_lock, hold_repository_outputs, load_data_dir_registry, + publish_hold, ) from refresh_data_dirs import hold_data_dirs @@ -1002,9 +1003,11 @@ def main() -> None: # rewrote them beside the run on dist/. held = contextlib.ExitStack() try: - held.enter_context( - hold_repository_outputs(str(Path(__file__).resolve().parent.parent)) - ) + root = str(Path(__file__).resolve().parent.parent) + held.enter_context(hold_repository_outputs(root)) + # Its steps write database.json, install/ and README.md themselves, + # and take the same lock to do so. + held.enter_context(publish_hold(root)) if not args.skip_packs: held.enter_context(hold_artifact_lock(args.output_dir)) except ArtifactLockBusy as exc: diff --git a/scripts/sitesources.py b/scripts/sitesources.py index faf307241..f42de04ae 100644 --- a/scripts/sitesources.py +++ b/scripts/sitesources.py @@ -735,13 +735,18 @@ def main() -> int: if not args.fetch and not any(path.is_dir() for path in cache_dirs): print(f"Error: no tree cache at {', '.join(map(str, cache_dirs))}", file=sys.stderr) return 1 - from common import load_emulator_profiles, write_if_changed # noqa: PLC0415 + from common import ( # noqa: PLC0415 + load_emulator_profiles, + repository_output_lock, + write_if_changed, + ) profiles = load_emulator_profiles(args.emulators_dir, skip_aliases=False) - held, unread = build_source_paths(profiles, cache_dirs, args.fetch) - held = keep_unread(held, unread, read_record(args.output)) - args.output.parent.mkdir(parents=True, exist_ok=True) - write_if_changed(str(args.output), json.dumps(held, indent=1, sort_keys=True) + "\n") + with repository_output_lock(str(args.output)): + held, unread = build_source_paths(profiles, cache_dirs, args.fetch) + held = keep_unread(held, unread, read_record(args.output)) + args.output.parent.mkdir(parents=True, exist_ok=True) + write_if_changed(str(args.output), json.dumps(held, indent=1, sort_keys=True) + "\n") print(f"{sum(map(len, held.values()))} paths held in {len(held)} trees, " f"{len(unread)} trees not cached: {args.output}") return 0 diff --git a/tests/test_artifact_lock.py b/tests/test_artifact_lock.py index 14e1b8de0..612bd87d8 100644 --- a/tests/test_artifact_lock.py +++ b/tests/test_artifact_lock.py @@ -23,6 +23,7 @@ from common import ( # noqa: E402 artifact_lock, hold_artifact_lock, hold_repository_outputs, + publish_hold, ) @@ -218,7 +219,7 @@ class PipelineHoldsItsFixedOutputs(unittest.TestCase): capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=120, ) self.assertEqual(proc.returncode, 1, proc.stdout + proc.stderr) - self.assertIn("another pipeline run is writing database.json", proc.stderr) + self.assertIn("another run is writing database.json", proc.stderr) self.assertNotIn("generate database", proc.stdout.lower()) def test_two_holds_of_the_repository_exclude_each_other(self): @@ -228,6 +229,59 @@ class PipelineHoldsItsFixedOutputs(unittest.TestCase): pass +class StandaloneWritersHonourTheRepositoryHold(unittest.TestCase): + """Only pipeline runs took the repository's lock: generate_db.py beside + a pipeline replaced database.json between its verify and its packs, and + a manifest run on install/ failed the pipeline's own manifest step.""" + + def _beside_a_pipeline(self, argv: list[str]) -> subprocess.CompletedProcess: + 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, + ) + + def test_a_writer_of_a_fixed_output_is_refused(self): + with hold_repository_outputs(str(REPO_ROOT)): + for argv in ( + ["scripts/generate_db.py", "--bios-dir", "bios", "--output", "database.json"], + ["scripts/generate_pack.py", "--all", "--manifest", "--offline", + "--output-dir", "install"], + ["scripts/generate_games_index.py", "--quiet"], + ): + with self.subTest(script=argv[0]): + proc = self._beside_a_pipeline(argv) + self.assertEqual(proc.returncode, 1, proc.stdout[-500:] + proc.stderr[-500:]) + self.assertIn("another run is writing database.json", proc.stderr) + self.assertNotIn("Traceback", proc.stderr) + + def _child(self, target: str, env: dict) -> subprocess.CompletedProcess: + code = ( + "import sys; sys.path.insert(0, 'scripts')\n" + "from common import artifact_lock, repository_output_lock\n" + f"with repository_output_lock({target!r}), artifact_lock({self.scratch!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_step_the_pipeline_spawns_passes(self): + """A pipeline holds the root and its --output-dir, and its steps + write both: the environment names the two, os.pathsep apart.""" + self.scratch = _tempdir(self) + with hold_repository_outputs(str(REPO_ROOT)), publish_hold(str(REPO_ROOT)), \ + hold_artifact_lock(self.scratch): + proc = self._child(str(REPO_ROOT / "install" / "games"), dict(os.environ)) + self.assertEqual(proc.returncode, 0, proc.stderr) + + def test_a_scratch_output_takes_nothing(self): + self.scratch = _tempdir(self) + env = {k: v for k, v in os.environ.items() if k != "RETROBIOS_HELD_ARTIFACT_DIR"} + with hold_repository_outputs(str(REPO_ROOT)): + proc = self._child(os.path.join(self.scratch, "database.json"), env) + self.assertEqual(proc.returncode, 0, proc.stderr) + + 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 From a82e9b6f7f7141e50567d7d70a1554fe168e0124 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:42:42 +0100 Subject: [PATCH 18/23] fix: system report reads the claims the pack reads --- scripts/packextras.py | 12 ++++++++++-- tests/test_system_selection.py | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/scripts/packextras.py b/scripts/packextras.py index 0ff7ec119..343827316 100644 --- a/scripts/packextras.py +++ b/scripts/packextras.py @@ -1552,11 +1552,19 @@ def extras_for_systems(extras: list[dict], system_ids: list[str]) -> list[dict]: def report_owned_by_systems(report: list[dict], system_ids: list[str]) -> list[dict]: - """Gap report entries whose extra a pack built with --system carries.""" + """Gap report entries whose extra a pack built with --system carries. + + The other cores whose claim rides on the entry count, as the builder + counts them: MAME 2009's taitofx1.zip went into a --system + panasonic-3do pack for MAME 2016 and was missing from the report. + """ + def owner(entry: dict) -> dict: + return {"source_system": entry.get("system"), "source_systems": entry.get("systems", [])} + return [ u for u in report if extras_for_systems( - [{"source_system": u.get("system"), "source_systems": u.get("systems", [])}], + [{**owner(u), "other_claims": [owner(c) for c in u.get("other_claims", [])]}], system_ids, ) ] diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index 48ad9eefa..b70436947 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -404,6 +404,32 @@ class AMissingInputFileIsNamed(unittest.TestCase): self._refused(argv, "ERROR: Registry not found: nosuch.yml") +class ASystemReportCountsTheClaimsThePackCounts(unittest.TestCase): + """`--platform retroarch --system panasonic-3do` packed MAME 2009's + taitofx1.zip, which MAME 2016 claims for that system, and the report's + core coverage did not name it: the builder read other_claims, verify + read only the entry's own system.""" + + def test_a_claim_riding_on_an_entry_selects_it(self): + from packextras import extras_for_systems, report_owned_by_systems + + entry = { + "name": "taitofx1.zip", "system": "arcade", "systems": ["arcade"], + "other_claims": [{"name": "taitofx1.zip", "system": None, + "systems": ["arcade", "panasonic-3do"]}], + } + extra = {"source_system": "arcade", "source_systems": ["arcade"], + "other_claims": [{"source_system": None, + "source_systems": ["arcade", "panasonic-3do"]}]} + for system in ("panasonic-3do", "arcade", "sony-playstation"): + with self.subTest(system=system): + self.assertEqual( + bool(report_owned_by_systems([entry], [system])), + bool(extras_for_systems([extra], [system])), + ) + self.assertEqual(report_owned_by_systems([entry], ["panasonic-3do"]), [entry]) + + class StrictVerifyFailsOnACriticalMiss(unittest.TestCase): """verify exited 0 with a required file missing on an md5 platform, so the pipeline printed "verify OK" over a pack the frontend rejects.""" From 7f6ba6952c91f0b08107289cc8cbf295b95c2281 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:45:07 +0100 Subject: [PATCH 19/23] feat: verify reports the required-only pack --- scripts/common.py | 13 ++++++++++ scripts/generate_pack.py | 15 ++---------- scripts/verify.py | 36 +++++++++++++++++++++++++-- tests/test_system_selection.py | 45 ++++++++++++++++++++++++++++++++++ wiki/advanced-usage.md | 9 +++++++ wiki/tools.md | 4 ++- 6 files changed, 106 insertions(+), 16 deletions(-) diff --git a/scripts/common.py b/scripts/common.py index 1c28b625a..b8cb33896 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -1220,6 +1220,19 @@ def archive_entry(archive: str, files: list[dict]) -> dict: return {**own, "members": members} if members else own +def archive_is_optional(entry: dict, archive: str, files: list[dict]) -> bool: + """Optional as a whole: its own entry and every member it carries. + + A --required-only pack leaves it out, and the report of that pack too. + geolith's _Required pack held neocd.zip and irrmaze.zip all the same. + """ + return entry.get("required") is False and all( + member.get("required") is False + for member in files + if member.get("archive") == archive + ) + + def holds_required_members(path: str, members: list[dict]) -> bool: """Whether an archive holds every member a profile requires inside it.""" crcs = zip_member_crcs(path) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index fa91eb16f..e922558c1 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -47,6 +47,7 @@ from common import ( archive_entry, profiles_for_systems, write_text_atomic, + archive_is_optional, artifact_lock, repository_output_lock, ArtifactLockBusy, @@ -1473,18 +1474,6 @@ class DamagedArchiveError(OSError): -def _archive_is_optional(archive_entry: dict, archive_name: str, files: list[dict]) -> bool: - """Optional as a whole: its own entry and every member it carries. - - geolith's _Required pack held neocd.zip and irrmaze.zip all the same. - """ - return archive_entry.get("required") is False and all( - member.get("required") is False - for member in files - if member.get("archive") == archive_name - ) - - def _pack_data_tree( zf: zipfile.ZipFile, local_cache: str, @@ -1673,7 +1662,7 @@ def generate_emulator_pack( ) own_entry = archive_entry(archive_name, files) - if required_only and _archive_is_optional(own_entry, archive_name, files): + if required_only and archive_is_optional(own_entry, archive_name, files): continue local_path, status = resolve_file( own_entry, diff --git a/scripts/verify.py b/scripts/verify.py index 147a8f80d..0f621eb4e 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -43,6 +43,7 @@ from common import ( suggest_names, require_platform, archive_entry, + archive_is_optional, archive_members, profiles_for_systems, PROFILE_IDENTITY_FIELDS, @@ -2056,12 +2057,16 @@ def verify_emulator( regions: list[str] | None = None, platforms_dir: str = _REPO_PLATFORMS, system_ids: list[str] | None = None, + required_only: bool = False, ) -> dict: """Verify files for specific emulator profiles. A region priority list narrows the report the same way a pack built with the same list would be narrowed, through the emulator pack's own drop set. `system_ids` keeps the entries of those systems, as the system pack does. + `required_only` reports on the pack built with --required-only: a loose + file or tree marked ``required: false`` is left out, and an archive when + it and every member it carries are optional, as the builder leaves them. """ from packextras import expand_agnostic_entries from packresolve import resolve_file @@ -2105,7 +2110,7 @@ def verify_emulator( if regions: from packextras import emulator_region_drops - region_drops = emulator_region_drops(selected, standalone, regions, db=db) + region_drops = emulator_region_drops(selected, standalone, regions, required_only, db=db) # The emulator pack walks profiles in name order and keeps the first # file at a path, folding case: a later profile's different file there @@ -2142,6 +2147,8 @@ def verify_emulator( bluemsx's Machines/ tree holds msx-emu's coleco.rom path in a --system colecovision pack.""" for dd in deliverable(profile.get("data_directories", []), standalone): + if required_only and dd.get("required") is False: + continue cache = (data_registry or {}).get(dd.get("ref", ""), {}).get("local_cache", "") if not cache or not os.path.isdir(cache): continue @@ -2170,6 +2177,17 @@ def verify_emulator( ) if system_ids: files = [fe for fe in files if entry_serves_systems(fe, profile, system_ids)] + if required_only: + optional_archives = { + fe["archive"] for fe in files + if fe.get("archive") + and archive_is_optional(archive_entry(fe["archive"], files), fe["archive"], files) + } + files = [ + fe for fe in files + if fe.get("archive") not in optional_archives + and (fe.get("archive") or fe.get("required") is not False) + ] if region_drops: # Loose files only, as the emulator pack: an archive ships whole, # and dropping kronos' regional stvbios.zip members rebuilt the @@ -2185,6 +2203,8 @@ def verify_emulator( for dd in deliverable( profile.get("data_directories", []), standalone ): + if required_only and dd.get("required") is False: + continue ref = dd.get("ref", "") if not ref: continue @@ -2516,6 +2536,7 @@ def verify_system( standalone: bool = False, regions: list[str] | None = None, platforms_dir: str = _REPO_PLATFORMS, + required_only: bool = False, ) -> dict: """Verify files for all emulators supporting given system IDs.""" profiles = load_emulator_profiles(emulators_dir) @@ -2541,7 +2562,7 @@ def verify_system( return verify_emulator( matching, emulators_dir, db, standalone, regions=regions, - platforms_dir=platforms_dir, system_ids=system_ids, + platforms_dir=platforms_dir, system_ids=system_ids, required_only=required_only, ) @@ -2680,6 +2701,7 @@ def _refuse_listing_narrowings( ("--destinations", args.destinations), ("--offline", args.offline), ("--no-game-data", getattr(args, "no_game_data", False)), + ("--required-only", args.required_only), ): if on: parser.error(f"{flag} is incompatible with {listing}") @@ -2708,6 +2730,8 @@ def _refuse_mode_flags(args: argparse.Namespace, parser: argparse.ArgumentParser "--no-game-data requires --platform or --all"), (args.destinations and getattr(args, "region", None), "--destinations describes the full pack and is incompatible with --region"), + (args.destinations and args.required_only, + "--destinations describes the full pack and is incompatible with --required-only"), ): if refused: parser.error(message) @@ -2776,6 +2800,11 @@ def _main(held: contextlib.ExitStack) -> None: help="Report on the pack built with --no-game-data: game data left " "out and named, firmware only", ) + parser.add_argument( + "--required-only", action="store_true", + help="Report on the pack built with --required-only: files the code " + "runs without are left out", + ) parser.add_argument( "--db", default=DEFAULT_DB, help="database file to read (default: %(default)s)", @@ -2867,6 +2896,7 @@ def _main(held: contextlib.ExitStack) -> None: result = verify_emulator( names, args.emulators_dir, db, args.standalone, regions=requested_regions, platforms_dir=args.platforms_dir, + required_only=args.required_only, ) if args.json: result["details"] = _json_details(result["details"], args.verbose) @@ -2881,6 +2911,7 @@ def _main(held: contextlib.ExitStack) -> None: result = verify_system( system_ids, args.emulators_dir, db, args.standalone, regions=requested_regions, platforms_dir=args.platforms_dir, + required_only=args.required_only, ) if args.json: result["details"] = _json_details(result["details"], args.verbose) @@ -2950,6 +2981,7 @@ def _main(held: contextlib.ExitStack) -> None: regions=requested_regions, system_filter=platform_systems, no_game_data=getattr(args, "no_game_data", False), + required_only=args.required_only, ) names = [ load_platform_config(p, args.platforms_dir).get("platform", p) diff --git a/tests/test_system_selection.py b/tests/test_system_selection.py index b70436947..c7016c539 100644 --- a/tests/test_system_selection.py +++ b/tests/test_system_selection.py @@ -430,6 +430,51 @@ class ASystemReportCountsTheClaimsThePackCounts(unittest.TestCase): self.assertEqual(report_owned_by_systems([entry], ["panasonic-3do"]), [entry]) +class VerifyReportsTheRequiredOnlyPack(unittest.TestCase): + """`generate_pack --emulator mame --required-only` held 164 archives and + the only report, `verify --emulator mame`, credited 2302 paths: + --required-only was refused by verify, and the archive rule existed in + the builder only.""" + + def setUp(self): + previous = os.getcwd() + os.chdir(REPO_ROOT) + self.addCleanup(os.chdir, previous) + + def test_the_report_holds_the_paths_the_pack_holds(self): + import contextlib + import io + + from verify import verify_emulator + + db = common.load_database("database.json") + for name in ("beetle_psx", "dolphin", "fbneo", "geolith", "mame"): + with self.subTest(emulator=name): + record = builder.PackRecord() + with contextlib.redirect_stdout(io.StringIO()), \ + contextlib.redirect_stderr(io.StringIO()): + builder.generate_emulator_pack( + [name], "emulators", db, "bios", "", offline=True, + platforms_dir="platforms", record=record, required_only=True, + ) + report = verify_emulator([name], "emulators", db, + platforms_dir="platforms", required_only=True) + full = verify_emulator([name], "emulators", db, platforms_dir="platforms") + self.assertEqual(sorted(report["pack_destinations"]), sorted(record.files)) + self.assertLessEqual(set(report["pack_destinations"]), + set(full["pack_destinations"])) + + def test_every_mode_takes_the_flag(self): + for argv in (["--platform", "lakka"], ["--emulator", "handy"], + ["--system", "atari-lynx"]): + with self.subTest(argv=argv): + proc = subprocess.run( + [sys.executable, "scripts/verify.py", *argv, "--required-only"], + cwd=REPO_ROOT, capture_output=True, text=True, timeout=600, check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr[-1500:]) + + class StrictVerifyFailsOnACriticalMiss(unittest.TestCase): """verify exited 0 with a required file missing on an md5 platform, so the pipeline printed "verify OK" over a pack the frontend rejects.""" diff --git a/wiki/advanced-usage.md b/wiki/advanced-usage.md index 814a6d03c..c17cc07ac 100644 --- a/wiki/advanced-usage.md +++ b/wiki/advanced-usage.md @@ -95,6 +95,15 @@ What counts as "required" depends on the platform YAML. For existence-mode platf For MD5-mode platforms (Batocera), all declared files are treated as required unless explicitly marked optional. +An emulator or system pack keeps the files and trees its profiles do not mark +`required: false`, and an archive unless it and every ROM it carries are +optional. `verify.py` takes the same flag and reports on that pack: + +```bash +python scripts/verify.py --platform batocera --required-only +python scripts/verify.py --emulator mame --required-only +``` + ### Region filtering Keep one regional BIOS per system instead of all of them: diff --git a/wiki/tools.md b/wiki/tools.md index 31086a61e..aa9e695a4 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -148,6 +148,7 @@ python scripts/verify.py --platform batocera --system psx # one system of a pla python scripts/verify.py --platform retroarch --target switch # filter by hardware python scripts/verify.py --platform recalbox --region us # regional priority list python scripts/verify.py --platform emuelec --no-game-data # the firmware-only pack, game data named +python scripts/verify.py --emulator mame --required-only # the pack built with --required-only python scripts/verify.py --all --destinations tmp/paths.json # also write the paths each pack holds python scripts/verify.py --all --offline # a release asset the cache lacks counts missing, as in an offline build python scripts/verify.py --list-platforms # list all platforms @@ -258,7 +259,8 @@ own verification mode ([discrepancy detection](verification-modes.md#discrepancy with the closest matches before any work. Without `--platform`, `--system` takes our ids only. - `--required-only`: exclude optional files and the data directories declared - `required: false`, keep only required + `required: false`, keep only required. `verify.py --required-only` reports + on the same file set, for `--platform`, `--emulator` and `--system` - `--no-game-data`: firmware only. Leaves out the platform's entries categorized game data and the cores' `category: game_data` files, and names each one. Tags the pack `_NoGameData`. `verify.py --no-game-data` and From cead17429b435bdfd6d9278ca639e6cadf636801 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 03:51:52 +0100 Subject: [PATCH 20/23] fix: compare narrowed emulator packs in step 5 --- scripts/pipeline.py | 118 +++++++++++++++++++------------- tests/test_pack_destinations.py | 24 ++++++- wiki/tools.md | 4 ++ 3 files changed, 99 insertions(+), 47 deletions(-) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index a7614af82..02e195a52 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -523,15 +523,43 @@ def check_pack_destinations( return all_ok +# The narrowings an emulator or system pack takes, compared by +# check_emulator_packs. A test holds this to the builder's parameters. +EMULATOR_NARROWINGS_COMPARED = frozenset({"standalone", "regions", "required_only"}) + + +def emulator_variants(profiles: list[dict], full: bool) -> list[tuple[str, dict]]: + """(label, builder arguments) of each narrowed emulator pack compared. + + --required-only always, and each region the profiles' files declare, + sampled as check_narrowed_packs samples them unless *full*. A profile + without a regional file has nothing --region could withdraw. + """ + import region as region_mod # noqa: PLC0415 + + regions = sorted({ + r for profile in profiles for fe in profile.get("files") or [] + for r in region_mod.normalize_declared(fe.get("region")) + }) + variants: list[tuple[str, dict]] = [("", {}), (" --required-only", {"required_only": True})] + variants += [ + (f" --region {regions[i]}", {"regions": [regions[i]]}) + for i in _matrix_coverage(full, len(regions)) + ] + return variants + + def check_emulator_packs( emulators_dir: str = "emulators", platforms_dir: str = "platforms", - db_path: str = "database.json", + db_path: str = "database.json", full: bool = False, ) -> bool: """Build every emulator and system pack in memory and compare it with verify. - Each profile in each build it has (libretro, standalone): the paths the - pack fills with the profile's files against the ones verify credits. No - archive is written and no file read, so all of them take seconds. + Each profile in each build it has (libretro, standalone), full and under + each narrowing of emulator_variants: the paths the pack fills with the + profile's files against the ones verify credits. The region and + required-only rules are written twice, once per side, and only this + comparison holds them together. No archive is written and no file read. """ import generate_pack # noqa: PLC0415 import verify # noqa: PLC0415 @@ -549,6 +577,26 @@ def check_emulator_packs( db = load_database(db_path) zip_contents = build_zip_contents_index(db) profiles = load_emulator_profiles(emulators_dir, skip_aliases=False) + + def compare(names: list[str], standalone: bool, label: str, **narrowing) -> bool: + record = generate_pack.PackRecord() + with contextlib.redirect_stdout(io.StringIO()), \ + contextlib.redirect_stderr(io.StringIO()): + generate_pack.generate_emulator_pack( + names, emulators_dir, db, "bios", "", standalone, zip_contents, + offline=True, platforms_dir=platforms_dir, record=record, **narrowing, + ) + report = verify.verify_emulator( + names, emulators_dir, db, standalone, platforms_dir=platforms_dir, + **narrowing, + ) + lines = _set_difference_lines( + label, set(report["pack_destinations"]), set(record.files) + ) + if lines: + print("\n".join(lines)) + return not lines + all_ok = True compared = 0 for name in sorted(profiles): @@ -559,23 +607,10 @@ def check_emulator_packs( select_emulator_profiles([name], profiles, standalone) except ProfileSelectionError: continue - record = generate_pack.PackRecord() - with contextlib.redirect_stdout(io.StringIO()): - generate_pack.generate_emulator_pack( - [name], emulators_dir, db, "bios", "", standalone, zip_contents, - offline=True, platforms_dir=platforms_dir, record=record, - ) - report = verify.verify_emulator( - [name], emulators_dir, db, standalone, platforms_dir=platforms_dir, - ) - compared += 1 - label = f"{name}{' --standalone' if standalone else ''}" - lines = _set_difference_lines( - label, set(report["pack_destinations"]), set(record.files) - ) - if lines: - print("\n".join(lines)) - all_ok = False + for tag, narrowing in emulator_variants([profiles[name]], full): + compared += 1 + label = f"{name}{' --standalone' if standalone else ''}{tag}" + all_ok = compare([name], standalone, label, **narrowing) and all_ok print(f" {compared} emulator packs compared, {'all equal' if all_ok else 'differences above'}") # A system pack builds every profile of the system together: a placement # rule that only goes wrong when two profiles share a path (the bluemsx @@ -593,34 +628,24 @@ def check_emulator_packs( matching = profiles_for_systems(profiles, [sid], standalone) if len(matching) < 2: continue - record = generate_pack.PackRecord() - with contextlib.redirect_stdout(io.StringIO()), \ - contextlib.redirect_stderr(io.StringIO()): - generate_pack.generate_emulator_pack( - matching, emulators_dir, db, "bios", "", standalone, zip_contents, - offline=True, platforms_dir=platforms_dir, record=record, - system_ids=[sid], - ) - report = verify.verify_emulator( - matching, emulators_dir, db, standalone, platforms_dir=platforms_dir, - system_ids=[sid], - ) - compared += 1 - label = f"--system {sid}{' --standalone' if standalone else ''}" - lines = _set_difference_lines( - label, set(report["pack_destinations"]), set(record.files) - ) - if lines: - print("\n".join(lines)) - systems_ok = False + for tag, narrowing in emulator_variants( + [profiles[n] for n in matching if n in profiles], full + ): + compared += 1 + label = f"--system {sid}{' --standalone' if standalone else ''}{tag}" + systems_ok = compare( + matching, standalone, label, system_ids=[sid], **narrowing + ) and systems_ok print(f" {compared} system packs compared, " f"{'all equal' if systems_ok else 'differences above'}") return all_ok and systems_ok -# How each _narrowings() dimension is compared by check_narrowed_packs, or why -# it is not. A test reads the builder's signature against these two tables, so -# a new narrowing fails it until it is compared or a reason is written here. +# How each _narrowings() dimension of a platform pack is compared by +# check_narrowed_packs, or why it is not. A test reads the builder's +# signature against these two tables, so a new narrowing fails it until it +# is compared or a reason is written here. Emulator packs have their own +# table, EMULATOR_NARROWINGS_COMPARED. NARROWINGS_COMPARED = frozenset({"regions", "target_name", "required_only", "no_game_data"}) NARROWINGS_NOT_COMPARED = { "standalone": "an emulator pack mode, compared by check_emulator_packs", @@ -964,7 +989,8 @@ def main() -> None: parser.add_argument( "--full-narrowings", action="store_true", help="Step 5 compares every region, target and system of each " - "platform, not a sample of three (about 25 minutes)", + "platform, and every region of each emulator and system pack, not a " + "sample of three (about 25 minutes)", ) parser.add_argument( "--check-buildbot", @@ -1335,7 +1361,7 @@ def main() -> None: ) # Counts can agree while the paths differ, both ways at once. ok = check_pack_destinations(destinations_file, pack_output) and ok - ok = check_emulator_packs() and ok + ok = check_emulator_packs(full=args.full_narrowings) and ok ok = check_narrowed_packs( include_archived=args.include_archived, full=args.full_narrowings, ) and ok diff --git a/tests/test_pack_destinations.py b/tests/test_pack_destinations.py index 6d15a718f..65bd1277d 100644 --- a/tests/test_pack_destinations.py +++ b/tests/test_pack_destinations.py @@ -182,6 +182,28 @@ class EveryNarrowingIsCompared(unittest.TestCase): "compare the new narrowing in check_narrowed_packs, " "or write in NARROWINGS_NOT_COMPARED why it is not") + def test_each_emulator_pack_narrowing_is_compared(self): + """check_emulator_packs built every pack without --region or + --required-only, while the table above counted both as compared: + it describes the platform packs only.""" + import inspect + + import pipeline + from generate_pack import _narrowings, generate_emulator_pack + + taken = (set(inspect.signature(_narrowings).parameters) + & set(inspect.signature(generate_emulator_pack).parameters)) + self.assertEqual(taken, set(pipeline.EMULATOR_NARROWINGS_COMPARED)) + profile = {"files": [{"name": "a.bin", "region": "japan"}, + {"name": "b.bin", "region": ["north-america", "europe"]}]} + used = {key for _label, narrowing in pipeline.emulator_variants([profile], False) + for key in narrowing} + # standalone is the loop around the variants, both builds of each profile. + self.assertEqual(used, pipeline.EMULATOR_NARROWINGS_COMPARED - {"standalone"}) + regions = [n["regions"] for _l, n in pipeline.emulator_variants([profile], True) + if "regions" in n] + self.assertEqual(len(regions), 3) + def test_the_variants_reach_every_compared_dimension(self): import pipeline from common import load_emulator_profiles, load_platform_config @@ -246,7 +268,7 @@ class ThePipelineComparesPaths(unittest.TestCase): def test_the_run_compares_every_emulator_pack(self): source = (REPO_ROOT / "scripts" / "pipeline.py").read_text(encoding="utf-8") self.assertIn("ok = check_pack_destinations(destinations_file, pack_output) and ok", source) - self.assertIn("ok = check_emulator_packs() and ok", source) + self.assertIn("ok = check_emulator_packs(full=args.full_narrowings) and ok", source) self.assertIn('"--destinations", destinations_file', source) diff --git a/wiki/tools.md b/wiki/tools.md index aa9e695a4..5c2dccbc0 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -95,6 +95,10 @@ alone, run `python scripts/check_buildbot_system.py`. ones verify credits (`--destinations`), every emulator pack, each profile in each build, built in memory against `verify --emulator`, and every system pack that gathers several profiles against `verify --system`. + Each of them is also built with `--required-only` and with the regions + its files declare (first, middle and last, all of them with + `--full-narrowings`). 2,112 emulator and 1,493 system packs took about + two minutes on 2026-11-06. Equal counts once hid four RetroArch paths credited and never written. - **Each narrowed platform pack is compared too.** Step 5 builds in memory the `--required-only` and `--no-game-data` pack of every platform, and its From cf2ea6cd44e22734557cab4c250dee774813efcd Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 04:25:29 +0100 Subject: [PATCH 21/23] fix: annotate the emulator pack comparison --- scripts/pipeline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 02e195a52..2c2f23ede 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -578,7 +578,7 @@ def check_emulator_packs( zip_contents = build_zip_contents_index(db) profiles = load_emulator_profiles(emulators_dir, skip_aliases=False) - def compare(names: list[str], standalone: bool, label: str, **narrowing) -> bool: + def compare(names: list[str], standalone: bool, label: str, **narrowing: object) -> bool: record = generate_pack.PackRecord() with contextlib.redirect_stdout(io.StringIO()), \ contextlib.redirect_stderr(io.StringIO()): From c137d13621f084f68509ab60b7d12c0234edb0de Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 04:25:29 +0100 Subject: [PATCH 22/23] fix: readme holds the data trees in main --- scripts/generate_readme.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index 556caa839..55bdc5614 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -1035,11 +1035,7 @@ def main() -> None: help="platform YAML directory (default: %(default)s)", ) args = parser.parse_args() - with repository_output_lock("README.md"): - _write(args) - -def _write(args: argparse.Namespace) -> None: import contextlib from common import load_data_dir_registry @@ -1049,9 +1045,13 @@ def _write(args: argparse.Namespace) -> None: # Coverage resolves against data/: a refresh swapping a tree mid-read # published counts taken from half of it. - with contextlib.ExitStack() as held: + with repository_output_lock("README.md"), contextlib.ExitStack() as held: hold_data_dirs(held, load_data_dir_registry(args.platforms_dir)) readme = generate_readme(db, args.platforms_dir, args.db) + _write(readme) + + +def _write(readme: str) -> None: status = "Generated" if write_if_changed("README.md", readme) else "Unchanged" print(f"{status} ./README.md") From f63a84a7463044cab6fac546a0376abeda76790e Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 04:25:29 +0100 Subject: [PATCH 23/23] fix: check validate_pr platforms after git --- scripts/validate_pr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index cae324e65..61ada6903 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -799,7 +799,6 @@ def main() -> None: if os.path.realpath(os.getcwd()) != os.path.realpath(REPO_ROOT): args.files = [os.path.abspath(f) for f in args.files] work_from_repo_root(args, parser, ["db", "platforms_dir", "emulators_dir"]) - require_directory(args.platforms_dir, "--platforms-dir") files = _files_to_validate(args, parser) if not files: @@ -808,6 +807,7 @@ def main() -> None: return db = _load_database_if_any(args.db) + require_directory(args.platforms_dir, "--platforms-dir") platform_hashes = load_platform_hashes(args.platforms_dir, db) profile_hashes = load_profile_hashes(args.emulators_dir, db)