From b4e5e42c976a934860b5079c9df5da138eed9eb9 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:46:48 +0200 Subject: [PATCH 1/6] fix: leave out core files the core refuses by size A core extra resolved by name shipped on existence platforms even when the core's own size check turns the copy away. The RetroPie pack carried the 128 KB Apple IIGS ROM as Basilisk II's ROM (min 512 KB). The builder, the manifest and verify now read validation.core_size_refusal, which accepts a copy any version of the same profile slot takes. --- scripts/generate_pack.py | 42 +++---- scripts/packextras.py | 11 +- scripts/packverify.py | 2 +- scripts/validation.py | 52 ++++++++- scripts/verify.py | 15 ++- tests/test_core_size_refusal.py | 195 ++++++++++++++++++++++++++++++++ wiki/testing-guide.md | 2 +- 7 files changed, 287 insertions(+), 32 deletions(-) create mode 100644 tests/test_core_size_refusal.py diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index fbc3f18f..b4b25c53 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -93,6 +93,7 @@ from validation import ( deliverable, filter_files_by_mode, with_archive_members, + extra_alternatives, ) yaml = require_yaml() @@ -592,14 +593,7 @@ def _matches_an_alternative(fe: dict, local_path: str, profiles: dict) -> bool: of Raze's table, eduke32's demo and retail NWINTER.GRP). The extra carries the first, and the held copy matching a sibling is no mismatch. """ - profile = profiles.get(fe.get("source_profile") or "", {}) - name = fe.get("source_name") or fe.get("name") - path = fe.get("source_path") or name - siblings = [ - entry for entry in profile.get("files") or [] - if isinstance(entry, dict) and entry.get("name") == name - and (entry.get("path") or name) == path - ] + siblings = extra_alternatives(fe, profiles) if len(siblings) < 2: return False actual = compute_hashes(local_path, frozenset({"sha1", "md5", "crc32"})) @@ -1054,7 +1048,7 @@ def generate_pack( core_count = 0 extra_resolver = ExtraResolver( db, bios_dir, zip_contents, data_registry, validation_index, - validation_owners, offline, + validation_owners, offline, profiles=emu_profiles, ) for fe in core_files: if required_only and fe.get("required") is False: @@ -3778,6 +3772,7 @@ def _manifest_core_entries( validation_index: dict | None = None, owners: dict | None = None, data_registry: dict | None = None, + profiles: dict | None = None, ) -> int: """Add the files a platform's cores need but its list does not name. @@ -3788,6 +3783,10 @@ def _manifest_core_entries( """ total_size = 0 extras_pfx = _detect_extras_prefix(config, base_dest) + resolver = ExtraResolver( + db, bios_dir, zip_contents, data_registry, validation_index or {}, + owners, offline, profiles=profiles, + ) for fe in core_files: if required_only and fe.get("required") is False: continue @@ -3833,20 +3832,12 @@ def _manifest_core_entries( ): continue - dest_hint = fe.get("destination", "") - # Resolved with the data directories, as the builder resolves the - # same extra: without them blueMSX's MSX2P.rom named a different - # dump than the one its pack carried. - local_path, status = resolve_file( - fe, - db, - bios_dir, - zip_contents, - dest_hint=dest_hint, - data_dir_registry=data_registry, - offline=offline, - ) - if status in ("not_found", "external", "user_provided") or not local_path: + # The builder's own lookup (ExtraResolver): the data directories, the + # copy the emulator accepts, and the size its code checks. Without + # the data directories blueMSX's MSX2P.rom named a different dump + # than the one its pack carried. + local_path, status, _disagreement = resolver.resolve(fe, dest) + if not local_path: source_emu = fe.get("source_profile") or fe.get("source_emulator", "") if not _claim_platform_omission( omitted_by_destination.get(full_dest), source_emu @@ -3860,10 +3851,6 @@ def _manifest_core_entries( [source_emu] if source_emu else [], ) continue - # The copy the emulator accepts, as the pack ships it. - local_path, _disagreement = validated_choice( - fe, local_path, db, validation_index or {}, bios_dir, None, dest, owners, - ) source_emu = fe.get("source_profile") or fe.get("source_emulator", "") @@ -4382,6 +4369,7 @@ def generate_manifest( seen_destinations, seen_lower, seen_parents, manifest_files, omitted_by_destination, record_omission, pack_only_sizes, selection, required_only, manifest_validation, manifest_owners, data_registry, + emu_profiles, ) for key, (entry, full_dest, dest) in platform_entries.items(): declared = declared_by.get(full_dest, set()) diff --git a/scripts/packextras.py b/scripts/packextras.py index 66f15b63..05e5df63 100644 --- a/scripts/packextras.py +++ b/scripts/packextras.py @@ -176,7 +176,10 @@ class ExtraResolver: validation_index: dict, validation_owners: dict | None, offline: bool | None = True, + *, + profiles: dict, ) -> None: + self.profiles = profiles self.db = db self.bios_dir = bios_dir self.zip_contents = zip_contents @@ -188,7 +191,7 @@ class ExtraResolver: def resolve(self, fe: dict, dest: str) -> tuple[str | None, str, str]: """(local copy or None, resolver status, the core's disagreement).""" from packresolve import resolve_file - from validation import validated_choice + from validation import core_size_refusal, extra_alternatives, validated_choice local_path, status = resolve_file( fe, self.db, self.bios_dir, self.zip_contents, @@ -201,6 +204,12 @@ class ExtraResolver: fe, local_path, self.db, self.validation_index, self.bios_dir, None, dest, self.validation_owners, ) + # The platform never reads the bytes, the core does: a copy it turns + # away by its own size check is not its file, on any platform. + if core_size_refusal( + fe, local_path, extra_alternatives(fe, self.profiles) + ): + return None, "not_found", "" return local_path, status, disagreement diff --git a/scripts/packverify.py b/scripts/packverify.py index 1a4150ec..f47b8c3a 100644 --- a/scripts/packverify.py +++ b/scripts/packverify.py @@ -581,7 +581,7 @@ def _builder_extras( resolver=ExtraResolver( db, DEFAULT_BIOS_DIR, build_zip_contents_index(db), data_registry, _build_validation_index(platform_profiles), - destination_owners(platform_profiles), True, + destination_owners(platform_profiles), True, profiles=emu_profiles, ), placed_local=placed_local, ) diff --git a/scripts/validation.py b/scripts/validation.py index 481386f2..6f216e67 100644 --- a/scripts/validation.py +++ b/scripts/validation.py @@ -9,7 +9,7 @@ from __future__ import annotations import os import zipfile -from collections.abc import Callable, Iterator, Mapping +from collections.abc import Callable, Iterator, Mapping, Sequence from common import ( adler32_of, @@ -961,3 +961,53 @@ def existence_discrepancy(file_entry: dict, local_path: str, platform_display: s f"packed per {platform_display} existence check: declared hash {declared}, " f"file md5 {actual['md5']} sha1 {actual['sha1']}" ) + + +def same_slot_entries(files: list, name: str, path: str) -> list[dict]: + """The entries of one profile that name the same file at the same path. + + A profile lists every version its code takes under one name (each GRP + of Raze's table, eduke32's demo and retail NWINTER.GRP): any of them + fills the slot. + """ + return [ + entry for entry in files or [] + if isinstance(entry, dict) and entry.get("name") == name + and (entry.get("path") or name) == path + ] + + +def extra_alternatives(fe: dict, profiles: Mapping | None) -> list[dict]: + """The versions the extra's own profile accepts at the extra's slot.""" + if not profiles: + return [] + profile = profiles.get(fe.get("source_profile") or "", {}) + name = fe.get("source_name") or fe.get("name", "") + return same_slot_entries( + profile.get("files") or [], name, fe.get("source_path") or name + ) + + +def core_size_refusal( + file_entry: dict, local_path: str | None, alternatives: Sequence[dict] = (), +) -> str | None: + """Why the core refuses this copy by the size its own code checks, or None. + + The one rule the builder, the install manifest and verify read for a + core's file. An existence platform never opens the bytes, so a copy whose + hash disagrees still ships to it, but a core that measures the file turns + one of the wrong size away: Basilisk II stops on the Apple IIGS' 128 KB + ROM (main_unix.cpp:692), so packing that file for it serves nothing. + ``alternatives`` are the other versions the profile accepts at the same + slot: a copy any of them takes is not refused. + """ + if not local_path or not os.path.isfile(local_path): + return None + size = os.path.getsize(local_path) + if any(name_match_size_ok(entry, size) for entry in (file_entry, *alternatives)): + return None + bounds = ( + f"{file_entry['size']}" if file_entry.get("size") is not None + else f"{file_entry.get('min_size', 0)} to {file_entry.get('max_size', 'any')}" + ) + return f"{size} bytes, the core checks for {bounds}" diff --git a/scripts/verify.py b/scripts/verify.py index cf5ae83c..61eab4fe 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -103,6 +103,8 @@ from validation import ( unread_on, outside_gap_scope, with_archive_members, + core_size_refusal, + same_slot_entries, ) DEFAULT_DB = "database.json" @@ -692,6 +694,17 @@ def find_undeclared_files( in_repo = _st != "not_found" and _lp is not None if in_repo and _st == "hash_mismatch" and shipped_on_mismatch: in_repo = False + if in_repo and "size" in _parse_validation(f.get("validation")): + # On any platform the core measures what it opens: a + # copy it turns away by its own size check is not + # held for it, the rule the builder's lookup applies + # (packextras.ExtraResolver). The Apple IIGS' 128 KB + # ROM is not Basilisk II's ROM. + _lp, _st = resolve_local_file(f, db, dest_hint=dest) + if core_size_refusal( + f, _lp, same_slot_entries(profile.get("files"), fname, dest) + ): + in_repo = False checks = _parse_validation(f.get("validation")) entry = { @@ -1350,7 +1363,7 @@ def verify_platform( region_drops, report_for_builder, ExtraResolver( db, DEFAULT_BIOS_DIR, zip_contents, data_dir_registry, validation_index, - validation_owners, + validation_owners, profiles=profiles, ), placed_kind, ) diff --git a/tests/test_core_size_refusal.py b/tests/test_core_size_refusal.py new file mode 100644 index 00000000..b0a3c09c --- /dev/null +++ b/tests/test_core_size_refusal.py @@ -0,0 +1,195 @@ +"""A core's file the core refuses by its own size check is not packed for it. + +An existence platform never opens the bytes, so a copy whose hash disagrees +still ships there. The core does open them: Basilisk II stops on any ROM +outside 64 KB to 1 MB (main_unix.cpp:692) and its profile asks for 512 KB +or more, yet the RetroPie pack carried the Apple IIGS' 128 KB ROM as its +ROM, the only copy of that name. The builder, the manifest and verify now +read one rule (validation.core_size_refusal). +""" + +from __future__ import annotations + +import hashlib +import os +import sys +import tempfile +import unittest +import zipfile +from pathlib import Path + +import yaml + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +import common # noqa: E402 +import generate_db # noqa: E402 +import generate_pack as builder # noqa: E402 +from validation import core_size_refusal # noqa: E402 +from verify import find_undeclared_files, verify_platform # noqa: E402 + +SIZED = """\ +emulator: Sized +type: libretro +systems: [mac] +cores: [sized] +files: + - name: ROM + path: ROM + required: true + min_size: 16 + max_size: 32 + validation: [size] +""" + +# A core that reads a ROM of its own under another path, with no size check: +# its copy makes the name "covered", which sends the sized core's entry +# through the builder's second pass as well. +UNSIZED = """\ +emulator: Unsized +type: libretro +systems: [iigs] +cores: [unsized] +files: + - name: ROM + path: iigs/ROM + required: true +""" + + +class _Collection(unittest.TestCase): + payload = b"" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self._cwd = os.getcwd() + os.chdir(self._tmp.name) + for d in ("emulators", "platforms", "bios/Apple"): + Path(d).mkdir(parents=True) + Path("bios/Apple/ROM").write_bytes(self.payload) + sha1 = hashlib.sha1(self.payload).hexdigest() + files = {sha1: { + "path": "bios/Apple/ROM", "name": "ROM", "size": len(self.payload), + "sha1": sha1, "md5": hashlib.md5(self.payload).hexdigest(), + "sha256": hashlib.sha256(self.payload).hexdigest(), "crc32": "00000001", + }} + self.db = {"files": files, "indexes": generate_db.build_indexes(files, {})} + self.config = { + "platform": "Exist", "verification_mode": "existence", + "cores": ["sized", "unsized"], + "systems": {"mac": {"files": []}, "iigs": {"files": []}}, + } + Path("platforms/exist.yml").write_text(yaml.dump(self.config)) + Path("platforms/_registry.yml").write_text( + yaml.dump({"platforms": {"exist": {"status": "active"}}}) + ) + Path("emulators/sized.yml").write_text(SIZED) + Path("emulators/unsized.yml").write_text(UNSIZED) + common._platform_config_cache.clear() + common._emulator_profiles_cache.clear() + self.profiles = common.load_emulator_profiles("emulators") + + def tearDown(self): + common._platform_config_cache.clear() + common._emulator_profiles_cache.clear() + os.chdir(self._cwd) + self._tmp.cleanup() + + def _report(self, profile: str) -> dict: + undeclared = find_undeclared_files( + self.config, "emulators", self.db, self.profiles + ) + return next(u for u in undeclared if u["profile"] == profile) + + def _pack_members(self) -> set[str]: + zip_path = builder.generate_pack( + "exist", "platforms", self.db, "bios", "out", + emulators_dir="emulators", emu_profiles=self.profiles, offline=True, + ) + with zipfile.ZipFile(zip_path) as zf: + return set(zf.namelist()) + + def _manifest(self) -> dict: + return builder.generate_manifest( + "exist", "platforms", self.db, "bios", "platforms/_registry.yml", + emulators_dir="emulators", emu_profiles=self.profiles, offline=True, + ) + + + +class SizeTheCoreRefuses(_Collection): + payload = b"8 bytes!" + + def test_the_rule(self): + entry = yaml.safe_load(SIZED)["files"][0] + self.assertIn("8 bytes", core_size_refusal(entry, "bios/Apple/ROM") or "") + self.assertIsNone(core_size_refusal({"name": "ROM", "min_size": 16}, "bios/Apple/ROM")) + + def test_the_report_does_not_count_it_held(self): + self.assertFalse(self._report("sized")["in_repo"]) + self.assertTrue(self._report("unsized")["in_repo"]) + + def test_the_pack_does_not_carry_it(self): + members = self._pack_members() + self.assertIn("iigs/ROM", members) + self.assertNotIn("ROM", members) + + def test_the_manifest_lists_it_as_omitted(self): + manifest = self._manifest() + self.assertEqual({f["dest"] for f in manifest["files"]}, {"iigs/ROM"}) + omitted = {o["dest"]: o for o in manifest["omitted_files"]} + self.assertEqual(omitted["ROM"]["cores"], ["sized"]) + + def test_verify_agrees_with_the_pack(self): + report = verify_platform(self.config, self.db, "emulators", self.profiles) + in_pack = { + u["path"] for u in report["undeclared_files"] + if u["in_repo"] and u.get("in_pack", True) + } + self.assertEqual(in_pack, {"iigs/ROM"}) + + def test_another_version_the_profile_accepts_fills_the_slot(self): + # Raze lists every BLOOD.RFF and SW.GRP its table knows under one + # name: the copy held is one of them, and the first entry's size + # does not refuse it. + Path("emulators/sized.yml").write_text( + SIZED + " - name: ROM\n path: ROM\n size: 8\n validation: [size]\n" + ) + common._emulator_profiles_cache.clear() + self.profiles = common.load_emulator_profiles("emulators") + self.assertTrue(self._report("sized")["in_repo"]) + self.assertIn("ROM", self._pack_members()) + self.assertIn("ROM", {f["dest"] for f in self._manifest()["files"]}) + + +class SizeTheCoreAccepts(_Collection): + """The control: a copy of a size the core accepts ships to it.""" + + payload = b"sixteen bytes ok" + + def test_the_report_counts_it_held(self): + self.assertTrue(self._report("sized")["in_repo"]) + + def test_the_pack_carries_it(self): + self.assertTrue({"ROM", "iigs/ROM"} <= self._pack_members()) + + def test_the_manifest_lists_it(self): + manifest = self._manifest() + self.assertEqual({f["dest"] for f in manifest["files"]}, {"ROM", "iigs/ROM"}) + + def test_verify_agrees_with_the_pack(self): + report = verify_platform(self.config, self.db, "emulators", self.profiles) + in_pack = { + u["path"] for u in report["undeclared_files"] + if u["in_repo"] and u.get("in_pack", True) + } + self.assertEqual(in_pack, {"ROM", "iigs/ROM"}) + + def test_the_rule_accepts_it(self): + entry = yaml.safe_load(SIZED)["files"][0] + self.assertIsNone(core_size_refusal(entry, "bios/Apple/ROM")) + + +if __name__ == "__main__": + unittest.main() diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index 30abcf46..1312a04b 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -39,7 +39,7 @@ Every module in `tests/`, by the area it guards. |------|----------|---------| | Core regression suites | synthetic | `test_e2e`, `test_audit_regressions`, `test_tool_messages` | | File resolution and identity | synthetic + collection | `test_adler32_resolution`, `test_agnostic_scan`, `test_agnostic_scan_mode`, `test_agnostic_scope`, `test_agnostic_substitute`, `test_archive_choice`, `test_bios_companions`, `test_bundled_identity`, `test_casefold_cache`, `test_db_aliases`, `test_declared_names`, `test_derivations`, `test_directory_entries`, `test_extra_identity`, `test_foreign_tree`, `test_foreign_tree_names`, `test_frontend_digest`, `test_game_data_homonyms`, `test_hashing`, `test_identity`, `test_md5_pack_context`, `test_name_tiebreak`, `test_nec_filing`, `test_owner_resolution`, `test_path_suffix`, `test_resolution_evidence`, `test_shipped_content`, `test_twin_declarations`, `test_validated_choice`, `test_variant_choice`, `test_variant_identity`, `test_zipped_member` | -| Verification, cross-reference and profile checks | synthetic + profiles | `test_crypto_aes`, `test_crypto_unique_data`, `test_crypto_verify`, `test_case_insensitive_platforms`, `test_checks_have_values`, `test_core_file_fills_declaration`, `test_cross_reference_sources`, `test_file_consumers`, `test_fileless_audit`, `test_gap_settlement`, `test_hidden_extras`, `test_launcher_layer`, `test_native_mode`, `test_no_case_collisions`, `test_path_conflict_status`, `test_path_null_copies`, `test_profile_checks`, `test_romm_hash_rule`, `test_shared_groups`, `test_standalone_mode`, `test_undeclared_dedup`, `test_unplaced_extras`, `test_validation_scope`, `test_verify_all_dir`, `test_verify_emulator_evidence`, `test_verify_listing`, `test_verify_pack_choice` | +| Verification, cross-reference and profile checks | synthetic + profiles | `test_crypto_aes`, `test_crypto_unique_data`, `test_crypto_verify`, `test_case_insensitive_platforms`, `test_checks_have_values`, `test_core_file_fills_declaration`, `test_core_size_refusal`, `test_cross_reference_sources`, `test_file_consumers`, `test_fileless_audit`, `test_gap_settlement`, `test_hidden_extras`, `test_launcher_layer`, `test_native_mode`, `test_no_case_collisions`, `test_path_conflict_status`, `test_path_null_copies`, `test_profile_checks`, `test_romm_hash_rule`, `test_shared_groups`, `test_standalone_mode`, `test_undeclared_dedup`, `test_unplaced_extras`, `test_validation_scope`, `test_verify_all_dir`, `test_verify_emulator_evidence`, `test_verify_listing`, `test_verify_pack_choice` | | Packs and manifests | synthetic + real packs | `test_artifact_lock`, `test_atomic_artifacts`, `test_batocera_cores`, `test_deterministic_zip`, `test_emulator_pack_counts`, `test_finalize_pack_names`, `test_large_file_cache`, `test_libretro_layout`, `test_manifest_entry`, `test_pack_conformance_dirs`, `test_pack_core_extras`, `test_pack_counts`, `test_pack_destinations`, `test_pack_failures`, `test_pack_file_names`, `test_pack_grouping`, `test_pack_integrity`, `test_pack_member_identity`, `test_pack_readme`, `test_platform_inheritance`, `test_platform_keys`, `test_required_only_archives`, `test_required_only_twins`, `test_split_extras`, `test_split_names`, `test_split_pack`, `test_system_pack_extras`, `test_system_pack_parity`, `test_torrentzip`, `test_which_pack` | | Slots and regions | synthetic + profiles | `test_region`, `test_region_agnostic_scan`, `test_region_groups`, `test_region_held`, `test_region_platform_index`, `test_region_required_only`, `test_slot`, `test_slots`, `test_slots_shipped_file` | | Hardware targets and systems | synthetic + target files | `test_extract_targets`, `test_lakka_targets`, `test_libretro_slugs`, `test_system_ids`, `test_system_selection`, `test_target_names`, `test_target_scrapers`, `test_target_systems`, `test_target_verify` | From 9a0501d9d30959435117fe61c90121436857a21a Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:50:03 +0200 Subject: [PATCH 2/6] fix: count an uncached release asset as fetched verify resolved offline, so a release asset absent from bios/ and the cache read as missing while an online build fetched and packed it. verify now reads the database record of the registered path and counts such a declaration present, listed as fetched at build time. --offline keeps the old count, and the offline pipeline passes it to its verify step. --- scripts/packresolve.py | 64 ++++++++++++++ scripts/pipeline.py | 4 + scripts/verify.py | 53 +++++++++++- tests/test_verify_release_asset.py | 131 +++++++++++++++++++++++++++++ wiki/testing-guide.md | 2 +- wiki/tools.md | 1 + 6 files changed, 251 insertions(+), 4 deletions(-) create mode 100644 tests/test_verify_release_asset.py diff --git a/scripts/packresolve.py b/scripts/packresolve.py index 16756ab4..d21ec1f1 100644 --- a/scripts/packresolve.py +++ b/scripts/packresolve.py @@ -139,6 +139,70 @@ def _verified_asset( return None +def run_is_offline(offline: bool | None = None) -> bool: + """Whether this run fetches nothing: --offline, or RETROBIOS_OFFLINE.""" + return bool( + (_OFFLINE if offline is None else offline) + or os.environ.get("RETROBIOS_OFFLINE") + ) + + +_RECORDS_BY_PATH: list[tuple[dict, int, dict[str, dict]]] = [] + + +def _records_by_path(db: dict) -> dict[str, dict]: + """The database records by path, built once per database.""" + files = db.get("files", {}) + if _RECORDS_BY_PATH: + cached, size, index = _RECORDS_BY_PATH[0] + if cached is files and size == len(files): + return index + index = {record.get("path"): record for record in files.values()} + _RECORDS_BY_PATH[:] = [(files, len(files), index)] + return index + + +def pending_release_asset( + file_entry: dict, db: dict, registered: list[str] | None = None, +) -> str | None: + """The release asset an online build fetches for an entry, if one does. + + The registered path whose database record carries what the entry + declares, read without a download: verify never fetches, and a release + asset the cache lacks was reported missing while an online build packed + it. ``registered`` is the .gitignore registry (load_registered_paths). + """ + from largefiles import asset_candidates, asset_names, load_registered_paths + + name = file_entry.get("name", "") + if not name or name.endswith("/") or file_entry.get("type") == "directory": + return None + declared = _declared_hashes(file_entry) + if file_entry.get("unsourceable") and not any(declared.values()): + return None + if file_entry.get("zipped_file") and not any(declared.values()): + return None + if registered is None: + registered = load_registered_paths() + assets = set(asset_candidates(name, registered)) + if not assets: + return None + records = _records_by_path(db) + declared_size = file_entry.get("size") + for path, asset in sorted(asset_names(registered).items()): + record = records.get(path) + if asset not in assets or record is None: + continue + if isinstance(declared_size, int) and record.get("size") != declared_size: + continue + if all( + not values or str(record.get(field, "")).lower() in values + for field, values in declared.items() + ): + return path + return None + + # Digest length in hex digits -> kind. 29 is the MD5 Batocera writes with # its last three digits cut, matched by prefix as the resolver does. _HASH_KINDS = {8: "crc32", 29: "md5", 32: "md5", 40: "sha1", 64: "sha256"} diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 9f89b4cd..cc5fa99a 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -873,6 +873,10 @@ def main() -> None: verify_cmd.append("--include-archived") if args.target: verify_cmd.extend(["--target", args.target]) + if args.offline: + # The packs of step 4 fetch no release asset: a missing one is + # missing on both sides. + verify_cmd.append("--offline") ok, verify_output = run(verify_cmd, STEP_LABELS["verify"]) results["verify"] = ok all_ok = all_ok and ok diff --git a/scripts/verify.py b/scripts/verify.py index 61eab4fe..5fe2688d 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -149,6 +149,11 @@ class Status: MISSING = "missing" +# A declaration whose file is a release asset the cache does not hold yet: +# an online build fetches it, verify reads the database record instead. +RELEASE_ASSET_PENDING = "release_asset_pending" + + class Severity: CRITICAL = "critical" # required file missing or bad hash (Recalbox RED) WARNING = "warning" # optional missing or hash mismatch (Recalbox YELLOW) @@ -1075,7 +1080,7 @@ def _resolve_for_platform( disk and the large-file cache: verify never downloads. A frontend that never reads the bytes takes the filename-free core's own image. """ - from packresolve import resolve_declaration + from packresolve import pending_release_asset, resolve_declaration, run_is_offline override = slot_overrides.get( sanitize_pack_path(file_entry.get("destination", file_entry.get("name", ""))) @@ -1084,6 +1089,12 @@ def _resolve_for_platform( file_entry, db, DEFAULT_BIOS_DIR, zip_contents, data_dir_registry, override, offline=True, ) + if status in ("not_found", "hash_mismatch") and not run_is_offline(): + # Not in the cache, and the release holds a copy that answers the + # declaration: an online build fetches and packs it (resolve_file). + asset = pending_release_asset(file_entry, db) + if asset: + return asset, RELEASE_ASSET_PENDING if status == "not_found" and not reads_file_contents(mode): found = agnostic_substitute(file_entry, sys_id, db, platform_profiles) if found: @@ -1234,7 +1245,23 @@ def verify_platform( file_entry.get("destination", file_entry.get("name", "")) ) stored = _storage_verdict(file_entry, emulator_pack=False) - if stored is not None: + if resolve_status == RELEASE_ASSET_PENDING: + # Counted as an online build counts it, which fetches the + # asset. Under --offline the builder cannot, and neither + # does this report (_resolve_for_platform). + result = { + "name": file_entry.get("name", ""), + "required": file_entry.get("required", True), + "status": Status.OK, + "release_asset": local_path, + "reason": "release asset, fetched when the pack is built", + } + packed_order.append(( + sys_id, f"{base_dest}/{destination}" if base_dest else destination, None, + )) + shipped_keys.add(_dest_key(file_entry, sys_id)) + local_path = None + elif stored is not None: result = stored local_path = None # The builder keeps the slot: instructions for a user_provided @@ -1744,6 +1771,12 @@ def print_platform_result( if missing: parts.append(f"{missing} missing") print(f"{label}: {', '.join(parts)} [{mode}]") + fetched = sorted({d["name"] for d in result["details"] if d.get("release_asset")}) + if fetched: + # Counted present: an online build downloads them. --offline counts + # them missing, as an offline build leaves them out. + shown = ", ".join(fetched[:10]) + (f" (+{len(fetched) - 10})" if len(fetched) > 10 else "") + print(f" Release assets fetched when the pack is built ({len(fetched)}): {shown}") fallbacks = result.get("region_fallbacks") or [] if fallbacks: from packreadme import _group_label @@ -2440,6 +2473,7 @@ def _refuse_listing_narrowings( ("--strict", args.strict), ("--verbose", args.verbose), ("--destinations", args.destinations), + ("--offline", args.offline), ): if on: parser.error(f"{flag} is incompatible with {listing}") @@ -2457,6 +2491,8 @@ def _refuse_mode_flags(args: argparse.Namespace, parser: argparse.ArgumentParser "--target is incompatible with --emulator and --system"), (args.destinations and not (args.platform or args.all), "--destinations requires --platform or --all"), + (args.offline and not (args.platform or args.all), + "--offline requires --platform or --all"), (args.destinations and getattr(args, "region", None), "--destinations describes the full pack and is incompatible with --region"), ): @@ -2536,6 +2572,11 @@ def _main(held: contextlib.ExitStack) -> None: help="Show emulator ground truth details", ) parser.add_argument("--json", action="store_true", help="JSON output") + parser.add_argument( + "--offline", action="store_true", + help="Report as an offline build packs: a release asset the cache " + "lacks is missing. Without it, it counts as fetched at build time", + ) parser.add_argument( "--destinations", metavar="FILE", help="With --platform or --all, also write the paths each pack holds " @@ -2552,6 +2593,10 @@ def _main(held: contextlib.ExitStack) -> None: work_from_repo_root(args, parser, ["db", "platforms_dir", "emulators_dir"]) if args.platform: require_platform(args.platform, args.platforms_dir) + if args.offline: + import packresolve + + packresolve.set_offline(True) requested_regions: list[str] = [] if getattr(args, "region", None): @@ -2696,7 +2741,9 @@ def _json_details(details: list[dict], verbose: bool) -> list[dict]: used to be accepted beside --json and dropped, the output byte for byte the same, so a script asking for the passing entries got nothing. """ - return list(details) if verbose else [d for d in details if d["status"] != Status.OK] + return list(details) if verbose else [ + d for d in details if d["status"] != Status.OK or d.get("release_asset") + ] def _exit_if_strict(strict: bool, results: list[dict]) -> None: diff --git a/tests/test_verify_release_asset.py b/tests/test_verify_release_asset.py new file mode 100644 index 00000000..f8c69c3b --- /dev/null +++ b/tests/test_verify_release_asset.py @@ -0,0 +1,131 @@ +"""verify counts a release asset the cache lacks as an online build does. + +verify never downloads. A declaration served by a release asset of the +large-files release, absent from bios/ and from .cache/large/, read as +missing there while an online build fetched it and packed it, so the two +disagreed on every checkout without the cache (CI among them). The +database record of the registered path says whether the asset answers the +declaration. Under --offline the builder fetches nothing, and verify then +counts the file missing as before. +""" + +from __future__ import annotations + +import hashlib +import os +import sys +import tempfile +import unittest +import zipfile +from pathlib import Path +from unittest import mock + +import yaml + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +import common # noqa: E402 +import generate_db # noqa: E402 +import generate_pack as builder # noqa: E402 +import packresolve # noqa: E402 +from verify import Status, verify_platform # noqa: E402 + +PAYLOAD = b"a firmware image the repository does not commit" +SHA1 = hashlib.sha1(PAYLOAD).hexdigest() +REGISTERED = ["bios/Big/big.bin"] + + +class ReleaseAssetNotCached(unittest.TestCase): + mode = "existence" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self._cwd = os.getcwd() + os.chdir(self._tmp.name) + for d in ("emulators", "platforms", "bios/Big"): + Path(d).mkdir(parents=True) + # Indexed by the database, gitignored, so absent from a checkout. + files = {SHA1: { + "path": REGISTERED[0], "name": "big.bin", "size": len(PAYLOAD), + "sha1": SHA1, "md5": hashlib.md5(PAYLOAD).hexdigest(), + "sha256": hashlib.sha256(PAYLOAD).hexdigest(), "crc32": "00000001", + }} + self.db = {"files": files, "indexes": generate_db.build_indexes(files, {})} + self.config = { + "platform": "Big", "verification_mode": self.mode, + "systems": {"sys": {"files": [{ + "name": "big.bin", "destination": "big.bin", "required": True, + "sha1": SHA1, "md5": hashlib.md5(PAYLOAD).hexdigest(), + }]}}, + } + Path("platforms/big.yml").write_text(yaml.dump(self.config)) + Path("platforms/_registry.yml").write_text( + yaml.dump({"platforms": {"big": {"status": "active"}}}) + ) + common._platform_config_cache.clear() + common._emulator_profiles_cache.clear() + patches = ( + mock.patch("largefiles.load_registered_paths", return_value=REGISTERED), + mock.patch.dict(os.environ, {"RETROBIOS_OFFLINE": ""}), + ) + for patch in patches: + patch.start() + self.addCleanup(patch.stop) + self.addCleanup(packresolve.set_offline, False) + packresolve.set_offline(False) + + def tearDown(self): + common._platform_config_cache.clear() + os.chdir(self._cwd) + self._tmp.cleanup() + + def _entry(self) -> dict: + report = verify_platform(self.config, self.db, "emulators", {}) + return next(d for d in report["details"] if d["name"] == "big.bin") + + def test_counted_as_fetched_when_the_pack_is_built(self): + entry = self._entry() + self.assertEqual(entry["status"], Status.OK) + self.assertEqual(entry["release_asset"], REGISTERED[0]) + + def test_offline_counts_it_missing(self): + packresolve.set_offline(True) + self.assertEqual(self._entry()["status"], Status.MISSING) + + def test_an_asset_with_other_content_is_not_counted(self): + self.config["systems"]["sys"]["files"][0]["sha1"] = "0" * 40 + self.assertEqual(self._entry()["status"], Status.MISSING) + + def test_the_online_build_packs_it(self): + cache = Path("cache/big.bin") + cache.parent.mkdir() + cache.write_bytes(PAYLOAD) + with mock.patch.object( + packresolve, "fetch_large_file", return_value=str(cache) + ): + zip_path = builder.generate_pack( + "big", "platforms", self.db, "bios", "out", + emulators_dir="emulators", emu_profiles={}, offline=False, + ) + with zipfile.ZipFile(zip_path) as zf: + self.assertEqual(zf.read("big.bin"), PAYLOAD) + self.assertEqual(self._entry()["status"], Status.OK) + + +class ReleaseAssetNotCachedMd5(ReleaseAssetNotCached): + mode = "md5" + + +class OfflinePipelineVerifiesOffline(unittest.TestCase): + """The offline pipeline builds packs without release assets: its verify + step has to count them the same way, or step 5 compares two answers.""" + + def test_verify_step_gets_offline(self): + source = (REPO_ROOT / "scripts" / "pipeline.py").read_text() + block = source[source.index("verify_cmd = ["):source.index("ok, verify_output = run(")] + self.assertIn('verify_cmd.append("--offline")', block) + + +if __name__ == "__main__": + unittest.main() diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index 1312a04b..ef2392a8 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -39,7 +39,7 @@ Every module in `tests/`, by the area it guards. |------|----------|---------| | Core regression suites | synthetic | `test_e2e`, `test_audit_regressions`, `test_tool_messages` | | File resolution and identity | synthetic + collection | `test_adler32_resolution`, `test_agnostic_scan`, `test_agnostic_scan_mode`, `test_agnostic_scope`, `test_agnostic_substitute`, `test_archive_choice`, `test_bios_companions`, `test_bundled_identity`, `test_casefold_cache`, `test_db_aliases`, `test_declared_names`, `test_derivations`, `test_directory_entries`, `test_extra_identity`, `test_foreign_tree`, `test_foreign_tree_names`, `test_frontend_digest`, `test_game_data_homonyms`, `test_hashing`, `test_identity`, `test_md5_pack_context`, `test_name_tiebreak`, `test_nec_filing`, `test_owner_resolution`, `test_path_suffix`, `test_resolution_evidence`, `test_shipped_content`, `test_twin_declarations`, `test_validated_choice`, `test_variant_choice`, `test_variant_identity`, `test_zipped_member` | -| Verification, cross-reference and profile checks | synthetic + profiles | `test_crypto_aes`, `test_crypto_unique_data`, `test_crypto_verify`, `test_case_insensitive_platforms`, `test_checks_have_values`, `test_core_file_fills_declaration`, `test_core_size_refusal`, `test_cross_reference_sources`, `test_file_consumers`, `test_fileless_audit`, `test_gap_settlement`, `test_hidden_extras`, `test_launcher_layer`, `test_native_mode`, `test_no_case_collisions`, `test_path_conflict_status`, `test_path_null_copies`, `test_profile_checks`, `test_romm_hash_rule`, `test_shared_groups`, `test_standalone_mode`, `test_undeclared_dedup`, `test_unplaced_extras`, `test_validation_scope`, `test_verify_all_dir`, `test_verify_emulator_evidence`, `test_verify_listing`, `test_verify_pack_choice` | +| Verification, cross-reference and profile checks | synthetic + profiles | `test_crypto_aes`, `test_crypto_unique_data`, `test_crypto_verify`, `test_case_insensitive_platforms`, `test_checks_have_values`, `test_core_file_fills_declaration`, `test_core_size_refusal`, `test_cross_reference_sources`, `test_file_consumers`, `test_fileless_audit`, `test_gap_settlement`, `test_hidden_extras`, `test_launcher_layer`, `test_native_mode`, `test_no_case_collisions`, `test_path_conflict_status`, `test_path_null_copies`, `test_profile_checks`, `test_romm_hash_rule`, `test_shared_groups`, `test_standalone_mode`, `test_undeclared_dedup`, `test_unplaced_extras`, `test_validation_scope`, `test_verify_all_dir`, `test_verify_emulator_evidence`, `test_verify_listing`, `test_verify_pack_choice`, `test_verify_release_asset` | | Packs and manifests | synthetic + real packs | `test_artifact_lock`, `test_atomic_artifacts`, `test_batocera_cores`, `test_deterministic_zip`, `test_emulator_pack_counts`, `test_finalize_pack_names`, `test_large_file_cache`, `test_libretro_layout`, `test_manifest_entry`, `test_pack_conformance_dirs`, `test_pack_core_extras`, `test_pack_counts`, `test_pack_destinations`, `test_pack_failures`, `test_pack_file_names`, `test_pack_grouping`, `test_pack_integrity`, `test_pack_member_identity`, `test_pack_readme`, `test_platform_inheritance`, `test_platform_keys`, `test_required_only_archives`, `test_required_only_twins`, `test_split_extras`, `test_split_names`, `test_split_pack`, `test_system_pack_extras`, `test_system_pack_parity`, `test_torrentzip`, `test_which_pack` | | Slots and regions | synthetic + profiles | `test_region`, `test_region_agnostic_scan`, `test_region_groups`, `test_region_held`, `test_region_platform_index`, `test_region_required_only`, `test_slot`, `test_slots`, `test_slots_shipped_file` | | Hardware targets and systems | synthetic + target files | `test_extract_targets`, `test_lakka_targets`, `test_libretro_slugs`, `test_system_ids`, `test_system_selection`, `test_target_names`, `test_target_scrapers`, `test_target_systems`, `test_target_verify` | diff --git a/wiki/tools.md b/wiki/tools.md index e3a61246..eec2ebfb 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -125,6 +125,7 @@ python scripts/verify.py --system atari-lynx # single system python scripts/verify.py --platform retroarch --target switch # filter by hardware python scripts/verify.py --platform recalbox --region us # regional priority list python scripts/verify.py --all --destinations tmp/paths.json # also write the paths each pack holds +python scripts/verify.py --all --offline # a release asset the cache lacks counts missing, as in an offline build python scripts/verify.py --list-emulators # list all emulators python scripts/verify.py --list-systems # list all systems python scripts/verify.py --platform retroarch --list-targets # list available targets From db0e10b687f93a482b98fe138f2906d33b2077fe Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:07:40 +0200 Subject: [PATCH 3/6] fix: count data directory readers in truth LRPS2 reads pcsx2/resources/GameIndex.yaml through its data directory and declares no file entry for it, so the truth entry other cores build named armsx2 and yaps2 alone and hash_conflict missed LRPS2's copy. A core whose data directory holds a file at an entry's path now folds in as a reader with the cached copy's hashes, unless the profile lists what it reads under that tree (yaps2, pcee2). --- scripts/export_native.py | 3 +- scripts/generate_truth.py | 3 + scripts/truth.py | 98 +++++++++++++++++++++++++++- tests/test_truth_data_dir_readers.py | 98 ++++++++++++++++++++++++++++ wiki/testing-guide.md | 2 +- 5 files changed, 201 insertions(+), 3 deletions(-) create mode 100644 tests/test_truth_data_dir_readers.py diff --git a/scripts/export_native.py b/scripts/export_native.py index fd2f0a71..dbdd2ee1 100644 --- a/scripts/export_native.py +++ b/scripts/export_native.py @@ -375,7 +375,8 @@ def current_truth(platform: str, config: dict, platforms_dir: str) -> dict: """The truth model generate_truth would write now, as YAML reads it back.""" registry, profiles, db = _truth_sources(platforms_dir) model = generate_platform_truth( - platform, config, registry.get(platform, {}), profiles, db=db + platform, config, registry.get(platform, {}), profiles, db=db, + data_registry=load_data_dir_registry(platforms_dir), ) return yaml_load(yaml.dump(model, default_flow_style=False, sort_keys=False, allow_unicode=True)) or {} diff --git a/scripts/generate_truth.py b/scripts/generate_truth.py index f2b9ce62..37e2728f 100644 --- a/scripts/generate_truth.py +++ b/scripts/generate_truth.py @@ -16,6 +16,7 @@ sys.path.insert(0, os.path.dirname(__file__)) from common import ( platform_arg, list_registered_platforms, + load_data_dir_registry, load_database, load_emulator_profiles, load_platform_config, @@ -77,6 +78,7 @@ def main(argv: list[str] | None = None) -> None: registry = (yaml_load(f) or {}).get("platforms", {}) profiles = load_emulator_profiles(args.emulators_dir) + data_registry = load_data_dir_registry(args.platforms_dir) # Load database (optional) # The default may be absent, but a --db that was named must exist. @@ -132,6 +134,7 @@ def main(argv: list[str] | None = None) -> None: profiles, db=db, target_cores=target_cores, + data_registry=data_registry, ) out_path = os.path.join(output_dir, f"{name}.yml") diff --git a/scripts/truth.py b/scripts/truth.py index d9739ca3..3488136d 100644 --- a/scripts/truth.py +++ b/scripts/truth.py @@ -7,15 +7,18 @@ and diffs truth against scraped platform data to find divergences. from __future__ import annotations import copy +import os import sys +from pathlib import Path from common import ( _norm_system_id, + load_data_dir_registry, platform_layouts, resolve_platform_cores, standalone_set as platform_standalone_set, ) -from hashing import parse_md5_list +from hashing import compute_hashes, parse_md5_list from validation import ( filter_files_by_mode, installed_with_emulator, @@ -508,6 +511,90 @@ def _declaration_verdict(entry: dict, declaration: dict, db: dict | None) -> str return "undecided" +_REPO_PLATFORMS = Path(__file__).resolve().parent.parent / "platforms" + + +def _fold_data_directory_readers( + systems: dict, + profiles: dict, + cores: list[str], + standalone_set: set[str], + platform_name: str, + data_registry: dict, +) -> None: + """Count a core that reads a data directory among the readers of its files. + + LRPS2 reads pcsx2/resources/GameIndex.yaml from its data directory and + declares no file entry for it, so the entry other cores build for that + path named armsx2 and yaps2 alone: RetroArch's System.dat line, which + describes LRPS2's copy, was paired with armsx2's revision and nothing + said the cores disagree. The copy in the directory's cache is what the + reader reads, so its hashes fold in as that core's declaration and a + different value marks hash_conflict. Only entries other cores already + build are touched: a directory's other files are not BIOS entries. + """ + by_path: dict[str, list[dict]] = {} + for sys_data in systems.values(): + for entry in sys_data.get("files", []): + by_path.setdefault(entry.get("path") or entry.get("name", ""), []).append(entry) + if not by_path: + return + for emu_name in cores: + for view, standalone in platform_layouts( + emu_name, profiles[emu_name], standalone_set + ): + trees = [ + tree for tree in filter_files_by_mode( + [t for t in view.get("data_directories") or [] if isinstance(t, dict)], + standalone, + ) + if tree.get("ref") in data_registry + ] + for tree in trees: + cache = data_registry[tree["ref"]].get("local_cache", "") + if not cache or not os.path.isdir(cache): + continue + # Below the build's own folder, as the emulator pack lays the + # tree out: PPSSPP's assets sit under PPSSPP/, not at the root. + structure = (profiles[emu_name].get("pack_structure") or {}).get( + "standalone" if standalone else "libretro", "" + ) + prefix = "/".join( + part.strip("/") for part in (structure, str(tree.get("destination") or "")) + if part and part.strip("/") + ) + prefix = f"{prefix}/" if prefix else "" + if prefix and any( + str(f.get("path") or "").startswith(prefix) + for f in view.get("files") or [] if isinstance(f, dict) + ): + # The profile names what it reads in the tree: yaps2 + # lists its resources, pcee2 lists patches.zip alone + # (its build compiles GameIndex.yaml and the shaders in). + continue + installed = platform_name in (tree.get("installed_on") or []) + for path, entries in by_path.items(): + if not path.startswith(prefix): + continue + copy_path = os.path.join(cache, path[len(prefix):]) + if not os.path.isfile(copy_path): + continue + held = compute_hashes(copy_path, frozenset({"sha1", "md5", "crc32"})) + declaration = { + "name": os.path.basename(path), + "required": bool(tree.get("required", False)), + "source_ref": tree.get("source_ref"), + **held, + "_opens_by_name": True, + "_brought": None if installed else ( + f"{emu_name}:{'standalone' if standalone else 'libretro'}" + ), + } + for entry in entries: + if emu_name not in entry.get("_cores", set()): + _fold_into(entry, dict(declaration), emu_name) + + def generate_platform_truth( platform_name: str, config: dict, @@ -515,6 +602,7 @@ def generate_platform_truth( profiles: dict[str, dict], db: dict | None = None, target_cores: set[str] | None = None, + data_registry: dict | None = None, ) -> dict: """Generate ground-truth system data for a platform from emulator profiles. @@ -526,6 +614,8 @@ def generate_platform_truth( profiles: all loaded emulator profiles db: optional database for hash enrichment target_cores: optional hardware target core filter + data_registry: the data directory registry (_data_dirs.yml), read + from the repository's platforms/ when not given Returns a dict with platform metadata, systems, and per-file details including which cores reference each file. @@ -702,6 +792,12 @@ def generate_platform_truth( ) sys_cov["unprofiled"].add(emu_name) + _fold_data_directory_readers( + systems, profiles, sorted(cores_profiled), standalone_set, platform_name, + load_data_dir_registry(str(_REPO_PLATFORMS)) if data_registry is None + else data_registry, + ) + for sys_data in systems.values(): _require_by_slot(sys_data.get("files", [])) diff --git a/tests/test_truth_data_dir_readers.py b/tests/test_truth_data_dir_readers.py new file mode 100644 index 00000000..f5897883 --- /dev/null +++ b/tests/test_truth_data_dir_readers.py @@ -0,0 +1,98 @@ +"""A core that reads a file through its data directory is one of its readers. + +LRPS2 reads pcsx2/resources/GameIndex.yaml from the tree its data directory +reference names and declares no file entry for it. The truth entry other +cores build for that path named armsx2 and yaps2 alone, so the export paired +RetroArch's System.dat line, which describes LRPS2's copy, with armsx2's +revision, and hash_conflict did not see LRPS2's value. +""" + +from __future__ import annotations + +import hashlib +import os +import sys +import tempfile +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "scripts")) +sys.path.insert(0, str(REPO_ROOT / "scripts" / "exporter")) + +from exporter.baseline import checked_hashes # noqa: E402 +from truth import generate_platform_truth # noqa: E402 + +GAME_INDEX = "pcsx2/resources/GameIndex.yaml" +DECLARED = b"the revision armsx2 declares" +TREE_COPY = b"the revision the tree holds" + + +def _profile(name: str, **fields) -> dict: + return {"emulator": name, "type": "libretro", "systems": ["sony-playstation-2"], **fields} + + +class DataDirectoryReaders(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(self._tmp.cleanup) + root = Path(self._tmp.name) + for tree, content in (("lrps2", TREE_COPY), ("resources", TREE_COPY)): + relative = GAME_INDEX if tree == "lrps2" else "GameIndex.yaml" + copy = root / tree / relative + copy.parent.mkdir(parents=True) + copy.write_bytes(content) + self.registry = { + "lrps2": {"local_cache": str(root / "lrps2")}, + "resources": {"local_cache": str(root / "resources")}, + } + self.profiles = { + "armsx2": _profile("armsx2", files=[{ + "name": "GameIndex.yaml", "path": GAME_INDEX, "system": "sony-playstation-2", + "sha1": hashlib.sha1(DECLARED).hexdigest(), + }]), + "lrps2": _profile("lrps2", data_directories=[ + {"ref": "lrps2", "destination": ""}, + ], files=[{"name": ".bin", "path": "pcsx2/bios/.bin"}]), + # Names what it reads in the tree, patches.zip alone: the tree + # adds no reader of GameIndex.yaml (pcee2 compiles it in). + "listed": _profile("listed", data_directories=[ + {"ref": "resources", "destination": "pcsx2/resources"}, + ], files=[{"name": "patches.zip", "path": "pcsx2/resources/patches.zip"}]), + } + self.config = {"cores": ["armsx2", "lrps2", "listed"], + "systems": {"sony-playstation-2": {"files": []}}} + + def _entry(self, registry: dict) -> dict: + model = generate_platform_truth( + "plat", self.config, {}, self.profiles, data_registry=registry, + ) + return next( + f for f in model["systems"]["sony-playstation-2"]["files"] + if f["name"] == "GameIndex.yaml" + ) + + def test_the_reader_counts_and_its_copy_conflicts(self): + entry = self._entry(self.registry) + self.assertEqual(entry["_cores"], ["armsx2", "lrps2"]) + self.assertIn("sha1", entry["hash_conflict"]) + # The kept value describes one revision among those the cores read: + # it pairs a line, it never corrects one. + self.assertNotIn("sha1", checked_hashes(entry)) + + def test_without_the_cache_nothing_is_folded(self): + entry = self._entry({}) + self.assertEqual(entry["_cores"], ["armsx2"]) + self.assertNotIn("hash_conflict", entry) + + def test_the_same_copy_adds_a_reader_and_no_conflict(self): + tree_copy = Path(self.registry["lrps2"]["local_cache"]) / GAME_INDEX + tree_copy.write_bytes(DECLARED) + entry = self._entry(self.registry) + self.assertEqual(entry["_cores"], ["armsx2", "lrps2"]) + self.assertNotIn("hash_conflict", entry) + + +if __name__ == "__main__": + os.environ.setdefault("RETROBIOS_OFFLINE", "1") + unittest.main() diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index ef2392a8..bf11ba88 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -44,7 +44,7 @@ Every module in `tests/`, by the area it guards. | Slots and regions | synthetic + profiles | `test_region`, `test_region_agnostic_scan`, `test_region_groups`, `test_region_held`, `test_region_platform_index`, `test_region_required_only`, `test_slot`, `test_slots`, `test_slots_shipped_file` | | Hardware targets and systems | synthetic + target files | `test_extract_targets`, `test_lakka_targets`, `test_libretro_slugs`, `test_system_ids`, `test_system_selection`, `test_target_names`, `test_target_scrapers`, `test_target_systems`, `test_target_verify` | | Installer and downloads | loopback server | `test_check_release`, `test_download`, `test_install`, `test_install_cores`, `test_install_experience`, `test_install_selection` | -| Truth and native export | synthetic + cached originals | `test_export_counts`, `test_export_native_layer`, `test_export_report`, `test_exporters`, `test_truth_diff`, `test_truth_layout` | +| Truth and native export | synthetic + cached originals | `test_export_counts`, `test_export_native_layer`, `test_export_report`, `test_exporters`, `test_truth_data_dir_readers`, `test_truth_diff`, `test_truth_layout` | | Scrapers, upstream and provenance | inline sources, synthetic | `test_check_freshness`, `test_dedup`, `test_fbneo_parser`, `test_find_pinned_revision`, `test_hash_merge`, `test_launchers`, `test_launchers_emudeck`, `test_launchers_recalbox`, `test_launchers_retrobat`, `test_launchers_retrodeck`, `test_launchers_retropie`, `test_launchers_rocknix`, `test_mame_devices`, `test_mame_parser`, `test_mame_ref_audit`, `test_profile_sync`, `test_provenance`, `test_refresh_data_dirs`, `test_refresh_failures`, `test_refresh_stale`, `test_retrodeck_scraper_errors`, `test_romset_recipes`, `test_scraper_contract`, `test_upstream`, `test_upstream_index` | | Site, docs and repository contracts | rendered pages, sources | `test_contributing_text`, `test_data_dir_refs`, `test_dataclass_defaults`, `test_doc_pages`, `test_doc_surfaces`, `test_optional_dependencies`, `test_prose_style`, `test_site_availability`, `test_site_cores`, `test_site_exports`, `test_site_platform_hashes`, `test_site_render`, `test_site_system_links`, `test_site_validation`, `test_site_write`, `test_validate_pr`, `test_validate_pr_flow`, `test_validate_pr_hashes`, `test_workflow_paths` | From 7ca6adca56088ee6eb97f4a9a58737ac860ffeb8 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:17:30 +0200 Subject: [PATCH 4/6] fix: check narrowed packs against their own list After --all-variants, --verify-packs printed "hashes checked during the build" for the platform-only, truth and required-only packs and compared none of them, so one missing a file its narrowed list requires passed. Each pack is now held to the platform list narrowed the way it was built: source and required_only, read from its name, reach the baseline, the region grouping and the replay of the core extras. --- scripts/generate_pack.py | 37 +++++++++-- scripts/packextras.py | 10 ++- scripts/packverify.py | 21 ++++++- tests/test_pack_core_extras.py | 101 +++++++++++++++++++++++++++++++ tests/test_verify_pack_choice.py | 33 +++++----- 5 files changed, 175 insertions(+), 27 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index b4b25c53..6bfd3c0c 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -2632,16 +2632,15 @@ def _run_verify_packs(args: argparse.Namespace) -> None: # A group pack and a pack built for the platform alone can both be # present, and each one is a pack of this platform. for pack_name in found: - if _narrows_contents(pack_name): - # Only reached after an --all-variants build, whose stage 1 - # re-hashed every member against the collection above. The - # platform's list describes the full pack, so a narrowed one - # is not compared to it. + variant = _pack_variant(pack_name) + if variant is None: + # One BIOS per slot: the slot choice is not replayed here. print( f" {platform_name} [{pack_name}]: hashes checked during the build; " - "narrowed by design, so not compared to the platform's full list" + "one per slot by design, so not compared to the platform's full list" ) continue + source, required_only = variant zip_path = os.path.join(args.output_dir, pack_name) result = verify_pack_against_platform( zip_path, @@ -2653,6 +2652,8 @@ def _run_verify_packs(args: argparse.Namespace) -> None: regions=verify_regions, data_registry=verify_data_registry, target_cores=tc, + source=source, + required_only=required_only, ) (ok, _checked, _present, errors, bl_checked, bl_present, core_checked, core_present, bl_excluded) = result @@ -4732,6 +4733,30 @@ def _narrows_contents(pack_name: str) -> bool: return any(f"{tag}_" in pack_name for tag in _CONTENT_NARROWING_TAGS) +def _pack_variant(pack_name: str) -> tuple[str, bool] | None: + """The (source, required_only) a pack was built as, read from its name. + + None for a one-per-slot pack, whose slot choice the check does not + replay. The tags come from the name builder, as for _narrows_contents. + """ + def tagged(**dimension: object) -> bool: + tags = [ + tag for tag, _label in _narrowings( + str(dimension.get("source", "full")), None, None, + bool(dimension.get("one_per_slot", False)), + bool(dimension.get("required_only", False)), + ) + ] + return all(f"{tag}_" in pack_name for tag in tags) + + if tagged(one_per_slot=True): + return None + source = next( + (s for s in ("platform", "truth") if tagged(source=s)), "full" + ) + return source, tagged(required_only=True) + + def _pack_archives(output_dir: str) -> list[str]: """The ZIPs of a directory that are packs. diff --git a/scripts/packextras.py b/scripts/packextras.py index 05e5df63..e150ca9f 100644 --- a/scripts/packextras.py +++ b/scripts/packextras.py @@ -226,6 +226,8 @@ def hidden_extras( resolver: ExtraResolver | None = None, placed_kind: dict[str, str] | None = None, placed_local: dict[str, str] | None = None, + include_all: bool = False, + required_only: bool = False, ) -> dict[tuple[str, str, str], str]: """Report entries the builder holds and still leaves out, with the reason. @@ -247,7 +249,9 @@ def hidden_extras( report entry names. ``placed_local`` receives the local copy the builder writes at each of those destinations, for the pack integrity check. ``resolver`` is the builder's lookup. An extra it cannot resolve is - skipped and takes no path, as the builder skips it. + skipped and takes no path, as the builder skips it. ``include_all`` and + ``required_only`` are the builder's for a --source truth or a + --required-only pack: an optional extra is skipped before it takes a path. """ from packpaths import _register_path, placement_block @@ -267,8 +271,10 @@ def hidden_extras( hidden: dict[tuple[str, str, str], str] = {} for fe in _collect_emulator_extras( config, emulators_dir, db, set(shipped), base_dest, profiles, - target_cores=target_cores, undeclared=report, + target_cores=target_cores, include_all=include_all, undeclared=report, ): + if required_only and fe.get("required") is False: + continue dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if not dest or (region_drops and dest in region_drops): continue diff --git a/scripts/packverify.py b/scripts/packverify.py index f47b8c3a..dfc7f806 100644 --- a/scripts/packverify.py +++ b/scripts/packverify.py @@ -535,6 +535,8 @@ def _builder_extras( target_cores: set[str] | None, region_drops: set[str], data_registry: dict | None, + include_all: bool = False, + required_only: bool = False, ) -> dict[str, str]: """{full destination: local copy} of each core extra the builder writes. @@ -584,6 +586,8 @@ def _builder_extras( destination_owners(platform_profiles), True, profiles=emu_profiles, ), placed_local=placed_local, + include_all=include_all, + required_only=required_only, ) return placed_local @@ -607,13 +611,18 @@ def verify_pack_against_platform( regions: list[str] | None = None, data_registry: dict | None = None, target_cores: set[str] | None = None, + source: str = "full", + required_only: bool = False, ) -> tuple[bool, int, int, list[str], int, int, int, int, int]: """Verify a pack ZIP against its platform config and core requirements. A region priority list narrows the expectation to what the builder would have packed, using the same selection function. A hardware target narrows it the same way and for the same reason: the systems it removes are not - missing from the pack, they were never asked for. + missing from the pack, they were never asked for. ``source`` and + ``required_only`` are the variant the pack was built as: a platform-only + pack is held to the platform's list alone, a truth pack to the cores' + files alone, and a required-only pack to the required entries of both. Checks: 1. Every baseline file declared by the platform exists in the ZIP @@ -659,6 +668,9 @@ def verify_pack_against_platform( base_dest, emu_profiles, target_cores=target_cores, + include_extras=(source != "platform"), + include_all=(source == "truth"), + required_only=required_only, held_out=held, ) region_drops = region_mod.resolve_region_drops( @@ -700,8 +712,10 @@ def verify_pack_against_platform( for i in range(1, len(parts)): zip_parents.add("/".join(parts[:i])) baseline_groups: dict[str, list[dict]] = {} - for _sys_id, system in expected_systems.items(): + for _sys_id, system in (expected_systems if source != "truth" else {}).items(): for fe in system.get("files", []): + if required_only and fe.get("required") is False: + continue dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if not dest: continue @@ -787,11 +801,12 @@ def verify_pack_against_platform( # holds the platform's other dump. core_checked = 0 core_present = 0 - if db is not None: + if db is not None and source != "platform": placed_local = _builder_extras( config, emulators_dir, db, emu_profiles, baseline_groups, base_dest, is_flat, zip_set, zip_lower, target_cores, region_drops, data_registry, + include_all=(source == "truth"), required_only=required_only, ) for full, local in sorted(placed_local.items()): core_checked += 1 diff --git a/tests/test_pack_core_extras.py b/tests/test_pack_core_extras.py index 9cd50724..14cacd04 100644 --- a/tests/test_pack_core_extras.py +++ b/tests/test_pack_core_extras.py @@ -87,5 +87,106 @@ class CoreFilesAsTheBuilderPlacesThem(unittest.TestCase): ) +class NarrowedPacksAgainstTheirOwnList(unittest.TestCase): + """--verify-packs after --all-variants said "hashes checked during the + build" for the five narrowed packs and compared none of them: a narrowed + pack missing a file its narrowed list requires passed. Each variant is + now held to the platform list narrowed the way it was built.""" + + def setUp(self): + import common + import generate_db + import generate_pack + + self.gp = generate_pack + tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(tmp.cleanup) + previous = os.getcwd() + os.chdir(tmp.name) + self.addCleanup(os.chdir, previous) + files: dict = {} + for rel in ("bios/p/req.bin", "bios/p/opt.bin", "bios/c/core.bin", "bios/c/extra.bin"): + payload = rel.encode() + Path(rel).parent.mkdir(parents=True, exist_ok=True) + Path(rel).write_bytes(payload) + sha1 = hashlib.sha1(payload).hexdigest() + files[sha1] = {"path": rel, "name": Path(rel).name, "size": len(payload), + "sha1": sha1, "md5": hashlib.md5(payload).hexdigest(), + "sha256": hashlib.sha256(payload).hexdigest(), + "crc32": "00000001"} + self.db = {"files": files, "indexes": generate_db.build_indexes(files, {})} + Path("platforms").mkdir() + config = {"platform": "P", "verification_mode": "existence", "cores": ["c"], + "systems": {"s": {"files": [ + {"name": "req.bin", "destination": "req.bin", "required": True}, + {"name": "opt.bin", "destination": "opt.bin", "required": False}, + ]}}} + Path("platforms/p.yml").write_text(yaml.safe_dump(config), encoding="utf-8") + Path("platforms/_registry.yml").write_text( + yaml.safe_dump({"platforms": {"p": {"status": "active"}}}), encoding="utf-8" + ) + Path("emulators").mkdir() + Path("emulators/c.yml").write_text(yaml.safe_dump({ + "emulator": "c", "type": "libretro", "systems": ["s"], "files": [ + {"name": "core.bin", "path": "core.bin", "required": True}, + {"name": "extra.bin", "path": "extra.bin", "required": False}, + ]}), encoding="utf-8") + common._platform_config_cache.clear() + common._emulator_profiles_cache.clear() + self.addCleanup(common._platform_config_cache.clear) + self.addCleanup(common._emulator_profiles_cache.clear) + self.profiles = common.load_emulator_profiles("emulators") + + def _build(self, source: str, required: bool) -> tuple[str, set[str]]: + zip_path = self.gp.generate_pack( + "p", "platforms", self.db, "bios", "out", emulators_dir="emulators", + emu_profiles=self.profiles, source=source, required_only=required, + offline=True, + ) + with zipfile.ZipFile(zip_path) as zf: + return zip_path, set(zf.namelist()) - {"README.txt"} + + def _check(self, zip_path: str, source: str, required: bool) -> tuple: + return packverify.verify_pack_against_platform( + zip_path, "p", "platforms", self.db, emulators_dir="emulators", + emu_profiles=self.profiles, source=source, required_only=required, + ) + + def test_every_variant_passes_as_built(self): + expected = { + ("full", False): {"req.bin", "opt.bin", "core.bin", "extra.bin"}, + ("full", True): {"req.bin", "core.bin"}, + ("platform", False): {"req.bin", "opt.bin"}, + ("platform", True): {"req.bin"}, + ("truth", False): {"core.bin", "extra.bin"}, + ("truth", True): {"core.bin"}, + } + for (source, required), members in expected.items(): + with self.subTest(source=source, required=required): + zip_path, held = self._build(source, required) + self.assertEqual(held, members) + ok, _c, _p, errors, *_rest = self._check(zip_path, source, required) + self.assertTrue(ok, errors) + self.assertEqual( + self.gp._pack_variant(os.path.basename(zip_path)), (source, required) + ) + + def test_a_narrowed_pack_missing_a_required_file_fails(self): + for source, required, lost in ( + ("full", True, "core.bin"), ("platform", True, "req.bin"), + ("truth", False, "extra.bin"), ("truth", True, "core.bin"), + ): + with self.subTest(source=source, required=required): + zip_path, held = self._build(source, required) + with zipfile.ZipFile(zip_path) as zf: + kept = {n: zf.read(n) for n in held if n != lost} + with zipfile.ZipFile(zip_path, "w") as zf: + for name, data in kept.items(): + zf.writestr(name, data) + ok, _c, _p, errors, *_rest = self._check(zip_path, source, required) + self.assertFalse(ok) + self.assertTrue(any(lost in e for e in errors), errors) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_verify_pack_choice.py b/tests/test_verify_pack_choice.py index 75ab4c5f..9ead675d 100644 --- a/tests/test_verify_pack_choice.py +++ b/tests/test_verify_pack_choice.py @@ -63,8 +63,13 @@ class VerifyPackChoice(unittest.TestCase): def record(zip_path, platform_name, *a, **kw): checked.append((platform_name, os.path.basename(zip_path))) + self.variants.append( + (kw.get("source", "full"), kw.get("required_only", False)) + ) return _ok_result() + self.variants: list[tuple[str, bool]] = [] + with mock.patch.object(self.gp, "verify_pack_against_platform", record): self.gp._run_verify_packs(args) return checked @@ -94,26 +99,22 @@ class VerifyPackChoice(unittest.TestCase): self._run(self._args()) self.assertEqual(ctx.exception.code, 1) - def test_all_variants_names_every_variant_it_built(self): - """After --all-variants the check opened the full pack only: the five - narrowed packs the same run wrote passed without a line.""" - import contextlib - import io - - names = [] + def test_all_variants_checks_every_variant_as_it_was_built(self): + """After --all-variants the check opened the full pack only, and then + said "hashes checked during the build" for the five narrowed packs: + one missing a file its narrowed list requires passed. Each is now + held to the platform's list narrowed the same way.""" + names = {} for source, required in self.gp.ALL_VARIANTS: tags = "".join( tag for tag, _ in self.gp._narrowings(source, None, None, False, required) ) - names.append(f"{self.stem}{tags}_BIOS_Pack.zip") - self._touch(names[-1]) - out = io.StringIO() - with contextlib.redirect_stdout(out): - checked = self._run(self._args(all_variants=True)) - self.assertEqual(checked, [("retroarch", f"{self.stem}_BIOS_Pack.zip")]) - printed = out.getvalue() - for name in names[1:]: - self.assertIn(f"[{name}]: hashes checked during the build", printed) + names[f"{self.stem}{tags}_BIOS_Pack.zip"] = (source, required) + self._touch(f"{self.stem}{tags}_BIOS_Pack.zip") + checked = self._run(self._args(all_variants=True)) + self.assertEqual( + dict(zip((name for _p, name in checked), self.variants)), names + ) def test_the_run_ends_on_a_summary(self): """A failing pack was named only above its own error lines, and the From ef69687fe1fdaf98756e9dc5290bf7a19f9c0913 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:18:53 +0200 Subject: [PATCH 5/6] fix: replay held omissions in install regions The builder keeps a held regional fallback when the requested file is missing (since 2026-10-17), and the comment beside the manifest's omissions still said the opposite. A file the builder holds but the manifest cannot download sat in omitted_files counted unheld, so the installer kept a fallback the pack drops. Such an omission now carries held: true and narrow() counts it held, as the builder does. --- install.ps1 | 2 +- install.py | 8 +- install.sh | 2 +- schemas/install-manifest.schema.json | 4 + scripts/generate_pack.py | 30 ++++---- tests/test_install_selection.py | 110 +++++++++++++++++++++++++++ 6 files changed, 139 insertions(+), 17 deletions(-) diff --git a/install.ps1 b/install.ps1 index 25f414a8..16e613e2 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "7edd155bb1f50a71007cd3500d6e41003e969dfa3c09570ba517399e94faa428" + $defaultInstallSha256 = "fe8de4eda0919c268d4d2c0d571c7d0dc1cf675d7604263766f5f99a35aa3724" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 7f73526d..9cd3e52e 100644 --- a/install.py +++ b/install.py @@ -1754,6 +1754,8 @@ def _check_omission(entry: dict, dest: str) -> None: raise ValueError(f"invalid omitted required flag for {dest}") if entry.get("reason") not in OMISSION_REASONS: raise ValueError(f"invalid omission reason for {dest}") + if "held" in entry and entry["held"] is not True: + raise ValueError(f"invalid omitted held flag for {dest}") cores = entry.get("cores") if cores is not None and ( not isinstance(cores, list) @@ -2516,8 +2518,10 @@ def narrow( omitted = _filter_by_target(omitted, cores) if regions: # The builder's held set: what the collection serves. An omission - # answers the request but takes no installable file's place. - held = {f["dest"] for f in files} + # answers the request but takes no installable file's place, unless + # the builder holds it (held: true) and only this download lacks it: + # the pack drops the fallback it displaces, and so does this. + held = {f["dest"] for f in files} | {o["dest"] for o in omitted if o.get("held")} dropped = region_drops(files + omitted, regions, systems, held) files = [f for f in files if f["dest"] not in dropped] omitted = [o for o in omitted if o["dest"] not in dropped] diff --git a/install.sh b/install.sh index f5a9af01..6f563d44 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="7edd155bb1f50a71007cd3500d6e41003e969dfa3c09570ba517399e94faa428" +DEFAULT_INSTALL_SHA256="fe8de4eda0919c268d4d2c0d571c7d0dc1cf675d7604263766f5f99a35aa3724" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/schemas/install-manifest.schema.json b/schemas/install-manifest.schema.json index 0174cc1b..cca54788 100644 --- a/schemas/install-manifest.schema.json +++ b/schemas/install-manifest.schema.json @@ -170,6 +170,10 @@ "below_platform_folder" ] }, + "held": { + "description": "The pack builder holds this file and the installer cannot download it. Region narrowing counts it held, as the builder does, so it displaces the same fallback.", + "const": true + }, "cores": { "oneOf": [ { diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 6bfd3c0c..5ae48da9 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -3890,7 +3890,7 @@ def _manifest_core_entries( # without the cores served here leaves the slot to it, missing, # and the installer narrowing to those cores has to say so too. entry.setdefault("absent_claims", []).append( - {k: v for k, v in displaced.items() if k != "dest"} + {k: v for k, v in displaced.items() if k not in ("dest", "held")} ) total_size += file_size seen_destinations.add(full_dest) @@ -3937,8 +3937,9 @@ def _manifest_selection( required_only: bool, regions: list[str] | None, undeclared: list[dict] | None = None, -) -> tuple[set[str], SelectionIndex]: - """Destinations a region list removes, and the groups the installer replays. +) -> tuple[set[str], SelectionIndex, set[str]]: + """Destinations a region list removes, the groups the installer replays, + and the destinations the builder holds. Grouped as the pack groups them, so a region the installer applies to the full manifest withdraws what `--region` withdraws from the pack. @@ -3970,7 +3971,7 @@ def _manifest_selection( ) return drops, SelectionIndex.build( region_groups, region_index, pack_systems, extras, required_only - ) + ), held def _record_absent( @@ -4132,6 +4133,9 @@ def generate_manifest( reason: str, cores: list[str] | None, ) -> None: + bare = sanitize_pack_path( + file_entry.get("destination", file_entry.get("name", "")) + ) omitted_by_destination[full_destination] = { "dest": manifest_destination(full_destination), "name": str(file_entry.get("name") or ""), @@ -4139,14 +4143,14 @@ def generate_manifest( "required": bool(file_entry.get("required", True)), "reason": reason, "cores": cores, - # The builder counts an unavailable regional file in its groups: - # the installer must too, or it keeps a fallback the pack drops. - **selection.fields( - sanitize_pack_path( - file_entry.get("destination", file_entry.get("name", "")) - ), - str(file_entry.get("name") or ""), - ), + # An omission keeps its region groups, as the builder groups it: + # a requested file the collection lacks answers the request + # without displacing a held fallback (resolve_region_drops, + # held). One the builder holds and the installer cannot fetch + # does displace it in the pack, so it says so, and the installer + # drops the same fallback. + **selection.fields(bare, str(file_entry.get("name") or "")), + **({"held": True} if bare in builder_held else {}), } # Read by the region grouping and by phase 2 with the same arguments. @@ -4158,7 +4162,7 @@ def generate_manifest( if source != "platform" else None ) - region_drops, selection = _manifest_selection( + region_drops, selection, builder_held = _manifest_selection( config, pack_systems, emulators_dir, db, base_dest, emu_profiles, target_cores, source, required_only, regions, undeclared=undeclared, ) diff --git a/tests/test_install_selection.py b/tests/test_install_selection.py index 2dcfb9d5..1318029b 100644 --- a/tests/test_install_selection.py +++ b/tests/test_install_selection.py @@ -124,6 +124,45 @@ class TheRegionRuleIsTheBuilders(unittest.TestCase): {o["dest"] for o in omitted} - drops, ) + def test_narrow_counts_what_the_builder_holds(self): + """A file the builder holds and the installer cannot download sits in + omitted_files. Counted unheld, it left the installer a fallback the + pack drops. The manifest marks it held, and narrow replays the + builder's held set: installable files plus held omissions.""" + rng = random.Random(20261021) + replayed = 0 + for trial in range(500): + groups, index, entries, requested, held = self._case(rng) + pack_only = {dest for dest in held if rng.random() < 0.3} + files = [dict(e, size=1, cores=None) for e in entries + if e["dest"] in held - pack_only] + omitted = [dict(e, cores=None, **({"held": True} if e["dest"] in pack_only else {})) + for e in entries if e["dest"] not in held - pack_only] + kept, still_omitted = install.narrow(files, omitted, [], [], requested) + drops = region.resolve_region_drops(groups, index, requested, held) + unheld = region.resolve_region_drops( + groups, index, requested, held - pack_only + ) + replayed += drops != unheld + with self.subTest(trial=trial): + self.assertEqual( + {f["dest"] for f in kept}, + {f["dest"] for f in files} - drops, + ) + self.assertEqual( + {o["dest"] for o in still_omitted}, + {o["dest"] for o in omitted} - drops, + ) + # The cases where counting them unheld answers otherwise exist. + self.assertGreater(replayed, 0) + + def test_the_held_flag_is_checked_at_the_boundary(self): + entry = {"name": "a.bin", "system": "s", "required": True, + "reason": "not_found", "cores": None} + install._check_omission({**entry, "held": True}, "a.bin") + with self.assertRaises(ValueError): + install._check_omission({**entry, "held": "yes"}, "a.bin") + def test_a_lone_core_file_leaves_its_systems_drop(self): """Dolphin's GC/EUR/IPL.bin is a core extra no system names, plus the GameCube's own declaration. The builder skips the one-file group, so @@ -706,6 +745,77 @@ class InstallMatchesATargetManifest(unittest.TestCase): +class AHeldFileTheInstallerCannotFetch(unittest.TestCase): + """The builder holds a release asset the manifest run had no copy of: it + counts in the builder's groups and displaces the American fallback for + --region japan, while the manifest can only list it as omitted. Counted + unheld there, the installer kept the fallback the pack drops.""" + + def setUp(self): + import common + import generate_db + import generate_pack + + self.gp = generate_pack + tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(tmp.cleanup) + previous = os.getcwd() + os.chdir(tmp.name) + self.addCleanup(os.chdir, previous) + Path("bios/Sys").mkdir(parents=True) + Path("bios/Sys/us.bin").write_bytes(b"us") + sha1 = hashlib.sha1(b"us").hexdigest() + files = {sha1: {"path": "bios/Sys/us.bin", "name": "us.bin", "size": 2, + "sha1": sha1, "md5": hashlib.md5(b"us").hexdigest(), + "sha256": hashlib.sha256(b"us").hexdigest(), + "crc32": "00000001"}} + self.db = {"files": files, "indexes": generate_db.build_indexes(files, {})} + Path("platforms").mkdir() + Path("platforms/p.yml").write_text(json.dumps({ + "platform": "P", "verification_mode": "existence", "cores": ["core"], + "systems": {"sys": {"files": []}}, + })) + Path("platforms/_registry.yml").write_text( + json.dumps({"platforms": {"p": {"status": "active"}}}) + ) + Path("emulators").mkdir() + Path("emulators/core.yml").write_text(json.dumps({ + "emulator": "core", "type": "libretro", "systems": ["sys"], "files": [ + # A release asset: held by the builder whether or not this + # run has a copy to describe. + {"name": "jp.bin", "system": "sys", "region": ["japan"], + "storage": "large_file", "required": True}, + {"name": "us.bin", "system": "sys", "region": ["north-america"], + "required": True}, + ]})) + common._platform_config_cache.clear() + common._emulator_profiles_cache.clear() + self.addCleanup(common._platform_config_cache.clear) + self.addCleanup(common._emulator_profiles_cache.clear) + self.profiles = common.load_emulator_profiles("emulators") + + def _manifest(self, regions): + return self.gp.generate_manifest( + "p", "platforms", self.db, "bios", "platforms/_registry.yml", + emulators_dir="emulators", emu_profiles=self.profiles, + regions=regions, offline=True, + ) + + def test_the_installer_drops_the_fallback_the_pack_drops(self): + full = self._manifest(None) + omitted = {o["dest"]: o for o in full["omitted_files"]} + self.assertTrue(omitted["jp.bin"].get("held")) + built = self._manifest(["japan"]) + self.assertNotIn("us.bin", {f["dest"] for f in built["files"]}) + files, still = install.narrow( + full["files"], full["omitted_files"], [], [], ["japan"] + ) + self.assertEqual({f["dest"] for f in files}, {f["dest"] for f in built["files"]}) + self.assertEqual( + {o["dest"] for o in still}, {o["dest"] for o in built["omitted_files"]} + ) + + class ACoreWithNothingHeldIsAnOmission(unittest.TestCase): """DOSBox serves SC-55/WAVEROM3.BIN, DOSBox Pure wants it and nothing is held for it. A pack built for a target without DOSBox lists it as left From 6ae0dcbbf1b7e05da8ace72e84b4f5ce08041dc4 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 11:36:39 +0200 Subject: [PATCH 6/6] fix: read the manifest's extras lookup in tests The manifest's core extras now go through ExtraResolver, which calls validated_choice: the test reads that path instead of a direct call. --- tests/test_validated_choice.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tests/test_validated_choice.py b/tests/test_validated_choice.py index 74f73068..3ad102bd 100644 --- a/tests/test_validated_choice.py +++ b/tests/test_validated_choice.py @@ -97,9 +97,28 @@ class ValidatedChoice(unittest.TestCase): for call in ast.walk(node) ) } - self.assertLessEqual( - {"generate_pack", "generate_manifest", "_manifest_core_entries"}, callers + self.assertLessEqual({"generate_pack", "generate_manifest"}, callers) + # The manifest's core extras go through the builder's own lookup, + # which reads validated_choice (packextras.ExtraResolver). + resolvers = { + node.name + for node in ast.walk(tree) + if isinstance(node, ast.FunctionDef) + and any( + isinstance(call, ast.Call) and getattr(call.func, "id", None) == "ExtraResolver" + for call in ast.walk(node) + ) + } + self.assertLessEqual({"generate_pack", "_manifest_core_entries"}, resolvers) + extras = ast.parse((REPO_ROOT / "scripts" / "packextras.py").read_text(encoding="utf-8")) + resolve = next( + node for node in ast.walk(extras) + if isinstance(node, ast.FunctionDef) and node.name == "resolve" ) + self.assertTrue(any( + isinstance(call, ast.Call) and getattr(call.func, "id", None) == "validated_choice" + for call in ast.walk(resolve) + )) def test_core_extras_are_judged_too(self): """The baseline loop judged a platform's own files while the core-extras loop