Imports: replace distro inputs consistently and use ros2 names

Author: tdejager

README.md

@@ -70,6 +70,7 @@ combines the two, and the files next to each:
 
 - **`vinca.yaml`:**
   - The package selection (`packages_select_by_deps`) lives in `shared/` for every distribution; vinca skips packages a distribution's rosdistro doesn't have.
+  - `package_name_mode: new`: recipes and dependencies use `ros2-<pkg>` names only; no legacy alias packages are generated.
   - Exceptions, in `shared/` or a distribution: `packages_exclude` (not built, dropped from other packages' dependencies) and `packages_skip` (not built, dependents keep the dependency), each as plain names or under `- if: <condition>`. ROS packages mapped to conda-forge in `robostack.yaml` are skipped automatically.
   - `mutex_package`: name, `upper_bound` and the common `run_constraints` are shared. Each distribution sets its own `version`, and its `run_constraints` replace the shared constraint on the same package (e.g. jazzy's `libprotobuf 7.35.*`) or add new ones.
   - Other keys come from the distribution if it sets them, else from `shared/`. A distribution that differs from a shared setting just sets it.
@@ -152,5 +153,13 @@ pixi run rs <distro> render-pinning && pixi run sort && pixi run rs check
 ```
 
 The import leaves out what `shared/` already provides (shared package selections and entries, derived keys) and keeps comments. It writes `distro.yaml` from the old repository's `pixi.toml` and `vinca_pinning.yaml`. Run it again to pick up changes made in the old repository until that is archived.
+Imports validate and stage inputs before replacing the destination, and roll back
+if replacement fails. The importer owns `vinca.yaml`, `distro.yaml`, `ci.yaml`,
+`pkg_additional_info.yaml`, both rosdistro snapshot/additional-recipe files, and
+the whole `patch/` directory. Optional files removed upstream are removed on
+re-import. Unrelated local entries, including `work/`, `tests/`, and rendered pins,
+are preserved. `--distro` changes the destination identity; legacy package names
+are still converted using the source distribution's name.
+Modern package lists retain scalar and nested `if`/`then`/`else` branches, including
+empty branches; importing does not resolve away platform-specific selections.
 
-**Check:** re-importing lyrical from its repository reproduces `distros/lyrical/` exactly. The full linux-64 recipe sets of rolling, humble and lyrical generated here are byte-identical to those of their own repositories.

shared/vinca.yaml

@@ -1,6 +1,6 @@
 # vinca settings shared by every distribution; distros/<d>/vinca.yaml adds its own
 # (lists are combined, other keys from the distribution win). See tools/robostack.py.
-package_name_mode: both
+package_name_mode: new
 # relative to this file; each distribution adds its own (see distros/<d>/vinca.yaml)
 conda_index:
   - robostack.yaml

tools/import_distro.py

@@ -19,6 +19,12 @@
                            differ from shared/pinning/overrides.yaml (vinca_pinning.yaml)
 - ci.yaml                  reset to the defaults
 
+Reimports replace these importer-owned files and the entire patch/ tree, including
+local edits; absent optional files are removed. Other distro-local entries (such
+as work/, tests/, and rendered pinning) are retained without copying them. Inputs
+are staged and validated first; replacement uses same-filesystem renames with
+rollback on failure, rather than replacing a nonempty directory on Windows.
+
 Afterwards: `pixi run rs <distro> render-pinning`, `pixi run sort`, `pixi run rs check`.
 """
 
@@ -32,6 +38,8 @@
 import subprocess
 import sys
 import tarfile
+import tempfile
+import tomllib
 from pathlib import Path
 
 import yaml
@@ -40,6 +48,11 @@
 import robostack as rs
 
 DERIVED_KEYS = ("conda_index", "patch_dir", "rosdistro_snapshot", "rosdistro_additional_recipes", "skip_existing")
+OWNED_FILES = {
+    "vinca.yaml", "pkg_additional_info.yaml", "patch",
+    "rosdistro_snapshot.yaml", "rosdistro_additional_recipes.yaml",
+    "distro.yaml", "ci.yaml",
+}
 
 
 def _ry() -> YAML:
@@ -112,44 +125,108 @@ def __init__(self, repo: Path, ref: str | None):
     def text(self, path: str) -> str | None:
         if self.ref is None:
             p = self.repo / path
-            return p.read_text() if p.is_file() else None
-        out = subprocess.run(["git", "-C", str(self.repo), "show", f"{self.ref}:{path}"], capture_output=True, text=True)
-        return out.stdout if out.returncode == 0 else None
+            return p.read_text() if p.exists() else None
+        # A missing optional file is different from a failed Git operation.
+        listing = subprocess.run(
+            ["git", "-C", str(self.repo), "ls-tree", self.ref, "--", path],
+            capture_output=True, text=True, check=True,
+        )
+        if not listing.stdout:
+            return None
+        return subprocess.run(
+            ["git", "-C", str(self.repo), "show", f"{self.ref}:{path}"],
+            capture_output=True, text=True, check=True,
+        ).stdout
 
     def copy_dir(self, path: str, dest: Path) -> None:
         if self.ref is None:
             shutil.copytree(self.repo / path, dest / path, dirs_exist_ok=True)
             return
         data = subprocess.run(["git", "-C", str(self.repo), "archive", self.ref, path], capture_output=True, check=True).stdout
-        tarfile.open(fileobj=io.BytesIO(data)).extractall(dest, filter="data")
-
-
-def import_distro(src: Source, distro: str | None) -> str:
+        with tarfile.open(fileobj=io.BytesIO(data)) as archive:
+            archive.extractall(dest, filter="data")
+
+
+def _mapping(text: str | None, name: str, *, required: bool = False):
+    if text is None:
+        if required:
+            raise ValueError(f"Missing required input: {name}")
+        return {}
+    data = _ry().load(text)
+    if data is None and not required:
+        return {}
+    if not isinstance(data, dict):
+        raise ValueError(f"{name} must contain a YAML mapping")
+    return data
+
+
+def _distro_name(value: str | None) -> str:
+    if not isinstance(value, str) or not re.fullmatch(r"[a-z][a-z0-9_-]*", value):
+        raise ValueError(f"Invalid ros_distro name: {value!r}")
+    return value
+
+
+def _validate_package_entries(value, key: str) -> None:
+    """Validate modern package lists without evaluating away selector branches."""
+    if value is None or isinstance(value, str):
+        return
+    if isinstance(value, list):
+        for item in value:
+            _validate_package_entries(item, key)
+        return
+    if isinstance(value, dict) and "if" in value and "then" in value:
+        _validate_package_entries(value["then"], key)
+        _validate_package_entries(value.get("else"), key)
+        return
+    raise ValueError(f"vinca.yaml {key} must contain package names or if/then selectors")
+
+
+def _build_import(src: Source, distro: str, source_distro: str, vinca, dest: Path) -> None:
     ry = _ry()
-    vinca = ry.load(src.text("vinca.yaml"))
-    distro = distro or vinca["ros_distro"]
-    dest = rs.DISTROS / distro
-    dest.mkdir(parents=True, exist_ok=True)
-
+    for key in (*rs.LIST_KEYS, "packages_skip_by_deps", "packages_remove_from_deps"):
+        items = vinca.get(key)
+        if items is None:
+            continue
+        if not isinstance(items, list):
+            raise ValueError(f"vinca.yaml {key} must be a list")
+        if key in rs.LIST_KEYS:
+            _validate_package_entries(items, key)
+            continue
+        # Legacy exclusion conversion below expects flat if/then lists.
+        for item in items:
+            if isinstance(item, str):
+                continue
+            if (not isinstance(item, dict) or "if" not in item
+                    or not isinstance(item.get("then"), list)
+                    or any(not isinstance(name, str) for name in item["then"])):
+                raise ValueError(f"vinca.yaml {key} must contain package names or if/then lists")
+    snapshots = {
+        name: src.text(name)
+        for name in ("rosdistro_snapshot.yaml", "rosdistro_additional_recipes.yaml")
+    }
+    for name, text in snapshots.items():
+        data = _mapping(text, name, required=name == "rosdistro_snapshot.yaml")
+        if any(not isinstance(key, str) or not isinstance(value, dict) for key, value in data.items()):
+            raise ValueError(f"{name} must map package names to metadata mappings")
     # vinca.yaml: drop what shared/ and the tooling provide
-    shared = yaml.safe_load((rs.SHARED / "vinca.yaml").read_text()) or {}
+    shared = _mapping((rs.SHARED / "vinca.yaml").read_text(), "shared/vinca.yaml", required=True)
     for key in DERIVED_KEYS:
         vinca.pop(key, None)
     # on top of shared/vinca.yaml, with its own patches and snapshot
     for i, (key, value) in enumerate([
         ("extends", "../../shared/vinca.yaml"),
-        ("ros_distro", vinca.get("ros_distro", distro)),
+        ("ros_distro", distro),
         ("patch_dir", "patch"),
         ("rosdistro_snapshot", "rosdistro_snapshot.yaml"),
-        ("rosdistro_additional_recipes", "rosdistro_additional_recipes.yaml"),
+        ("rosdistro_additional_recipes",
+         "rosdistro_additional_recipes.yaml" if snapshots["rosdistro_additional_recipes.yaml"] is not None else None),
     ]):
         vinca.pop(key, None)
         vinca.insert(i, key, value)
-    mode = vinca.get("package_name_mode", "legacy")
-    if mode == shared.get("package_name_mode"):
-        vinca.pop("package_name_mode", None)
-    elif "package_name_mode" not in vinca:
-        vinca.insert(5, "package_name_mode", mode)
+    # This repository has cut over to ros2-* names; never import legacy aliases.
+    vinca.pop("package_name_mode", None)
+    if shared.get("package_name_mode") != "new":
+        vinca.insert(5, "package_name_mode", "new")
     _convert_exclusions(vinca)
     for key in rs.LIST_KEYS:
         common = {_key(i) for i in shared.get(key) or []}
@@ -163,12 +240,16 @@ def import_distro(src: Source, distro: str | None) -> str:
         ry.dump(vinca, fh)
 
     # as they are (patch/dependencies.yaml is filtered below)
-    shutil.rmtree(dest / "patch", ignore_errors=True)
     src.copy_dir("patch", dest)
-    for patch in (dest / "patch").glob(f"ros-{distro}-*.patch"):
-        patch.rename(patch.with_name("ros2-" + patch.name.removeprefix(f"ros-{distro}-")))
-    for name in ("rosdistro_snapshot.yaml", "rosdistro_additional_recipes.yaml"):
-        if (text := src.text(name)) is not None:
+    if not (dest / "patch").is_dir():
+        raise ValueError("Missing required input: patch/")
+    for patch in (dest / "patch").glob(f"ros-{source_distro}-*.patch"):
+        renamed = patch.with_name("ros2-" + patch.name.removeprefix(f"ros-{source_distro}-"))
+        if renamed.exists():
+            raise ValueError(f"Patch rename would overwrite {renamed.name}")
+        patch.rename(renamed)
+    for name, text in snapshots.items():
+        if text is not None:
             (dest / name).write_text(text)
 
     # per-package files: drop entries identical to shared/ (keeping build numbers)
@@ -179,9 +260,11 @@ def import_distro(src: Source, distro: str | None) -> str:
         text = src.text(name)
         if text is None:
             continue
-        text = re.sub(rf"\bros-{distro}-(?=[a-z0-9])", "ros2-", text)
-        data = ry.load(text) or {}
-        common = yaml.safe_load((rs.SHARED / shared_file).read_text()) or {}
+        text = re.sub(rf"\bros-{re.escape(source_distro)}-(?=[a-z0-9])", "ros2-", text)
+        data = _mapping(text, name)
+        if any(not isinstance(key, str) or not isinstance(value, dict) for key, value in data.items()):
+            raise ValueError(f"{name} must map package names to settings mappings")
+        common = _mapping((rs.SHARED / shared_file).read_text(), f"shared/{shared_file}")
         for key in list(data):
             if key not in common:
                 continue
@@ -199,12 +282,17 @@ def import_distro(src: Source, distro: str | None) -> str:
 
     # distro.yaml
     pixi = src.text("pixi.toml") or ""
+    tomllib.loads(pixi)
     settings: dict = {}
-    if m := re.search(r'upload\s*=\s*"rattler-build upload prefix -c (\S+)', pixi):
+    if m := re.search(r'upload\s*=\s*"rattler-build upload prefix -c ([^\s"]+)', pixi):
         settings.update(channel_name=m.group(1), upload_target="prefix")
-    elif m := re.search(r'upload\s*=\s*"rattler-build upload anaconda -o (\S+)', pixi):
+    elif m := re.search(r'upload\s*=\s*"rattler-build upload anaconda -o ([^\s"]+)', pixi):
         settings.update(channel_name=m.group(1), upload_target="anaconda")
-    pinning = yaml.safe_load(src.text("vinca_pinning.yaml") or "") or {}
+    pinning = _mapping(src.text("vinca_pinning.yaml"), "vinca_pinning.yaml")
+    if pinning.get("migrations") is not None and not isinstance(pinning["migrations"], list):
+        raise ValueError("vinca_pinning.yaml migrations must be a list")
+    if pinning.get("pinning_overrides") is not None and not isinstance(pinning["pinning_overrides"], dict):
+        raise ValueError("vinca_pinning.yaml pinning_overrides must be a mapping")
     if pinning.get("conda_forge_pinning_version"):
         settings["conda_forge_pinning_version"] = str(pinning["conda_forge_pinning_version"])
         settings["conda_forge_migrations"] = list(pinning.get("migrations") or [])
@@ -216,14 +304,79 @@ def import_distro(src: Source, distro: str | None) -> str:
     (dest / "distro.yaml").write_text(
         f"# Settings of ros-{distro} that differ from the other distributions\n"
         "# (everything else is shared, see the README).\n"
-        + yaml.safe_dump(settings, sort_keys=False, default_flow_style=None, width=100)
+        + yaml.safe_dump(json.loads(json.dumps(settings)), sort_keys=False, default_flow_style=None, width=100)
     )
-    (dest / "ci.yaml").write_text((rs.ROOT / "tools" / "ci_default.yaml").read_text())
+    ci = (rs.ROOT / "tools" / "ci_default.yaml").read_text()
+    _mapping(ci, "tools/ci_default.yaml", required=True)
+    (dest / "ci.yaml").write_text(ci)
+
+
+def _publish(staged: Path, dest: Path, backup: Path) -> None:
+    if not dest.exists():
+        staged.rename(dest)
+        return
+    preserved = []
+    dest.rename(backup)
+    try:
+        # Moving these entries preserves large work trees, symlinks and metadata.
+        for entry in backup.iterdir():
+            if entry.name not in OWNED_FILES:
+                entry.rename(staged / entry.name)
+                preserved.append(entry.name)
+        staged.rename(dest)
+    except BaseException:
+        try:
+            for name in reversed(preserved):
+                (staged / name).rename(backup / name)
+            backup.rename(dest)
+        except BaseException as rollback_error:
+            raise RuntimeError(
+                f"Import rollback failed; retained all recovery data in {staged.parent}"
+            ) from rollback_error
+        raise
+
+
+def import_distro(src: Source, distro: str | None) -> str:
+    vinca = _mapping(src.text("vinca.yaml"), "vinca.yaml", required=True)
+    source_distro = _distro_name(vinca.get("ros_distro"))
+    distro = _distro_name(distro if distro is not None else source_distro)
+    dest = rs.DISTROS / distro
+    if dest.is_symlink() or (dest.exists() and not dest.is_dir()):
+        raise ValueError(f"Import destination must be a real directory: {dest}")
+    for name in OWNED_FILES - {"patch"}:
+        if (dest / name).is_dir():
+            raise ValueError(f"Refusing to replace a directory with an imported file: {dest / name}")
+
+    rs.DISTROS.mkdir(parents=True, exist_ok=True)
+    transaction = Path(tempfile.mkdtemp(prefix=f".import-{distro}-", dir=rs.DISTROS))
+    staged, backup = transaction / "new", transaction / "previous"
+    staged.mkdir()
+    published = False
+    try:
+        _build_import(src, distro, source_distro, vinca, staged)
+        _publish(staged, dest, backup)
+        published = True
+    finally:
+        # Failed rollback must never cause cleanup to delete preserved user data.
+        if published or not backup.exists():
+            try:
+                shutil.rmtree(transaction)
+            except OSError as error:
+                print(f"Warning: could not remove import staging directory {transaction}: {error}", file=sys.stderr)
     return distro
 
 
 def main() -> int:
-    parser = argparse.ArgumentParser(description=__doc__.splitlines()[0])
+    parser = argparse.ArgumentParser(
+        description=__doc__.splitlines()[0],
+        epilog=(
+            "Reimports overwrite local edits to vinca.yaml, distro.yaml, ci.yaml, "
+            "snapshot/additional-recipes YAML, pkg_additional_info.yaml and all of patch/. "
+            "Missing optional inputs remove their old copies. Other distro-local files, "
+            "including work/, tests/ and rendered pinning, are preserved without copying. "
+            "Inputs are validated before replacement; failed replacement is rolled back."
+        ),
+    )
     parser.add_argument("repo", type=Path, help="checkout of RoboStack/ros-<distro>")
     parser.add_argument("--ref", help="git ref to import (default: the working tree)")
     parser.add_argument("--distro", help="default: ros_distro of its vinca.yaml")