fix: keep each recalbox block and its comments

scripts/exporter/recalbox_exporter.py

@@ -13,7 +13,7 @@ from __future__ import annotations
 
 import sys
 from pathlib import Path
-from xml.etree.ElementTree import ParseError
+from xml.etree.ElementTree import Comment, ParseError
 from xml.sax.saxutils import quoteattr
 
 sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
@@ -123,27 +123,32 @@ class Exporter(BaseExporter):
         ]
         # Recalbox's own file is the base, so an element our transcription
         # does not carry (added or renamed after a scrape) stays as written,
-        # and its system order with it.
-        theirs: list[tuple[str, str, list[dict[str, str]]]] = []
+        # and its system order with it. A platform can hold several blocks
+        # (Pentagon and Scorpion are both zxspectrum), and an entry the
+        # maintainer commented out stays commented out.
+        theirs: list[tuple[str, str, list[dict[str, str] | str]]] = []
         original = originals.get(self.native_filename(), "")
         if original:
             try:
-                root = parse_untrusted_xml(original, self.native_filename())
+                root = parse_untrusted_xml(original, self.native_filename(), comments=True)
             except (ParseError, ValueError):
                 root = None
             if root is not None:
                 theirs = [
-                    (s.get("platform", ""), s.get("fullname", ""),
-                     [dict(b.attrib) for b in s.iter("bios")])
+                    (s.get("platform", ""), s.get("fullname", ""), [
+                        f"<!--{child.text}-->" if child.tag is Comment else dict(child.attrib)
+                        for child in s
+                        if child.tag is Comment or child.tag == "bios"
+                    ])
                     for s in root.iter("system")
                 ]
 
-        ours: dict[str, tuple[NativeSystem, dict[str, str]]] = {}
+        ours: dict[str, tuple[NativeSystem, dict[str, tuple[NativeFile, str]]]] = {}
         for system in systems.values():
             writable = [fe for fe in system.files if self.writable(fe)]
             if writable:
                 ours[system.native_id] = (system, {
-                    self._path(fe): self._bios_element(fe, system.native_id)
+                    self._path(fe): (fe, self._bios_element(fe, system.native_id))
                     for fe in writable
                 })
 
@@ -158,18 +163,37 @@ class Exporter(BaseExporter):
             lines.extend(body)
             lines.append("  </system>")
 
-        for platform, fullname, bios in theirs:
-            if platform not in ours:
-                emit(platform, fullname, [element(attrs) for attrs in bios])
-                continue
-            system, elements = ours.pop(platform)
-            body = [elements.pop(attrs.get("path", ""), None) or element(attrs)
-                    for attrs in bios]
-            body.extend(elements.values())
-            emit(platform, self.display_name(system), body)
+        # Each block of theirs corrects the entries it holds.
+        blocks: list[tuple[str, str, list[str]]] = []
+        for platform, fullname, children in theirs:
+            elements = ours.get(platform, (None, {}))[1]
+            body: list[str] = []
+            for child in children:
+                if isinstance(child, str):
+                    body.append(f"    {child}")
+                    continue
+                mine = elements.pop(child.get("path", ""), None)
+                body.append(mine[1] if mine else element(child))
+            blocks.append((platform, fullname, body))
+
+        # An addition joins the block named like its file, else the first
+        # block of its platform.
+        first: dict[str, int] = {}
+        named: dict[tuple[str, str], int] = {}
+        for index, (platform, fullname, _) in enumerate(blocks):
+            first.setdefault(platform, index)
+            named.setdefault((platform, fullname), index)
+        for platform in [p for p in ours if p in first]:
+            _, elements = ours.pop(platform)
+            for fe, rendered in elements.values():
+                key = (platform, str(fe.native("native_name", "")))
+                blocks[named.get(key, first[platform])][2].append(rendered)
+
+        for platform, fullname, body in blocks:
+            emit(platform, fullname, body)
         for platform in sorted(ours):
             system, elements = ours[platform]
-            emit(platform, self.display_name(system), list(elements.values()))
+            emit(platform, self.display_name(system), [r for _, r in elements.values()])
 
         lines.append("</biosList>")
         lines.append("")

scripts/safeparse.py

@@ -13,7 +13,9 @@ except ImportError:  # optional at import time
     yaml = None
 
 
-def parse_untrusted_xml(content: str | bytes, label: str = "XML") -> ET.Element:
+def parse_untrusted_xml(
+    content: str | bytes, label: str = "XML", comments: bool = False
+) -> ET.Element:
     """Parse XML fetched from a third party.
 
     ElementTree expands internal entities, so a document that declares them
@@ -25,10 +27,14 @@ def parse_untrusted_xml(content: str | bytes, label: str = "XML") -> ET.Element:
     The check targets <!ENTITY rather than <!DOCTYPE because Logiqx DATs
     legitimately carry a doctype pointing at logiqx.com, and ElementTree does
     not resolve external entities, so a doctype alone fetches nothing.
+    `comments` keeps comments in the tree, for a file rewritten in place.
     """
     text = content if isinstance(content, str) else content.decode("utf-8", "replace")
     if "<!ENTITY" in text.upper():
         raise ValueError(f"XML entity declarations are not allowed in {label}")
+    if comments:
+        parser = ET.XMLParser(target=ET.TreeBuilder(insert_comments=True))
+        return ET.fromstring(content, parser=parser)
     return ET.fromstring(content)
 
 def require_yaml():

tests/test_exporters.py

@@ -590,6 +590,57 @@ class RecalboxExport(unittest.TestCase):
         self.assertIn("scph5000.bin", paths)
         self.assertNotIn("psx/scph5000.bin", paths)
 
+    def test_two_blocks_of_one_platform_stay_two(self):
+        """Recalbox files Pentagon and Scorpion under zxspectrum, apart.
+
+        Merged, the Scorpion ROMs sat under the Pentagon name and the BIOS
+        manager lost a section; the line the maintainer commented out went
+        with the rewrite.
+        """
+        old, new = "1" * 32, "2" * 32
+
+        def declared(name, label, md5):
+            return {
+                "name": name, "destination": f"fuse/{name}",
+                "native_system": "zxspectrum", "native_name": label,
+                "native_path": f"fuse/{name}", "md5": md5,
+                "core": "libretro/fuse", "required": False,
+            }
+
+        pentagon = "Pentagon 128/512/1024 (ZX Spectrum clone)"
+        scorpion = "Scorpion 256K (ZX Spectrum clone)"
+        scraped = {"systems": {"sinclair-zxspectrum": {
+            "native_id": "zxspectrum",
+            "files": [
+                declared("128p-0.rom", pentagon, "3" * 32),
+                declared("256s-0.rom", scorpion, old),
+            ],
+        }}}
+        truth = {"systems": {"sinclair-zxspectrum": {"files": [{
+            "name": "256s-0.rom", "path": "fuse/256s-0.rom", "md5": new,
+            "required": False, "_cores": ["fuse"],
+        }]}}}
+        original = (
+            '<?xml version="1.0" encoding="UTF-8"?>\n<biosList>\n'
+            f'  <system fullname="{pentagon}" platform="zxspectrum">\n'
+            f'    <bios path="fuse/128p-0.rom" md5="{"3" * 32}" core="libretro/fuse" />\n'
+            '    <!--bios path="fuse/gluck.rom" md5="x" core="libretro/fuse" /-->\n'
+            "  </system>\n"
+            f'  <system fullname="{scorpion}" platform="zxspectrum">\n'
+            f'    <bios path="fuse/256s-0.rom" md5="{old}" core="libretro/fuse" />\n'
+            "  </system>\n</biosList>\n"
+        )
+        systems, report = build_native_model(truth, scraped)
+        produced = discover_exporters()["recalbox"]().render(
+            systems, report, {"es_bios.xml": original}, None
+        )["es_bios.xml"]
+        self.assertIn('<!--bios path="fuse/gluck.rom"', produced)
+        blocks = {
+            s.get("fullname"): [b.get("md5") for b in s.iter("bios")]
+            for s in ET.fromstring(produced).iter("system")
+        }
+        self.assertEqual(blocks, {pentagon: ["3" * 32], scorpion: [new]})
+
     def test_a_truncated_export_is_reported(self):
         exporter, systems, _ = self._render()
         empty = {