From 7d1b8435bc582307accb8265be10d7e6cafae712 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:30:53 +0200 Subject: [PATCH] fix: rank same-named archives by what they hold The name step only ranked archive copies by members declared through archive sub-entries, and the contents an archive entry lists never reached the resolver, so the first copy indexed won. contents now ranks copies by crc32, or by member name where the code opens by name, and the report and both extra passes carry it. With no layout declared, a primary holding every ROM of the others wins; two revisions keep the index order. --- scripts/common.py | 59 ++++++++++++++++++++++---- scripts/packextras.py | 6 +++ scripts/verify.py | 2 + scripts/ziptools.py | 21 ++++++++++ tests/test_archive_choice.py | 80 ++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+), 7 deletions(-) diff --git a/scripts/common.py b/scripts/common.py index 4677c690..98839cb6 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -870,7 +870,29 @@ def archive_entry(archive: str, files: list[dict]) -> dict: def holds_required_members(path: str, members: list[dict]) -> bool: """Whether an archive holds every member a profile requires inside it.""" crcs = zip_member_crcs(path) - return all(m["crc32"] in crcs for m in members if m["required"]) + names = zip_member_names(path) + return all(_member_held(m, crcs, names) for m in members if m["required"]) + + +def contents_members(file_entry: dict) -> list[dict]: + """The members an archive entry's ``contents:`` lists, as ranking keys. + + A member is its crc32 where the profile declares one, its name where the + code opens it by name alone: Flycast reads epr-21329.ic27 in + hod2bios.zip by filename, and a copy without it has no Japan BIOS. + """ + return [ + {"crc32": str(m["crc32"]).lower(), "required": True} + if m.get("crc32") else {"name": str(m["name"]), "required": True} + for m in file_entry.get("contents") or [] + if m.get("crc32") or m.get("name") + ] + + +def _member_held(member: dict, crcs: frozenset[str], names: frozenset[str]) -> bool: + if member.get("crc32"): + return member["crc32"] in crcs + return member["name"].rsplit("/", 1)[-1].casefold() in names def _holding_members(paths: list[str], members: list[dict]) -> list[str]: @@ -881,16 +903,35 @@ def _holding_members(paths: list[str], members: list[dict]) -> list[str]: """ def held(path: str) -> tuple[int, int]: crcs = zip_member_crcs(path) - return ( - sum(1 for m in members if m["required"] and m["crc32"] in crcs), - sum(1 for m in members if m["crc32"] in crcs), - ) + names = zip_member_names(path) + found = [m for m in members if _member_held(m, crcs, names)] + return sum(1 for m in found if m["required"]), len(found) scores = {path: held(path) for path in paths} best = max(scores.values(), default=(0, 0)) return [path for path in paths if scores[path] == best] +def _covering_archives(paths: list[str]) -> list[str]: + """Same-named archives, those holding every ROM each other copy holds. + + Without a declared layout, a primary that is a superset of the other + primaries misses nothing any of them holds. Two copies that each hold a + ROM the other lacks are different revisions, and nothing here can + choose: the order is kept. Index order alone used to decide, and + deleting an unrelated duplicate flipped which copy a core received. + """ + # A variant is served only when no primary is, so the primaries are + # the copies this choice is between. + pool = [path for path in paths if "/.variants/" not in path] or paths + crcs = {path: zip_member_crcs(path) for path in pool} + covering = [ + path for path in pool + if crcs[path] and all(crcs[other] <= crcs[path] for other in pool) + ] + return covering + [path for path in paths if path not in covering] + + def _by_affinity(paths: list[str], file_entry: dict, dest_hint: str) -> list[str]: """Same-named candidates, the likeliest first. @@ -1231,8 +1272,11 @@ def resolve_local_file( if candidates: if zipped_file: candidates = [p for p in candidates if ".zip" in os.path.basename(p)] - if file_entry.get("members"): - candidates = _holding_members(candidates, file_entry["members"]) + members = file_entry.get("members") or contents_members(file_entry) + if members: + candidates = _holding_members(candidates, members) + elif len(candidates) > 1 and name.casefold().endswith(".zip"): + candidates = _covering_archives(candidates) candidates = _by_affinity(candidates, file_entry, dest_hint) # A copy outside another emulator's tree comes first: the # affinity read pcsx2 in bios/Other/armsx2/pcsx2/ and served @@ -2361,6 +2405,7 @@ from ziptools import ( # noqa: E402,F401 _zip_contents_cache, _BOUNDED_RATIO_METHODS, zip_member_crcs, + zip_member_names, ) from artifacts import ( # noqa: E402,F401 write_if_changed, diff --git a/scripts/packextras.py b/scripts/packextras.py index 773dd501..3a36f010 100644 --- a/scripts/packextras.py +++ b/scripts/packextras.py @@ -776,6 +776,8 @@ def _collect_emulator_extras( for identity in PROFILE_IDENTITY_FIELDS: if f.get(identity) not in (None, "", []): extra[identity] = f[identity] + if f.get("contents"): + extra["contents"] = f["contents"] extra["claim_order"] = (2, 0, emu_name) if _take(extra, full_dest, seen_dests, taken): extras.append(extra) @@ -823,6 +825,10 @@ def _report_extra(u: dict, name: str, dest: str) -> dict: } if u.get("members"): extra["members"] = u["members"] + # The archive layout the core opens ranks same-named copies by what + # they hold; without it the first copy indexed won. + if u.get("contents"): + extra["contents"] = u["contents"] # Keep every reproducible identity constraint carried by the source # profile. Some profiles expose CRC32/SHA-256 only through their # validation block; dropping those fields here would let a same-named diff --git a/scripts/verify.py b/scripts/verify.py index 756e3256..a2af70bd 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -638,6 +638,8 @@ def find_undeclared_files( "sha1": f.get("sha1"), "md5": f.get("md5"), } + if f.get("contents"): + entry["contents"] = f["contents"] if duplicate: first.setdefault("other_claims", []).append(entry) continue diff --git a/scripts/ziptools.py b/scripts/ziptools.py index a14d6f0f..9a8cabe4 100644 --- a/scripts/ziptools.py +++ b/scripts/ziptools.py @@ -75,6 +75,27 @@ def _member_crcs(path: str, mtime: float) -> frozenset[str]: return frozenset() +@functools.lru_cache(maxsize=4096) +def _member_names(path: str, mtime: float) -> frozenset[str]: + try: + with zipfile.ZipFile(path) as archive: + return frozenset( + info.filename.rsplit("/", 1)[-1].casefold() + for info in archive.infolist() if not info.is_dir() + ) + except (zipfile.BadZipFile, OSError): + return frozenset() + + +def zip_member_names(path: str) -> frozenset[str]: + """Casefolded base name of each member, read from the central directory.""" + try: + mtime = os.path.getmtime(path) + except OSError: + return frozenset() + return _member_names(path, mtime) + + def zip_member_crcs(path: str) -> frozenset[str]: """CRC-32 of each member, read from the central directory.""" try: diff --git a/tests/test_archive_choice.py b/tests/test_archive_choice.py index 3c6a6774..860f6dbd 100644 --- a/tests/test_archive_choice.py +++ b/tests/test_archive_choice.py @@ -269,5 +269,85 @@ class AnArchiveHashedByItsMembers(unittest.TestCase): self.assertEqual(marks, {(True, "composite"), (False, None)}) +class AnArchiveRankedByWhatItHolds(unittest.TestCase): + """Two primaries answered hod2bios.zip and the first one indexed won: + Flycast received a set without epr-21329.ic27, its Japan BIOS, once an + unrelated duplicate was deleted and the index order flipped.""" + + def setUp(self): + directory = tempfile.mkdtemp(dir=REPO_ROOT / "tmp") + self.addCleanup(shutil.rmtree, directory, True) + self.root = Path(directory) + self.roms = {f"epr-213{n}.ic27": bytes([n]) * 16 for n in (29, 30, 31, 32)} + + def _db(self, copies: list[tuple[str, list[str]]]) -> dict: + files, by_name = {}, {} + for relative, members in copies: + path = self.root / relative + path.parent.mkdir(parents=True, exist_ok=True) + with zipfile.ZipFile(path, "w") as zf: + for member in members: + zf.writestr(member, self.roms.get(member, member.encode())) + sha1 = hashlib.sha1(path.read_bytes()).hexdigest() + files[sha1] = {"path": str(path), "name": "hod2bios.zip", "sha1": sha1} + by_name.setdefault("hod2bios.zip", []).append(sha1) + return {"files": files, "indexes": {"by_name": by_name, "by_md5": {}, + "by_path_suffix": {}, "by_crc32": {}}} + + def _resolve(self, entry: dict, db: dict) -> str: + from common import resolve_local_file + + path, _status = resolve_local_file(entry, db) + return str(Path(path).relative_to(self.root)) + + def test_member_names_the_code_opens_choose_the_copy(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27", "epr-21331.ic27"]), + ("bios/MAME/hod2bios.zip", list(self.roms))]) + entry = {"name": "hod2bios.zip", "contents": [ + {"name": name, "size": 16} for name in ("epr-21331.ic27", "epr-21330.ic27", + "epr-21329.ic27")]} + self.assertEqual(self._resolve(entry, db), "bios/MAME/hod2bios.zip") + + def test_member_crcs_choose_the_copy(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27", "renamed.ic27"]), + ("bios/MAME/hod2bios.zip", ["epr-21330.ic27", "epr-21332.ic27"])]) + entry = {"name": "hod2bios.zip", "contents": [ + {"name": "other-name.ic27", "crc32": format(zlib.crc32(self.roms[name]), "08x")} + for name in ("epr-21330.ic27", "epr-21332.ic27")]} + self.assertEqual(self._resolve(entry, db), "bios/MAME/hod2bios.zip") + + def test_without_a_layout_a_superset_primary_wins(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27"]), + ("bios/MAME/hod2bios.zip", list(self.roms))]) + self.assertEqual(self._resolve({"name": "hod2bios.zip"}, db), + "bios/MAME/hod2bios.zip") + + def test_two_revisions_keep_the_index_order(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27", "epr-21331.ic27"]), + ("bios/MAME/hod2bios.zip", ["epr-21330.ic27", "epr-21332.ic27"])]) + self.assertEqual(self._resolve({"name": "hod2bios.zip"}, db), + "bios/Arcade/hod2bios.zip") + + def test_a_variant_does_not_stop_a_primary_from_covering(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27"]), + ("bios/MAME/hod2bios.zip", list(self.roms)), + ("bios/Arcade/.variants/hod2bios.zip", ["proto.ic27"])]) + self.assertEqual(self._resolve({"name": "hod2bios.zip"}, db), + "bios/MAME/hod2bios.zip") + + def test_every_claim_path_carries_the_layout(self): + from packextras import _report_extra + + contents = [{"name": "epr-21329.ic27", "size": 16}] + extra = _report_extra({"name": "hod2bios.zip", "contents": contents}, + "hod2bios.zip", "dc/hod2bios.zip") + self.assertEqual(extra["contents"], contents) + # The second pass builds its extra from the profile entry itself. + source = (REPO_ROOT / "scripts" / "packextras.py").read_text() + self.assertEqual(source.count('extra["contents"] = '), 2) + report = (REPO_ROOT / "scripts" / "verify.py").read_text() + self.assertIn('entry["contents"] = f["contents"]', report) + + if __name__ == "__main__": unittest.main()