From 543cb504bb9f05bb9d08caaf8166296dd2e83da6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Sat, 28 Mar 2026 12:57:30 +0100 Subject: [PATCH] fix: skip file/directory path conflicts in pack generation --- scripts/generate_pack.py | 56 ++++++++++++++++++++++++++++ tests/test_e2e.py | 80 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 3207b6ae..0cfbc666 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -198,6 +198,36 @@ def _sanitize_path(raw: str) -> str: return "/".join(parts) +def _path_parents(dest: str) -> list[str]: + """Return all parent directory segments of a path.""" + parts = dest.split("/") + return ["/".join(parts[:i]) for i in range(1, len(parts))] + + +def _has_path_conflict(dest: str, seen_files: set[str], + seen_parents: set[str]) -> bool: + """Check if dest conflicts with existing paths (file vs directory). + + Returns True if adding dest would create an impossible extraction: + - A parent of dest is already a file (e.g., adding X/Y when X is a file) + - dest itself is already used as a directory (e.g., adding X when X/Y exists) + """ + for parent in _path_parents(dest): + if parent in seen_files: + return True + if dest in seen_parents: + return True + return False + + +def _register_path(dest: str, seen_files: set[str], + seen_parents: set[str]) -> None: + """Track a file path and its parent directories.""" + seen_files.add(dest) + for parent in _path_parents(dest): + seen_parents.add(parent) + + def resolve_file(file_entry: dict, db: dict, bios_dir: str, zip_contents: dict | None = None, dest_hint: str = "") -> tuple[str | None, str]: @@ -381,6 +411,7 @@ def generate_pack( user_provided = [] seen_destinations: set[str] = set() seen_lower: set[str] = set() # only used when case_insensitive=True + seen_parents: set[str] = set() # parent dirs of added files (path conflict detection) # Per-file status: worst status wins (missing > untested > ok) file_status: dict[str, str] = {} file_reasons: dict[str, str] = {} @@ -435,12 +466,16 @@ def generate_pack( dedup_key = full_dest already_packed = dedup_key in seen_destinations or (case_insensitive and dedup_key.lower() in seen_lower) + if _has_path_conflict(full_dest, seen_destinations, seen_parents): + continue + storage = file_entry.get("storage", "embedded") if storage == "user_provided": if already_packed: continue seen_destinations.add(dedup_key) + _register_path(dedup_key, seen_destinations, seen_parents) if case_insensitive: seen_lower.add(dedup_key.lower()) file_status.setdefault(dedup_key, "ok") @@ -467,6 +502,7 @@ def generate_pack( else: zf.write(tmp_path, full_dest) seen_destinations.add(dedup_key) + _register_path(dedup_key, seen_destinations, seen_parents) if case_insensitive: seen_lower.add(dedup_key.lower()) file_status.setdefault(dedup_key, "ok") @@ -543,6 +579,7 @@ def generate_pack( if already_packed: continue seen_destinations.add(dedup_key) + _register_path(dedup_key, seen_destinations, seen_parents) if case_insensitive: seen_lower.add(dedup_key.lower()) @@ -589,6 +626,9 @@ def generate_pack( # Skip case-insensitive duplicates (Windows/macOS FS safety) if full_dest.lower() in seen_lower and case_insensitive: continue + # Skip file/directory path conflicts (e.g., SGB1.sfc file vs SGB1.sfc/ dir) + if _has_path_conflict(full_dest, seen_destinations, seen_parents): + continue local_path, status = resolve_file(fe, db, bios_dir, zip_contents) if status in ("not_found", "external", "user_provided"): @@ -599,6 +639,7 @@ def generate_pack( else: zf.write(local_path, full_dest) seen_destinations.add(full_dest) + _register_path(full_dest, seen_destinations, seen_parents) if case_insensitive: seen_lower.add(full_dest.lower()) core_count += 1 @@ -632,7 +673,10 @@ def generate_pack( full = f"{dd_prefix}/{rel}" if full in seen_destinations or full.lower() in seen_lower and case_insensitive: continue + if _has_path_conflict(full, seen_destinations, seen_parents): + continue seen_destinations.add(full) + _register_path(full, seen_destinations, seen_parents) if case_insensitive: seen_lower.add(full.lower()) zf.write(src, full) @@ -776,6 +820,7 @@ def generate_emulator_pack( missing_files = [] seen_destinations: set[str] = set() seen_lower: set[str] = set() + seen_parents: set[str] = set() # parent dirs of added files (path conflict detection) seen_hashes: set[str] = set() # SHA1 dedup for same file, different path data_dir_notices: list[str] = [] data_registry = load_data_dir_registry( @@ -810,7 +855,10 @@ def generate_emulator_pack( full = f"{dd_dest}/{rel}" if dd_dest else rel if full.lower() in seen_lower: continue + if _has_path_conflict(full, seen_destinations, seen_parents): + continue seen_destinations.add(full) + _register_path(full, seen_destinations, seen_parents) seen_lower.add(full.lower()) zf.write(src, full) total_files += 1 @@ -837,6 +885,8 @@ def generate_emulator_pack( if archive_dest.lower() in seen_lower: continue + if _has_path_conflict(archive_dest, seen_destinations, seen_parents): + continue archive_entry = {"name": archive_name} local_path, status = resolve_file(archive_entry, db, bios_dir, zip_contents) @@ -846,6 +896,7 @@ def generate_emulator_pack( else: zf.write(local_path, archive_dest) seen_destinations.add(archive_dest) + _register_path(archive_dest, seen_destinations, seen_parents) seen_lower.add(archive_dest.lower()) total_files += 1 else: @@ -864,10 +915,13 @@ def generate_emulator_pack( if dest.lower() in seen_lower: continue + if _has_path_conflict(dest, seen_destinations, seen_parents): + continue storage = fe.get("storage", "embedded") if storage == "user_provided": seen_destinations.add(dest) + _register_path(dest, seen_destinations, seen_parents) seen_lower.add(dest.lower()) instr = fe.get("instructions", "Please provide this file manually.") instr_name = f"INSTRUCTIONS_{fe['name']}.txt" @@ -887,6 +941,7 @@ def generate_emulator_pack( if download_external(fe, tmp_path): zf.write(tmp_path, dest) seen_destinations.add(dest) + _register_path(dest, seen_destinations, seen_parents) seen_lower.add(dest.lower()) total_files += 1 else: @@ -915,6 +970,7 @@ def generate_emulator_pack( else: zf.write(local_path, dest) seen_destinations.add(dest) + _register_path(dest, seen_destinations, seen_parents) seen_lower.add(dest.lower()) total_files += 1 diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 40e5fad1..002f443d 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -2207,6 +2207,86 @@ class TestE2E(unittest.TestCase): records, _ = parse_firmware_database(fragment) self.assertEqual(records[0]["name"], "good.bin") + def test_157_path_conflict_helpers(self): + """_has_path_conflict detects file/directory naming collisions.""" + from generate_pack import _has_path_conflict, _register_path + + seen_files: set[str] = set() + seen_parents: set[str] = set() + + # Register system/SGB1.sfc as a file + _register_path("system/SGB1.sfc", seen_files, seen_parents) + + # Adding system/SGB1.sfc/program.rom should conflict (parent is a file) + self.assertTrue(_has_path_conflict("system/SGB1.sfc/program.rom", + seen_files, seen_parents)) + + # Adding system/other.bin should not conflict + self.assertFalse(_has_path_conflict("system/other.bin", + seen_files, seen_parents)) + + # Reverse: register a nested path first, then check flat + seen_files2: set[str] = set() + seen_parents2: set[str] = set() + _register_path("system/SGB2.sfc/program.rom", seen_files2, seen_parents2) + + # Adding system/SGB2.sfc as a file should conflict (it's a directory) + self.assertTrue(_has_path_conflict("system/SGB2.sfc", + seen_files2, seen_parents2)) + + # Adding system/SGB2.sfc/boot.rom should not conflict (sibling in same dir) + self.assertFalse(_has_path_conflict("system/SGB2.sfc/boot.rom", + seen_files2, seen_parents2)) + + def test_158_pack_skips_file_directory_conflict(self): + """Pack generation skips entries that conflict with existing paths.""" + from generate_pack import generate_pack + + output_dir = os.path.join(self.root, "pack_conflict") + os.makedirs(output_dir, exist_ok=True) + + # Platform declares SGB1.sfc as a flat file + config = { + "platform": "ConflictTest", + "verification_mode": "existence", + "base_destination": "system", + "systems": { + "test-sys": { + "files": [ + {"name": "present_req.bin", "destination": "present_req.bin", + "required": True}, + ], + }, + }, + } + with open(os.path.join(self.platforms_dir, "test_conflict.yml"), "w") as fh: + yaml.dump(config, fh) + + # Create an emulator profile with a nested path that conflicts + emu = { + "emulator": "ConflictCore", + "type": "libretro", + "systems": ["test-sys"], + "files": [ + {"name": "present_req.bin/nested.rom", "required": False}, + ], + } + with open(os.path.join(self.emulators_dir, "conflict_core.yml"), "w") as fh: + yaml.dump(emu, fh) + + zip_path = generate_pack( + "test_conflict", self.platforms_dir, self.db, self.bios_dir, + output_dir, emulators_dir=self.emulators_dir, + ) + self.assertIsNotNone(zip_path) + with zipfile.ZipFile(zip_path) as zf: + names = zf.namelist() + # Flat file should be present + self.assertTrue(any("present_req.bin" in n and "/" + "nested" not in n + for n in names)) + # Nested conflict should NOT be present + self.assertFalse(any("nested.rom" in n for n in names)) + if __name__ == "__main__": unittest.main()