From 593b277bc4784631121b8b76eb818b053635475b Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Sun, 23 Aug 2026 07:18:41 +0200 Subject: [PATCH] refactor: one place decides which profiles answer The verifier and the builder each resolved the profiles a run names, in thirty-five lines that differed only in how they failed: one exits, the other returns empty-handed. An alias is the same binary under another name and a launcher only starts an emulator, so neither has requirements of its own, and both refusals have to say the same thing. common raises now and each caller chooses its own ending. Six tests hold the refusals, one of them reading both sources so a copy cannot grow back. The manifest's core-complement phase comes out of generate_manifest in the same pass, 60 to 34. Verified inert: manifests identical entry for entry, and the Handy pack rebuilds to the same bytes. --- scripts/common.py | 48 ++++++++ scripts/generate_pack.py | 243 ++++++++++++++++++++------------------ scripts/verify.py | 53 ++------- tests/test_native_mode.py | 64 ++++++++++ 4 files changed, 246 insertions(+), 162 deletions(-) diff --git a/scripts/common.py b/scripts/common.py index 9b36bf94..2bfe1fed 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -838,6 +838,54 @@ def get_mame_clone_map() -> dict[str, str]: _emulator_profiles_cache: dict[tuple[str, bool], dict[str, dict]] = {} +class ProfileSelectionError(ValueError): + """A named profile cannot answer for itself.""" + + +def select_emulator_profiles( + profile_names: list[str], + all_profiles: dict, + standalone: bool = False, +) -> list[tuple[str, dict]]: + """Resolve named profiles, refusing the ones that answer for others. + + An alias is the same binary under another name and a launcher only + starts an emulator, so neither has requirements of its own: naming one + is a question about the wrong profile, and the error says which one to + ask instead. Raising rather than exiting lets the verifier stop and the + builder return empty-handed from one rule. + """ + selected: list[tuple[str, dict]] = [] + for name in profile_names: + if name not in all_profiles: + available = sorted( + key + for key, value in all_profiles.items() + if value.get("type") not in ("alias", "test") + ) + raise ProfileSelectionError( + f"emulator '{name}' not found\n" + f"Available: {', '.join(available[:10])}..." + ) + profile = all_profiles[name] + kind = profile.get("type", "libretro") + if kind == "alias": + raise ProfileSelectionError( + f"{name} is an alias of {profile.get('alias_of', '?')} " + f"-use --emulator {profile.get('alias_of', '?')}" + ) + if kind == "launcher": + raise ProfileSelectionError( + f"{name} is a launcher -use the emulator it launches" + ) + if standalone and "standalone" not in kind: + raise ProfileSelectionError( + f"{name} ({kind}) does not support --standalone" + ) + selected.append((name, profile)) + return selected + + def load_emulator_profiles( emulators_dir: str, skip_aliases: bool = True, diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 85d11c6d..e1589c91 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -51,8 +51,10 @@ from common import ( parse_md5_list, require_yaml, resolution_is_hash_exact, + ProfileSelectionError, resolve_local_file, sanitize_pack_path, + select_emulator_profiles, yaml_load, ) import packresolve @@ -1194,40 +1196,11 @@ def generate_emulator_pack( if zip_contents is None: zip_contents = build_zip_contents_index(db) - # Resolve and validate profile names - selected: list[tuple[str, dict]] = [] - for name in profile_names: - if name not in all_profiles: - available = sorted( - k - for k, v in all_profiles.items() - if v.get("type") not in ("alias", "test") - ) - print(f"Error: emulator '{name}' not found", file=sys.stderr) - print(f"Available: {', '.join(available[:10])}...", file=sys.stderr) - return None - p = all_profiles[name] - if p.get("type") == "alias": - alias_of = p.get("alias_of", "?") - print( - f"Error: {name} is an alias of {alias_of} -use --emulator {alias_of}", - file=sys.stderr, - ) - return None - if p.get("type") == "launcher": - print( - f"Error: {name} is a launcher -use the emulator it launches", - file=sys.stderr, - ) - return None - ptype = p.get("type", "libretro") - if standalone and "standalone" not in ptype: - print( - f"Error: {name} ({ptype}) does not support --standalone", - file=sys.stderr, - ) - return None - selected.append((name, p)) + try: + selected = select_emulator_profiles(profile_names, all_profiles, standalone) + except ProfileSelectionError as exc: + print(f"Error: {exc}", file=sys.stderr) + return None # ZIP naming display_names = [p.get("emulator", n).replace(" ", "") for n, p in selected] @@ -2635,6 +2608,119 @@ def _get_repo_path(sha1: str, db: dict) -> str: return entry.get("path", "") +def _manifest_core_entries( + core_files: list, + config: dict, + db: dict, + bios_dir: str, + base_dest: str, + repo_root: str, + zip_contents, + offline, + region_drops: set, + case_insensitive: bool, + seen_destinations: set, + seen_lower: set, + seen_parents: set, + manifest_files: list, + omitted_by_destination: dict, + record_omission, +) -> int: + """Add the files a platform's cores need but its list does not name. + + The installer fetches by hash, so an entry records the copy this + repository holds rather than the value the profile declares: an + upstream hash carried by no local file leaves the entry with no URL. + Returns the bytes added. + """ + total_size = 0 + extras_pfx = _detect_extras_prefix(config, base_dest) + for fe in core_files: + dest = sanitize_pack_path(fe.get("destination", fe["name"])) + if not dest: + continue + if region_drops and dest in region_drops: + continue + if extras_pfx: + if not dest.startswith(f"{extras_pfx}/"): + full_dest = f"{extras_pfx}/{dest}" + else: + full_dest = dest + else: + full_dest = dest + + if full_dest in seen_destinations: + continue + if case_insensitive and full_dest.lower() in seen_lower: + continue + if _has_path_conflict(full_dest, seen_destinations, seen_parents): + continue + + dest_hint = fe.get("destination", "") + local_path, status = resolve_file( + fe, + db, + bios_dir, + zip_contents, + dest_hint=dest_hint, + offline=offline, + ) + if status in ("not_found", "external", "user_provided") or not local_path: + source_emu = fe.get("source_profile") or fe.get("source_emulator", "") + systems = _extra_system_ids(fe) + record_omission( + full_dest, + fe, + systems[0] if systems else "", + status, + [source_emu] if source_emu else [], + ) + continue + + sha1 = "" + sha256 = "" + file_size = 0 + if local_path and os.path.exists(local_path): + file_size = os.path.getsize(local_path) + hashes = compute_hashes(local_path) + sha1 = hashes["sha1"] + sha256 = hashes["sha256"] + + repo_path = _get_repo_path(sha1, db) if sha1 else "" + source_emu = fe.get("source_profile") or fe.get("source_emulator", "") + + # Manifest dests are relative to base_destination; keep the inferred + # extras prefix when it is an internal layout dir (RetroDECK bios/). + manifest_dest = full_dest + if base_dest and manifest_dest.startswith(f"{base_dest}/"): + manifest_dest = manifest_dest[len(base_dest) + 1:] + + entry = { + "dest": manifest_dest, + "sha1": sha1, + "sha256": sha256, + "size": file_size, + "repo_path": repo_path, + "cores": [source_emu] if source_emu else [], + } + + if _is_large_file(local_path or "", repo_root): + entry["storage"] = "release" + entry["release_asset"] = ( + os.path.basename(local_path) if local_path else fe["name"] + ) + + manifest_files.append(entry) + omitted_by_destination.pop(full_dest, None) + total_size += file_size + seen_destinations.add(full_dest) + _register_path(full_dest, seen_destinations, seen_parents) + if case_insensitive: + seen_lower.add(full_dest.lower()) + + return total_size + + def generate_manifest( platform_name: str, platforms_dir: str, @@ -2845,89 +2931,12 @@ def generate_manifest( ) else: core_files = [] - extras_pfx = _detect_extras_prefix(config, base_dest) - for fe in core_files: - dest = sanitize_pack_path(fe.get("destination", fe["name"])) - if not dest: - continue - if region_drops and dest in region_drops: - continue - if extras_pfx: - if not dest.startswith(f"{extras_pfx}/"): - full_dest = f"{extras_pfx}/{dest}" - else: - full_dest = dest - else: - full_dest = dest - - if full_dest in seen_destinations: - continue - if case_insensitive and full_dest.lower() in seen_lower: - continue - if _has_path_conflict(full_dest, seen_destinations, seen_parents): - continue - - dest_hint = fe.get("destination", "") - local_path, status = resolve_file( - fe, - db, - bios_dir, - zip_contents, - dest_hint=dest_hint, - offline=offline, - ) - if status in ("not_found", "external", "user_provided") or not local_path: - source_emu = fe.get("source_profile") or fe.get("source_emulator", "") - systems = _extra_system_ids(fe) - record_omission( - full_dest, - fe, - systems[0] if systems else "", - status, - [source_emu] if source_emu else [], - ) - continue - - sha1 = "" - sha256 = "" - file_size = 0 - if local_path and os.path.exists(local_path): - file_size = os.path.getsize(local_path) - hashes = compute_hashes(local_path) - sha1 = hashes["sha1"] - sha256 = hashes["sha256"] - - repo_path = _get_repo_path(sha1, db) if sha1 else "" - source_emu = fe.get("source_profile") or fe.get("source_emulator", "") - - # Manifest dests are relative to base_destination; keep the inferred - # extras prefix when it is an internal layout dir (RetroDECK bios/). - manifest_dest = full_dest - if base_dest and manifest_dest.startswith(f"{base_dest}/"): - manifest_dest = manifest_dest[len(base_dest) + 1:] - - entry = { - "dest": manifest_dest, - "sha1": sha1, - "sha256": sha256, - "size": file_size, - "repo_path": repo_path, - "cores": [source_emu] if source_emu else [], - } - - if _is_large_file(local_path or "", repo_root): - entry["storage"] = "release" - entry["release_asset"] = ( - os.path.basename(local_path) if local_path else fe["name"] - ) - - manifest_files.append(entry) - omitted_by_destination.pop(full_dest, None) - total_size += file_size - seen_destinations.add(full_dest) - _register_path(full_dest, seen_destinations, seen_parents) - if case_insensitive: - seen_lower.add(full_dest.lower()) + total_size += _manifest_core_entries( + core_files, config, db, bios_dir, base_dest, repo_root, + zip_contents, offline, region_drops, case_insensitive, + seen_destinations, seen_lower, seen_parents, manifest_files, + omitted_by_destination, record_omission, + ) # No phase 3 (data directories) -skipped for manifest diff --git a/scripts/verify.py b/scripts/verify.py index 86ff0cbf..00e61245 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -48,8 +48,10 @@ from common import ( md5sum, require_yaml, resolve_local_file, + ProfileSelectionError, resolve_platform_cores, sanitize_pack_path, + select_emulator_profiles, ) yaml = require_yaml() @@ -1254,51 +1256,12 @@ def _select_profiles( all_profiles: dict, standalone: bool, ) -> list[tuple[str, dict]]: - """Resolve the named profiles, refusing the ones that answer for others. - - An alias is the same binary under another name and a launcher only - starts an emulator, so neither has requirements of its own: naming - one is a question about the wrong profile, and the answer says which - one to ask instead. - """ - # Resolve profile names, reject alias/launcher - selected: list[tuple[str, dict]] = [] - for name in profile_names: - if name not in all_profiles: - available = sorted( - k - for k, v in all_profiles.items() - if v.get("type") not in ("alias", "test") - ) - print(f"Error: emulator '{name}' not found", file=sys.stderr) - print(f"Available: {', '.join(available[:10])}...", file=sys.stderr) - sys.exit(1) - p = all_profiles[name] - if p.get("type") == "alias": - alias_of = p.get("alias_of", "?") - print( - f"Error: {name} is an alias of {alias_of} -use --emulator {alias_of}", - file=sys.stderr, - ) - sys.exit(1) - if p.get("type") == "launcher": - print( - f"Error: {name} is a launcher -use the emulator it launches", - file=sys.stderr, - ) - sys.exit(1) - # Check standalone capability - ptype = p.get("type", "libretro") - if standalone and "standalone" not in ptype: - print( - f"Error: {name} ({ptype}) does not support --standalone", - file=sys.stderr, - ) - sys.exit(1) - selected.append((name, p)) - - return selected - + """Resolve the named profiles, stopping the run on a bad name.""" + try: + return select_emulator_profiles(profile_names, all_profiles, standalone) + except ProfileSelectionError as exc: + print(f"Error: {exc}", file=sys.stderr) + sys.exit(1) def verify_emulator( profile_names: list[str], diff --git a/tests/test_native_mode.py b/tests/test_native_mode.py index 51f9f974..d8884f0b 100644 --- a/tests/test_native_mode.py +++ b/tests/test_native_mode.py @@ -415,3 +415,67 @@ class SkippingIsNotSettling(unittest.TestCase): def test_the_profile_that_cannot_use_it_does_not_report_it(self): self.assertNotIn(("a_standalone_only", "shared.bin"), self._report()) + + +class OneProfileSelector(unittest.TestCase): + """The verifier and the builder refuse the same names for the same reasons. + + Both resolved the named profiles themselves, in thirty-five lines that + differed only in how they failed: one exits, the other returns + empty-handed. Two copies of a refusal drift the way every other pair in + this repository has. + """ + + PROFILES = { + "real": {"emulator": "Real", "type": "libretro", "files": []}, + "ghost": {"emulator": "Ghost", "type": "alias", "alias_of": "real"}, + "starter": {"emulator": "Starter", "type": "launcher"}, + "lib_only": {"emulator": "LibOnly", "type": "libretro"}, + } + + def _select(self, names, standalone=False): + from common import ProfileSelectionError, select_emulator_profiles + + try: + return select_emulator_profiles(names, self.PROFILES, standalone), None + except ProfileSelectionError as exc: + return None, str(exc) + + def test_a_real_profile_resolves(self): + selected, error = self._select(["real"]) + self.assertIsNone(error) + self.assertEqual([n for n, _p in selected], ["real"]) + + def test_an_alias_names_the_profile_to_ask_instead(self): + _selected, error = self._select(["ghost"]) + self.assertIn("alias of real", error) + self.assertIn("--emulator real", error) + + def test_a_launcher_is_refused(self): + _selected, error = self._select(["starter"]) + self.assertIn("launcher", error) + + def test_standalone_is_refused_on_a_libretro_only_profile(self): + _selected, error = self._select(["lib_only"], standalone=True) + self.assertIn("does not support --standalone", error) + self.assertIsNone(self._select(["lib_only"])[1]) + + def test_an_unknown_name_lists_what_exists(self): + _selected, error = self._select(["absent"]) + self.assertIn("not found", error) + self.assertIn("real", error) + self.assertNotIn("ghost", error, "an alias is not something to suggest") + + def test_both_entry_points_route_through_it(self): + """A copy would let one accept what the other refuses.""" + scripts = Path(__file__).resolve().parent.parent / "scripts" + for name in ("verify.py", "generate_pack.py"): + text = (scripts / name).read_text() + self.assertIn( + "select_emulator_profiles", text, + f"{name} must ask common, not re-derive the refusal", + ) + self.assertNotIn( + 'is a launcher -use the emulator it launches', text, + f"{name} still spells the refusal itself", + )