From 43337c67197e422f868fa1f140eeb8704fd25429 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:51:55 +0200 Subject: [PATCH 01/10] fix: match release packs by exact name release_record rebuilt nothing from the name builder: a substring match took RetroArch_v1.22.2_AndroidArm64V8A_BIOS_Pack.zip for the RetroArch pack. The record now refuses any pack that is not a registered platform's full pack and stores the platforms each pack serves, which the readers use instead of the pack name. --- release.json | 34 ++++++++++++ schemas/release.schema.json | 8 ++- scripts/release_notes.py | 26 ++++++--- scripts/release_record.py | 103 +++++++++++++++++++++++++----------- tests/test_doc_surfaces.py | 4 +- tests/test_pack_counts.py | 46 +++++++++++++--- wiki/tools.md | 2 +- 7 files changed, 176 insertions(+), 47 deletions(-) diff --git a/release.json b/release.json index df9c894a..962167ef 100644 --- a/release.json +++ b/release.json @@ -9,6 +9,9 @@ "Batocera_43.1_BIOS_Pack.part1of3.zip", "Batocera_43.1_BIOS_Pack.part2of3.zip", "Batocera_43.1_BIOS_Pack.part3of3.zip" + ], + "platforms": [ + "batocera" ] }, "BizHawk_2.11.1_BIOS_Pack.zip": { @@ -17,6 +20,9 @@ "download_size": 1469752558, "assets": [ "BizHawk_2.11.1_BIOS_Pack.zip" + ], + "platforms": [ + "bizhawk" ] }, "EmuDeck_2.7EA_BIOS_Pack.zip": { @@ -25,6 +31,9 @@ "download_size": 1939696164, "assets": [ "EmuDeck_2.7EA_BIOS_Pack.zip" + ], + "platforms": [ + "emudeck" ] }, "MiSTer_FPGA_2026-09-29_BIOS_Pack.zip": { @@ -33,6 +42,9 @@ "download_size": 18787872, "assets": [ "MiSTer_FPGA_2026-09-29_BIOS_Pack.zip" + ], + "platforms": [ + "misterfpga" ] }, "ROCKNIX_20261001_BIOS_Pack.zip": { @@ -42,6 +54,9 @@ "assets": [ "ROCKNIX_20261001_BIOS_Pack.part1of2.zip", "ROCKNIX_20261001_BIOS_Pack.part2of2.zip" + ], + "platforms": [ + "rocknix" ] }, "Recalbox_10.1.1_BIOS_Pack.zip": { @@ -51,6 +66,9 @@ "assets": [ "Recalbox_10.1.1_BIOS_Pack.part1of2.zip", "Recalbox_10.1.1_BIOS_Pack.part2of2.zip" + ], + "platforms": [ + "recalbox" ] }, "RetroArch_Lakka_v1.22.2_BIOS_Pack.zip": { @@ -60,6 +78,10 @@ "assets": [ "RetroArch_Lakka_v1.22.2_BIOS_Pack.part1of2.zip", "RetroArch_Lakka_v1.22.2_BIOS_Pack.part2of2.zip" + ], + "platforms": [ + "lakka", + "retroarch" ] }, "RetroBat_8.2.1_BIOS_Pack.zip": { @@ -70,6 +92,9 @@ "RetroBat_8.2.1_BIOS_Pack.part1of3.zip", "RetroBat_8.2.1_BIOS_Pack.part2of3.zip", "RetroBat_8.2.1_BIOS_Pack.part3of3.zip" + ], + "platforms": [ + "retrobat" ] }, "RetroDECK_0.10.10b_BIOS_Pack.zip": { @@ -80,6 +105,9 @@ "RetroDECK_0.10.10b_BIOS_Pack.part1of3.zip", "RetroDECK_0.10.10b_BIOS_Pack.part2of3.zip", "RetroDECK_0.10.10b_BIOS_Pack.part3of3.zip" + ], + "platforms": [ + "retrodeck" ] }, "RetroPie_4.8.12_BIOS_Pack.zip": { @@ -90,6 +118,9 @@ "RetroPie_4.8.12_BIOS_Pack.part1of3.zip", "RetroPie_4.8.12_BIOS_Pack.part2of3.zip", "RetroPie_4.8.12_BIOS_Pack.part3of3.zip" + ], + "platforms": [ + "retropie" ] }, "RomM_5.3.1_BIOS_Pack.zip": { @@ -98,6 +129,9 @@ "download_size": 973353236, "assets": [ "RomM_5.3.1_BIOS_Pack.zip" + ], + "platforms": [ + "romm" ] } } diff --git a/schemas/release.schema.json b/schemas/release.schema.json index a84a6ae3..34e8bcde 100644 --- a/schemas/release.schema.json +++ b/schemas/release.schema.json @@ -15,8 +15,14 @@ "additionalProperties": { "type": "object", "additionalProperties": false, - "required": ["files", "extracted_size", "download_size", "assets"], + "required": ["files", "extracted_size", "download_size", "assets", "platforms"], "properties": { + "platforms": { + "type": "array", + "minItems": 1, + "uniqueItems": true, + "items": { "type": "string" } + }, "files": { "type": "integer", "minimum": 1 }, "extracted_size": { "type": "integer", "minimum": 0 }, "download_size": { "type": "integer", "minimum": 1 }, diff --git a/scripts/release_notes.py b/scripts/release_notes.py index 06a111c1..995d18f6 100644 --- a/scripts/release_notes.py +++ b/scripts/release_notes.py @@ -14,6 +14,7 @@ Usage: from __future__ import annotations import argparse +import re import sys from pathlib import Path @@ -37,18 +38,27 @@ def _display(pack: str) -> str: def _extract_folders(record: dict, platforms_dir: str) -> dict[str, str]: """Pack name to the folder the registry says it extracts into.""" + registry = load_platform_registry(platforms_dir) folders: dict[str, str] = {} - for key, entry in sorted(load_platform_registry(platforms_dir).items()): - if not entry.get("extract_to"): - continue - needle = release_record._match_key(key) - for name in sorted(record.get("packs", {})): - # The name a pack starts with: RetroArch_Lakka is RetroArch's. - if needle and release_record._match_key(name).startswith(needle): - folders.setdefault(name, entry["extract_to"]) + for name, pack in sorted(record.get("packs", {}).items()): + # A shared pack extracts where the platform it is named after does: + # RetroArch_Lakka is RetroArch's. + key = _match_key(name) + for platform in sorted( + pack.get("platforms", ()), key=lambda p: key.find(_match_key(p)) % (len(key) + 1) + ): + folder = registry.get(platform, {}).get("extract_to") + if folder: + folders[name] = folder + break return folders +def _match_key(value: str) -> str: + """Letters and digits only: the platform is `misterfpga`, the pack MiSTer_FPGA.""" + return re.sub(r"[^a-z0-9]", "", value.lower()) + + def _links(tag: str, assets: list[str]) -> str: base = f"https://github.com/{REPO}/releases/download/{tag}" if len(assets) == 1: diff --git a/scripts/release_record.py b/scripts/release_record.py index 3c110e5f..f8510686 100644 --- a/scripts/release_record.py +++ b/scripts/release_record.py @@ -18,39 +18,70 @@ from __future__ import annotations import argparse import json -import re import sys import zipfile from pathlib import Path import split_pack -from common import ArtifactLockBusy, artifact_lock, write_text_atomic +from common import ( + ArtifactLockBusy, + artifact_lock, + list_registered_platforms, + write_text_atomic, +) RECORD = "release.json" -def _match_key(value: str) -> str: - """Letters and digits only: the platform is `misterfpga`, the pack MiSTer_FPGA.""" - return re.sub(r"[^a-z0-9]", "", value.lower()) - - class UnfinishedSplitError(Exception): """A pack lies beside its own parts: a split is running or was killed.""" -def build_record(dist: Path, tag: str) -> dict: +class NotAReleasePackError(Exception): + """A pack in the directory is not the full pack of a registered platform.""" + + +def release_owners(platforms_dir: str = "platforms") -> dict[str, list[str]]: + """Full pack file name to the platforms it serves, as the builder names it. + + Rebuilt from the name builder, never matched by substring: matched by + substring, RetroArch_v1.22.2_AndroidArm64V8A_BIOS_Pack.zip sorts before + the full pack and was taken for the RetroArch pack. + """ + import generate_pack + + platforms = list_registered_platforms(platforms_dir, include_archived=True) + owners: dict[str, list[str]] = {} + for platform, names in generate_pack._expected_pack_names( + platforms, platforms_dir, None, None + ).items(): + for name in names: + owners.setdefault(name, []).append(platform) + return {name: sorted(serves) for name, serves in owners.items()} + + +def build_record( + dist: Path, + tag: str, + platforms_dir: str = "platforms", + owners: dict[str, list[str]] | None = None, +) -> dict: """Read every pack of a directory, parts counted once across them. Read under the shared artifact lock, so a split that is reading its parts back cannot be recorded half done. A pack still beside its parts after that is a split that never finished: summed, it doubled the download size - and named an asset that is never published. + and named an asset that is never published. Each pack carries the + platforms it serves, so a reader finds a platform's pack by that list + after a later version or grouping has renamed the pack main would build. """ + if owners is None: + owners = release_owners(platforms_dir) with artifact_lock(str(dist), exclusive=False): - return _read_record(Path(dist), tag) + return _read_record(Path(dist), tag, owners) -def _read_record(dist: Path, tag: str) -> dict: +def _read_record(dist: Path, tag: str, owners: dict[str, list[str]]) -> dict: assets: dict[str, list[Path]] = {} for path in sorted(dist.glob("*_BIOS_Pack*.zip")): assets.setdefault(split_pack.pack_of(path.name), []).append(path) @@ -60,6 +91,12 @@ def _read_record(dist: Path, tag: str) -> dict: raise UnfinishedSplitError( f"{whole[0].name} lies beside its parts: the split did not finish" ) + foreign = sorted(name for name in assets if name not in owners) + if foreign: + raise NotAReleasePackError( + f"not the full pack of a registered platform: {', '.join(foreign)}" + " (a narrowed or older build left in the directory)" + ) packs = {} for name, paths in sorted(assets.items()): @@ -73,6 +110,7 @@ def _read_record(dist: Path, tag: str) -> dict: "extracted_size": sum(members.values()), "download_size": sum(path.stat().st_size for path in paths), "assets": [path.name for path in paths], + "platforms": owners[name], } return {"tag": tag, "packs": packs} @@ -86,15 +124,18 @@ def load_record(path: str | Path = RECORD) -> dict: return {} +def pack_name_for(platform: str, record: dict) -> str | None: + """Name of the release pack that serves a platform, if it has one.""" + for name, pack in sorted(record.get("packs", {}).items()): + if platform in pack.get("platforms", ()): + return name + return None + + def pack_for(platform: str, record: dict) -> dict | None: """The pack of the release that serves a platform, if it has one.""" - needle = _match_key(platform) - if not needle: - return None - for name, pack in sorted(record.get("packs", {}).items()): - if needle in _match_key(name): - return pack - return None + name = pack_name_for(platform, record) + return record["packs"][name] if name else None def manifest_mismatches(record: dict, install_dir: str | Path) -> list[str]: @@ -103,17 +144,15 @@ def manifest_mismatches(record: dict, install_dir: str | Path) -> list[str]: for manifest_path in sorted(Path(install_dir).glob("*.json")): manifest = json.loads(manifest_path.read_text(encoding="utf-8")) expected = manifest.get("pack_files") - if expected is None: + name = pack_name_for(manifest_path.stem, record) + if expected is None or name is None: continue - for name, pack in sorted(record.get("packs", {}).items()): - if _match_key(manifest_path.stem) not in _match_key(name): - continue - if pack["files"] != expected: - found.append( - f"{name} holds {pack['files']} files, " - f"install/{manifest_path.name} expects {expected}" - ) - break + pack = record["packs"][name] + if pack["files"] != expected: + found.append( + f"{name} holds {pack['files']} files, " + f"install/{manifest_path.name} expects {expected}" + ) return found @@ -131,6 +170,10 @@ def main() -> int: "--install-dir", default="install", type=Path, help="manifests the archives are compared with (default: %(default)s)", ) + parser.add_argument( + "--platforms-dir", default="platforms", + help="registry the pack names are rebuilt from (default: %(default)s)", + ) parser.add_argument( "--output", default=RECORD, type=Path, help="record to write (default: %(default)s)", @@ -138,8 +181,8 @@ def main() -> int: args = parser.parse_args() try: - record = build_record(args.dist, args.tag) - except (UnfinishedSplitError, ArtifactLockBusy) as exc: + record = build_record(args.dist, args.tag, args.platforms_dir) + except (UnfinishedSplitError, NotAReleasePackError, ArtifactLockBusy) as exc: print(f"Error: {exc}", file=sys.stderr) return 1 if not record["packs"]: diff --git a/tests/test_doc_surfaces.py b/tests/test_doc_surfaces.py index 6d825714..ced78597 100644 --- a/tests/test_doc_surfaces.py +++ b/tests/test_doc_surfaces.py @@ -40,12 +40,14 @@ SPLIT_RECORD = { "Batocera_1_BIOS_Pack.part1of2.zip", "Batocera_1_BIOS_Pack.part2of2.zip", ], + "platforms": ["batocera"], }, "RomM_1_BIOS_Pack.zip": { "files": 1, "extracted_size": 1024**2, "download_size": 1024**2, "assets": ["RomM_1_BIOS_Pack.zip"], + "platforms": ["romm"], }, }, } @@ -244,7 +246,7 @@ class SizesComeFromTheRecord(unittest.TestCase): tag, platform, download, extracted = quoted.groups() if tag != RECORD.get("tag"): self.skipTest(f"release.json records {RECORD.get('tag')}, not {tag}") - pack = release_record.pack_for(platform, RECORD) + pack = release_record.pack_for(platform.lower(), RECORD) self.assertEqual(download, format_size(pack["download_size"])) self.assertEqual(extracted, format_size(pack["extracted_size"])) diff --git a/tests/test_pack_counts.py b/tests/test_pack_counts.py index be858eff..aa556bfb 100644 --- a/tests/test_pack_counts.py +++ b/tests/test_pack_counts.py @@ -283,12 +283,14 @@ RECORD = { "RetroArch_Lakka_v1.22.2_BIOS_Pack.zip.001", "RetroArch_Lakka_v1.22.2_BIOS_Pack.zip.002", ], + "platforms": ["lakka", "retroarch"], }, "MiSTer_FPGA_2026-08-29_BIOS_Pack.zip": { "files": 74, "extracted_size": 17000000, "download_size": 16769418, "assets": ["MiSTer_FPGA_2026-08-29_BIOS_Pack.zip"], + "platforms": ["misterfpga"], }, }, } @@ -340,6 +342,11 @@ class TheTableDescribesTheRelease(unittest.TestCase): class ReleaseRecord(unittest.TestCase): + OWNERS = { + "Whole_1.0_BIOS_Pack.zip": ["whole"], + "Demo_2.0_BIOS_Pack.zip": ["demo"], + } + def setUp(self): self._tmp = tempfile.TemporaryDirectory() self.dist = Path(self._tmp.name) / "dist" @@ -364,7 +371,7 @@ class ReleaseRecord(unittest.TestCase): return path def test_a_pack_in_parts_is_counted_once_across_them(self): - record = release_record.build_record(self.dist, "v1") + record = release_record.build_record(self.dist, "v1", owners=self.OWNERS) self.assertEqual(record["tag"], "v1") demo = record["packs"]["Demo_2.0_BIOS_Pack.zip"] self.assertEqual(demo["files"], 6) @@ -378,7 +385,7 @@ class ReleaseRecord(unittest.TestCase): def test_a_pack_that_disagrees_with_its_manifest_is_named(self): """The count main predicts and the count the archive holds are two computations of one thing. Apart, one of them is wrong.""" - record = release_record.build_record(self.dist, "v1") + record = release_record.build_record(self.dist, "v1", owners=self.OWNERS) (self.install / "demo.json").write_text(json.dumps({"pack_files": 6})) (self.install / "whole.json").write_text(json.dumps({"pack_files": 5})) self.assertEqual( @@ -395,12 +402,38 @@ class ReleaseRecord(unittest.TestCase): {f"dir/file{n}.bin": bytes([n]) * 700 + bytes(range(256)) for n in range(6)}, ) with self.assertRaises(release_record.UnfinishedSplitError): - release_record.build_record(self.dist, "v1") + release_record.build_record(self.dist, "v1", owners=self.OWNERS) + + def test_a_narrowed_pack_is_refused_not_recorded(self): + """A pack that sorts before the full one (Android targets, Asia) was + taken for the platform's pack by a substring match.""" + self._pack("Whole_1.0_Asia_BIOS_Pack.zip", {"a.bin": b"a"}) + with self.assertRaises(release_record.NotAReleasePackError) as raised: + release_record.build_record(self.dist, "v1", owners=self.OWNERS) + self.assertIn("Whole_1.0_Asia_BIOS_Pack.zip", str(raised.exception)) + + def test_a_platform_finds_the_pack_that_serves_it_by_name(self): + record = release_record.build_record(self.dist, "v1", owners=self.OWNERS) + record["packs"]["Whole_1.0_AAA_BIOS_Pack.zip"] = dict( + record["packs"]["Whole_1.0_BIOS_Pack.zip"], files=99, platforms=["other"] + ) + self.assertEqual( + release_record.pack_name_for("whole", record), "Whole_1.0_BIOS_Pack.zip" + ) + self.assertEqual(release_record.pack_for("whole", record)["files"], 2) + + def test_the_owners_are_the_full_pack_names_the_builder_writes(self): + owners = release_record.release_owners(str(REPO_ROOT / "platforms")) + retroarch = [name for name, serves in owners.items() if "retroarch" in serves] + self.assertEqual(len(retroarch), 1, retroarch) + for tags in ("_AndroidArm64V8A", "_Asia", "_Required"): + narrowed = retroarch[0].replace("_BIOS_Pack.zip", f"{tags}_BIOS_Pack.zip") + self.assertNotIn(narrowed, owners) def test_a_directory_being_written_is_not_read(self): with common.artifact_lock(str(self.dist)), \ self.assertRaises(common.ArtifactLockBusy): - release_record.build_record(self.dist, "v1") + release_record.build_record(self.dist, "v1", owners=self.OWNERS) class TheCollectionTotalNamesTheCollection(unittest.TestCase): @@ -483,10 +516,11 @@ class TheReleaseNotesReadTheRecord(unittest.TestCase): "RetroArch_Lakka_v1.22.2_BIOS_Pack.zip": { "files": 8504, "extracted_size": 6_400_000_000, "download_size": 4_000_000_000, "assets": ["RetroArch_Lakka_v1.22.2_BIOS_Pack.part1of2.zip", - "RetroArch_Lakka_v1.22.2_BIOS_Pack.part2of2.zip"]}, + "RetroArch_Lakka_v1.22.2_BIOS_Pack.part2of2.zip"], + "platforms": ["lakka", "retroarch"]}, "EmuDeck_2.7EA_BIOS_Pack.zip": { "files": 620, "extracted_size": 3_500_000_000, "download_size": 3_000_000_000, - "assets": ["EmuDeck_2.7EA_BIOS_Pack.zip"]}, + "assets": ["EmuDeck_2.7EA_BIOS_Pack.zip"], "platforms": ["emudeck"]}, }} def render(self, record=None): diff --git a/wiki/tools.md b/wiki/tools.md index 5a42d3ac..976d16dd 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -678,7 +678,7 @@ Further options, by script: | `provenance_report.py` | `--provenance-dir`, `--write-derivable PATH` (with `--derive`, and the pipeline writes `derivable.json`) | | `refresh_stale.py` | `--freshness-arg FLAG` passes a flag through to `check_freshness.py`, repeatable | | `bundled_only.py` | `--strict` (exit 1 while a collected file only an emulator install provides remains, pipeline step 4d), `--json`, `--install-dir`. Lists every copy of such a file, the database keeping one path per content | -| `release_record.py` | `--install-dir` (manifests the archives are held against), `--output` | +| `release_record.py` | `--install-dir` (manifests the archives are held against), `--platforms-dir` (registry the full pack names are rebuilt from: any other pack in the directory is refused), `--output` | | `release_notes.py` | `--whats-new FILE` (required, appended as written), `--record`, `--platforms-dir`, `--output` | | `restore_large_files.py` | `--cache` (downloaded assets), `--gitignore`, `--db` | | `romset_recipes.py` | `--dat-recipes PATH`, a snapshot or a directory of them (default `recipes/`) | From 12cc71b63415238b0da965d45f54d3232bdc843f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:53:15 +0200 Subject: [PATCH 02/10] fix: hold the pipeline's fixed outputs for a run The pipeline locked only --output-dir while it also writes database.json, install/, README.md and docs/. A run on a scratch output directory could start beside the run on dist/ and rewrite them. Every run now holds the repository's .lock first. --- scripts/artifacts.py | 27 +++++++++++++++++++++++++++ scripts/common.py | 1 + scripts/pipeline.py | 18 +++++++++++++----- tests/test_artifact_lock.py | 30 +++++++++++++++++++++++++++++- wiki/tools.md | 7 +++++-- 5 files changed, 75 insertions(+), 8 deletions(-) diff --git a/scripts/artifacts.py b/scripts/artifacts.py index e0442d53..49627b19 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -217,6 +217,33 @@ def hold_artifact_lock(directory: str) -> Iterator[None]: else: os.environ[HELD_LOCK_ENV] = previous + +# What a pipeline run writes at fixed paths, whatever its --output-dir. +REPOSITORY_OUTPUTS = ("database.json", "install/", "README.md", "CONTRIBUTING.md", "docs/") + + +@contextlib.contextmanager +def hold_repository_outputs(root: str = ".") -> Iterator[None]: + """Hold the outputs a pipeline writes at fixed paths, for the whole run. + + The output-directory lock let a run on tmp/packtest start beside a run + 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. + """ + 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"(lock: {os.path.join(root, '.lock')}). Wait for it to finish." + ) from exc + with stack: + yield + + def _build_timestamp(db: dict | None = None) -> str: """Timestamp for generated artifacts. diff --git a/scripts/common.py b/scripts/common.py index 35b53465..396a0f23 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -2810,6 +2810,7 @@ from artifacts import ( # noqa: E402,F401 ArtifactLockBusy, artifact_lock, hold_artifact_lock, + hold_repository_outputs, _TIMESTAMP_PATTERNS, _strip_timestamps, ) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 02971049..67a60f47 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -68,6 +68,7 @@ from common import ( artifact_lock, github_token, hold_artifact_lock, + hold_repository_outputs, load_data_dir_registry, ) from refresh_data_dirs import hold_data_dirs @@ -700,13 +701,20 @@ def main() -> None: # hold lasts until this process exits: taken step by step, it let another # run purge the packs between the build and the integrity check, which # then reported every platform SKIP and passed. + # database.json, install/, README.md and docs/ sit at fixed paths, so + # every run holds them too: a run on a scratch --output-dir otherwise + # rewrote them beside the run on dist/. held = contextlib.ExitStack() - if not args.skip_packs: - try: + try: + held.enter_context( + hold_repository_outputs(str(Path(__file__).resolve().parent.parent)) + ) + if not args.skip_packs: held.enter_context(hold_artifact_lock(args.output_dir)) - except ArtifactLockBusy as exc: - print(f"ERROR: {exc}", file=sys.stderr) - sys.exit(1) + except ArtifactLockBusy as exc: + held.close() + print(f"ERROR: {exc}", file=sys.stderr) + sys.exit(1) all_ok = True total_start = time.monotonic() diff --git a/tests/test_artifact_lock.py b/tests/test_artifact_lock.py index 7e60fd0c..14e1b8de 100644 --- a/tests/test_artifact_lock.py +++ b/tests/test_artifact_lock.py @@ -18,7 +18,12 @@ from pathlib import Path REPO_ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(REPO_ROOT / "scripts")) -from common import ArtifactLockBusy, artifact_lock, hold_artifact_lock # noqa: E402 +from common import ( # noqa: E402 + ArtifactLockBusy, + artifact_lock, + hold_artifact_lock, + hold_repository_outputs, +) def _tempdir(test: unittest.TestCase) -> str: @@ -200,6 +205,29 @@ class PackLockCliTest(unittest.TestCase): self.assertIn("is in use by another run", proc.stderr) +class PipelineHoldsItsFixedOutputs(unittest.TestCase): + """A pipeline on a scratch --output-dir still rewrites database.json, + install/, README.md and docs/: only the output directory was locked.""" + + def test_a_second_pipeline_is_refused_whatever_its_output_dir(self): + scratch = _tempdir(self) + with hold_repository_outputs(str(REPO_ROOT)): + proc = subprocess.run( + [sys.executable, "scripts/pipeline.py", "--offline", "--skip-docs", + "--output-dir", scratch], + capture_output=True, text=True, cwd=str(REPO_ROOT), timeout=120, + ) + self.assertEqual(proc.returncode, 1, proc.stdout + proc.stderr) + self.assertIn("another pipeline 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): + with hold_repository_outputs(str(REPO_ROOT)), \ + self.assertRaises(ArtifactLockBusy): + with hold_repository_outputs(str(REPO_ROOT)): + pass + + 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 diff --git a/wiki/tools.md b/wiki/tools.md index 976d16dd..fdb2d81b 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -88,8 +88,11 @@ alone, run `python scripts/check_buildbot_system.py`. rewrites nothing, so a diff on a generated file always means content. - **One run per output directory.** Pack generation takes a lock on `/.lock`, and a second run on the same folder stops at once - rather than read a half-written ZIP. Experiments use a scratch - `--output-dir` under `tmp/`. + rather than read a half-written ZIP. Pack experiments with + `generate_pack.py` use a scratch `--output-dir` under `tmp/`. A pipeline + run also writes `database.json`, `install/`, `README.md` and `docs/` at + fixed paths, so it holds the repository's `.lock` as well, and a second + pipeline is refused whatever its `--output-dir`. - **Read the exit status before the output.** A generator that crashed leaves the previous artifact in place, which reads exactly like a run that changed nothing. From 563ce1ebf2d0e68e74b053b7ba3c7b0fe1438de5 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:55:50 +0200 Subject: [PATCH 03/10] fix: read fbneo dats at one checked commit The FBNeo import recorded the listing's blob sha for bytes fetched from a branch URL a CDN caches, and --fetch still read the tip. The revision is now resolved to a commit, listed and downloaded there, each DAT is held against its blob sha, and a DAT read at a named revision is labelled with it without replacing the tip's recorded blobs. --- scripts/scraper/romset_dat_importer.py | 57 +++++++++++++++--- tests/test_romset_recipes.py | 82 ++++++++++++++++++++++++++ wiki/tools.md | 11 +++- 3 files changed, 141 insertions(+), 9 deletions(-) diff --git a/scripts/scraper/romset_dat_importer.py b/scripts/scraper/romset_dat_importer.py index 78ef91c0..80e663cd 100644 --- a/scripts/scraper/romset_dat_importer.py +++ b/scripts/scraper/romset_dat_importer.py @@ -11,6 +11,7 @@ in its repository, so both are fetched without a browser: python -m scripts.scraper.romset_dat_importer --source mame --fetch python -m scripts.scraper.romset_dat_importer --source mame --fetch mame0289 python -m scripts.scraper.romset_dat_importer --source fbneo --fetch + python -m scripts.scraper.romset_dat_importer --source fbneo --fetch v1.0.0.02 python -m scripts.scraper.romset_dat_importer --source mame --pack local.dat Only sets the collection has a reason to know are kept, so the committed @@ -33,6 +34,7 @@ import os import re import shutil import sys +import urllib.parse import urllib.request import zipfile from collections.abc import Callable, Iterator @@ -374,7 +376,7 @@ MAME_LISTXML_URL = ( "https://github.com/mamedev/mame/releases/download/{tag}/{tag}lx.zip" ) MAME_LATEST_API = "https://api.github.com/repos/mamedev/mame/releases/latest" -FBNEO_DATS_API = "https://api.github.com/repos/libretro/FBNeo/contents/dats" +FBNEO_API = "https://api.github.com/repos/libretro/FBNeo" def _api_json(url: str) -> object: @@ -400,7 +402,8 @@ def latest_mame_tag() -> str: def fetch_tag(source: str, requested: str) -> str: - """The tag a ``--fetch`` names: bare, the newest MAME release.""" + """The revision a ``--fetch`` names: bare, the newest MAME release or + the tip of FBNeo's default branch.""" if source == "mame" and requested == "latest": tag = latest_mame_tag() print(f" Derniere release MAME : {tag}") @@ -408,6 +411,10 @@ def fetch_tag(source: str, requested: str) -> str: return requested +class BlobMismatchError(ValueError): + """A downloaded DAT is not the blob the repository listing names.""" + + def git_blob_sha(data: bytes) -> str: """The sha git gives a blob, so a cached file can be checked against a repository listing without any other state.""" @@ -452,7 +459,15 @@ def fetch_pack( {}, ) - listing = _api_json(FBNEO_DATS_API) + # The revision is resolved to a commit first and everything is read at + # that commit: a branch URL is served through a CDN cache, so the bytes + # could predate the listing that named their blob. + ref = "HEAD" if tag == "latest" else urllib.parse.quote(tag, safe="") + commit = _api_json(f"{FBNEO_API}/commits/{ref}") + sha = commit.get("sha") if isinstance(commit, dict) else None + if not sha: + raise BlobMismatchError(f"libretro/FBNeo has no revision {tag}") + listing = _api_json(f"{FBNEO_API}/contents/dats?ref={sha}") target = cache_dir / "fbneo-dats" target.mkdir(parents=True, exist_ok=True) blobs: dict[str, str] = {} @@ -465,7 +480,17 @@ def fetch_pack( if destination.is_file() and git_blob_sha(destination.read_bytes()) != item["sha"]: destination.unlink() _download(item["download_url"], destination) + if git_blob_sha(destination.read_bytes()) != item["sha"]: + destination.unlink() + raise BlobMismatchError( + f"{item['name']} downloaded from {item['download_url']} is not " + f"blob {item['sha']} of libretro/FBNeo {sha}" + ) blobs[item["name"]] = item["sha"] + # A DAT the revision does not carry is not read from an older fetch. + for stale in target.glob("*"): + if stale.is_file() and stale.name not in blobs: + stale.unlink() return target, blobs @@ -509,8 +534,9 @@ def main() -> int: "--fetch", nargs="?", const="latest", - help="download upstream instead of using --pack " - "(a MAME tag such as mame0289, or bare for the newest release)", + help="download upstream instead of using --pack: a MAME tag such as " + "mame0289 or an FBNeo tag, branch or commit; bare, the newest MAME " + "release or the tip of FBNeo's default branch", ) parser.add_argument( "--cache-dir", default=".cache/dats", @@ -547,11 +573,24 @@ def main() -> int: args = parser.parse_args() blobs: dict[str, str] = {} + # FBNeo's DAT headers keep one version across commits, so a DAT read at + # a named revision is labelled with it: under the bare header name it + # merged into the tip's recipes. Its blobs describe that revision, not + # the upstream state the freshness check compares with, and are not + # recorded. + revision = "" if args.fetch: - pack, blobs = fetch_pack( - args.source, fetch_tag(args.source, args.fetch), Path(args.cache_dir) - ) + try: + pack, blobs = fetch_pack( + args.source, fetch_tag(args.source, args.fetch), Path(args.cache_dir) + ) + except BlobMismatchError as exc: + print(f"ERROR: {exc}", file=sys.stderr) + return 1 print(f" Recupere {pack}") + if args.source == "fbneo" and args.fetch != "latest": + revision = args.fetch + blobs = {} elif args.pack: pack = Path(args.pack) else: @@ -596,6 +635,8 @@ def main() -> int: if not dat.roms: skipped += 1 continue + if revision: + dat.name = f"{dat.name or member} @ {revision}" found = recipe_entries(dat, keep, targets) if not found: continue diff --git a/tests/test_romset_recipes.py b/tests/test_romset_recipes.py index 3e5d30ed..e4d57feb 100644 --- a/tests/test_romset_recipes.py +++ b/tests/test_romset_recipes.py @@ -694,6 +694,88 @@ class AnOversizedDatIsNamed(unittest.TestCase): +FBNEO_DAT = ( + b'\n
FinalBurn Neo - Demo' + b"1.0.0.03
" + b'Demo BIOS' + b'' + b"\n" +) + + +class FbneoFetchReadsOneRevision(unittest.TestCase): + """The FBNeo import listed blobs at the tip but downloaded branch URLs a + CDN caches, and --fetch read the tip whatever was named.""" + + def setUp(self): + from scripts.scraper import romset_dat_importer as importer + + self.importer = importer + self._tmp = tempfile.TemporaryDirectory(dir=TMP_ROOT) + self.dir = Path(self._tmp.name) + self.calls: list[str] = [] + self.served = FBNEO_DAT + + def tearDown(self): + self._tmp.cleanup() + + def _api(self, url: str) -> object: + self.calls.append(url) + if "/commits/" in url: + return {"sha": "c0ffee"} + return [{ + "type": "file", "name": "Demo.dat", + "sha": self.importer.git_blob_sha(FBNEO_DAT), + "download_url": "https://raw.githubusercontent.com/libretro/FBNeo/c0ffee/dats/Demo.dat", + }] + + def _download(self, url: str, destination: Path) -> Path: + destination.write_bytes(self.served) + return destination + + def _patched(self): + from unittest import mock + + return mock.patch.multiple( + self.importer, _api_json=self._api, _download=self._download + ) + + def test_bytes_that_are_not_the_listed_blob_are_refused(self): + self.served = FBNEO_DAT.replace(b"1.0.0.03", b"1.0.0.02") + with self._patched(), self.assertRaises(self.importer.BlobMismatchError): + self.importer.fetch_pack("fbneo", "latest", self.dir) + self.assertFalse((self.dir / "fbneo-dats" / "Demo.dat").exists()) + + def test_the_named_revision_is_the_one_listed(self): + with self._patched(): + target, blobs = self.importer.fetch_pack("fbneo", "v1.0.0.02", self.dir) + self.assertEqual( + self.calls, + [f"{self.importer.FBNEO_API}/commits/v1.0.0.02", + f"{self.importer.FBNEO_API}/contents/dats?ref=c0ffee"], + ) + self.assertEqual(list(blobs), ["Demo.dat"]) + + def test_a_revision_import_is_labelled_and_leaves_the_tip_blobs(self): + import contextlib + import io + from unittest import mock + + output = self.dir / "fbneo.json" + output.write_text(json.dumps({"upstream": {"blobs": {"Demo.dat": "tip"}}})) + argv = ["romset_dat_importer", "--source", "fbneo", "--fetch", "v1.0.0.02", + "--all", "--cache-dir", str(self.dir), "--output", str(output)] + with self._patched(), mock.patch.object(sys, "argv", argv), \ + contextlib.redirect_stdout(io.StringIO()): + self.assertEqual(self.importer.main(), 0) + snapshot = json.loads(output.read_text()) + self.assertEqual(snapshot["upstream"]["blobs"], {"Demo.dat": "tip"}) + self.assertEqual( + [entry["dat"] for entry in snapshot["entries"]], + ["FinalBurn Neo - Demo @ v1.0.0.02"], + ) + + class LooseRomsAreAtoms(unittest.TestCase): """AtomPool read only ZIP members: nmk004.zip stayed unidentified while its one ROM sat loose in bios/Arcade/FBNeo/nmk004.bin.""" diff --git a/wiki/tools.md b/wiki/tools.md index fdb2d81b..840fdbc8 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -651,7 +651,7 @@ python scripts/refresh_stale.py --only platforms,targets --jobs 6 | `generate_site.py` | Generate all MkDocs site pages (this documentation) | | `validate_site.py` | Validate rendered metadata, headings, image alternatives, JSON-LD, local resources, links and fragments | | `romset_recipes.py` | Identify which emulator version an arcade archive matches, and rebuild a pinned archive from ROMs already held | -| `scraper/romset_dat_importer.py` | Fetch and import per-set recipes from MAME `-listxml` or FBNeo DATs into `recipes/`. `--fetch` alone takes the newest MAME release, and FBNeo DATs are refreshed by git blob sha | +| `scraper/romset_dat_importer.py` | Fetch and import per-set recipes from MAME `-listxml` or FBNeo DATs into `recipes/`. `--fetch` alone takes the newest MAME release, and FBNeo DATs are refreshed by git blob sha. `--fetch ` reads FBNeo at that tag, branch or commit | | `deterministic_zip.py` | Rebuild MAME BIOS ZIPs deterministically (same ROMs = same hash) | | `torrentzip.py` | Build TorrentZip archives for MAME/FBNeo ROM sets (archive bytes depend only on contents) | | `crypto_verify.py` | 3DS RSA signature and AES crypto verification | @@ -756,8 +756,17 @@ FBNeo keeps its DATs in the repository. python -m scripts.scraper.romset_dat_importer --source mame --fetch mame0289 python -m scripts.scraper.romset_dat_importer --source mame --fetch mame0250 python -m scripts.scraper.romset_dat_importer --source fbneo --fetch +python -m scripts.scraper.romset_dat_importer --source fbneo --fetch v1.0.0.02 ``` +FBNeo DATs are read at one commit: the revision named (tag, branch or commit, +the default branch when bare) is resolved first, and every DAT downloaded is +held against the blob sha the listing gives, so a cached branch URL cannot +serve older bytes. Their headers keep one version across commits, so DATs read +at a named revision are labelled ` @ ` and accumulate +beside the tip's. Only an import of the tip records the blobs the freshness +check compares with. + Snapshots land in `recipes/`, not `provenance/`: the latter holds dump catalogues and a recipe is not one. Versions accumulate rather than replace each other, because a platform pins the archive of whichever version its list was built From 9bb1a73f910aba18a579ead735e7774bbd4d7e74 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:56:49 +0200 Subject: [PATCH 04/10] fix: hold every data tree, present or not export_native and cross_reference read data/ without the shared hold, so a refresh could swap a tree mid-walk. hold_data_dirs skipped a cache not yet on disk, so a refresh could install it between verify and the packs of one pipeline run. Both now hold every registered tree. --- scripts/cross_reference.py | 9 +++++++++ scripts/export_native.py | 11 +++++++++++ scripts/refresh_data_dirs.py | 6 ++++-- tests/test_audit_regressions.py | 18 +++++++++++++++++- 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/scripts/cross_reference.py b/scripts/cross_reference.py index f82966b5..35486f37 100644 --- a/scripts/cross_reference.py +++ b/scripts/cross_reference.py @@ -959,6 +959,15 @@ def main() -> None: if args.platform: require_platform(args.platform, args.platforms_dir) + import contextlib + + from refresh_data_dirs import hold_data_dirs + + # Files resolve against data/: the trees stay put until the process + # ends, which releases the hold. + held = contextlib.ExitStack() + hold_data_dirs(held, load_data_dir_registry(args.platforms_dir)) + profiles = load_emulator_profiles(args.emulators_dir) emulator = args.emulator if emulator: diff --git a/scripts/export_native.py b/scripts/export_native.py index 1ccddb68..47db1e0a 100644 --- a/scripts/export_native.py +++ b/scripts/export_native.py @@ -898,6 +898,17 @@ def main() -> None: require_platform(args.platform, args.platforms_dir) platforms = [args.platform] + if not args.refresh_cache: + import contextlib + + from refresh_data_dirs import hold_data_dirs + + # Removal candidates are proved against data/: a refresh swapping a + # tree mid-walk listed entries only that tree holds as removable. + # The process exit releases the hold. + held = contextlib.ExitStack() + hold_data_dirs(held, load_data_dir_registry(args.platforms_dir)) + sys.exit( run( platforms, diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index f1f0c4b5..53e40891 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -99,11 +99,13 @@ def hold_data_dirs(stack: contextlib.ExitStack, registry: dict) -> None: later. A refresh swapping the tree in between shipped bytes nobody verified, or died on a file the new tree no longer has. A refresh waits for the holders. Never called before a refresh in the same process: - the exclusive lock would wait on this one. + the exclusive lock would wait on this one. A cache not yet on disk is + held too: the lock sits beside the tree, and skipped, a refresh could + install the tree after verify ran and before the packs read it. """ for entry in registry.values(): cache = entry.get("local_cache", "") - if cache and os.path.isdir(cache): + if cache: stack.enter_context(cache_lock(cache, shared=True)) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index d12efd68..f5afb746 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -3761,6 +3761,21 @@ class ADataDirectoryIsHeldWhileRead(unittest.TestCase): self.assertFalse(exclusive_free()) self.assertTrue(exclusive_free()) + def test_a_cache_not_yet_on_disk_is_held_too(self): + """Skipped while absent, a tree a refresh installed mid-run was + missing for verify and present for the packs of the same run.""" + import fcntl + + from scripts import refresh_data_dirs + + cache = self.tmp / "absent" + lock = cache.with_name(f".{cache.name}.lock") + with contextlib.ExitStack() as held: + refresh_data_dirs.hold_data_dirs(held, {"absent": {"local_cache": str(cache)}}) + self.assertFalse(cache.exists()) + with open(lock, "a") as handle, self.assertRaises(BlockingIOError): + fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + def test_both_entry_points_hold_the_trees(self): pack = (ROOT / "scripts" / "generate_pack.py").read_text() verify_src = (ROOT / "scripts" / "verify.py").read_text() @@ -5446,7 +5461,8 @@ class ReportsHoldTheDataTrees(unittest.TestCase): def test_each_entry_point_holds_them(self): import ast - for script in ("generate_readme.py", "generate_site.py", "slots.py"): + for script in ("generate_readme.py", "generate_site.py", "slots.py", + "export_native.py", "cross_reference.py"): tree = ast.parse((ROOT / "scripts" / script).read_text(encoding="utf-8")) main = next( node for node in tree.body From cc57d44de5b482c5ae63c82a2fa752e21e9f83a1 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:58:58 +0200 Subject: [PATCH 05/10] fix: order inherited rescrapes, end on verify refresh_stale ran a parent's rescrape and its children's truth and export jobs side by side, so a child read the parent mid-rewrite. A job now starts once the rescrapes of its ancestors have ended, and a child no row names gets its export rewritten after them. A rescrape chain ends on verify.py --platform, whose summary closes the report. --- scripts/refresh_stale.py | 149 +++++++++++++++++++++++++++++++++--- tests/test_refresh_stale.py | 52 ++++++++++++- wiki/tools.md | 11 ++- 3 files changed, 197 insertions(+), 15 deletions(-) diff --git a/scripts/refresh_stale.py b/scripts/refresh_stale.py index fa951404..3946d771 100755 --- a/scripts/refresh_stale.py +++ b/scripts/refresh_stale.py @@ -11,7 +11,10 @@ Mapping (stale -> command): platforms/ python -m scripts.scraper._scraper -o platforms/.yml then the native refresh below: the original - the export patches must be the one just scraped + the export patches must be the one just scraped, + then python scripts/verify.py --platform , + whose summary the report prints: a rescrape can + add systems whose files are missing native/ python scripts/generate_truth.py --platform , then python scripts/export_native.py --platform --refresh-cache, then the export itself, which @@ -25,6 +28,10 @@ Mapping (stale -> command): catalogs/fbneo recipes python -m scripts.scraper.romset_dat_importer --source fbneo --fetch +A platform that inherits from a rescraped one (retropie and lakka from +retroarch) reads the parent's file: its jobs start once the parent's chain +has ended, and its export is rewritten even when no row named it. + Surfaced only (no refresh): data rows that are not a _data_dirs.yml key (the registry itself, buildbot assets it does not track), coreinfo gaps, redump/no-intro/tosec packs, CI pins (install.py SHA-256, PyPI, pinned actions), profile_sync. Each needs @@ -45,7 +52,7 @@ import re import subprocess import sys import time -from concurrent.futures import ThreadPoolExecutor, as_completed +from concurrent.futures import FIRST_COMPLETED, ThreadPoolExecutor, wait from dataclasses import dataclass from functools import lru_cache from pathlib import Path @@ -72,6 +79,8 @@ class Job: log_path: Path # Commands run after `command`, in order, only while each one succeeds. then: tuple[tuple[str, ...], ...] = () + # Platforms whose rescrape job must end before this one starts. + after: frozenset[str] = frozenset() @dataclass(frozen=True) @@ -94,6 +103,30 @@ def _load_platform_registry() -> dict[str, dict]: return (yaml.safe_load(fh) or {}).get("platforms") or {} +def _platform_parents(registry: dict[str, dict]) -> dict[str, str]: + """Platform to the platform its YAML inherits from.""" + import yaml + + parents: dict[str, str] = {} + for name in registry: + path = REPO_ROOT / "platforms" / f"{name}.yml" + if not path.is_file(): + continue + with path.open(encoding="utf-8") as fh: + parent = (yaml.safe_load(fh) or {}).get("inherits") + if parent: + parents[name] = str(parent) + return parents + + +def _ancestors(platform: str, parents: dict[str, str]) -> set[str]: + found: set[str] = set() + while platform in parents and parents[platform] not in found: + platform = parents[platform] + found.add(platform) + return found + + @lru_cache(maxsize=1) def _data_dir_keys() -> frozenset[str]: """The entries refresh_data_dirs.py --key accepts.""" @@ -129,15 +162,21 @@ def _slug(subject: str) -> str: def plan_jobs( - findings: list[dict], registry: dict[str, dict] + findings: list[dict], + registry: dict[str, dict], + parents: dict[str, str] | None = None, ) -> tuple[list[Job], list[dict], list[dict]]: """Split findings into (refreshable jobs, surfaced-only, ignored). Surfaced-only entries carry stale state that only a human can resolve (new .info file, manual DAT download, PyPI bump). Ignored entries are anything not STALE (OK, SKIPPED, UNKNOWN, ERROR). ERROR is also surfaced - so the final report mentions it. + so the final report mentions it. ``parents`` maps a platform to the one + it inherits from: a child's jobs wait for its rescraped ancestors, and a + child no row names gets its export rewritten after them. """ + if parents is None: + parents = _platform_parents(registry) jobs: list[Job] = [] seen: set[tuple[str, str]] = set() surfaced: list[dict] = [] @@ -189,16 +228,92 @@ def plan_jobs( tuple(_truth_refresh(subject)), tuple(_native_refresh(subject)), tuple(_export_refresh(subject)), + tuple(_verify(subject)), ) elif area == "native": then = (tuple(_native_refresh(subject)), tuple(_export_refresh(subject))) + after: frozenset[str] = frozenset() + if area in ("platforms", "native"): + after = frozenset(_ancestors(subject, parents) & rescraped) jobs.append( - Job(area=area, subject=subject, command=command, log_path=log, then=then) + Job(area=area, subject=subject, command=command, log_path=log, + then=then, after=after) ) + # The export of a child is built from its parent's file: rewritten once + # the parent is, or the next check reports it stale. + covered = {j.subject for j in jobs if j.area in ("platforms", "native")} + for child in sorted(parents): + after = frozenset(_ancestors(child, parents) & rescraped) + if not after or child in covered: + continue + jobs.append(Job( + area="native", + subject=child, + command=_truth_refresh(child), + log_path=LOG_DIR / f"native__{_slug(child)}.log", + then=(tuple(_native_refresh(child)), tuple(_export_refresh(child))), + after=after, + )) + return jobs, surfaced, ignored +def _verify(platform: str) -> list[str]: + """The check a rescraped platform file is read with before it is kept.""" + return [sys.executable, "scripts/verify.py", "--platform", platform] + + +VERIFY_MARK = "scripts/verify.py --platform" + + +def verify_summary(path: Path) -> str: + """First line verify printed in a job log, empty when it did not run.""" + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + return "" + lines = text.splitlines() + for index, line in enumerate(lines): + if line.startswith("$ ") and VERIFY_MARK in line: + return next( + (rest.strip() for rest in lines[index + 1:] + if rest.strip() and not rest.startswith("$ ")), + "", + ) + return "" + + +def run_jobs(jobs: list[Job], env: dict[str, str], workers: int) -> list[JobResult]: + """Run jobs in parallel, each once the rescrapes it waits for have ended. + + A failed parent still releases its children: its file is unchanged, and + the child reads it as it is. + """ + results: list[JobResult] = [] + ended: set[str] = set() + waiting = list(jobs) + with ThreadPoolExecutor(max_workers=max(1, workers)) as pool: + running: dict = {} + while waiting or running: + ready = [job for job in waiting if job.after <= ended] + for job in ready: + waiting.remove(job) + running[pool.submit(run_job, job, env)] = job + if not running: + raise RuntimeError( + "jobs wait on rescrapes that are not planned: " + + ", ".join(sorted(j.subject for j in waiting)) + ) + done, _ = wait(running, return_when=FIRST_COMPLETED) + for future in done: + job = running.pop(future) + results.append(future.result()) + if job.area == "platforms": + ended.add(job.subject) + return results + + def _truth_refresh(platform: str) -> list[str]: return [sys.executable, "scripts/generate_truth.py", "--platform", platform] @@ -380,6 +495,22 @@ def render( if r.returncode != 0 and r.tail: lines.append(f" -> {r.tail}") + rescraped = sorted( + (r for r in results if r.job.area == "platforms" and r.returncode == 0), + key=lambda r: r.job.subject, + ) + if rescraped: + lines.append( + "\n[verify] a rescrape can add systems: read each report " + "before keeping platforms/.yml" + ) + for r in rescraped: + summary = verify_summary(r.job.log_path) or "(no verify output)" + lines.append(f" {r.job.subject:12} {summary}") + lines.append( + f" {'':12} python scripts/verify.py --platform {r.job.subject}" + ) + if surfaced: lines.append("\n[manual review needed]") for f in surfaced: @@ -457,6 +588,8 @@ def main() -> int: f"{len(ignored)} already clean/skipped") for job in sorted(jobs, key=lambda j: (j.area, j.subject)): print(f" {job.area:10} {job.subject:28} {' '.join(job.command)}") + if job.after: + print(f" {'':10} {'':28} after {', '.join(sorted(job.after))}") for command in job.then: print(f" {'':10} {'':28} then {' '.join(command)}") if surfaced: @@ -468,11 +601,7 @@ def main() -> int: LOG_DIR.mkdir(parents=True, exist_ok=True) env = _github_token_env() - results: list[JobResult] = [] - with ThreadPoolExecutor(max_workers=max(1, args.jobs)) as pool: - futures = [pool.submit(run_job, job, env) for job in jobs] - for future in as_completed(futures): - results.append(future.result()) + results = run_jobs(jobs, env, args.jobs) if args.json: payload = { diff --git a/tests/test_refresh_stale.py b/tests/test_refresh_stale.py index 3b7b01eb..446bc3b2 100644 --- a/tests/test_refresh_stale.py +++ b/tests/test_refresh_stale.py @@ -30,8 +30,8 @@ def finding(area: str, subject: str, status: str = "STALE") -> dict: class Planning(unittest.TestCase): - def _plan(self, *findings: dict): - return rs.plan_jobs(list(findings), REGISTRY) + def _plan(self, *findings: dict, parents: dict | None = None): + return rs.plan_jobs(list(findings), REGISTRY, parents or {}) def test_a_stale_platform_is_rescraped_then_its_original_is_refreshed(self): jobs, surfaced, _ = self._plan(finding("platforms", "retrobat")) @@ -47,9 +47,28 @@ class Planning(unittest.TestCase): [list(c[1:]) for c in jobs[0].then], [["scripts/generate_truth.py", "--platform", "retrobat"], ["scripts/export_native.py", "--platform", "retrobat", "--refresh-cache"], - ["scripts/export_native.py", "--platform", "retrobat"]], + ["scripts/export_native.py", "--platform", "retrobat"], + ["scripts/verify.py", "--platform", "retrobat"]], ) + def test_a_child_waits_for_its_rescraped_parent(self): + """retropie inherits retroarch: run beside the parent's rescrape, its + truth read the old parent and its export was refused as stale.""" + registry = dict(REGISTRY, retroarch={"scraper": "libretro"}, + retropie={"scraper": "retropie"}) + parents = {"retropie": "retroarch", "lakka": "retroarch"} + jobs, _, _ = rs.plan_jobs( + [finding("platforms", "retroarch"), finding("platforms", "retropie"), + finding("platforms", "retrobat")], + registry, parents, + ) + after = {(j.area, j.subject): j.after for j in jobs} + self.assertEqual(after[("platforms", "retroarch")], frozenset()) + self.assertEqual(after[("platforms", "retrobat")], frozenset()) + self.assertEqual(after[("platforms", "retropie")], frozenset({"retroarch"})) + # lakka has no row and no scraper: its export follows the parent's. + self.assertEqual(after[("native", "lakka")], frozenset({"retroarch"})) + def test_a_native_row_of_a_rescraped_platform_is_not_a_second_job(self): """Two jobs would race: the refresh must follow the scrape.""" jobs, _, ignored = self._plan( @@ -132,6 +151,33 @@ class Planning(unittest.TestCase): class Running(unittest.TestCase): + def test_a_child_starts_after_its_parent_ends(self): + with tempfile.TemporaryDirectory() as directory: + order = Path(directory) / "order" + def job(area, subject, after=()): + code = f"open({str(order)!r}, 'a').write({subject!r} + chr(10))" + if subject == "parent": + code = "import time; time.sleep(0.5); " + code + return rs.Job(area, subject, [sys.executable, "-c", code], + Path(directory) / f"{subject}.log", (), frozenset(after)) + rs.run_jobs([job("native", "child", ["parent"]), + job("platforms", "parent")], {}, 4) + self.assertEqual(order.read_text().split(), ["parent", "child"]) + + def test_the_report_prints_verify_after_a_rescrape(self): + (REPO_ROOT / "tmp").mkdir(exist_ok=True) + with tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") as directory: + log = Path(directory) / "x.log" + log.write_text( + "$ python -m scripts.scraper.x_scraper\nscraped\n" + "$ python scripts/verify.py --platform x\n" + "X: 10/12 OK, 2 missing [md5]\n" + ) + job = rs.Job("platforms", "x", ["true"], log) + text = rs.render([rs.JobResult(job, 0, 1.0, "")], [], []) + self.assertIn("X: 10/12 OK, 2 missing [md5]", text) + self.assertIn("python scripts/verify.py --platform x", text) + def _job(self, directory: str, command: list[str], then=()) -> rs.Job: return rs.Job("platforms", "x", command, Path(directory) / "x.log", then) diff --git a/wiki/tools.md b/wiki/tools.md index 840fdbc8..f8d9e3d4 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -611,8 +611,8 @@ committed. | Stale row | Command | |-----------|---------| -| `platforms/` | the platform scraper, then the `native` refresh for the same platform | -| `native/` | `export_native.py --platform --refresh-cache` | +| `platforms/` | the platform scraper, then the `native` refresh for the same platform, then `verify.py --platform ` | +| `native/` | `generate_truth.py --platform `, `export_native.py --platform --refresh-cache`, then the export itself | | `targets/` | the target scraper | | `data/` | `refresh_data_dirs.py --key --force` | | `catalogs/mame recipes`, `catalogs/fbneo recipes` | `scraper/romset_dat_importer.py --source --fetch` | @@ -623,6 +623,13 @@ pins, and `profile_sync`. A failed scraper leaves its cached original untouched. Each job writes its output to `tmp/refresh_stale/`, and a GitHub token is taken from `GITHUB_TOKEN` or from `gh auth token`. +A platform that inherits from a rescraped one (`lakka` and `retropie` from +`retroarch`) reads the parent's file, so its jobs start once the parent's have +ended, and its export is rewritten even when no row named it. A rescrape can +add systems whose files the collection lacks: the report ends with the +`verify.py --platform` summary of every rescraped platform, to read before +keeping the new YAML. + ```bash python scripts/refresh_stale.py --dry-run python scripts/refresh_stale.py From f3586001ab125f7686a851d4bf31132f2636c9e3 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:00:36 +0200 Subject: [PATCH 06/10] fix: fail freshness on unknown, name the refresher check_freshness exited 0 when no upstream answered. An unknown row now holds the exit code, and a profile skipped by nature is reported skipped so it does not hold it forever. --only names the valid areas, a stale native row names its platform, and the summary names refresh_stale.py. --- scripts/check_freshness.py | 47 ++++++++++++++++++++++++++++++++--- scripts/refresh_stale.py | 4 ++- tests/test_check_freshness.py | 30 +++++++++++++++++++++- wiki/tools.md | 10 ++++++-- 4 files changed, 83 insertions(+), 8 deletions(-) diff --git a/scripts/check_freshness.py b/scripts/check_freshness.py index 604fcfcb..689a9d22 100644 --- a/scripts/check_freshness.py +++ b/scripts/check_freshness.py @@ -518,7 +518,10 @@ def check_native( wanted, cache_dir / name, recorded, index, _transcribed_at(platforms_dir, name) ) if reason: - reason += ", export_native.py --refresh-cache fetches it" + reason += ( + f", export_native.py --platform {name} --refresh-cache" + " fetches it, then the export is written again" + ) else: reason = published_export_state( name, exporter_class, truth_dir, platforms_dir, cache_dir, published @@ -1299,7 +1302,15 @@ def summarize_profile_sync(report: object) -> list[Finding]: remote=announced, detail=detail)) continue if row.get("skipped"): - findings.append(Finding("profiles", name, UNKNOWN, detail=str(row["skipped"]))) + # A skip profile_sync counts as a defect (no repository answers, + # no resolvable pin) is unknown. One that is the profile's nature + # (an upstream declared gone, a host no forge API serves, no + # source_ref) is never checkable and does not hold the exit code. + from profile_sync import ProfileReport + + reason = str(row["skipped"]) + status = UNKNOWN if reason.startswith(ProfileReport.UNCHECKABLE) else SKIPPED + findings.append(Finding("profiles", name, status, detail=reason)) continue review = row.get("needs_review") or 0 pin, head = str(row.get("pin") or ""), str(row.get("head") or "") @@ -1360,6 +1371,13 @@ def render(findings: list[Finding]) -> str: counts = {status: sum(1 for f in findings if f.status == status) for status in STATUSES} summary = ", ".join(f"{n} {status.lower()}" for status, n in counts.items() if n) lines.append(f"\nFRESHNESS: {summary}") + if counts[STALE]: + lines.append( + "refresh_stale.py runs the refresher behind every stale row " + "(--dry-run lists them first)" + ) + if counts[UNKNOWN]: + lines.append("an unknown row is an upstream that could not be read: not up to date") return "\n".join(lines) @@ -1420,7 +1438,7 @@ def main() -> int: areas = tuple(a.strip() for a in args.only.split(",")) if args.only else AREAS unknown = sorted(set(areas) - set(AREAS)) if unknown: - parser.error(f"unknown area: {', '.join(unknown)}") + parser.error(unknown_areas_message(unknown)) platforms_dir = Path(args.platforms_dir) profiles = load_emulator_profiles(args.emulators_dir, skip_aliases=False) @@ -1455,7 +1473,28 @@ def main() -> int: print(json.dumps([asdict(f) for f in findings], indent=2)) else: print(render(findings)) - return 1 if any(f.status in (STALE, ERROR) for f in findings) else 0 + return exit_code(findings) + + +def exit_code(findings: list[Finding]) -> int: + """Zero only when every upstream asked answered and nothing is stale. + + An unknown row is an upstream that could not be read: a run without + network gave 28 of them and exited 0, the answer of a fresh collection. + """ + return 1 if any(f.status in (STALE, ERROR, UNKNOWN) for f in findings) else 0 + + +def unknown_areas_message(unknown: list[str]) -> str: + """The refusal of --only, naming the areas it accepts.""" + import difflib + + hints = [ + f"{name} (did you mean {close[0]}?)" if close else name + for name in unknown + for close in [difflib.get_close_matches(name, AREAS, n=1)] + ] + return f"unknown area: {', '.join(hints)}. Areas: {', '.join(AREAS)}" if __name__ == "__main__": diff --git a/scripts/refresh_stale.py b/scripts/refresh_stale.py index 3946d771..d79b6488 100755 --- a/scripts/refresh_stale.py +++ b/scripts/refresh_stale.py @@ -570,7 +570,9 @@ def main() -> int: areas = tuple(a.strip() for a in args.only.split(",") if a.strip()) unknown = sorted(set(areas) - set(all_areas)) if unknown: - parser.error(f"unknown area: {', '.join(unknown)}") + parser.error( + f"unknown area: {', '.join(unknown)}. Areas: {', '.join(all_areas)}" + ) else: areas = all_areas diff --git a/tests/test_check_freshness.py b/tests/test_check_freshness.py index 664fc636..bd781c81 100644 --- a/tests/test_check_freshness.py +++ b/tests/test_check_freshness.py @@ -377,7 +377,8 @@ class ProfileSummaryTests(unittest.TestCase): {"name": "a", "pin": "1" * 40, "head": "1" * 40, "needs_review": 0}, {"name": "b", "pin": "1" * 40, "head": "2" * 40, "needs_review": 0}, {"name": "c", "pin": "1" * 40, "head": "2" * 40, "needs_review": 3}, - {"name": "d", "skipped": "host does not resolve"}, + {"name": "d", "skipped": "no declared repository answers"}, + {"name": "e", "skipped": "upstream gone (declared): archived 2019"}, ] findings = cf.summarize_profile_sync(report) by_subject = {f.subject: f for f in findings} @@ -385,6 +386,8 @@ class ProfileSummaryTests(unittest.TestCase): self.assertEqual(by_subject["profile_sync"].local, "1 at tip, 1 anchored past the tip") self.assertEqual(by_subject["c"].status, cf.STALE) self.assertEqual(by_subject["d"].status, cf.UNKNOWN) + # Never checkable by nature: it does not hold the exit code. + self.assertEqual(by_subject["e"].status, cf.SKIPPED) self.assertNotIn("a", by_subject) self.assertNotIn("b", by_subject) @@ -442,6 +445,31 @@ class RepositoryWiringTests(unittest.TestCase): self.assertIn("[ci]", text) self.assertIn("[data]", text) + def test_an_unanswered_upstream_is_not_fresh(self): + """With no network every data row was unknown and the run exited 0, + the answer of a collection with nothing to refresh.""" + self.assertEqual(cf.exit_code([cf.Finding("data", "a", cf.UNKNOWN)]), 1) + self.assertEqual(cf.exit_code([cf.Finding("data", "a", cf.SKIPPED), + cf.Finding("data", "b", cf.OK)]), 0) + + def test_stale_rows_name_the_command_that_refreshes_them(self): + text = cf.render([cf.Finding("native", "batocera", cf.STALE)]) + self.assertIn("refresh_stale.py", text) + self.assertNotIn("refresh_stale.py", cf.render([cf.Finding("ci", "a", cf.OK)])) + + def test_a_stale_original_names_its_platform(self): + """The hint refetched the originals of all twelve platforms to fix + one, and wrote no export.""" + source = Path(cf.__file__).read_text(encoding="utf-8") + hint = source.index('" fetches it, then the export is written again"') + self.assertIn("--platform {name} --refresh-cache", source[hint - 120:hint]) + + def test_an_unknown_area_names_the_valid_ones(self): + message = cf.unknown_areas_message(["platform"]) + self.assertIn("did you mean platforms?", message) + for area in cf.AREAS: + self.assertIn(area, message) + def test_a_target_of_every_declared_core_does_not_age(self): """RetroBat, RetroDECK and RomM name one target that runs every core they declare. With nothing hand-kept, there is no date to watch.""" diff --git a/wiki/tools.md b/wiki/tools.md index f8d9e3d4..173caa6f 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -600,8 +600,14 @@ python scripts/check_freshness.py --offline # local ages only python scripts/check_freshness.py --record-coreinfo virtualjaguar # .info read, record it ``` -The exit code is non-zero when anything is stale. Emulator profiles are the -one layer left to `profile_sync.py`, which the `--profiles` flag folds in. +The exit code is non-zero when anything is stale or an upstream asked could +not be read (`UNKNOWN`), so a run that reached no remote does not pass for a +fresh collection. `--offline` asks no upstream and its rows are `SKIPPED`. +Emulator profiles are the one layer left to `profile_sync.py`, which the +`--profiles` flag folds in. A profile skipped by nature (upstream declared +gone, a host no forge API serves, no `source_ref`) is `SKIPPED`, one no +declared repository answers is `UNKNOWN`. When rows are stale, the summary +names `refresh_stale.py`, which runs the refresher behind each of them. ### refresh_stale.py From 6c076884ed650585600e4b05c15f607be504c9a9 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:02:05 +0200 Subject: [PATCH 07/10] fix: let auto_fetch prove sha256 and crc32 entries verify_content read sha1 then md5 only, so an entry pinned by sha256 or crc32 could be searched and never accepted, and one without any hash was announced as searched. The strongest declared hash now decides, crc32 counts with its size, the database lookup reads sha256 and crc32, and an entry with nothing to prove a download by is reported, not searched. --- scripts/auto_fetch.py | 84 +++++++++++++++++++++++++++------ tests/test_audit_regressions.py | 46 ++++++++++++++++++ wiki/tools.md | 2 +- 3 files changed, 116 insertions(+), 16 deletions(-) diff --git a/scripts/auto_fetch.py b/scripts/auto_fetch.py index cb04c680..c39587b7 100644 --- a/scripts/auto_fetch.py +++ b/scripts/auto_fetch.py @@ -25,6 +25,7 @@ import sys import urllib.error import urllib.parse import urllib.request +import zlib from pathlib import Path sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) @@ -101,6 +102,8 @@ def find_missing(config: dict, db: dict) -> list[dict]: "system": sys_id, "sha1": file_entry.get("sha1"), "md5": file_entry.get("md5"), + "sha256": file_entry.get("sha256"), + "crc32": file_entry.get("crc32"), "size": file_entry.get("size"), "destination": file_entry.get("destination", name), }) @@ -112,7 +115,9 @@ def find_missing(config: dict, db: dict) -> list[dict]: "system": u.get("system") or "", "sha1": u.get("sha1"), "md5": u.get("md5"), - "size": None, + "sha256": u.get("sha256"), + "crc32": u.get("crc32"), + "size": u.get("size"), "destination": u.get("path") or u["name"], "emulator": u.get("emulator", ""), }) @@ -131,14 +136,45 @@ def _accepted(entry: dict, field: str) -> set[str]: return {str(v).strip().lower() for v in values if str(v).strip()} +def _crc32s(entry: dict) -> set[str]: + return {value.removeprefix("0x").zfill(8) for value in _accepted(entry, "crc32")} + + +def _size(entry: dict) -> int | None: + try: + return int(entry["size"]) if entry.get("size") is not None else None + except (TypeError, ValueError): + return None + + +def verifiable(entry: dict) -> bool: + """Whether a download can be proved to be this file. + + A sha1, sha256 or md5 proves it, a crc32 only with the size beside it, + the way the resolver accepts one. Without any of them a search can find + a file of that name and never accept it. + """ + if any(_accepted(entry, field) for field in ("sha1", "sha256", "md5")): + return True + return bool(_crc32s(entry)) and _size(entry) is not None + + def verify_content(data: bytes, expected: dict) -> bool: - """Verify downloaded content matches one of the expected hashes.""" - sha1s = _accepted(expected, "sha1") - if sha1s: - return hashlib.sha1(data).hexdigest() in sha1s - md5s = _accepted(expected, "md5") - if md5s: - return hashlib.md5(data).hexdigest() in md5s + """Verify downloaded content matches one of the expected hashes. + + The strongest hash the entry declares decides. + """ + for field, digest in ( + ("sha1", hashlib.sha1), + ("sha256", hashlib.sha256), + ("md5", hashlib.md5), + ): + accepted = _accepted(expected, field) + if accepted: + return digest(data).hexdigest() in accepted + crcs, size = _crc32s(expected), _size(expected) + if crcs and size is not None: + return len(data) == size and f"{zlib.crc32(data) & 0xFFFFFFFF:08x}" in crcs return False @@ -158,11 +194,20 @@ def step1_crossref_db(entry: dict, db: dict) -> str | None: for sha1 in sorted(_accepted(entry, "sha1")): if sha1 in files: return files[sha1]["path"] - by_md5 = db.get("indexes", {}).get("by_md5", {}) - for md5 in sorted(_accepted(entry, "md5")): - sha1_match = by_md5.get(md5) - if sha1_match and sha1_match in files: - return files[sha1_match]["path"] + indexes = db.get("indexes", {}) + for field, index in (("sha256", "by_sha256"), ("md5", "by_md5")): + by_hash = indexes.get(index, {}) + for value in sorted(_accepted(entry, field)): + sha1_match = by_hash.get(value) + if sha1_match and sha1_match in files: + return files[sha1_match]["path"] + size = _size(entry) + if size is not None: + by_crc32 = indexes.get("by_crc32", {}) + for crc in sorted(_crc32s(entry)): + sha1_match = by_crc32.get(crc) + if sha1_match in files and files[sha1_match].get("size") == size: + return files[sha1_match]["path"] return None @@ -352,7 +397,7 @@ def fetch_missing( dry_run: bool = False, ) -> dict: """Run the 5-step auto-fetch pipeline for missing files.""" - stats = {"found": 0, "not_found": 0, "errors": 0} + stats = {"found": 0, "not_found": 0, "errors": 0, "unverifiable": 0} still_missing = [] for entry in missing: @@ -366,6 +411,14 @@ def fetch_missing( stats["found"] += 1 continue + if not verifiable(entry): + # Searched, a file of that name could only ever be refused. + print(" [-] No sha1, sha256, md5 or crc32 with size to check a " + "download against: not searched") + still_missing.append(entry) + stats["unverifiable"] += 1 + continue + if dry_run: print(" [DRY RUN] Would search branches, repos, archive.org") still_missing.append(entry) @@ -511,7 +564,8 @@ def main() -> None: stats = result["stats"] print( f"\n Results: {stats['found']} found, {stats['not_found']} not found, " - f"{stats['errors']} not fully searched" + f"{stats['errors']} not fully searched, " + f"{stats['unverifiable']} without a hash to search by" ) if args.create_issues and all_still_missing: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index f5afb746..a263fb3e 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -5598,6 +5598,52 @@ class AnUnansweredSearchIsNotANotFound(unittest.TestCase): self.assertEqual(result["stats"]["errors"], 0) +class AutoFetchProvesEveryHashItIsGiven(unittest.TestCase): + """auto_fetch checked a download by sha1 or md5 only: RetroDECK's + dsifirmware.bin, pinned by sha256 alone, was announced as searched and + could never be accepted.""" + + DATA = b"firmware bytes" + + def test_sha256_and_crc32_with_size_prove_a_download(self): + import hashlib + import zlib + + from scripts import auto_fetch + + sha256 = {"sha256": hashlib.sha256(self.DATA).hexdigest()} + self.assertTrue(auto_fetch.verify_content(self.DATA, sha256)) + self.assertFalse(auto_fetch.verify_content(b"other", sha256)) + crc = {"crc32": f"{zlib.crc32(self.DATA):08X}", "size": len(self.DATA)} + self.assertTrue(auto_fetch.verify_content(self.DATA, crc)) + self.assertFalse(auto_fetch.verify_content(self.DATA, dict(crc, size=1))) + self.assertFalse(auto_fetch.verifiable({"crc32": crc["crc32"]})) + + def test_a_held_sha256_is_found_in_the_database(self): + from scripts import auto_fetch + + db = {"files": {"s": {"path": "bios/x/dsi.bin", "size": 3}}, + "indexes": {"by_sha256": {"f" * 64: "s"}}} + self.assertEqual( + auto_fetch.step1_crossref_db({"sha256": "F" * 64}, db), "bios/x/dsi.bin" + ) + + def test_an_entry_without_a_hash_is_not_searched(self): + from unittest import mock + + from scripts import auto_fetch + + entry = {"name": "DICROM.ROM", "system": "fujitsu-fm7"} + out = io.StringIO() + with mock.patch.object(auto_fetch, "step2_scan_branches") as branches, \ + contextlib.redirect_stdout(out): + result = auto_fetch.fetch_missing([entry], {"files": {}}, "bios", dry_run=True) + branches.assert_not_called() + self.assertEqual(result["stats"]["unverifiable"], 1) + self.assertNotIn("Would search", out.getvalue()) + self.assertIn("not searched", out.getvalue()) + + class AForcedRehashIsCheckedAgainstTheLastOne(unittest.TestCase): """This host has computed a wrong SHA-1 under load. The pipeline's forced rebuild threw the previous digests away and wrote whatever it read.""" diff --git a/wiki/tools.md b/wiki/tools.md index 173caa6f..51be8bab 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -653,7 +653,7 @@ python scripts/refresh_stale.py --only platforms,targets --jobs 6 | `region_audit.py` | Confront every profile `region:` with the regions No-Intro names for the same dump (`--json`, `--db`, `--emulators-dir`). An opinion, never an authority: the code decides, and a disagreement is a line to read in the emulator's source | | `validate_pr.py` | Validate BIOS files in pull requests, post markdown report | | `validate_schemas.py` | Validate the data contracts: schemas and semantic invariants. `--source-only` checks `emulators/` and `platforms/` alone, which is what PR validation runs | -| `auto_fetch.py` | Fetch missing BIOS files from known sources (4-step pipeline) | +| `auto_fetch.py` | Fetch missing BIOS files from known sources (4-step pipeline). A download is accepted by the sha1, sha256 or md5 the entry declares, or its crc32 with the size. An entry with none of them is reported and not searched | | `list_platforms.py` | List active platforms (`--include-archived` adds the archived ones, used by CI) | | `download.py` | Download a pack from GitHub releases, every part checked (Python, stdlib only) | | `download.sh` | Same, as a shell one-liner (`curl` + `unzip`) | From d16f8fd17a8df9cb5530b2fd0cc12563c32b54e6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:04:20 +0200 Subject: [PATCH 08/10] fix: accept downloads by cryptographic hash only A crc32 and a size can be met by crafted bytes, so a remote download matched on them could land in bios/. verify_content now requires every sha256, sha1 and md5 the entry declares to match, and an entry declaring none is reported as not verifiable from the network and nothing is written. crc32 and size still find a file the database already holds. --- scripts/auto_fetch.py | 54 +++++++++++++++++---------------- tests/test_audit_regressions.py | 45 ++++++++++++++++++++++----- wiki/tools.md | 2 +- 3 files changed, 67 insertions(+), 34 deletions(-) diff --git a/scripts/auto_fetch.py b/scripts/auto_fetch.py index c39587b7..609160a6 100644 --- a/scripts/auto_fetch.py +++ b/scripts/auto_fetch.py @@ -25,7 +25,6 @@ import sys import urllib.error import urllib.parse import urllib.request -import zlib from pathlib import Path sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) @@ -147,35 +146,38 @@ def _size(entry: dict) -> int | None: return None -def verifiable(entry: dict) -> bool: - """Whether a download can be proved to be this file. +CRYPTO_HASHES = ( + ("sha256", hashlib.sha256), + ("sha1", hashlib.sha1), + ("md5", hashlib.md5), +) - A sha1, sha256 or md5 proves it, a crc32 only with the size beside it, - the way the resolver accepts one. Without any of them a search can find - a file of that name and never accept it. + +def verifiable(entry: dict) -> bool: + """Whether a network download can be proved to be this file. + + Only a cryptographic hash proves it. A crc32 and a size can be met by + crafted bytes, so they identify a file the database already holds + (step 1) and never accept one a remote source serves. """ - if any(_accepted(entry, field) for field in ("sha1", "sha256", "md5")): - return True - return bool(_crc32s(entry)) and _size(entry) is not None + return any(_accepted(entry, field) for field, _digest in CRYPTO_HASHES) def verify_content(data: bytes, expected: dict) -> bool: - """Verify downloaded content matches one of the expected hashes. + """Whether downloaded bytes match every cryptographic hash declared. - The strongest hash the entry declares decides. + Each declared field must hold the bytes' digest among its accepted + values. An entry declaring none is refused. """ - for field, digest in ( - ("sha1", hashlib.sha1), - ("sha256", hashlib.sha256), - ("md5", hashlib.md5), - ): - accepted = _accepted(expected, field) - if accepted: - return digest(data).hexdigest() in accepted - crcs, size = _crc32s(expected), _size(expected) - if crcs and size is not None: - return len(data) == size and f"{zlib.crc32(data) & 0xFFFFFFFF:08x}" in crcs - return False + declared = [ + (accepted, digest) + for field, digest in CRYPTO_HASHES + for accepted in [_accepted(expected, field)] + if accepted + ] + return bool(declared) and all( + digest(data).hexdigest() in accepted for accepted, digest in declared + ) def _read_limited( @@ -413,8 +415,8 @@ def fetch_missing( if not verifiable(entry): # Searched, a file of that name could only ever be refused. - print(" [-] No sha1, sha256, md5 or crc32 with size to check a " - "download against: not searched") + print(" [-] Cannot be verified from the network: declares no " + "cryptographic hash. Not searched, nothing written") still_missing.append(entry) stats["unverifiable"] += 1 continue @@ -565,7 +567,7 @@ def main() -> None: print( f"\n Results: {stats['found']} found, {stats['not_found']} not found, " f"{stats['errors']} not fully searched, " - f"{stats['unverifiable']} without a hash to search by" + f"{stats['unverifiable']} without a cryptographic hash" ) if args.create_issues and all_still_missing: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index a263fb3e..3fe7785d 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -5605,19 +5605,50 @@ class AutoFetchProvesEveryHashItIsGiven(unittest.TestCase): DATA = b"firmware bytes" - def test_sha256_and_crc32_with_size_prove_a_download(self): + def test_a_sha256_proves_a_download(self): import hashlib - import zlib from scripts import auto_fetch sha256 = {"sha256": hashlib.sha256(self.DATA).hexdigest()} self.assertTrue(auto_fetch.verify_content(self.DATA, sha256)) self.assertFalse(auto_fetch.verify_content(b"other", sha256)) - crc = {"crc32": f"{zlib.crc32(self.DATA):08X}", "size": len(self.DATA)} - self.assertTrue(auto_fetch.verify_content(self.DATA, crc)) - self.assertFalse(auto_fetch.verify_content(self.DATA, dict(crc, size=1))) - self.assertFalse(auto_fetch.verifiable({"crc32": crc["crc32"]})) + + def test_every_declared_cryptographic_hash_must_match(self): + import hashlib + + from scripts import auto_fetch + + both = {"sha256": hashlib.sha256(self.DATA).hexdigest(), "md5": "0" * 32} + self.assertFalse(auto_fetch.verify_content(self.DATA, both)) + both["md5"] = hashlib.md5(self.DATA).hexdigest() + self.assertTrue(auto_fetch.verify_content(self.DATA, both)) + + def test_crc32_and_size_never_accept_a_download(self): + """A crc32 can be forged: bytes a source crafts to it would land in + bios/.""" + import urllib.request + import zlib + from unittest import mock + + from scripts import auto_fetch + + crc = {"name": "forged.bin", "system": "test-system", + "crc32": f"{zlib.crc32(self.DATA):08X}", "size": len(self.DATA)} + self.assertFalse(auto_fetch.verify_content(self.DATA, crc)) + self.assertFalse(auto_fetch.verifiable(crc)) + + served = io.BytesIO(self.DATA) + with tempfile.TemporaryDirectory() as bios, \ + mock.patch.object(urllib.request, "urlopen", lambda *a, **k: served), \ + mock.patch.object(auto_fetch, "step2_scan_branches", + lambda e: self.DATA), \ + contextlib.redirect_stdout(io.StringIO()) as out: + result = auto_fetch.fetch_missing([crc], {"files": {}}, bios) + self.assertEqual(list(Path(bios).rglob("*")), []) + self.assertEqual(result["stats"]["found"], 0) + self.assertEqual(result["stats"]["unverifiable"], 1) + self.assertIn("declares no cryptographic hash", out.getvalue()) def test_a_held_sha256_is_found_in_the_database(self): from scripts import auto_fetch @@ -5641,7 +5672,7 @@ class AutoFetchProvesEveryHashItIsGiven(unittest.TestCase): branches.assert_not_called() self.assertEqual(result["stats"]["unverifiable"], 1) self.assertNotIn("Would search", out.getvalue()) - self.assertIn("not searched", out.getvalue()) + self.assertIn("Not searched", out.getvalue()) class AForcedRehashIsCheckedAgainstTheLastOne(unittest.TestCase): diff --git a/wiki/tools.md b/wiki/tools.md index 51be8bab..cbaaf6ce 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -653,7 +653,7 @@ python scripts/refresh_stale.py --only platforms,targets --jobs 6 | `region_audit.py` | Confront every profile `region:` with the regions No-Intro names for the same dump (`--json`, `--db`, `--emulators-dir`). An opinion, never an authority: the code decides, and a disagreement is a line to read in the emulator's source | | `validate_pr.py` | Validate BIOS files in pull requests, post markdown report | | `validate_schemas.py` | Validate the data contracts: schemas and semantic invariants. `--source-only` checks `emulators/` and `platforms/` alone, which is what PR validation runs | -| `auto_fetch.py` | Fetch missing BIOS files from known sources (4-step pipeline). A download is accepted by the sha1, sha256 or md5 the entry declares, or its crc32 with the size. An entry with none of them is reported and not searched | +| `auto_fetch.py` | Fetch missing BIOS files from known sources (4-step pipeline). A download is accepted only when every cryptographic hash the entry declares (sha256, sha1, md5) matches. crc32 and size find a file the database already holds, never accept a download. An entry with no cryptographic hash is reported and not searched | | `list_platforms.py` | List active platforms (`--include-archived` adds the archived ones, used by CI) | | `download.py` | Download a pack from GitHub releases, every part checked (Python, stdlib only) | | `download.sh` | Same, as a shell one-liner (`curl` + `unzip`) | From a74a56abc4a8391141e7857fb97c3e80f77023d7 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:05:10 +0200 Subject: [PATCH 09/10] feat: name the profile a missing dump would fill The acquisition report never read a profile, so XM7's required DICROM.ROM sat among 2 287 targets at the rank of dumps no core reads. A missing entry that a profile reads and the collection lacks is now listed first with the profile, matched by hash or, for a hashless entry with a size, by name and size. 29 entries on the current snapshots. --- scripts/provenance_report.py | 136 ++++++++++++++++++++++++++++++++++- tests/test_provenance.py | 49 +++++++++++++ wiki/tools.md | 7 ++ 3 files changed, 190 insertions(+), 2 deletions(-) diff --git a/scripts/provenance_report.py b/scripts/provenance_report.py index 5ab713a5..44c7ca5b 100644 --- a/scripts/provenance_report.py +++ b/scripts/provenance_report.py @@ -10,6 +10,10 @@ Usage: python scripts/provenance_report.py python scripts/provenance_report.py --missing python scripts/provenance_report.py --json + +A missing dump that is a file an emulator profile reads and the collection +lacks is named with that profile: by hash when the profile declares one, by +name and size when it declares a size and no hash. """ from __future__ import annotations @@ -26,10 +30,117 @@ sys.path.insert(0, os.path.dirname(__file__)) from common import ( DEFAULT_PROVENANCE_DIR, load_database, + load_emulator_profiles, load_provenance_snapshots, write_if_changed, ) +HASH_FIELDS = ("sha1", "md5", "sha256") + + +def _values(entry: dict, field: str) -> set[str]: + """Every value a profile or catalog entry gives for a hash field.""" + value = entry.get(field) + values = value if isinstance(value, list) else str(value or "").split(",") + return {str(v).strip().lower() for v in values if str(v).strip()} + + +def _crc_key(value: str) -> str: + return value.removeprefix("0x").zfill(8) + + +def _fits(size: int | None, entry: dict) -> bool: + """Whether a size meets what a profile entry states about it.""" + if size is None: + return False + declared = entry.get("size") + if declared is not None: + accepted = declared if isinstance(declared, list) else [declared] + return size in {int(value) for value in accepted} + low, high = entry.get("min_size"), entry.get("max_size") + return (low is None or size >= int(low)) and (high is None or size <= int(high)) + + +def profile_needs(profiles: dict, db: dict) -> dict: + """The files emulator profiles read that the collection does not hold. + + Keyed by every hash a profile accepts, and, for an entry that declares + a size and no hash, by its names casefolded. An entry with neither + cannot be told apart from any same-named dump and is left out. + """ + files = db.get("files", {}) + indexes = db.get("indexes", {}) + held_hashes = { + "sha1": set(files), + "md5": set(indexes.get("by_md5", {})), + "sha256": set(indexes.get("by_sha256", {})), + } + held_crc = { + (_crc_key(str(f.get("crc32", "")).lower()), f.get("size")) + for f in files.values() if f.get("crc32") + } + sizes_by_name: dict[str, set[int]] = {} + for record in files.values(): + sizes_by_name.setdefault(str(record.get("name", "")).casefold(), set()).add( + record.get("size") + ) + + by_hash: dict[tuple, list[dict]] = {} + by_name: dict[str, list[tuple[dict, dict]]] = {} + for emulator, profile in sorted(profiles.items()): + for entry in profile.get("files", []) or []: + name = str(entry.get("name", "")) + if not name or name.endswith("/"): + continue + need = {"emulator": emulator, "name": name, + "required": bool(entry.get("required"))} + hashes = {(field, v) for field in HASH_FIELDS for v in _values(entry, field)} + crcs = {_crc_key(v) for v in _values(entry, "crc32")} + declared = entry.get("size") + sizes = [] if declared is None else ( + declared if isinstance(declared, list) else [declared] + ) + crc_keys = {(crc, int(size)) for crc in crcs for size in sizes} + if hashes or crc_keys: + if any(v in held_hashes[field] for field, v in hashes) or ( + crc_keys & held_crc + ): + continue + for key in hashes | {("crc32", crc, size) for crc, size in crc_keys}: + by_hash.setdefault(key, []).append(need) + continue + if all(entry.get(k) is None for k in ("size", "min_size", "max_size")): + continue + names = [name, *(entry.get("aliases") or [])] + if any( + _fits(held, entry) + for alias in names + for held in sizes_by_name.get(str(alias).casefold(), ()) + ): + continue + for alias in names: + by_name.setdefault(str(alias).casefold(), []).append((need, entry)) + return {"by_hash": by_hash, "by_name": by_name} + + +def needs_filled(entry: dict, needs: dict) -> list[dict]: + """The profile needs a catalog dump answers, required ones first.""" + found: dict[tuple[str, str], dict] = {} + keys = [(field, v) for field in HASH_FIELDS for v in _values(entry, field)] + if entry.get("size") is not None: + keys += [("crc32", _crc_key(v), entry["size"]) for v in _values(entry, "crc32")] + for key in keys: + for need in needs["by_hash"].get(key, []): + found.setdefault((need["emulator"], need["name"]), {**need, "by": key[0]}) + size = entry.get("size") + name = str(entry.get("name", "")).casefold() + for need, declared in needs["by_name"].get(name, []): + if _fits(size, declared): + found.setdefault( + (need["emulator"], need["name"]), {**need, "by": "name and size"} + ) + return sorted(found.values(), key=lambda n: (not n["required"], n["emulator"], n["name"])) + def archive_members(db: dict) -> dict[tuple[str, int], list[tuple[str, str]]]: """(crc32, size) of every member of the collection's ZIPs -> (zip, member). @@ -153,6 +264,7 @@ def build_report( snapshots: dict, members: dict | None = None, romsets: dict | None = None, + needs: dict | None = None, ) -> dict: """Compare each snapshot against the collection. @@ -213,6 +325,9 @@ def build_report( holders = romsets_holding(entry, romsets) if holders: entry["romsets"] = holders + filled = needs_filled(entry, needs) if needs else [] + if filled: + entry["needed_by"] = filled out_of_scope = len({ dump_identity(e) for e in unmatched if e.get("dat", "") not in covered_dats }) @@ -245,6 +360,10 @@ def main() -> int: "byte swaps, concatenations). Reads the collection, about 35 s", ) parser.add_argument("--json", action="store_true", help="Full report as JSON") + parser.add_argument( + "--emulators-dir", default="emulators", + help="profiles whose unheld files a missing dump answers (default: %(default)s)", + ) parser.add_argument( "--write-derivable", metavar="PATH", @@ -261,7 +380,8 @@ def main() -> int: print(f"No provenance snapshots in {args.provenance_dir}/") return 0 - report = build_report(db, snapshots, archive_members(db)) + needs = profile_needs(load_emulator_profiles(args.emulators_dir), db) + report = build_report(db, snapshots, archive_members(db), needs=needs) derived: dict[str, dict] = {} if args.derive: from derivations import derivable # noqa: PLC0415 @@ -299,7 +419,8 @@ def main() -> int: f"does not cover, not counted as targets" ) if args.missing: - for entry in data["missing"]: + # A dump an emulator reads comes first: it fills a hole in a pack. + for entry in sorted(data["missing"], key=lambda e: not e.get("needed_by")): label = entry.get("description") or entry["name"] source = derived.get(entry.get("sha1", "")) if source: @@ -307,6 +428,10 @@ def main() -> int: f"of {' + '.join(source['from'])}") else: print(f" MISSING {entry['name']} ({label}) sha1={entry.get('sha1')}") + for need in entry.get("needed_by", []): + print(f" needed by {need['emulator']} as {need['name']} " + f"({'required' if need['required'] else 'optional'}, " + f"matched by {need['by']})") for hit in entry.get("romsets", []): versions = hit["dats"][0] if len(hit["dats"]) == 1 else ( f"{hit['dats'][0]} to {hit['dats'][-1]}" @@ -320,6 +445,13 @@ def main() -> int: if args.derive: line += f", {len(derived)} of them derivable from held dumps" print(line) + needed = { + dump_identity(e) + for data in report.values() for e in data["missing"] if e.get("needed_by") + } + if needed: + print(f" {len(needed)} of them a profile reads and the collection lacks" + + ("" if args.missing else " (--missing names them)")) return 0 diff --git a/tests/test_provenance.py b/tests/test_provenance.py index ab0917ca..94dbe322 100644 --- a/tests/test_provenance.py +++ b/tests/test_provenance.py @@ -554,6 +554,55 @@ class OneDumpIsCountedOnce(unittest.TestCase): self.assertEqual(provenance_report.distinct_missing(twice), 1) +class AMissingDumpNamesTheProfileItFills(unittest.TestCase): + """XM7 requires DICROM.ROM and the collection lacks it, yet the report + listed No-Intro's dump at the rank of a Kickstart beta no core reads.""" + + PROFILES = { + "xm7": {"files": [ + {"name": "DICROM.ROM", "required": True, "min_size": 262144}, + {"name": "held.rom", "required": True, "size": 16}, + ]}, + "hashed": {"files": [{"name": "x.bin", "sha1": "c" * 40}]}, + } + + def _report(self, db): + from scripts import provenance_report # noqa: PLC0415 + + snapshots = {"no-intro": {"entries": [ + {"dat": "FM-7", "name": "DICROM.rom", "sha1": "a" * 40, "size": 262144}, + {"dat": "FM-7", "name": "held.rom", "sha1": "b" * 40, "size": 16}, + {"dat": "FM-7", "name": "Other Name.rom", "sha1": "c" * 40, "size": 8}, + {"dat": "FM-7", "name": "unread.rom", "sha1": "d" * 40, "size": 8}, + ]}} + needs = provenance_report.profile_needs(self.PROFILES, db) + return provenance_report.build_report(db, snapshots, {}, {}, needs) + + def test_a_dump_a_profile_reads_names_it(self): + db = {"files": {"b" * 40: {"name": "held.rom", "md5": "", "size": 16}}} + missing = {e["name"]: e.get("needed_by") for e in self._report(db)["no-intro"]["missing"]} + self.assertEqual(missing["DICROM.rom"], [ + {"emulator": "xm7", "name": "DICROM.ROM", "required": True, + "by": "name and size"}, + ]) + self.assertEqual(missing["Other Name.rom"][0]["by"], "sha1") + self.assertIsNone(missing["unread.rom"]) + + def test_a_size_outside_the_profile_range_fills_nothing(self): + from scripts import provenance_report # noqa: PLC0415 + + needs = provenance_report.profile_needs(self.PROFILES, {"files": {}}) + small = {"name": "DICROM.rom", "size": 1024} + self.assertEqual(provenance_report.needs_filled(small, needs), []) + + def test_a_need_the_collection_answers_is_not_one(self): + from scripts import provenance_report # noqa: PLC0415 + + db = {"files": {"e" * 40: {"name": "dicrom.rom", "size": 262144}}} + needs = provenance_report.profile_needs(self.PROFILES, db) + self.assertNotIn("dicrom.rom", needs["by_name"]) + + class TheWantedPageAsksOnlyForWhatCannotBeMade(unittest.TestCase): """The site's Wanted page asked the public for 79 dumps the collection already produces (a Kickstart's odd bytes, a character ROM's halves).""" diff --git a/wiki/tools.md b/wiki/tools.md index cbaaf6ce..6fc7679e 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -897,6 +897,13 @@ The Amiga Kickstart chip pairs and TOSEC's split C64 and VES ROMs are found this way: 67 entries on the current snapshots, recorded in `derivable.json` by pipeline step 1b. +A missing entry that is a file an emulator profile reads and the collection +lacks is listed first, with `needed by as ` and whether the +profile requires it. The match is by hash when the profile declares one, by +name and size when it declares a size or size range and no hash (XM7's +`DICROM.ROM`, which No-Intro lists for the FM77AV40SX). `--emulators-dir` +names the profiles read. + | Catalog | Acquisition | Notes | |---------|------------|-------| | Redump | direct fetch from redump.info `/static/bios` | 4 BIOS DATs. redump.org serves stale DATs | From 648db6bcbfcdef6976758ddefd397e85222fc8d9 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:06:13 +0200 Subject: [PATCH 10/10] fix: merge mame devices when the pin moves Step 2a advanced mame.yml's core_version and upstream_commit to a new release and left the device ROM sets on the previous one. A plain run that moves the pin now fetches that release's listxml into .cache/dats and runs the device merge, which checks version and commit itself. --- scripts/pipeline.py | 2 +- scripts/scraper/mame_hash_scraper.py | 30 +++++++++++++++++++ tests/test_mame_devices.py | 43 ++++++++++++++++++++++++++++ wiki/tools.md | 4 +++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 67a60f47..fa117bc8 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -7,7 +7,7 @@ Steps, numbered as the run prints them: 1b provenance_report.py dump-catalog coverage, derivable.json 1c romset_recipes.py archive identification, reconstruction targets 2 refresh_data_dirs.py Dolphin Sys, PPSSPP assets and the other trees - 2a mame_hash_scraper MAME BIOS hashes from the driver source + 2a mame_hash_scraper MAME BIOS hashes, and device sets on a new release 2a2 fbneo_hash_scraper FBNeo BIOS hashes 2b check_buildbot_system.py buildbot freshness (--check-buildbot) 2b2 check_release_assets.py large-files release vs gitignored files diff --git a/scripts/scraper/mame_hash_scraper.py b/scripts/scraper/mame_hash_scraper.py index 3a5a850b..e3726fc5 100644 --- a/scripts/scraper/mame_hash_scraper.py +++ b/scripts/scraper/mame_hash_scraper.py @@ -334,6 +334,8 @@ def _run(args: argparse.Namespace) -> None: print(" no profiles with mamedev/mame upstream found") return + main_profile = _EMULATORS_DIR / "mame.yml" + pinned_before = _pinned_version(main_profile) for profile_path in profiles: is_main = profile_path.name == "mame.yml" diff = compute_diff(str(profile_path), str(_CACHE_PATH), mode="mame") @@ -356,6 +358,34 @@ def _run(args: argparse.Namespace) -> None: print() if args.dry_run: print("(dry run, no files modified)") + return + pinned_after = _pinned_version(main_profile) + if pinned_after != pinned_before: + # The device sets describe the release mame.yml is pinned on: left + # behind, the profile names two releases at once. + print(f"mame.yml moved from {pinned_before} to {pinned_after}: " + "merging the device ROM sets of that release") + _merge_devices(argparse.Namespace( + listxml=fetch_release_listxml(pinned_after), dry_run=False, + )) + + +def _pinned_version(profile_path: Path) -> str: + profile = yaml_load(profile_path.read_text(encoding="utf-8")) if profile_path.is_file() else {} + return str((profile or {}).get("core_version") or "") + + +def fetch_release_listxml(version: str) -> Path: + """The -listxml MAME publishes for a release, in the DAT importer's cache. + + 0.289 is the tag mame0289 and the asset mame0289lx.zip. + """ + from .romset_dat_importer import MAME_LISTXML_URL, _download + + tag = "mame" + version.replace(".", "") + return _download( + MAME_LISTXML_URL.format(tag=tag), _ROOT / ".cache" / "dats" / f"{tag}lx.zip" + ) def release_profile( diff --git a/tests/test_mame_devices.py b/tests/test_mame_devices.py index 12cc7069..b1542922 100644 --- a/tests/test_mame_devices.py +++ b/tests/test_mame_devices.py @@ -373,5 +373,48 @@ class Cli(unittest.TestCase): self.assertEqual(raised.exception.code, 2) +class TheDevicesFollowTheRelease(unittest.TestCase): + """Pipeline step 2a moved mame.yml to a new release and left the device + sets on the lines of the previous one: the profile named two releases.""" + + def _run(self, moves: bool, dry_run: bool = False) -> list: + import argparse + from unittest import mock + + from scripts.scraper import mame_hash_scraper as scraper + + with tempfile.TemporaryDirectory() as directory: + profile = Path(directory) / "mame.yml" + profile.write_text('core_version: "0.289"\nfiles: []\n') + + def merge(path, *_args, **_kwargs): + if moves: + profile.write_text('core_version: "0.290"\nfiles: []\n') + + merged = [] + with mock.patch.multiple( + scraper, + _EMULATORS_DIR=Path(directory), + _fetch_hashes=lambda force: {"bios_sets": {}, "version": "0.290"}, + _find_mame_profiles=lambda: [profile], + compute_diff=lambda *a, **k: {"updated": ["x"], "added": []}, + _format_diff=lambda *a, **k: [], + merge_mame_profile=merge, + fetch_release_listxml=lambda version: Path(f"mame{version}lx.zip"), + _merge_devices=merged.append, + ), mock.patch("sys.stdout", new=io.StringIO()): + scraper._run(argparse.Namespace(force=False, json=False, dry_run=dry_run)) + return merged + + def test_a_moved_pin_merges_that_release_devices(self): + (args,) = self._run(moves=True) + self.assertEqual(args.listxml, Path("mame0.290lx.zip")) + self.assertFalse(args.dry_run) + + def test_an_unmoved_pin_leaves_the_devices(self): + self.assertEqual(self._run(moves=False), []) + self.assertEqual(self._run(moves=True, dry_run=True), []) + + if __name__ == "__main__": unittest.main() diff --git a/wiki/tools.md b/wiki/tools.md index 6fc7679e..9685ef25 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -854,6 +854,10 @@ go under the `Device ROM sets (generated from MAME -listxml)` comment at the end of `files:` with `required: false`. A set the profile already lists keeps every field a reader wrote and only gets its contents refreshed. +A plain run (pipeline step 2a) that moves `mame.yml` to a new release +downloads that release's `-listxml` into `.cache/dats/` and runs the device +merge itself, so the BIOS root sets and the device sets name one release. + ```bash python -m scripts.scraper.mame_hash_scraper --devices --listxml .cache/dats/mame0289lx.zip --dry-run python -m scripts.scraper.mame_hash_scraper --devices --listxml .cache/dats/mame0289lx.zip