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] 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` |