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