From b33d0451757ebe6126621c4591ec792c13c4e6e0 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 12 Aug 2026 06:43:47 +0200 Subject: [PATCH] fix: group region candidates once for both sides The builder and the coverage report each grouped their own candidates before asking which regional alternatives to withdraw. The builder grouped the platform files and the core extras; the report grouped the platform files alone, and keyed them on an unsanitized destination. So a region run withdrew 73 files from a recalbox pack while the report withdrew 14, and described the other 59 as covered by a pack that would not carry them. platform_region_groups builds the grouping once and both sides read it. The extras it returns are keyed by emulator, name and path: Dolphin declares three IPL.bin that differ by path alone, and a name-keyed map withdraws the wrong one. Manifests are byte-identical before and after. --- scripts/common.py | 11 ++ scripts/generate_pack.py | 222 ++++++++++++++++++++------------------- scripts/verify.py | 37 +++++-- tests/test_region.py | 115 ++++++++++++++++++++ 4 files changed, 267 insertions(+), 118 deletions(-) diff --git a/scripts/common.py b/scripts/common.py index 79b0e00d..5a49ccca 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -1505,6 +1505,17 @@ def fetch_large_file( MAX_ZIP_MEMBERS = 100_000 +def sanitize_pack_path(raw: str) -> str: + """Strip traversal components from a relative destination. + + The builder and the coverage report key their region grouping on this + value, so they have to derive it the same way: a destination normalized on + one side only would be looked up under a key the other side never emits. + """ + raw = raw.replace("\\", "/") + return "/".join(p for p in raw.split("/") if p and p not in ("..", ".")) + + MAX_ZIP_MEMBER_SIZE = 8 * 1024 * 1024 * 1024 # The largest generated pack is already ~5 GB uncompressed and the collection # only grows; this bounds a malicious archive without capping a real one. diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 85f1eb87..d19e9d16 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -50,6 +50,7 @@ from common import ( require_yaml, resolution_is_hash_exact, resolve_local_file, + sanitize_pack_path, yaml_load, ) import region as region_mod @@ -289,7 +290,7 @@ def _pack_member_groups( for sys_id, system in pack_systems.items(): members = groups.setdefault(sys_id, []) for fe in system.get("files", []): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) + dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if dest: members.append((dest, fe.get("name", ""))) if source == "platform": @@ -302,7 +303,7 @@ def _pack_member_groups( config, emulators_dir, db, set(), base_dest, emu_profiles, target_cores=target_cores, include_all=(source == "truth"), ): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) + dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if not dest: continue for sys_id in emu_systems.get(fe.get("source_emulator", ""), ["_extras"]): @@ -361,13 +362,6 @@ def _target_tag(target_name: str) -> str: ) -def _sanitize_path(raw: str) -> str: - """Strip path traversal components from a relative path.""" - raw = raw.replace("\\", "/") - parts = [p for p in raw.split("/") if p and p not in ("..", ".")] - return "/".join(parts) - - def _path_parents(dest: str) -> list[str]: """Return all parent directory segments of a path.""" parts = dest.split("/") @@ -709,6 +703,12 @@ def _collect_emulator_extras( "hle_fallback": u.get("hle_fallback", False), "source_emulator": u.get("emulator", ""), "source_profile": u.get("profile", ""), + # Identity of the report entry this extra came from. The name + # alone does not identify it: Dolphin declares three IPL.bin that + # differ only by path, and collapsing them onto one key withdraws + # the wrong regional variant from the report. + "source_name": u.get("name", ""), + "source_path": u.get("path") or "", "source_system": u.get("system"), "source_systems": u.get("systems", []), "region": u.get("region"), @@ -951,6 +951,68 @@ def _extra_system_ids(extra: dict) -> list[str]: return [str(value) for value in extra.get("source_systems", []) if value] +def platform_region_groups( + config: dict, + systems: dict, + emulators_dir: str, + db: dict | None, + base_dest: str, + emu_profiles: dict | None, + *, + target_cores: set[str] | None = None, + include_extras: bool = True, + include_all: bool = False, +) -> tuple[dict[str, list[tuple[str, str]]], dict[tuple[str, str, str], str]]: + """Group a platform's pack candidates the way region filtering reads them. + + Returns the groups and, for every core extra, the destination it was + grouped under, keyed by (emulator, name). verify.py needs that mapping to + withdraw from its report exactly what the builder withdraws from the pack: + grouping the declared files here and the core extras there would let the + two answer differently on the same request. + """ + groups: dict[str, list[tuple[str, str]]] = {} + for sys_id, system in systems.items(): + members = groups.setdefault(sys_id, []) + for file_entry in system.get("files", []): + dest = sanitize_pack_path( + file_entry.get("destination", file_entry.get("name", "")) + ) + if dest: + members.append((dest, file_entry.get("name", ""))) + + extra_dests: dict[tuple[str, str, str], str] = {} + if not include_extras or db is None: + return groups, extra_dests + + for extra in _collect_emulator_extras( + config, + emulators_dir, + db, + set(), + base_dest, + emu_profiles, + target_cores=target_cores, + include_all=include_all, + ): + dest = sanitize_pack_path(extra.get("destination", extra.get("name", ""))) + if not dest: + continue + name = extra.get("name", "") + extra_dests[ + ( + extra.get("source_emulator", ""), + extra.get("source_name", ""), + extra.get("source_path", ""), + ) + ] = dest + variant = extra.get("variant_group") + for sys_id in _extra_system_ids(extra) or ["_extras"]: + group_id = f"{sys_id}:variant:{variant}" if variant else sys_id + groups.setdefault(group_id, []).append((dest, name)) + return groups, extra_dests + + def _emulator_region_group(emu_name: str, profile: dict, file_entry: dict) -> str: """Stable group ID for regional alternatives within an emulator profile.""" variant = file_entry.get("variant_group") @@ -1403,7 +1465,7 @@ def generate_pack( for file_entry in system.get("files", []): if required_only and file_entry.get("required") is False: continue - dest = _sanitize_path( + dest = sanitize_pack_path( file_entry.get("destination", file_entry.get("name", "")) ) if not dest: @@ -1447,36 +1509,17 @@ def generate_pack( region_fallbacks: list[str] = [] if regions: region_index = region_mod.build_region_index(emu_profiles or {}) - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in pack_systems.items(): - members = region_groups.setdefault(sys_id, []) - for file_entry in system.get("files", []): - dest = _sanitize_path( - file_entry.get("destination", file_entry.get("name", "")) - ) - if dest: - members.append((dest, file_entry.get("name", ""))) - if source != "platform": - for fe in _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - target_cores=target_cores, - include_all=(source == "truth"), - ): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) - if not dest: - continue - systems = _extra_system_ids(fe) or ["_extras"] - for sys_id in systems: - variant = fe.get("variant_group") - group_id = f"{sys_id}:variant:{variant}" if variant else sys_id - region_groups.setdefault(group_id, []).append( - (dest, fe.get("name", "")) - ) + region_groups, _extra_dests = platform_region_groups( + config, + pack_systems, + emulators_dir, + db, + base_dest, + emu_profiles, + target_cores=target_cores, + include_extras=(source != "platform"), + include_all=(source == "truth"), + ) region_drops = region_mod.resolve_region_drops( region_groups, region_index, regions ) @@ -1510,7 +1553,7 @@ def generate_pack( for file_entry in system.get("files", []): if required_only and file_entry.get("required") is False: continue - dest = _sanitize_path(file_entry.get("destination", file_entry["name"])) + dest = sanitize_pack_path(file_entry.get("destination", file_entry["name"])) if region_drops and dest in region_drops: continue if not dest: @@ -1827,7 +1870,7 @@ def generate_pack( for fe in core_files: if required_only and fe.get("required") is False: continue - dest = _sanitize_path(fe.get("destination", fe["name"])) + dest = sanitize_pack_path(fe.get("destination", fe["name"])) if region_drops and dest in region_drops: continue if not dest: @@ -2020,7 +2063,7 @@ def _extract_zip_to_archive( for info in src.infolist(): if info.is_dir(): continue - clean_name = _sanitize_path(info.filename) + clean_name = sanitize_pack_path(info.filename) if not clean_name: continue data = src.read(info.filename) @@ -2105,7 +2148,7 @@ def _resolve_destination( else: rel = file_entry.get("name", "") - rel = _sanitize_path(rel) + rel = sanitize_pack_path(rel) # Prepend pack_structure prefix if pack_structure: @@ -2264,7 +2307,7 @@ def generate_emulator_pack( # Pack archives as units archive_prefix = profile.get("archive_prefix", "") for archive_name in sorted(archives): - archive_dest = _sanitize_path(archive_name) + archive_dest = sanitize_pack_path(archive_name) if archive_prefix: archive_dest = f"{archive_prefix}/{archive_dest}" if pack_structure: @@ -3661,36 +3704,17 @@ def generate_manifest( region_drops: set[str] = set() if regions: region_index = region_mod.build_region_index(emu_profiles) - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in pack_systems.items(): - members = region_groups.setdefault(sys_id, []) - for file_entry in system.get("files", []): - d = _sanitize_path( - file_entry.get("destination", file_entry.get("name", "")) - ) - if d: - members.append((d, file_entry.get("name", ""))) - if source != "platform": - for fe in _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - target_cores=target_cores, - include_all=(source == "truth"), - ): - d = _sanitize_path(fe.get("destination", fe.get("name", ""))) - if not d: - continue - systems = _extra_system_ids(fe) or ["_extras"] - for sid in systems: - variant = fe.get("variant_group") - group_id = f"{sid}:variant:{variant}" if variant else sid - region_groups.setdefault(group_id, []).append( - (d, fe.get("name", "")) - ) + region_groups, _extra_dests = platform_region_groups( + config, + pack_systems, + emulators_dir, + db, + base_dest, + emu_profiles, + target_cores=target_cores, + include_extras=(source != "platform"), + include_all=(source == "truth"), + ) region_drops = region_mod.resolve_region_drops( region_groups, region_index, regions ) @@ -3699,7 +3723,7 @@ def generate_manifest( if source != "truth": for sys_id, system in sorted(pack_systems.items()): for file_entry in system.get("files", []): - dest = _sanitize_path(file_entry.get("destination", file_entry["name"])) + dest = sanitize_pack_path(file_entry.get("destination", file_entry["name"])) if not dest: continue if region_drops and dest in region_drops: @@ -3806,7 +3830,7 @@ def generate_manifest( core_files = [] extras_pfx = _detect_extras_prefix(config, base_dest) for fe in core_files: - dest = _sanitize_path(fe.get("destination", fe["name"])) + dest = sanitize_pack_path(fe.get("destination", fe["name"])) if not dest: continue if region_drops and dest in region_drops: @@ -4385,36 +4409,14 @@ def verify_pack_against_platform( region_drops: set[str] = set() if regions: region_index = region_mod.build_region_index(emu_profiles) - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in config.get("systems", {}).items(): - members = region_groups.setdefault(sys_id, []) - for fe in system.get("files", []): - d = _sanitize_path(fe.get("destination", fe.get("name", ""))) - if d: - members.append((d, fe.get("name", ""))) - if db is not None: - for extra in _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - ): - d = _sanitize_path( - extra.get("destination", extra.get("name", "")) - ) - if not d: - continue - systems_for_extra = _extra_system_ids(extra) or ["_extras"] - for sys_id in systems_for_extra: - variant = extra.get("variant_group") - group_id = ( - f"{sys_id}:variant:{variant}" if variant else sys_id - ) - region_groups.setdefault(group_id, []).append( - (d, extra.get("name", "")) - ) + region_groups, _extra_dests = platform_region_groups( + config, + config.get("systems", {}), + emulators_dir, + db, + base_dest, + emu_profiles, + ) region_drops = region_mod.resolve_region_drops( region_groups, region_index, regions ) @@ -4468,7 +4470,7 @@ def verify_pack_against_platform( baseline_groups: dict[str, list[dict]] = {} for _sys_id, system in config.get("systems", {}).items(): for fe in system.get("files", []): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) + dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if not dest: continue if region_drops and dest in region_drops: @@ -4555,10 +4557,10 @@ def verify_pack_against_platform( extras_pfx = _detect_extras_prefix(config, base_dest) for fe in core_files: raw_dest = fe.get("destination", fe.get("name", "")) - dest = _sanitize_path(raw_dest) + dest = sanitize_pack_path(raw_dest) if not dest: continue - if region_drops and _sanitize_path(dest) in region_drops: + if region_drops and sanitize_pack_path(dest) in region_drops: continue if extras_pfx and not (is_flat and extras_pfx == base_dest): if not dest.startswith(f"{extras_pfx}/"): diff --git a/scripts/verify.py b/scripts/verify.py index 53559158..b3e8317d 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -49,6 +49,7 @@ from common import ( require_yaml, resolve_local_file, resolve_platform_cores, + sanitize_pack_path, ) yaml = require_yaml() @@ -803,16 +804,25 @@ def verify_platform( file_severity: dict[str, str] = {} region_drops: set[str] = set() + region_extra_dests: dict[tuple[str, str, str], str] = {} if regions: import region as region_mod - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in verify_systems.items(): - members = region_groups.setdefault(sys_id, []) - for fe in system.get("files", []): - dest = fe.get("destination", fe.get("name", "")) - if dest: - members.append((dest, fe.get("name", ""))) + # The builder owns pack composition, so it owns the grouping the region + # pass reads. Grouping the platform files here and the core extras + # there let the two answer differently on one request: the report kept + # every core extra a region run withdraws from the pack. + from generate_pack import platform_region_groups + + region_groups, region_extra_dests = platform_region_groups( + config, + verify_systems, + emulators_dir, + db, + config.get("base_destination", ""), + profiles, + target_cores=target_cores, + ) region_drops = region_mod.resolve_region_drops( region_groups, region_mod.build_region_index(profiles), regions ) @@ -820,7 +830,9 @@ def verify_platform( for sys_id, system in verify_systems.items(): for file_entry in system.get("files", []): if region_drops and ( - file_entry.get("destination", file_entry.get("name", "")) + sanitize_pack_path( + file_entry.get("destination", file_entry.get("name", "")) + ) in region_drops ): continue @@ -916,6 +928,15 @@ def verify_platform( target_cores=target_cores, data_names=supplemental_names, ) + if region_drops: + undeclared = [ + u + for u in undeclared + if region_extra_dests.get( + (u.get("emulator", ""), u.get("name", ""), u.get("path") or "") + ) + not in region_drops + ] exclusions = find_exclusion_notes( config, emulators_dir, emu_profiles, target_cores=target_cores ) diff --git a/tests/test_region.py b/tests/test_region.py index f1e25646..94bd4ec2 100644 --- a/tests/test_region.py +++ b/tests/test_region.py @@ -5,6 +5,7 @@ from __future__ import annotations import os import sys import unittest +from pathlib import Path sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) @@ -450,5 +451,119 @@ class TestVerifyModesHonourRegion(unittest.TestCase): self.assertNotEqual(plain, filtered) +class TestReportAndBuilderNarrowTogether(unittest.TestCase): + """A region run must withdraw the same files from report and pack. + + The two sides grouped their candidates separately: the builder grouped the + platform files and the core extras, the report only the platform files. + So the report kept every core extra a region run withdraws from the pack + -on recalbox --region us, 59 files it described as covered. + """ + + def _fixture(self): + import hashlib + import tempfile + + tmp = tempfile.TemporaryDirectory() + root = Path(tmp.name) + (root / "emulators").mkdir() + db = {"files": {}, "indexes": { + "by_name": {}, "by_md5": {}, "by_sha256": {}, + "by_crc32": {}, "by_path_suffix": {}, + }} + # Two regional alternatives of one system, both held locally. + for fname, tag in (("us.bin", b"US BYTES"), ("jp.bin", b"JP BYTES")): + blob = root / fname + blob.write_bytes(tag) + sha1 = hashlib.sha1(tag).hexdigest() + db["files"][sha1] = { + "path": str(blob), "name": fname, "size": len(tag), + "sha1": sha1, "md5": hashlib.md5(tag).hexdigest(), + "sha256": hashlib.sha256(tag).hexdigest(), "crc32": "00000000", + } + db["indexes"]["by_name"][fname] = [sha1] + db["indexes"]["by_md5"][db["files"][sha1]["md5"]] = sha1 + (root / "emulators" / "demo.yml").write_text( + "emulator: demo\n" + "type: libretro\n" + "display_name: Demo\n" + "systems: [demo-system]\n" + "cores: [demo]\n" + "files:\n" + " - name: us.bin\n" + " system: demo-system\n" + " region: [north-america]\n" + " required: true\n" + " - name: jp.bin\n" + " system: demo-system\n" + " region: [japan]\n" + " required: true\n" + ) + config = { + "platform": "Demo", + "verification_mode": "existence", + "cores": ["demo"], + "base_destination": "", + "systems": {}, + } + return tmp, root, db, config + + def test_the_report_withdraws_what_the_builder_withdraws(self): + sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) + import common + import generate_pack as builder + import verify as reporter + + tmp, root, db, config = self._fixture() + try: + common._emulator_profiles_cache.clear() + profiles = common.load_emulator_profiles(str(root / "emulators")) + groups, extra_dests = builder.platform_region_groups( + config, config["systems"], str(root / "emulators"), + db, "", profiles, + ) + drops = region.resolve_region_drops( + groups, region.build_region_index(profiles), ["north-america"] + ) + self.assertEqual( + {d for d in drops}, {"jp.bin"}, + "the builder must drop the Japanese alternative, not the US one", + ) + + plain = reporter.verify_platform( + config, db, str(root / "emulators"), profiles, + supplemental_names=set(), + ) + narrowed = reporter.verify_platform( + config, db, str(root / "emulators"), profiles, + supplemental_names=set(), regions=["north-america"], + ) + before = {u["name"] for u in plain["undeclared_files"]} + after = {u["name"] for u in narrowed["undeclared_files"]} + self.assertEqual(before, {"us.bin", "jp.bin"}) + self.assertEqual( + before - after, + {d for d in drops}, + "the report must withdraw exactly the builder's drop set", + ) + finally: + common._emulator_profiles_cache.clear() + tmp.cleanup() + + def test_one_grouping_pass_serves_both_sides(self): + """A second hand-rolled grouping is how the two drifted apart.""" + scripts = Path(__file__).resolve().parent.parent / "scripts" + hand_rolled = 0 + for name in ("generate_pack.py", "verify.py"): + for line in (scripts / name).read_text().splitlines(): + if "region_groups.setdefault(" in line: + hand_rolled += 1 + self.assertLessEqual( + hand_rolled, 2, + "platform region grouping belongs to platform_region_groups; " + "the only other pass is the per-emulator pack shape", + ) + + if __name__ == "__main__": unittest.main()