From 236f30860eb9bca814ab63dd92047a3e328a3add Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:08:48 +0200 Subject: [PATCH] fix: check core files where the builder puts them The pack integrity check rebuilt the core extras with no taken paths and tested presence alone, so it credited core files at paths the platform's own file holds and printed a core count the builder contradicted. It now replays the builder's placement through packextras.hidden_extras and compares each placed member with the copy the builder resolves. --- scripts/packextras.py | 7 +- scripts/packverify.py | 153 +++++++++++++++++++++------------ tests/test_pack_core_extras.py | 91 ++++++++++++++++++++ wiki/testing-guide.md | 2 +- 4 files changed, 194 insertions(+), 59 deletions(-) create mode 100644 tests/test_pack_core_extras.py diff --git a/scripts/packextras.py b/scripts/packextras.py index 9861f3aa..66f15b63 100644 --- a/scripts/packextras.py +++ b/scripts/packextras.py @@ -216,6 +216,7 @@ def hidden_extras( report: list[dict] | None = None, resolver: ExtraResolver | None = None, placed_kind: dict[str, str] | None = None, + placed_local: dict[str, str] | None = None, ) -> dict[tuple[str, str, str], str]: """Report entries the builder holds and still leaves out, with the reason. @@ -234,7 +235,9 @@ def hidden_extras( receives the full destination of each held extra the pack carries, and ``placed_kind`` its category, carried by the extra itself: the second pass, the archive-prefix pass and the agnostic scan copy files no gap - report entry names. ``resolver`` is the builder's lookup. An extra it cannot resolve is + 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. """ from packpaths import _register_path, placement_block @@ -292,6 +295,8 @@ def hidden_extras( placed.add(full) if placed_kind is not None and local: placed_kind[full] = fe.get("category", "bios") + if placed_local is not None and local: + placed_local[full] = local return hidden diff --git a/scripts/packverify.py b/scripts/packverify.py index 94c13046..a48d251f 100644 --- a/scripts/packverify.py +++ b/scripts/packverify.py @@ -10,10 +10,7 @@ from collections import Counter from auto_fetch import DEFAULT_BIOS_DIR from artifacts import _build_timestamp -from packextras import _collect_emulator_extras -from packextras import _detect_extras_prefix from packpaths import _has_path_conflict -from packpaths import _register_path from ziptools import build_zip_contents_index from ziptools import check_inside_zip from nativemode import digest_algorithm @@ -525,6 +522,81 @@ def _structural_errors(zf: zipfile.ZipFile, zip_set: set) -> list[str]: return errors +def _builder_extras( + config: dict, + emulators_dir: str, + db: dict, + emu_profiles: dict, + baseline_groups: dict[str, list[dict]], + base_dest: str, + is_flat: bool, + zip_set: set[str], + zip_lower: dict[str, str], + target_cores: set[str] | None, + region_drops: set[str], + data_registry: dict | None, +) -> dict[str, str]: + """{full destination: local copy} of each core extra the builder writes. + + The platform's paths come first, as in the builder: a destination the + pack holds whose declarations resolve to a file, or that the platform + keeps for a user_provided or external entry. One whose declarations + resolve to nothing is left free, since the builder then places a core's + file there. + """ + from packextras import ExtraResolver, hidden_extras + from packresolve import resolve_declaration + from validation import _build_validation_index, destination_owners + + shipped: dict[str, str | None] = {} + for expected, declarations in baseline_groups.items(): + if expected not in zip_set and expected.lower() not in zip_lower: + continue + unflat = ( + f"{base_dest}/{expected}" + if is_flat and base_dest and not expected.startswith(f"{base_dest}/") + else expected + ) + if any(fe.get("storage") in ("user_provided", "external") for fe in declarations): + shipped[unflat] = None + continue + local = next( + ( + found for fe in declarations + if (found := resolve_declaration( + fe, db, DEFAULT_BIOS_DIR, None, data_registry, None, offline=True, + )[0]) + ), + None, + ) + if local: + shipped[unflat] = local + platform_profiles = { + name: emu_profiles[name] for name in resolve_platform_cores(config, emu_profiles) + } + placed_local: dict[str, str] = {} + hidden_extras( + config, emulators_dir, db, emu_profiles, shipped, target_cores, + region_drops or None, + resolver=ExtraResolver( + db, DEFAULT_BIOS_DIR, build_zip_contents_index(db), data_registry, + _build_validation_index(platform_profiles), + destination_owners(platform_profiles), True, + ), + placed_local=placed_local, + ) + return placed_local + + +def _same_content(zf: zipfile.ZipFile, member: str, local: str) -> bool: + """Whether a pack member carries the bytes of a local copy, or the + members of an archive the builder normalized.""" + with open(local, "rb") as handle: + if zf.read(member) == handle.read(): + return True + return local.lower().endswith(".zip") and _same_members(zf, member, local) + + def verify_pack_against_platform( zip_path: str, platform_name: str, @@ -707,66 +779,33 @@ def verify_pack_against_platform( ): errors.append(member_errors[0]) - # 2. Core extras presence (files from emulator profiles, in repo) - # Mirror the pack builder's skip logic: only count files that - # can actually be resolved and don't have path conflicts. + # 2. Core extras: the builder's own placement replayed + # (packextras.hidden_extras, which verify reads too), after the + # platform files it wrote. Each placed extra must sit at its path + # with the bytes the builder resolved: an empty seen set and a + # presence test credited a core file at a path where the pack + # holds the platform's other dump. core_checked = 0 core_present = 0 if db is not None: - core_files = _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - target_cores=target_cores, + 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, ) - seen_conformance: set[str] = set(zip_set) - seen_parents: set[str] = set() - for n in zip_set: - parts = n.split("/") - for i in range(1, len(parts)): - seen_parents.add("/".join(parts[:i])) - extras_pfx = _detect_extras_prefix(config, base_dest) - for fe in core_files: - raw_dest = fe.get("destination", fe.get("name", "")) - dest = sanitize_pack_path(raw_dest) - if not dest: - continue - if region_drops and sanitize_pack_path(dest) in region_drops: - continue - # The builder's own placement: the prefix, then the flattening - # of a pack written at the BIOS root. A destination that - # already starts with the prefix (aethersx2's bios/) - # lands at the root of a flat pack, not under bios/. - if extras_pfx and not dest.startswith(f"{extras_pfx}/"): - full = f"{extras_pfx}/{dest}" - else: - full = dest - full = _flat(full, base_dest, is_flat) - # Skip path conflicts (same logic as pack builder) - if _has_path_conflict(full, seen_conformance, seen_parents): - continue - # Skip unresolvable files (game_data dirs, etc.) - local_path, status = resolve_file( - fe, - db, - "bios", - {}, - dest_hint=raw_dest, - data_dir_registry=data_registry, - offline=True, - ) - if status in ("not_found", "external", "user_provided"): - continue + for full, local in sorted(placed_local.items()): core_checked += 1 - if full in zip_set or full.lower() in zip_lower: - core_present += 1 - seen_conformance.add(full) - _register_path(full, seen_conformance, seen_parents) - else: + member = _flat(full, base_dest, is_flat) + member = member if member in zip_set else zip_lower.get(member.lower()) + if member is None: errors.append(f"core missing: {full}") + elif not _same_content(zf, member, local): + errors.append( + f"core file differs: {member} is not the {os.path.basename(local)}" + " the builder places there" + ) + else: + core_present += 1 checked = baseline_checked + core_checked present = baseline_present + core_present diff --git a/tests/test_pack_core_extras.py b/tests/test_pack_core_extras.py new file mode 100644 index 00000000..9cd50724 --- /dev/null +++ b/tests/test_pack_core_extras.py @@ -0,0 +1,91 @@ +"""The pack integrity check counts the core files the builder places. + +It rebuilt the core extras with an empty set of taken paths and tested +presence alone. On a Recalbox pack it credited 68 core files at a path the +platform's own file holds, 15 of them other bytes (fuse/trdos.rom, the PS1 +BIOS DuckStation reads), and printed a core count that contradicted the +builder's in the same run. +""" + +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 packverify # noqa: E402 + + +def _held(files: dict, by_name: dict, path: str, payload: bytes) -> str: + Path(path).parent.mkdir(parents=True, exist_ok=True) + Path(path).write_bytes(payload) + sha1 = hashlib.sha1(payload).hexdigest() + name = Path(path).name + files[sha1] = {"path": path, "name": name, "sha1": sha1, + "md5": hashlib.md5(payload).hexdigest(), "size": len(payload)} + by_name.setdefault(name, []).append(sha1) + return sha1 + + +class CoreFilesAsTheBuilderPlacesThem(unittest.TestCase): + def setUp(self): + 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 = {} + by_name: dict = {} + _held(files, by_name, "bios/plat/trdos.rom", b"platform dump") + core_trdos = _held(files, by_name, "bios/core/trdos.rom", b"core dump") + _held(files, by_name, "bios/core/other.rom", b"core file") + self.db = {"files": files, "indexes": { + "by_name": by_name, "by_md5": {e["md5"]: s for s, e in files.items()}, + "by_path_suffix": {}, "by_crc32": {}, "by_sha256": {}}} + Path("platforms").mkdir() + config = {"platform": "P", "verification_mode": "existence", "cores": ["fuse"], + "systems": {"zx": {"files": [ + {"name": "trdos.rom", "destination": "fuse/trdos.rom"}]}}} + Path("platforms/p.yml").write_text(yaml.safe_dump(config), encoding="utf-8") + Path("emulators").mkdir() + self.profiles = {"fuse": { + "emulator": "fuse", "type": "libretro", "systems": ["zx"], "files": [ + {"name": "trdos.rom", "path": "fuse/trdos.rom", "sha1": core_trdos}, + {"name": "other.rom", "path": "core/other.rom"}, + ]}} + + def _check(self, other: bytes) -> tuple: + with zipfile.ZipFile("pack.zip", "w") as zf: + zf.writestr("fuse/trdos.rom", b"platform dump") + zf.writestr("core/other.rom", other) + return packverify.verify_pack_against_platform( + "pack.zip", "p", "platforms", self.db, emulators_dir="emulators", + emu_profiles=self.profiles, + ) + + def test_a_path_the_platform_file_holds_is_not_a_core_file(self): + ok, _checked, _present, errors, _bc, _bp, core_checked, core_present, _ex = ( + self._check(b"core file") + ) + self.assertTrue(ok, errors) + self.assertEqual((core_checked, core_present), (1, 1)) + + def test_other_bytes_at_a_core_path_are_an_error(self): + ok, _checked, _present, errors, *_rest = self._check(b"something else") + self.assertFalse(ok) + self.assertTrue( + any(e.startswith("core file differs: core/other.rom") for e in errors), errors + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index da47320f..4f05b17a 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -38,7 +38,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` | -| 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_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` | +| 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` | | Installer and downloads | loopback server | `test_check_release`, `test_download`, `test_install`, `test_install_cores`, `test_install_experience`, `test_install_selection` |