Skip to content

Commit 765bf70

Browse files
docs+ci: clarify vinca's 3 exclusion mechanisms, add orphaned-patch check
vinca.yaml comments: packages_select_by_deps's if-wrapper (primary way to exclude a package's own recipe), packages_skip_by_deps (transitive pull-in only, does not stop direct selection), and packages_remove_from_deps (a third, independent way to fully exclude a package's own build via resolve_pkgname()/should_skip_pkg -- which also inseparably strips it from other recipes' deps) are easy to conflate; this came up repeatedly this session, including a mechanism that was initially mis-explained. Verified against the vinca revision this repo locks (pypi git+vinca lockfile entry). check_orphaned_platform_patches.py replicates vinca's exact patch-glob + add_package_name_variants() logic to catch a real bug class: a platform-specific patch silently never wired into any recipe because its filename uses a different name-prefix than another patch for the same package (setdefault() in add_package_name_variants is a no-op once either prefix already has its own entry) -- this is exactly what orphaned ros-jazzy-sick-scan-xd.osx.patch for months on ros-jazzy. Wired into `pixi run check-patches` via a new check-orphaned-patches task dependency so it runs on every PR. Zero hits currently on humble. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 9b45423 commit 765bf70

3 files changed

Lines changed: 225 additions & 1 deletion

File tree

check_orphaned_platform_patches.py

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
#!/usr/bin/env python3
2+
"""
3+
check_orphaned_platform_patches.py
4+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5+
Detect patch files in ``patch/`` that vinca will silently never wire
6+
into any recipe's ``patches:`` list.
7+
8+
Background
9+
----------
10+
vinca (see ``vinca/main.py`` around the ``patch_dir`` glob, and
11+
``vinca/utils.py::add_package_name_variants``) builds a dict keyed by
12+
the patch filename's prefix (everything before an optional
13+
``.osx``/``.win``/``.linux``/``.unix``/``.emscripten`` suffix), then
14+
cross-links name-prefix variants of the *same* logical package
15+
(``X`` <-> ``ros-X`` <-> ``ros2-X`` <-> ``ros-<distro>-X``) via
16+
``dict.setdefault()``.
17+
18+
``setdefault`` only fills in a key that is still *absent*. If a
19+
package has a plain patch under one prefix (say ``ros2-foo.patch``)
20+
and a platform-specific patch under a *different* prefix (say
21+
``ros-jazzy-foo.osx.patch``), both prefixes already exist as their own
22+
dict entries by the time the cross-link step runs, so the two never
23+
merge. Whichever prefix vinca does *not* resolve as the package's
24+
final conda name for a given recipe simply never appears in that
25+
recipe's ``patches:`` list -- with no error and no warning. This
26+
exact bug orphaned ``ros-jazzy-sick-scan-xd.osx.patch`` for months
27+
before it was renamed to ``ros2-sick-scan-xd.osx.patch`` (matching the
28+
prefix jazzy actually resolves sick_scan_xd's own patch under).
29+
30+
``check_patches_clean_apply.py`` does not catch this: it verifies that
31+
every patch file on disk applies cleanly to source, but never checks
32+
whether vinca's real name-resolution would actually attach that file
33+
to any package's generated recipe at all.
34+
35+
What this script does
36+
----------------------
37+
Replicates vinca's exact patch-dict-construction and
38+
``add_package_name_variants`` shortname-stripping logic (kept in sync
39+
with whatever revision this repo's ``pixi.toml`` pins vinca to -- if
40+
that mechanism ever changes upstream, re-check this script). It groups
41+
patch-file prefixes by their computed "shortname" and flags any group
42+
where more than one *distinct* literal prefix was actually used by a
43+
file on disk: only one of those prefixes can ever be the resolved
44+
package name for a given recipe, so content under the others is dead.
45+
46+
Exit code is non-zero (and the offending groups are printed) if any
47+
such collision is found.
48+
"""
49+
50+
from __future__ import annotations
51+
52+
import glob
53+
import os
54+
import re
55+
import sys
56+
from pathlib import Path
57+
58+
REPO_ROOT = Path(__file__).resolve().parent
59+
PATCH_DIR = REPO_ROOT / "patch"
60+
61+
_ROS_DISTRO_RE = re.compile(r"^ros_distro:\s*(\S+)\s*$", re.MULTILINE)
62+
63+
64+
def get_ros_distro() -> str:
65+
vinca_yaml = (REPO_ROOT / "vinca.yaml").read_text()
66+
match = _ROS_DISTRO_RE.search(vinca_yaml)
67+
if not match:
68+
print("Could not find 'ros_distro:' in vinca.yaml", file=sys.stderr)
69+
sys.exit(2)
70+
return match.group(1)
71+
72+
73+
def build_patches_dict(patch_dir: Path) -> dict[str, dict[str, list[str]]]:
74+
"""Mirrors the glob loop in vinca/main.py that builds vinca_conf['_patches']."""
75+
patches: dict[str, dict[str, list[str]]] = {}
76+
for x in sorted(glob.glob(os.path.join(str(patch_dir), "*.patch"))):
77+
splitted = os.path.basename(x).split(".")
78+
if splitted[0] not in patches:
79+
patches[splitted[0]] = {
80+
"any": [],
81+
"osx": [],
82+
"linux": [],
83+
"win": [],
84+
"emscripten": [],
85+
}
86+
if len(splitted) == 3:
87+
if splitted[1] in ("osx", "linux", "win", "emscripten"):
88+
patches[splitted[0]][splitted[1]].append(x)
89+
continue
90+
if splitted[1] == "unix":
91+
patches[splitted[0]]["linux"].append(x)
92+
patches[splitted[0]]["osx"].append(x)
93+
continue
94+
patches[splitted[0]]["any"].append(x)
95+
return patches
96+
97+
98+
def shortname_of(name: str, ros_distro: str) -> str:
99+
"""Mirrors the prefix-stripping in vinca/utils.py::add_package_name_variants."""
100+
legacy_prefix = f"ros-{ros_distro}-"
101+
if name.startswith(legacy_prefix):
102+
return name[len(legacy_prefix):]
103+
elif name.startswith("ros2-"):
104+
return name[len("ros2-"):]
105+
elif name.startswith("ros-"):
106+
return name[len("ros-"):]
107+
else:
108+
return name
109+
110+
111+
def main() -> int:
112+
ros_distro = get_ros_distro()
113+
patches = build_patches_dict(PATCH_DIR)
114+
115+
groups: dict[str, list[str]] = {}
116+
for prefix in patches:
117+
groups.setdefault(shortname_of(prefix, ros_distro), []).append(prefix)
118+
119+
collisions = {
120+
shortname: prefixes
121+
for shortname, prefixes in groups.items()
122+
if len(prefixes) > 1
123+
}
124+
125+
if not collisions:
126+
print(f"OK: no orphaned platform-specific patches ({len(patches)} patch-file prefixes scanned).")
127+
return 0
128+
129+
print(
130+
"ORPHANED PLATFORM PATCH RISK: the following packages have patch files "
131+
"spread across more than one name-prefix variant. vinca's "
132+
"add_package_name_variants() cross-links prefix variants via "
133+
"dict.setdefault(), which is a no-op once a variant already exists as its "
134+
"own entry -- so only ONE of the prefixes below will end up attached to "
135+
"the package's real generated recipe; any platform-specific patch under "
136+
"the others is silently never applied.\n",
137+
file=sys.stderr,
138+
)
139+
for shortname, prefixes in sorted(collisions.items()):
140+
print(f" {shortname}:", file=sys.stderr)
141+
for prefix in sorted(prefixes):
142+
files = [
143+
os.path.basename(f)
144+
for platform_files in patches[prefix].values()
145+
for f in platform_files
146+
]
147+
print(f" {prefix}: {', '.join(sorted(files))}", file=sys.stderr)
148+
print(
149+
"\nFix: rename the patch file(s) so every file for a given package shares "
150+
"the SAME name prefix (matching whichever prefix that package's own "
151+
"recipe.yaml actually resolves to -- check recipes/<pkg>/*/recipe.yaml's "
152+
"source.patches entries, or regenerate recipes locally and inspect).",
153+
file=sys.stderr,
154+
)
155+
return 1
156+
157+
158+
if __name__ == "__main__":
159+
sys.exit(main())

pixi.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ git = "*"
4141
[tasks]
4242
generate-recipes = { cmd = "vinca -m", depends-on = ["remove-recipes"] }
4343
generate-gha-workflows = { cmd = "vinca-gha --trigger-branch dummy_build_branch_as_it_is_unused -d ./recipes", depends-on = ["generate-recipes"] }
44-
check-patches = { cmd = "python check_patches_clean_apply.py", depends-on = ["generate-recipes"] }
44+
check-orphaned-patches = { cmd = "python check_orphaned_platform_patches.py", description = "Detect patch/ files vinca's add_package_name_variants() will never wire into any recipe because a same-package patch exists under a different name-prefix variant (see script docstring)." }
45+
check-patches = { cmd = "python check_patches_clean_apply.py", depends-on = ["generate-recipes", "check-orphaned-patches"] }
4546
check-deps = { cmd = "python check_dependency_compat.py", depends-on = ["generate-recipes"], description = "Solve one fake package containing every non-ROS dependency plus the mutex constraints to find pin conflicts before building anything. Run `python check_dependency_compat.py --stale` to list already-built artifacts that conflict with the current pins." }
4647
create_snapshot = { cmd = "vinca-snapshot -d humble -o rosdistro_snapshot.yaml" }
4748
upload = "rattler-build upload anaconda -o robostack-staging -a $ANACONDA_API_TOKEN"

vinca.yaml

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,58 @@ rosdistro_additional_recipes: rosdistro_additional_recipes.yaml
3131
skip_existing:
3232
- https://conda.anaconda.org/robostack-staging/
3333

34+
# --------------------------------------------------------------------------
35+
# Three related but DISTINCT ways to exclude a package -- read this before
36+
# adding to any of the three (vinca revision pinned in pixi.toml verified
37+
# against on 2026-09-06; re-check vinca/main.py + vinca/resolve.py if that
38+
# pin ever moves, in case the mechanism changes):
39+
#
40+
# 1. packages_select_by_deps, wrapped in `if: not <platform> then: [...]`
41+
# (see below). This is the PRIMARY way to exclude a package's own
42+
# recipe: vinca/main.py::get_selected_packages adds every name listed
43+
# here to `selected_packages` unconditionally, before any skip-list is
44+
# even consulted -- so simply never listing a package under the
45+
# current platform's branch here is what keeps its own recipe from
46+
# being generated.
47+
#
48+
# 2. packages_skip_by_deps (this list) only affects TRANSITIVE pull-in:
49+
# it's passed as `ignore_pkgs` to `distro.get_depends()`, so it stops
50+
# some OTHER selected package's dependency-closure walk from dragging
51+
# the excluded package back in. It does NOT stop a package listed
52+
# directly in packages_select_by_deps from being selected -- the two
53+
# lists answer different questions ("should X be pulled in as someone
54+
# else's dependency?" vs. "should X itself be built?").
55+
#
56+
# 3. packages_remove_from_deps (further below) is checked by
57+
# vinca/resolve.py::should_skip_pkg via resolve_pkgname(), which is
58+
# called BOTH when vinca resolves a package's OWN final name (in
59+
# generate_fat_source -- an empty result makes it `continue`, i.e.
60+
# skip generating that package's recipe entirely) AND when it resolves
61+
# OTHER packages' host/run dependency names. So this list is really a
62+
# THIRD, independent way to fully exclude a package's own build -- and
63+
# it ALSO strips the package's name from every other recipe's
64+
# dependency list as an inseparable side effect. If some other
65+
# selected package legitimately needs to depend on the package you're
66+
# excluding, packages_remove_from_deps is the WRONG tool (it removes
67+
# both); use packages_select_by_deps's if-wrapper (+ packages_skip_by_deps
68+
# if something's dependency closure would otherwise repull it) instead.
69+
#
70+
# Worked examples from real fixes in this repo:
71+
# - robot_localization (win-64): needed ALL THREE together, because
72+
# clearpath_control (itself unconditionally selected) transitively
73+
# depended on it -- (1) alone left it selected via clearpath_control's
74+
# closure. See commits 411b2f7d, 929734a9.
75+
# - rclc_examples (win-64): excluded via (3) ALONE -- it has NO
76+
# `if: not win` wrapper under packages_select_by_deps at all, and
77+
# isn't in packages_skip_by_deps either; it's only ever excluded
78+
# because it's listed under packages_remove_from_deps's win branch.
79+
# A win-64 compile bug in this package looked "never fixed" until it
80+
# was traced to the package being fully excluded by this mechanism.
81+
# - ros-jazzy mavlink/mavros/mavros_extras/libmavconn: (1) alone was
82+
# sufficient there -- confirmed via full recipe regeneration that no
83+
# other selected package referenced them, so (2)/(3) were never
84+
# needed for that case.
85+
# --------------------------------------------------------------------------
3486
packages_skip_by_deps:
3587

3688
- if: not linux
@@ -102,6 +154,12 @@ packages_skip_by_deps:
102154
# to attempt blindly; skip for now.
103155
- as2_behaviors_trajectory_generation
104156

157+
# See the "Three related but DISTINCT ways to exclude a package" block above
158+
# packages_skip_by_deps: this list (3) ALSO fully excludes a package's own
159+
# recipe (not just its appearance in others' deps) -- use packages_select_by_deps's
160+
# if-wrapper instead if you only want to stop this package's own build while
161+
# still letting another selected package depend on it.
162+
105163
packages_remove_from_deps:
106164

107165
- if: not linux
@@ -154,6 +212,12 @@ packages_remove_from_deps:
154212
then:
155213
- robot_localization
156214

215+
# See the "Three related but DISTINCT ways to exclude a package" block above
216+
# packages_skip_by_deps: an `if: not <platform> then: [pkg]` branch here is
217+
# mechanism (1), the primary way to exclude pkg's own recipe for that
218+
# platform. Add to packages_skip_by_deps too if something else's dependency
219+
# closure would otherwise still pull it back in.
220+
157221
packages_select_by_deps:
158222

159223
# These are the packages that are build on all platforms, including wasm32

0 commit comments

Comments
 (0)