From a8b92320931f5f49884da67c22a6f09db3cf61d0 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:06:26 +0200 Subject: [PATCH] fix: arbitrate slots on the file the pack ships --- scripts/generate_pack.py | 82 +--------------------------- scripts/packresolve.py | 88 +++++++++++++++++++++++++++++- scripts/slots.py | 43 +++++++++++---- tests/test_slots_shipped_file.py | 92 ++++++++++++++++++++++++++++++++ 4 files changed, 213 insertions(+), 92 deletions(-) create mode 100644 tests/test_slots_shipped_file.py diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 0c59318a..d14983b3 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -55,7 +55,6 @@ from common import ( MANUFACTURER_PREFIXES, parse_md5_list, require_yaml, - resolution_is_hash_exact, ProfileSelectionError, resolve_local_file, sanitize_pack_path, @@ -435,86 +434,6 @@ def _report_pack_counts( f"{core_count} from cores), {', '.join(parts)} [{verification_mode}]" ) -def _preferred_entries( - pack_systems: dict, - db: dict, - bios_dir: str, - base_dest: str, - required_only: bool, - zip_contents, - offline, -) -> dict[str, int]: - """Which declaration wins when several claim the same destination. - - A platform can declare one destination in more than one system, bare - in one and hash-constrained in another. First-come dedup would let - the bare entry pack whatever answers to the name, so a constrained - sibling that resolves by content claims the destination instead. - The choice is about the baseline and reads no data directory, for the - pack, the manifest and verify alike: verify alone passed the registry - and could crown a declaration only a data/ cache satisfies. - """ - preferred_entries: dict[str, int] = {} - dest_entries: dict[str, list[dict]] = {} - for sys_id, system in sorted(pack_systems.items()): - for file_entry in system.get("files", []): - if required_only and file_entry.get("required") is False: - continue - dest = sanitize_pack_path( - file_entry.get("destination", file_entry.get("name", "")) - ) - if not dest: - continue - full = f"{base_dest}/{dest}" if base_dest else dest - dest_entries.setdefault(full, []).append(file_entry) - for full, entries in dest_entries.items(): - if len(entries) < 2: - continue - constrained = [ - fe - for fe in entries - if fe.get("md5") or fe.get("sha1") or fe.get("zipped_file") - ] - if not constrained: - continue - resolved = [ - (fe, *resolve_file( - fe, - db, - bios_dir, - zip_contents, - offline=offline, - )) - for fe in constrained - ] - members = [fe for fe in entries if fe.get("zipped_file")] - if members: - # One archive declared once per ROM it must hold: the archive - # to ship is the one the most declarations accept, not the first - # that answers one of them (adam_fdc.zip: a one-member MAME set - # answered first, the eight-member set sat beside it). - def accepted(path: str | None) -> int: - if not path or not path.endswith(".zip"): - return -1 - return sum(inner_rom_check(fe, path) == "ok" for fe in members) - - fe, path, _st = max(resolved, key=lambda item: accepted(item[1])) - if accepted(path) > 0: - preferred_entries[full] = id(fe) - continue - best = None - for fe, _lp, _st in resolved: - if _lp and _st == "md5_exact": - best = fe - break - if best is None and _lp and resolution_is_hash_exact(_st): - best = fe - if best is not None: - preferred_entries[full] = id(best) - - return preferred_entries - - def _select_variants( config: dict, pack_systems: dict, @@ -4069,6 +3988,7 @@ from packverify import ( # noqa: E402,F401 # facade name would keep saying False after set_offline() changed the flag. from packresolve import ( # noqa: E402,F401 _detect_hash_type, + preferred_entries as _preferred_entries, parse_hash_input, parse_hash_file, lookup_hashes, diff --git a/scripts/packresolve.py b/scripts/packresolve.py index 93170131..fe81ee6c 100644 --- a/scripts/packresolve.py +++ b/scripts/packresolve.py @@ -10,7 +10,13 @@ import re _HEX_RE = re.compile(r"\b([0-9a-fA-F]{8,40})\b") from largefiles import fetch_large_file import os -from common import declared_hash_verdict, resolve_local_file +from common import ( + declared_hash_verdict, + resolution_is_hash_exact, + resolve_local_file, + sanitize_pack_path, +) +from validation import inner_rom_check # CLI-wide network policy. Public helpers can override it explicitly, while # every CLI mode inherits --offline (including large-file release fallback). _OFFLINE = False @@ -220,3 +226,83 @@ def resolve_file( return path, status return None, "not_found" + + +def preferred_entries( + pack_systems: dict, + db: dict, + bios_dir: str, + base_dest: str, + required_only: bool, + zip_contents, + offline, +) -> dict[str, int]: + """Which declaration wins when several claim the same destination. + + A platform can declare one destination in more than one system, bare + in one and hash-constrained in another. First-come dedup would let + the bare entry pack whatever answers to the name, so a constrained + sibling that resolves by content claims the destination instead. + The choice is about the baseline and reads no data directory, for the + pack, the manifest and verify alike: verify alone passed the registry + and could crown a declaration only a data/ cache satisfies. + """ + preferred_entries: dict[str, int] = {} + dest_entries: dict[str, list[dict]] = {} + for sys_id, system in sorted(pack_systems.items()): + for file_entry in system.get("files", []): + if required_only and file_entry.get("required") is False: + continue + dest = sanitize_pack_path( + file_entry.get("destination", file_entry.get("name", "")) + ) + if not dest: + continue + full = f"{base_dest}/{dest}" if base_dest else dest + dest_entries.setdefault(full, []).append(file_entry) + for full, entries in dest_entries.items(): + if len(entries) < 2: + continue + constrained = [ + fe + for fe in entries + if fe.get("md5") or fe.get("sha1") or fe.get("zipped_file") + ] + if not constrained: + continue + resolved = [ + (fe, *resolve_file( + fe, + db, + bios_dir, + zip_contents, + offline=offline, + )) + for fe in constrained + ] + members = [fe for fe in entries if fe.get("zipped_file")] + if members: + # One archive declared once per ROM it must hold: the archive + # to ship is the one the most declarations accept, not the first + # that answers one of them (adam_fdc.zip: a one-member MAME set + # answered first, the eight-member set sat beside it). + def accepted(path: str | None) -> int: + if not path or not path.endswith(".zip"): + return -1 + return sum(inner_rom_check(fe, path) == "ok" for fe in members) + + fe, path, _st = max(resolved, key=lambda item: accepted(item[1])) + if accepted(path) > 0: + preferred_entries[full] = id(fe) + continue + best = None + for fe, _lp, _st in resolved: + if _lp and _st == "md5_exact": + best = fe + break + if best is None and _lp and resolution_is_hash_exact(_st): + best = fe + if best is not None: + preferred_entries[full] = id(best) + + return preferred_entries diff --git a/scripts/slots.py b/scripts/slots.py index 1c042fca..d5ca771e 100644 --- a/scripts/slots.py +++ b/scripts/slots.py @@ -28,6 +28,7 @@ from common import ( resolve_local_file, runs_standalone, ) +from packresolve import preferred_entries from validation import outside_gap_scope # A profile entry can prove a slot without declaring a hash: Dolphin names no @@ -67,9 +68,14 @@ class Conflict: return sorted({c.emulator for c in self.profile_claims if c.emulator}) -def _normalize(destination: str) -> str: - """Comparable form of a destination, since layers write it differently.""" - return destination.strip().strip("/").replace("\\", "/").casefold() +def _normalize(destination: str, fold: bool = True) -> str: + """Comparable form of a destination, since layers write it differently. + + Case is folded only where the platform's filesystem folds it: on Linux + RetroDECK ships bios/DISK.ROM and bios/disk.rom side by side. + """ + cleaned = destination.strip().strip("/").replace("\\", "/") + return cleaned.casefold() if fold else cleaned def build_claim_index(claims: list[Claim]) -> dict[str, list[Claim]]: @@ -236,16 +242,32 @@ def find_conflicts( Only proven claims are compared. A claim resolved by filename alone asserts nothing about content and cannot contradict anything. """ + fold = bool(config.get("case_insensitive_fs", False)) + # The builder's own choice among declarations sharing a destination, + # read from the same function: the most accepting archive for zipped_file + # members, then md5_exact. A rule of its own here compared profiles with + # a file the pack does not ship (Batocera adam_fdc.zip). + shipped = set( + preferred_entries( + config.get("systems") or {}, db, "bios", base_dest, False, + zip_contents, True, + ).values() + ) by_dest: dict[str, Claim] = {} for claim in platform_claims( config, db, base_dest, zip_contents, data_dir_registry ): - key = _normalize(claim.destination) + key = _normalize(claim.destination, fold) held = by_dest.get(key) - # The pack ships the declaration a hash proves (_preferred_entries), - # so a bare sibling met first must not stand for the destination: - # it would hide the contradiction with the file actually shipped. - if held is None or _claim_rank(claim) > _claim_rank(held): + if held is not None and id(held.entry) in shipped: + continue + # A bare sibling met first must not stand for the destination: it + # would hide the contradiction with the file actually shipped. + if ( + held is None + or id(claim.entry) in shipped + or _claim_rank(claim) > _claim_rank(held) + ): by_dest[key] = claim # Grouped before judging: a profile may declare several revisions that are @@ -257,7 +279,7 @@ def find_conflicts( profiles, db, base_dest, standalone_cores, zip_contents, data_dir_registry, config=config, ): - key = _normalize(claim.destination) + key = _normalize(claim.destination, fold) platform = by_dest.get(key) if platform is None or not platform.is_proven or not claim.is_proven: continue @@ -446,6 +468,7 @@ def find_collisions( upstream list carries. RetroDECK aims a PC-88 disk subsystem ROM and a CoCo disk ROM at one bios/disk.rom. """ + fold = bool(config.get("case_insensitive_fs", False)) by_dest: dict[str, list[dict]] = {} for system in (config.get("systems") or {}).values(): for entry in system.get("files") or []: @@ -453,7 +476,7 @@ def find_collisions( continue dest = entry.get("destination") or entry.get("name") or "" if dest: - by_dest.setdefault(_normalize(dest), []).append(entry) + by_dest.setdefault(_normalize(dest, fold), []).append(entry) collisions = [] for key, entries in sorted(by_dest.items()): diff --git a/tests/test_slots_shipped_file.py b/tests/test_slots_shipped_file.py new file mode 100644 index 00000000..c0865fb8 --- /dev/null +++ b/tests/test_slots_shipped_file.py @@ -0,0 +1,92 @@ +"""Slot arbitration models the file the pack ships at a destination. + +slots.py chose the platform's file by a rule of its own and folded case on +every platform. The builder keeps case on Linux frontends and picks, among +declarations sharing a destination, the archive most zipped_file members +accept, then the md5_exact one. RetroDECK's MSX ``DISK.ROM`` was reported as +colliding with ``disk.rom``, which ships beside it, and Batocera's +``adam_fdc.zip`` was taken for the one-member MAME set. +Reads the repository: skips when the database or the files are absent. +""" + +from __future__ import annotations + +import copy +import hashlib +import os +import sys +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +from common import load_database, load_platform_config # noqa: E402 +import slots # noqa: E402 + +MAME_ADAM = "bios/Arcade/MAME/adam_fdc.zip" +MSX_DISK = "bios/Microsoft/MSX/DISK.ROM" + + +def _needs(*paths: str) -> None: + missing = [p for p in ("database.json", *paths) if not (REPO_ROOT / p).exists()] + if missing: + raise unittest.SkipTest(f"repository data absent: {missing}") + + +class SlotsReadTheShippedFile(unittest.TestCase): + def setUp(self): + previous = os.getcwd() + os.chdir(REPO_ROOT) + self.addCleanup(os.chdir, previous) + + def test_case_kept_where_the_filesystem_keeps_it(self): + _needs(MSX_DISK) + db = load_database("database.json") + config = load_platform_config("retrodeck") + self.assertFalse(config.get("case_insensitive_fs")) + listed = { + path + for collision in slots.find_collisions(config, db) + for path in collision.resolved + } + self.assertNotIn(MSX_DISK, listed) + + folded = copy.deepcopy(config) + folded["case_insensitive_fs"] = True + listed = { + path + for collision in slots.find_collisions(folded, db) + for path in collision.resolved + } + self.assertIn(MSX_DISK, listed) + + def test_profile_judged_against_the_builders_archive(self): + _needs(MAME_ADAM) + db = load_database("database.json") + config = load_platform_config("batocera") + # The declaration the one-member MAME set answers comes first: the + # YAML's order must not decide which archive is believed to ship, + # the builder takes the one most members accept. + files = config["systems"]["adam"]["files"] + files.sort(key=lambda fe: not str(fe.get("zipped_file", "")).startswith("adam disk")) + md5 = hashlib.md5((REPO_ROOT / MAME_ADAM).read_bytes()).hexdigest() + profiles = { + "probe": { + "emulator": "probe", + "type": "libretro", + "files": [ + {"name": "adam_fdc.zip", "md5": md5, "system": "coleco-adam"} + ], + } + } + conflicts = slots.find_conflicts( + config, profiles, db, config.get("base_destination", "") + ) + shipped = {c.destination: c.platform_claim.local_path for c in conflicts} + self.assertIn("bios/adam_fdc.zip", shipped) + self.assertNotEqual(shipped["bios/adam_fdc.zip"], MAME_ADAM) + + +if __name__ == "__main__": + unittest.main()