diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index da96a087..7e5f2134 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -1545,9 +1545,16 @@ def generate_system_pack( offline=offline, ) if result: - # Rename to system-based name - rgn_tag = f"_{region_mod.region_tag(regions)}" if regions else "" - new_name = f"{sys_display}{rgn_tag}_BIOS_Pack.zip" + # Rename to system-based name. Every dimension goes through the one + # list: keeping only the region tag made --system X --required-only + # overwrite the pack built without it. + tags = "".join( + tag + for tag, _label in _narrowings( + "full", regions, None, False, required_only, standalone=standalone + ) + ) + new_name = f"{sys_display}{tags}_BIOS_Pack.zip" new_path = os.path.join(output_dir, new_name) if new_path != result: os.rename(result, new_path) @@ -1616,8 +1623,12 @@ def generate_split_packs( source, regions, target_name, one_per_slot, required_only ) ) + # Two groupings write different files; without the tag they accumulate in + # one directory under one SHA256SUMS.txt. + group_tag = "" if group_by == "system" else f"_By{group_by.title()}" split_dir = os.path.join( - output_dir, f"{platform_display.replace(' ', '_')}{split_tags}_Split" + output_dir, + f"{platform_display.replace(' ', '_')}{split_tags}{group_tag}_Split", ) os.makedirs(split_dir, exist_ok=True) @@ -1768,7 +1779,17 @@ def generate_md5_pack( plat_file_index[alias.lower()] = fe context_name = plat_display if platform_name else (emu_display or "Custom") - zip_name = f"{context_name.replace(' ', '_')}_Custom_BIOS_Pack.zip" + # --standalone changes the destination layout, so a run with it must not + # take the name of a run without it. + custom_tags = "".join( + tag + for tag, _label in _narrowings( + "full", None, None, False, False, standalone=standalone + ) + ) + zip_name = ( + f"{context_name.replace(' ', '_')}_Custom{custom_tags}_BIOS_Pack.zip" + ) zip_path = os.path.join(output_dir, zip_name) os.makedirs(output_dir, exist_ok=True) @@ -3149,6 +3170,37 @@ def _target_cores_for( return cache.get(platform_name) +def _content_narrowing_tags() -> tuple[str, ...]: + """Tags naming a pack that holds fewer files than the platform declares. + + Read from the name builder rather than retyped, so the two cannot drift. + Region and target are absent on purpose: both are handed to the check + itself, which narrows its expectation instead of skipping it. + """ + dimensions = ( + {"source": "platform"}, + {"source": "truth"}, + {"one_per_slot": True}, + {"required_only": True}, + ) + tags: list[str] = [] + for dimension in dimensions: + tags.extend( + tag + for tag, _label in _narrowings( + dimension.get("source", "full"), + None, + None, + dimension.get("one_per_slot", False), + dimension.get("required_only", False), + ) + ) + return tuple(tags) + + +_CONTENT_NARROWING_TAGS = _content_narrowing_tags() + + def _narrows_contents(pack_name: str) -> bool: """True when a pack holds fewer files than the platform declares. @@ -3157,10 +3209,7 @@ def _narrows_contents(pack_name: str) -> bool: Region is not listed: the region filter is passed to the check itself, and a hardware target is passed the same way. """ - return any( - tag in pack_name - for tag in ("_Platform_", "_Truth_", "_Required", "_OnePerSlot") - ) + return any(f"{tag}_" in pack_name for tag in _CONTENT_NARROWING_TAGS) def verify_and_finalize_packs( diff --git a/scripts/generate_truth.py b/scripts/generate_truth.py index 5b4a33bd..2dc815a0 100644 --- a/scripts/generate_truth.py +++ b/scripts/generate_truth.py @@ -10,6 +10,7 @@ from __future__ import annotations import argparse import os +import re import sys sys.path.insert(0, os.path.dirname(__file__)) @@ -87,7 +88,14 @@ def main(argv: list[str] | None = None) -> None: else: platforms = [args.platform] - os.makedirs(args.output_dir, exist_ok=True) + # A target-narrowed model is a different artifact: written under the + # platform's own name it overwrote the full one, and diff_truth then + # compared a narrowed model against the whole scrape. + output_dir = args.output_dir + if args.target: + slug = re.sub(r"[^a-z0-9]+", "-", args.target.strip().lower()).strip("-") + output_dir = os.path.join(output_dir, slug or "target") + os.makedirs(output_dir, exist_ok=True) for name in platforms: # Resolve target cores @@ -120,7 +128,7 @@ def main(argv: list[str] | None = None) -> None: target_cores=target_cores, ) - out_path = os.path.join(args.output_dir, f"{name}.yml") + out_path = os.path.join(output_dir, f"{name}.yml") with open(out_path, "w") as f: yaml.dump( result, diff --git a/scripts/pipeline.py b/scripts/pipeline.py index 48feab43..f7514a47 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -347,6 +347,15 @@ def main(): print("\n--- 2b check buildbot system: SKIPPED (--offline) ---") # Step 2c: Generate truth YAMLs + # A targeted run writes its model in a subdirectory of its own, and the + # diff has to read the same one or it compares a narrowed model against + # the whole scrape. + truth_dir = Path(args.output_dir) / "truth" + if args.target: + truth_dir = truth_dir / re.sub( + r"[^a-z0-9]+", "-", args.target.strip().lower() + ).strip("-") + if args.with_truth or args.with_export: truth_cmd = [ sys.executable, @@ -366,7 +375,7 @@ def main(): # Step 2d: Diff truth vs scraped if args.with_truth or args.with_export: diff_cmd = [sys.executable, "scripts/diff_truth.py", "--all"] - diff_cmd.extend(["--truth-dir", str(Path(args.output_dir) / "truth")]) + diff_cmd.extend(["--truth-dir", str(truth_dir)]) ok, _ = run(diff_cmd, "2d diff truth") results["diff_truth"] = ok all_ok = all_ok and ok diff --git a/tests/test_e2e.py b/tests/test_e2e.py index cc0412ca..ffb1045e 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -5917,9 +5917,41 @@ struct BurnDriver BurnDrvneogeo = { 'source_tag = {"platform": "_Platform"', 'req_suffix = "_required"', 'slot_tag = "_OnePerSlot"', + # The --system rename kept the region tag alone, the custom pack + # took no tag at all, and the skip list retyped four of them. + '_Custom_BIOS_Pack.zip"', + '{rgn_tag}_BIOS_Pack.zip"', + '("_Platform_", "_Truth_"', ): self.assertNotIn(hand_rolled, source, hand_rolled) - self.assertGreaterEqual(source.count("_narrowings("), 5) + self.assertGreaterEqual(source.count("_narrowings("), 8) + + def test_a_narrowed_system_pack_does_not_take_the_full_name(self): + """Every dimension has to show in the filename or two builds collide. + + --system X --required-only produced the same name as --system X and + overwrote it, because the rename kept only the region tag. + """ + import subprocess + + repo = os.path.join(os.path.dirname(__file__), "..") + names = {} + for label, extra in (("full", []), ("required", ["--required-only"])): + out = os.path.join(self.root, f"sysname_{label}") + proc = subprocess.run( + [sys.executable, "scripts/generate_pack.py", "--system", + "atari-lynx", "--offline", *extra, "--output-dir", out], + capture_output=True, text=True, cwd=repo, timeout=300, + ) + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + names[label] = sorted( + f for f in os.listdir(out) if f.endswith("_BIOS_Pack.zip") + ) + self.assertTrue(names["full"] and names["required"]) + self.assertNotEqual( + names["full"], names["required"], + "a required-only system pack took the name of the full one", + ) def test_a_full_pack_is_not_announced_as_narrowed(self): from generate_pack import _build_readme, _narrowings diff --git a/tests/test_truth_diff.py b/tests/test_truth_diff.py index 19d0fb30..153fd4bc 100644 --- a/tests/test_truth_diff.py +++ b/tests/test_truth_diff.py @@ -9,6 +9,7 @@ discrepancy that does not exist. Content decides, as everywhere else here. from __future__ import annotations +import pathlib import sys import unittest from pathlib import Path @@ -23,6 +24,38 @@ def _entry(name: str, **hashes) -> dict: return {"name": name, **hashes} +class ATargetedModelIsItsOwnArtifact(unittest.TestCase): + """A narrowed truth model must not take the full model's place. + + generate_truth wrote dist/truth/.yml whatever --target said, so + a targeted run overwrote the full model and diff_truth then compared a + narrowed model against the whole scrape. + """ + + def test_a_target_writes_beside_the_full_model(self): + import subprocess + import tempfile + + repo = pathlib.Path(__file__).resolve().parent.parent + with tempfile.TemporaryDirectory(dir=str(repo / "tmp")) as directory: + for extra in ([], ["--target", "browser"]): + proc = subprocess.run( + [sys.executable, "scripts/generate_truth.py", "--platform", + "romm", *extra, "--output-dir", directory], + capture_output=True, text=True, cwd=str(repo), timeout=400, + ) + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + produced = sorted( + str(p.relative_to(directory)) + for p in pathlib.Path(directory).rglob("*.yml") + ) + self.assertEqual( + produced, + ["browser/romm.yml", "romm.yml"], + "the targeted model did not land beside the full one", + ) + + class RenameMatching(unittest.TestCase): def test_a_shared_sha1_pairs_the_two_names(self): truth = [_entry("bios_CD_U.bin", sha1="a" * 40)]