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