From 81b1ee5e1939cd774415296c741c94c7cd874a8b Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 18:52:57 -0400 Subject: [PATCH] Decide target_map renames in one place, and list renamed exclusions exclude_targets holds CMake names. The diff applies target_map before deciding participation, so a Bazel target mapped to an excluded CMake name is skipped. excluded_summary, which reports what was skipped and why, checked the raw Bazel label instead, so that target appeared nowhere in `excluded.bazel`: skipped, but not on the record. A new helper, _bazel_renames, returns each Bazel label with the name the diff compares it under (today, the target_map entries whose label exists), and is the one place that decision is made. diff_models renames the Bazel views with it and, as before, also applies every explicit target_map entry to dependency references (a map entry can align an external dependency that has no target node). excluded_summary checks exclusion on the renamed name while still listing the target under its Bazel label. The per-side summaries later in this series use the same helper, so they count exactly the targets the diff compares, and the automatic executable alignment extends it rather than deciding separately. Co-Authored-By: Claude Opus 5.5 --- scripts/diff.py | 26 ++++++++++++++++++++++---- tests/test_engine.py | 26 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/scripts/diff.py b/scripts/diff.py index 6ffeb8f..9e927f0 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -220,6 +220,16 @@ def _apply_target_map(views: Dict[str, TargetView], return renamed +def _bazel_renames(a: CanonicalModel, b: CanonicalModel, + cfg: "MigrationConfig") -> Dict[str, str]: + """Bazel label -> the name it is compared under, for every Bazel target the + diff renames (target_map). The ONE place that decision is made: the + comparison and the summaries apply it the same way, so an exclude_targets + entry (a CMake name) reaches a renamed Bazel target everywhere.""" + return {bazel: cmake for cmake, bazel in cfg.target_map.items() + if bazel in b.targets} + + _LIBRARY_KINDS = {TargetKind.STATIC, TargetKind.SHARED, TargetKind.OBJECT, TargetKind.INTERFACE} @@ -298,7 +308,12 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, # Reconstruct raw actions into comparable views (TUs + link flags + deps), # build-system-specific interpretation keyed on each model's tag. a_views = reconstruct(a) - b_views = _apply_target_map(reconstruct(b), cfg.target_map) + renames = _bazel_renames(a, b, cfg) + # Target renames come from the shared decision. Explicit target_map entries + # also rename DEPENDENCY references, even with no target node to rename. + view_map = dict(cfg.target_map) + view_map.update({name: label for label, name in renames.items()}) + b_views = _apply_target_map(reconstruct(b), view_map) a_names = _participating(a_views, cfg) b_names = _participating(b_views, cfg) @@ -481,15 +496,18 @@ def excluded_summary(a: CanonicalModel, b: CanonicalModel, if cfg.include_tests: participating.add(TargetRole.TEST) - def by_reason(model: CanonicalModel) -> dict: + def by_reason(model: CanonicalModel, renames: Dict[str, str]) -> dict: + # exclusion is decided on the name the diff compares under; the entry + # lists the target's own name (a Bazel label keeps its label) out: Dict[str, List[str]] = {} for name, t in sorted(reconstruct(model).items()): - if cfg.target_excluded(name): + if cfg.target_excluded(renames.get(name, name)): out.setdefault("config_excluded", []).append(name) elif t.role not in participating: out.setdefault(t.role.value, []).append(name) return out - return {"cmake": by_reason(a), "bazel": by_reason(b)} + return {"cmake": by_reason(a, {}), + "bazel": by_reason(b, _bazel_renames(a, b, cfg))} def summarize(discs: List[Discrepancy], diff --git a/tests/test_engine.py b/tests/test_engine.py index 3d28f8c..f21a261 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -600,6 +600,32 @@ def test_exclude_targets_suppresses_structural_diffs(): assert "benchmark" in res["excluded"]["cmake"]["config_excluded"] +def test_renamed_bazel_target_is_listed_as_excluded(): + # target_map renames pkg:app to app; exclude_targets names app. The diff + # skips it, so the excluded summary must list it on the Bazel side too. + a = _bs(CanonicalModel(), is_bazel=False); a.add(_exe("app", [], False)) + b = _bs(CanonicalModel(), is_bazel=True); b.add(_exe("pkg:app", [], True)) + cfg = MigrationConfig(target_map={"app": "pkg:app"}, exclude_targets={"app"}) + res = summarize(diff_models(a, b, cfg), a, b, cfg) + assert res["discrepancies"] == [], res + assert res["excluded"]["bazel"] == {"config_excluded": ["pkg:app"]}, res + assert res["excluded"]["cmake"] == {"config_excluded": ["app"]}, res + + +def test_target_map_still_renames_a_dependency_with_no_target_node(): + # An external dep spelled foo by CMake and pkg:foo by Bazel, aligned by a + # target_map entry; no pkg:foo target exists. The entry must still rename + # the dependency reference, as it always has. + def m(dep, is_bazel): + model = _bs(CanonicalModel(), is_bazel) + model.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, + deps=[Dependency(dep, external=True)], + actions=[tu_from_raw("src/a.cpp", [], is_bazel)])) + return model + a, b = m("foo", False), m("pkg:foo", True) + assert diff_models(a, b, MigrationConfig(target_map={"foo": "pkg:foo"})) == [] + + def test_config_loads_all_fields_from_json(): # The skill drives the diff via a any2bazel.json on disk, so loading must # populate every lever. Guards against a field added to the dataclass but