From 3e2a5279a150cfead6da9d7795784f99e952dfeb Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 6 Nov 2026 08:21:52 +0100 Subject: [PATCH] fix: split parts hold what the full pack holds Each part was built alone: the Other_Cores part never saw the platform's destinations and each part had its own case fold. RetroDECK's parts wrote bios/SGB1.sfc/ members beside the file bios/SGB1.sfc, RetroBat's wrote 21 openmsx/ twins of openMSX/ files, and a path two systems declare got each system's file. The full pack is now built in memory first and a part writes a declared file or core extra only where the full pack writes the same bytes. --- scripts/generate_pack.py | 93 +++++++++++++++++---------- tests/test_split_extras.py | 127 ++++++++++++++++++++++++++++++++++++- 2 files changed, 187 insertions(+), 33 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 458fa968b..907f0bb8f 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -26,6 +26,7 @@ import argparse import contextlib import functools import hashlib +import io import json import os import re @@ -158,6 +159,9 @@ class PackRecord: self.members: list[str] = [] self.files: list[str] = [] self.trees: list[str] = [] + # The collection file each copied member comes from: a split part + # writes a file only where the full pack writes the same bytes. + self.sources: dict[str, str] = {} def _add_pack_member(zf: zipfile.ZipFile, source_path: str, arcname: str) -> None: @@ -173,6 +177,7 @@ def _add_pack_member(zf: zipfile.ZipFile, source_path: str, arcname: str) -> Non """ if isinstance(zf, PackRecord): zf.members.append(arcname) + zf.sources[arcname] = source_path return info = zipfile.ZipInfo(filename=arcname, date_time=_FIXED_DATE_TIME) info.compress_type = _member_compression(arcname) @@ -806,6 +811,7 @@ def generate_pack( pack_name: str | None = None, no_game_data: bool = False, record: PackRecord | None = None, + within: dict[str, str] | None = None, ) -> str | None: """Generate a ZIP pack for a platform. @@ -819,6 +825,12 @@ def generate_pack( ``record`` builds the pack in memory (PackRecord): no archive is written, no external file fetched, and the paths are kept on it. + ``within`` is the full pack's PackRecord.sources, given to a split part: + a declared file or core extra is written only where the full pack writes + the same bytes. A part sees neither the other systems' declarations nor + their extras, so a path the full pack settles (a file/directory conflict, + a case twin, a path another system's file takes) was written again. + Returns the path to the generated ZIP, or None on failure. """ config = load_platform_config(platform_name, platforms_dir) @@ -1174,6 +1186,11 @@ def generate_pack( if already_packed: continue + if within is not None and not _same_bytes( + local_path, within.get(_flat(full_dest, base_dest, flatten)) + ): + # The full pack holds another system's file here. + continue if file_status.get(dedup_key) == "excluded": # An earlier declaration refused the file this one ships. file_status[dedup_key] = "untested" @@ -1281,6 +1298,8 @@ def generate_pack( ) flat_dest = _flat(full_dest, base_dest, flatten) + if within is not None and not _same_bytes(local_path, within.get(flat_dest)): + continue if local_path.endswith(".zip"): _add_zip_to_pack(local_path, flat_dest, zf, fe) else: @@ -1414,6 +1433,7 @@ def _extract_zip_to_archive( for name in map(sanitize_pack_path, src.namelist()) if name and not name.endswith("/") ) + target_zf.sources[dest_prefix] = source_zip return for info in src.infolist(): if info.is_dir(): @@ -1447,6 +1467,7 @@ def _add_zip_to_pack( """ if isinstance(target_zf, PackRecord): target_zf.members.append(dest_path) + target_zf.sources[dest_path] = source_zip return _check_zip_members(source_zip) _add_pack_member(target_zf, source_zip, dest_path) @@ -2075,8 +2096,14 @@ def generate_split_packs( one_per_slot: bool = False, offline: bool | None = None, no_game_data: bool = False, + records: dict[str, PackRecord] | None = None, ) -> list[str]: - """Generate split packs (one ZIP per system or manufacturer).""" + """Generate split packs (one ZIP per system or manufacturer). + + The parts add up to the full pack: each is built against the full pack's + own placement (``within``). ``records`` builds the parts in memory, keyed + by part name, with no archive written. + """ config = load_platform_config(platform_name, platforms_dir) # The split directory carries the same tags as the packs inside it, or two # differently narrowed runs write into one another. @@ -2099,10 +2126,36 @@ def generate_split_packs( f"{_platform_pack_stem([platform_name], platform_name, platforms_dir)}{split_tags}" ) split_dir = os.path.join(output_dir, f"{part_prefix}{group_tag}_Split") - os.makedirs(split_dir, exist_ok=True) + if records is None: + os.makedirs(split_dir, exist_ok=True) if emu_profiles is None: emu_profiles = load_emulator_profiles(emulators_dir) + narrowing = { + "emulators_dir": emulators_dir, "zip_contents": zip_contents, + "data_registry": data_registry, "emu_profiles": emu_profiles, + "target_cores": target_cores, "required_only": required_only, + "source": source, "regions": regions, "target_name": target_name, + "one_per_slot": one_per_slot, "offline": offline, + "no_game_data": no_game_data, + } + full = PackRecord() + with contextlib.redirect_stdout(io.StringIO()): + generate_pack( + platform_name, platforms_dir, db, bios_dir, split_dir, + record=full, **narrowing, + ) + + def part(pack_name: str, **scope: object) -> str | None: + record = PackRecord() if records is not None else None + zip_path = generate_pack( + platform_name, platforms_dir, db, bios_dir, split_dir, + pack_name=pack_name, record=record, within=full.sources, + **scope, **narrowing, + ) + if record is not None and zip_path: + records[pack_name] = record + return zip_path # The systems a core of the target runs: a part per declared system sent # each one the target drops to a refusal naming it as unknown. systems = platform_scope_systems(config, emu_profiles, target_cores) @@ -2136,27 +2189,9 @@ def generate_split_packs( results = [] for group_name, group_system_ids in sorted(groups.items()): group_extras = _extras_for_systems(all_extras, group_system_ids) - zip_path = generate_pack( - platform_name, - platforms_dir, - db, - bios_dir, - split_dir, - emulators_dir=emulators_dir, - zip_contents=zip_contents, - data_registry=data_registry, - emu_profiles=emu_profiles, - target_cores=target_cores, - required_only=required_only, - system_filter=group_system_ids, - precomputed_extras=group_extras, - source=source, - regions=regions, - target_name=target_name, - one_per_slot=one_per_slot, - offline=offline, - pack_name=f"{part_prefix}_{_name_part(group_name, '_')}_BIOS_Pack.zip", - no_game_data=no_game_data, + zip_path = part( + f"{part_prefix}_{_name_part(group_name, '_')}_BIOS_Pack.zip", + system_filter=group_system_ids, precomputed_extras=group_extras, ) if zip_path: results.append(zip_path) @@ -2172,15 +2207,9 @@ def generate_split_packs( } undistributed = [fe for fe in all_extras if id(fe) not in distributed] if undistributed: - zip_path = generate_pack( - platform_name, platforms_dir, db, bios_dir, split_dir, - emulators_dir=emulators_dir, zip_contents=zip_contents, - data_registry=data_registry, emu_profiles=emu_profiles, - target_cores=target_cores, required_only=required_only, - precomputed_extras=undistributed, extras_only=True, source=source, - regions=regions, target_name=target_name, one_per_slot=one_per_slot, - offline=offline, pack_name=f"{part_prefix}_Other_Cores_BIOS_Pack.zip", - no_game_data=no_game_data, + zip_path = part( + f"{part_prefix}_Other_Cores_BIOS_Pack.zip", + precomputed_extras=undistributed, extras_only=True, ) if zip_path: results.append(zip_path) diff --git a/tests/test_split_extras.py b/tests/test_split_extras.py index 568cd34be..e64fff4a3 100644 --- a/tests/test_split_extras.py +++ b/tests/test_split_extras.py @@ -7,6 +7,7 @@ and in no part (4507 of RetroArch's 5430). from __future__ import annotations +import hashlib import sys import tempfile import unittest @@ -19,7 +20,7 @@ REPO_ROOT = Path(__file__).resolve().parent.parent sys.path.insert(0, str(REPO_ROOT / "scripts")) from common import build_zip_contents_index, compute_hashes, load_emulator_profiles # noqa: E402 -from generate_pack import generate_split_packs # noqa: E402 +from generate_pack import PackRecord, generate_pack, generate_split_packs # noqa: E402 class SplitPartsCoverTheExtras(unittest.TestCase): @@ -70,5 +71,129 @@ class SplitPartsCoverTheExtras(unittest.TestCase): self.assertIn("core_c.bin", members) +class SplitPartsHoldWhatTheFullPackHolds(unittest.TestCase): + """A part sees neither the other systems' declarations nor their extras. + + RetroDECK's Other_Cores part wrote bios/SGB1.sfc/program.rom beside the + SNES part's file bios/SGB1.sfc, a file/directory clash the full pack + refuses. RetroBat's ColecoVision part wrote openmsx/... beside the MSX + part's openMSX/..., case twins the full pack folds. A path two systems + declare got each system's file, one per part. + """ + + def _build(self, platform: dict, profiles: dict[str, dict], blobs: dict[str, bytes]): + tmp = tempfile.TemporaryDirectory(dir=REPO_ROOT / "tmp") + self.addCleanup(tmp.cleanup) + root = Path(tmp.name) + (root / "platforms").mkdir() + (root / "emulators").mkdir() + files = {} + for rel, payload in blobs.items(): + path = root / "bios" / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(payload) + files[rel] = (str(path), compute_hashes(str(path))) + by_name: dict[str, list[str]] = {} + for rel, (_p, h) in files.items(): + by_name.setdefault(Path(rel).name, []).append(h["sha1"]) + db = { + "files": { + h["sha1"]: {"name": Path(rel).name, "md5": h["md5"], "sha1": h["sha1"], + "sha256": h["sha256"], "path": p, "size": 1} + for rel, (p, h) in files.items() + }, + "indexes": { + "by_md5": {h["md5"]: h["sha1"] for _p, h in files.values()}, + "by_name": by_name, "by_crc32": {}, "by_path_suffix": {}, + }, + } + (root / "platforms" / "_registry.yml").write_text( + yaml.dump({"platforms": {"plat": {"status": "active"}}}) + ) + (root / "platforms" / "plat.yml").write_text(yaml.dump(platform)) + for name, profile in profiles.items(): + (root / "emulators" / f"{name}.yml").write_text(yaml.dump(profile)) + args = dict( + emulators_dir=str(root / "emulators"), + zip_contents=build_zip_contents_index(db), + emu_profiles=load_emulator_profiles(str(root / "emulators")), + offline=True, + ) + full = PackRecord() + generate_pack( + "plat", str(root / "platforms"), db, str(root / "bios"), "", record=full, **args + ) + parts: dict[str, PackRecord] = {} + generate_split_packs( + "plat", str(root / "platforms"), db, str(root / "bios"), str(root / "dist"), + records=parts, **args, + ) + self.assertFalse((root / "dist").exists()) + return full, parts + + def _assert_parts_add_up(self, full: PackRecord, parts: dict[str, PackRecord]) -> None: + notes = {"README.txt"} + union = {m for r in parts.values() for m in r.members} - notes + self.assertEqual(union, set(full.members) - notes) + for record in parts.values(): + for member, source in record.sources.items(): + self.assertEqual( + compute_hashes(source)["sha1"], + compute_hashes(full.sources[member])["sha1"], member, + ) + + def test_a_file_directory_conflict_stays_out_of_every_part(self): + full, parts = self._build( + { + "platform": "Plat", "verification_mode": "existence", "cores": ["sgb"], + "systems": {"snes": {"files": [{"name": "SGB1.sfc"}]}}, + }, + {"sgb": { + "emulator": "SGB", "type": "libretro", "systems": ["sgb-other"], + "files": [{"name": "program.rom", "path": "SGB1.sfc/program.rom"}], + }}, + {"SGB1.sfc": b"cartridge", "sgb/program.rom": b"program"}, + ) + self.assertNotIn("SGB1.sfc/program.rom", full.members) + self._assert_parts_add_up(full, parts) + + def test_a_case_twin_is_written_once(self): + full, parts = self._build( + { + "platform": "Plat", "verification_mode": "existence", + "case_insensitive_fs": True, "cores": ["msxcore"], + "systems": { + "msx": {"files": [{"name": "disk.rom", "destination": "openMSX/disk.rom"}]}, + "coleco": {"files": []}, + }, + }, + {"msxcore": { + "emulator": "MSX Core", "type": "libretro", "systems": ["msx", "coleco"], + "files": [{"name": "disk.rom", "path": "openmsx/disk.rom"}], + }}, + {"MSX/disk.rom": b"disk"}, + ) + self.assertNotIn("openmsx/disk.rom", full.members) + self._assert_parts_add_up(full, parts) + + def test_a_path_two_systems_declare_holds_one_file(self): + full, parts = self._build( + { + "platform": "Plat", "verification_mode": "existence", + "systems": { + "coco": {"files": [{ + "name": "disk.rom", "sha1": hashlib.sha1(b"coco disk").hexdigest(), + }]}, + "pc88": {"files": [{ + "name": "disk.rom", "sha1": hashlib.sha1(b"pc-88 disk").hexdigest(), + }]}, + }, + }, + {}, + {"CoCo/disk.rom": b"coco disk", "PC88/disk.rom": b"pc-88 disk"}, + ) + self._assert_parts_add_up(full, parts) + + if __name__ == "__main__": unittest.main()