From 64f84e7ca6406b8ad46c31f8d9cf2e88411cdf43 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 6 Oct 2026 00:13:38 +0200 Subject: [PATCH] fix: verify the pack the flags name, by exact name --- scripts/generate_pack.py | 235 +++++++++++++++++++++---------- scripts/pipeline.py | 2 + tests/test_e2e.py | 23 +++ tests/test_verify_pack_choice.py | 138 ++++++++++++++++++ 4 files changed, 326 insertions(+), 72 deletions(-) create mode 100644 tests/test_verify_pack_choice.py diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index dbad2d53..0c2738a0 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -230,6 +230,41 @@ def _target_tag(target_name: str) -> str: ) +def _system_tag(system_filter: list[str] | None) -> str: + """Filename tag for a pack limited to some systems, empty for all.""" + if not system_filter: + return "" + display_parts = [] + for sid in system_filter: + s = sid.lower().replace("_", "-") + for prefix in MANUFACTURER_PREFIXES: + if s.startswith(prefix): + s = s[len(prefix) :] + break + display_parts.append("_".join(p.title() for p in s.split("-") if p)) + return "_" + "_".join(display_parts) + + +def _platform_pack_stem( + group_platforms: list[str], representative: str, platforms_dir: str +) -> str: + """Platform part of a pack name, version included. + + A group of identical platforms shares one pack named after all of them; + the builder and --verify-packs both read the name from here. + """ + rep_cfg = load_platform_config(representative, platforms_dir) + version = rep_cfg.get("version", rep_cfg.get("dat_version", "")) + version_tag = f"_{version.replace(' ', '')}" if version else "" + if len(group_platforms) <= 1: + return rep_cfg.get("platform", representative).replace(" ", "_") + version_tag + names = [ + load_platform_config(p, platforms_dir).get("platform", p) + for p in group_platforms + ] + return "_".join(n.replace(" ", "") for n in names) + version_tag + + @@ -618,27 +653,12 @@ def generate_pack( platform_display = config.get("platform", platform_name) base_dest = config.get("base_destination", "") - version = config.get("version", config.get("dat_version", "")) - version_tag = f"_{version.replace(' ', '')}" if version else "" narrowings = _narrowings( source, regions, target_name, one_per_slot, required_only ) narrow_tags = "".join(tag for tag, _label in narrowings) - - sys_tag = "" - if system_filter: - display_parts = [] - for sid in system_filter: - s = sid.lower().replace("_", "-") - for prefix in MANUFACTURER_PREFIXES: - if s.startswith(prefix): - s = s[len(prefix) :] - break - parts = s.split("-") - display_parts.append("_".join(p.title() for p in parts if p)) - sys_tag = "_" + "_".join(display_parts) - - zip_name = f"{platform_display.replace(' ', '_')}{version_tag}{narrow_tags}_BIOS_Pack{sys_tag}.zip" + stem = _platform_pack_stem([platform_name], platform_name, platforms_dir) + zip_name = f"{stem}{narrow_tags}_BIOS_Pack{_system_tag(system_filter)}.zip" zip_path = os.path.join(output_dir, zip_name) os.makedirs(output_dir, exist_ok=True) @@ -2056,7 +2076,7 @@ def _validate_args(args, parser): ): if given: parser.error(f"{flag} is incompatible with --from-md5") - if has_system and has_platform: + if has_system and (has_platform or has_all): if args.manifest: parser.error("--system is incompatible with --manifest") if args.split: @@ -2201,10 +2221,13 @@ def _run_verify_packs(args): with open(args.db) as f: verify_db = json.load(f) - platforms = list_registered_platforms(args.platforms_dir) if args.platform: platforms = [args.platform] - elif not args.all: + elif args.all: + platforms = list_registered_platforms( + args.platforms_dir, include_archived=args.include_archived + ) + else: print("ERROR: --verify-packs requires --platform or --all") sys.exit(1) @@ -2212,18 +2235,16 @@ def _run_verify_packs(args): verify_regions = getattr(args, "regions", None) verify_profiles = load_emulator_profiles(args.emulators_dir) verify_data_registry = load_data_dir_registry(args.platforms_dir) + expected = _expected_pack_names( + platforms, args.platforms_dir, verify_regions, args.target + ) + on_disk = ( + set(os.listdir(args.output_dir)) if os.path.isdir(args.output_dir) else set() + ) for platform_name in platforms: - config = load_platform_config(platform_name, args.platforms_dir) - display = config.get("platform", platform_name).replace(" ", "_") - - zip_path = None - if os.path.isdir(args.output_dir): - for entry in sorted(os.listdir(args.output_dir)): - if entry.endswith("_BIOS_Pack.zip") and display in entry: - zip_path = os.path.join(args.output_dir, entry) - break - if not zip_path: + found = sorted(expected[platform_name] & on_disk) + if not found: print(f" {platform_name}: SKIP (no pack in {args.output_dir})") # Naming a platform is asking about its pack. Answering SKIP and # exiting 0 says the pack passed; with --all, a platform whose pack @@ -2231,38 +2252,36 @@ def _run_verify_packs(args): if args.platform: all_ok = False continue - - if _narrows_contents(os.path.basename(zip_path)): - print(f" {platform_name}: SKIP (narrowed variant)") - continue - - result = verify_pack_against_platform( - zip_path, - platform_name, - args.platforms_dir, - db=verify_db, - emulators_dir=args.emulators_dir, - emu_profiles=verify_profiles, - regions=verify_regions, - data_registry=verify_data_registry, - target_cores=_target_cores_for( - platform_name, args.target, args.platforms_dir - ), - ) - ok, errors = result[0], result[3] - bl_checked, bl_present = result[4], result[5] - core_checked, core_present = result[6], result[7] - counts = ( - f"{bl_present}/{bl_checked} baseline, " - f"{core_present}/{core_checked} cores" - ) - if ok: - print(f" {platform_name}: OK ({counts})") - else: - print(f" {platform_name}: FAIL ({counts}, {len(errors)} errors)") - for err in errors[:5]: - print(f" {err}") - all_ok = False + tc = _target_cores_for(platform_name, args.target, args.platforms_dir) + # A group pack and a pack built for the platform alone can both be + # present; each one is a pack of this platform. + for pack_name in found: + zip_path = os.path.join(args.output_dir, pack_name) + result = verify_pack_against_platform( + zip_path, + platform_name, + args.platforms_dir, + db=verify_db, + emulators_dir=args.emulators_dir, + emu_profiles=verify_profiles, + regions=verify_regions, + data_registry=verify_data_registry, + target_cores=tc, + ) + ok, errors = result[0], result[3] + bl_checked, bl_present = result[4], result[5] + core_checked, core_present = result[6], result[7] + counts = ( + f"{bl_present}/{bl_checked} baseline, " + f"{core_present}/{core_checked} cores" + ) + if ok: + print(f" {platform_name} [{pack_name}]: OK ({counts})") + else: + print(f" {platform_name} [{pack_name}]: FAIL ({counts}, {len(errors)} errors)") + for err in errors[:5]: + print(f" {err}") + all_ok = False if not all_ok: sys.exit(1) @@ -2346,13 +2365,6 @@ def _run_platform_packs( offline=args.offline, ) if not args.split and zip_path and aliases: - rep_cfg = load_platform_config(representative, args.platforms_dir) - ver = rep_cfg.get("version", rep_cfg.get("dat_version", "")) - ver_tag = f"_{ver.replace(' ', '')}" if ver else "" - all_names = [ - load_platform_config(p, args.platforms_dir).get("platform", p) - for p in group_platforms - ] narrow_tags = "".join( tag for tag, _label in _narrowings( @@ -2363,9 +2375,12 @@ def _run_platform_packs( required_only, ) ) + stem = _platform_pack_stem( + group_platforms, representative, args.platforms_dir + ) combined = ( - "_".join(n.replace(" ", "") for n in all_names) - + f"{ver_tag}{narrow_tags}_BIOS_Pack.zip" + f"{stem}{narrow_tags}_BIOS_Pack" + f"{_system_tag(system_filter)}.zip" ) new_path = os.path.join(os.path.dirname(zip_path), combined) if new_path != zip_path: @@ -2518,6 +2533,32 @@ def main(): if args.one_per_slot and args.manifest_targets: parser.error("--one-per-slot is incompatible with --manifest-targets") + # --all-variants builds the six source x required combinations itself: + # a --source or --required-only beside it would be overridden, and the + # modes that build one pack (emulator, system, hashes) never read it. + if args.all_variants: + for flag, given in ( + ("--source", args.source != "full"), + ("--required-only", args.required_only), + ("--emulator", args.emulator), + ("--from-md5", args.from_md5 or args.from_md5_file), + ): + if given: + parser.error(f"{flag} is incompatible with --all-variants") + if args.system and not (args.platform or args.all): + parser.error("--all-variants requires --platform or --all") + if args.verify_packs and args.manifest: + parser.error("--verify-packs is incompatible with --manifest") + if args.verify_packs: + # The check that follows the build reads the full pack's name. + for flag, given in ( + ("--system", args.system), + ("--one-per-slot", args.one_per_slot), + ("--split", args.split), + ): + if given: + parser.error(f"{flag} is incompatible with --verify-packs") + # Quick-exit modes: --verify-packs alone = verify existing packs only # Combined with --all-variants, generation runs first then verify if args.verify_packs and not args.all_variants: @@ -2529,6 +2570,7 @@ def main(): ("--one-per-slot", args.one_per_slot), ("--required-only", args.required_only), ("--source", args.source != "full"), + ("--system", args.system), ): if given: parser.error(f"{flag} is incompatible with --verify-packs") @@ -2731,6 +2773,9 @@ def main(): target_cores_cache, system_filter, ) + if args.verify_packs: + with _pack_output_lock(args.output_dir, exclusive=False): + _run_verify_packs(args) # Manifest generation (JSON inventory for install.py) @@ -3269,6 +3314,52 @@ def _target_cores_for( return cache.get(platform_name) +def _expected_pack_names( + platforms: list[str], + platforms_dir: str, + regions: list[str] | None, + target_name: str | None, +) -> dict[str, set[str]]: + """Exact pack file names --verify-packs may check, per platform. + + Rebuilt from the name builder, never matched by substring: a substring + picks the full pack when a regional or targeted one was asked for, or a + narrowed pack when the full one was. A platform answers to its own name + and to the name of the group it shares a pack with. + """ + narrow_tags = "".join( + tag for tag, _label in _narrowings("full", regions, target_name, False, False) + ) + names: dict[str, set[str]] = { + p: {f"{_platform_pack_stem([p], p, platforms_dir)}{narrow_tags}_BIOS_Pack.zip"} + for p in platforms + } + # Groups depend on which platforms a run built: --all alone or with + # --include-archived. Both compositions are names a pack can carry. + for include_archived in (False, True): + pool = list_registered_platforms( + platforms_dir, include_archived=include_archived + ) + cache = None + if target_name: + cache, pool = build_target_cores_cache( + pool, target_name, platforms_dir, is_all=True + ) + for group, representative in group_identical_platforms( + pool, platforms_dir, cache + ): + if len(group) < 2: + continue + name = ( + f"{_platform_pack_stem(group, representative, platforms_dir)}" + f"{narrow_tags}_BIOS_Pack.zip" + ) + for p in group: + if p in names: + names[p].add(name) + return names + + def _content_narrowing_tags() -> tuple[str, ...]: """Tags naming a pack that holds fewer files than the platform declares. @@ -3347,7 +3438,7 @@ def verify_and_finalize_packs( # Map ZIP names to platform names pack_to_platform: dict[str, list[str]] = {} for name in _pack_archives(output_dir): - for pname in list_registered_platforms(platforms_dir): + for pname in list_registered_platforms(platforms_dir, include_archived=True): cfg = load_platform_config(pname, platforms_dir) display = cfg.get("platform", pname).replace(" ", "_") if display in name or display.replace("_", "") in name.replace("_", ""): diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 1932f619..1d7ca3df 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -249,6 +249,8 @@ def main(): help="Export native formats (implies --with-truth)", ) args = parser.parse_args() + if args.all_variants and args.source != "full": + parser.error("--source is incompatible with --all-variants") # A second run on the same output directory is refused before any work: # the database rebuild alone takes minutes, and the reader holding the diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 05e819b9..21e75627 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -5956,6 +5956,29 @@ struct BurnDriver BurnDrvneogeo = { ["--system", "sony-playstation"], "refuse"), (["--platform", "retroarch", "--split"], ["--system", "sony-playstation"], "refuse"), + # --all took the same filter and wrote twelve complete manifests. + (["--all", "--manifest"], ["--system", "sony-playstation"], "refuse"), + (["--all", "--split"], ["--system", "sony-playstation"], "refuse"), + # A pack built with --system is named after its systems; the check + # looked for the full pack instead. + (["--platform", "retroarch", "--verify-packs"], + ["--system", "sony-playstation"], "refuse"), + # --all-variants builds its own six combinations: it overrode these + # and the single-pack modes never read it at all. + (["--platform", "retroarch", "--all-variants"], ["--source", "truth"], + "refuse"), + (["--platform", "retroarch", "--all-variants"], ["--required-only"], + "refuse"), + (["--emulator", "duckstation"], ["--all-variants"], "refuse"), + (["--system", "sony-playstation"], ["--all-variants"], "refuse"), + (["--emulator", "duckstation", "--verify-packs"], ["--all-variants"], + "refuse"), + (["--all", "--manifest", "--all-variants"], ["--verify-packs"], + "refuse"), + (["--all", "--all-variants", "--verify-packs"], ["--one-per-slot"], + "refuse"), + (["--all", "--all-variants", "--verify-packs"], ["--split"], + "refuse"), ] for mode, flag, expected in matrix: with self.subTest(mode=mode, flag=flag): diff --git a/tests/test_verify_pack_choice.py b/tests/test_verify_pack_choice.py new file mode 100644 index 00000000..dc1b902e --- /dev/null +++ b/tests/test_verify_pack_choice.py @@ -0,0 +1,138 @@ +"""--verify-packs opens the pack the flags name, by its exact file name. + +The check took the first sorted *_BIOS_Pack.zip whose name contained the +platform name. With a full and a regional pack side by side, --region us +judged the full pack against the regional expectation and never opened the +regional one; a pack built with --source truth answered SKIP and exit 0. +""" + +from __future__ import annotations + +import argparse +import json +import os +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +REPO_ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO_ROOT / "scripts")) + + +def _ok_result() -> tuple: + return (True, 0, 0, [], 1, 1, 1, 1, 0) + + +class VerifyPackChoice(unittest.TestCase): + def setUp(self): + import generate_pack + + self.gp = generate_pack + previous = os.getcwd() + os.chdir(REPO_ROOT) + self.addCleanup(os.chdir, previous) + self.tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(self.tmp.cleanup) + self.out = Path(self.tmp.name) + self.db = self.out / "db.json" + self.db.write_text(json.dumps({"files": {}})) + self.stem = generate_pack._platform_pack_stem( + ["retroarch", "lakka"], "retroarch", "platforms" + ) + + def _args(self, **overrides) -> argparse.Namespace: + values = { + "db": str(self.db), + "platform": "retroarch", + "all": False, + "include_archived": False, + "emulators_dir": "emulators", + "platforms_dir": "platforms", + "output_dir": str(self.out), + "target": None, + "regions": [], + } + values.update(overrides) + return argparse.Namespace(**values) + + def _run(self, args) -> list[str]: + checked: list[str] = [] + + def record(zip_path, platform_name, *a, **kw): + checked.append((platform_name, os.path.basename(zip_path))) + return _ok_result() + + with mock.patch.object(self.gp, "verify_pack_against_platform", record): + self.gp._run_verify_packs(args) + return checked + + def _touch(self, name: str) -> None: + (self.out / name).write_bytes(b"") + + def test_region_opens_the_regional_pack(self): + import region as region_mod + + regions = region_mod.parse_requested("us") + tag = self.gp._narrowings("full", regions, None, False, False)[0][0] + self._touch(f"{self.stem}_BIOS_Pack.zip") + self._touch(f"{self.stem}{tag}_BIOS_Pack.zip") + checked = self._run(self._args(regions=regions)) + self.assertEqual(checked, [("retroarch", f"{self.stem}{tag}_BIOS_Pack.zip")]) + + def test_unnarrowed_check_ignores_the_regional_pack(self): + self._touch(f"{self.stem}_AsiaPacific_BIOS_Pack.zip") + self._touch(f"{self.stem}_BIOS_Pack.zip") + checked = self._run(self._args()) + self.assertEqual(checked, [("retroarch", f"{self.stem}_BIOS_Pack.zip")]) + + def test_named_platform_with_only_a_narrowed_pack_fails(self): + self._touch(f"{self.stem}_Truth_BIOS_Pack.zip") + with self.assertRaises(SystemExit) as ctx: + self._run(self._args()) + self.assertEqual(ctx.exception.code, 1) + + def test_include_archived_reaches_the_archived_pack(self): + stem = self.gp._platform_pack_stem(["retropie"], "retropie", "platforms") + self._touch(f"{stem}_BIOS_Pack.zip") + checked = self._run( + self._args(platform=None, all=True, include_archived=True) + ) + self.assertIn(("retropie", f"{stem}_BIOS_Pack.zip"), checked) + + +class GroupRenameKeepsSystemTag(unittest.TestCase): + def test_single_system_pack_does_not_take_the_full_name(self): + import generate_pack as gp + + previous = os.getcwd() + os.chdir(REPO_ROOT) + self.addCleanup(os.chdir, previous) + with tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") as tmp: + built = Path(tmp) / f"RetroArch_x_BIOS_Pack{gp._system_tag(['atari-lynx'])}.zip" + + def fake_generate(*a, **kw): + built.write_bytes(b"") + return str(built) + + args = argparse.Namespace( + all_variants=False, source="full", required_only=False, + platforms_dir="platforms", target=None, split=False, + include_extras=False, emulators_dir="emulators", regions=[], + one_per_slot=False, offline=True, bios_dir="bios", + output_dir=tmp, + ) + with mock.patch.object(gp, "generate_pack", fake_generate), \ + mock.patch.object(gp, "verify_and_finalize_packs", return_value=True): + gp._run_platform_packs( + args, [(["retroarch", "lakka"], "retroarch")], {}, {}, {}, + {}, {}, ["atari-lynx"], + ) + names = os.listdir(tmp) + stem = gp._platform_pack_stem(["retroarch", "lakka"], "retroarch", "platforms") + self.assertEqual(names, [f"{stem}_BIOS_Pack_Lynx.zip"]) + + +if __name__ == "__main__": + unittest.main()