From 4f689d1572393f19ca2f15cf31642cbffbf65713 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:22:00 -0400 Subject: [PATCH 01/11] Validate that bazel_args holds flags, not a command bazel_args is meant for the flags that make aquery mirror the real build (--config=release, -c opt). Anything that consumes it splices it into a bazel command line, but nothing checked its shape, and the SKILL.md text sits next to a literal `bazel aquery ...` line that is easy to paste in. Measured in any2bazel-lab: on a QGIS migration the agent recorded the whole aquery command as bazel_args. Its own diff never read the field, so the migration looked converged, while a consumer that spliced the field into `bazel aquery` failed on a second query expression and produced no parity result. config.load now validates the list and raises ValueError naming the first bad entry; diff.py turns that into a one-line error. An entry must be a flag, or the value of the flag just before it when that flag has no '='. So the well-formed ["-c", "opt"] is accepted, which a simpler "every entry starts with -" rule would reject. SKILL.md shows one right and one wrong example. In the lab's later QGIS run with this change, every bazel_args entry was a valid flag. Lab-Proposal: P22 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 9 ++++--- scripts/config.py | 41 +++++++++++++++++++++++++++++--- scripts/diff.py | 5 +++- tests/test_engine.py | 56 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 7 deletions(-) diff --git a/SKILL.md b/SKILL.md index 62ad5c0..9010834 100644 --- a/SKILL.md +++ b/SKILL.md @@ -112,9 +112,12 @@ whether its frontend reads it: ``` Fields: -- **`bazel_args`** — the extra args aquery must run with to mirror the real - build (see [Extract the Bazel action graph](#extract-bazel-graph)). Recorded so - the comparison is reproducible; read by the operator, not the diff. +- **`bazel_args`** — the extra **flags** aquery must run with to mirror the + real build (see [Extract the Bazel action graph](#extract-bazel-graph)), e.g. + `["--config=release"]` or `["-c", "opt"]`; never a command + (`["aquery", "mnemonic(...)", ...]` is wrong). Recorded so the comparison is + reproducible; `diff.py` refuses an entry that is neither a flag nor a flag's + value. - **`target_map`** — `reference_name → bazel_name` for intentionally renamed **executables**. Bazel targets are keyed by full label, so the value is usually `:name` (e.g. `"bssl": ":bssl"`). Libraries need no mapping. diff --git a/scripts/config.py b/scripts/config.py index fd17cbd..4d2cbb2 100644 --- a/scripts/config.py +++ b/scripts/config.py @@ -100,9 +100,12 @@ class MigrationConfig: # Unlike `ignore` (flags/defines), this is the only lever for missing_tu / # missing_target on whole subtrees. Each entry is a reviewable decision. exclude_targets: Set[str] = field(default_factory=set) - # The bazel aquery invocation this migration is compared against, recorded + # The FLAGS appended to `bazel build` / `bazel aquery` so the Bazel side + # mirrors the real build (e.g. "--config=release", or "-c" "opt"), recorded # so the comparison is reproducible. Read by the skill/operator, not by the - # diff itself (the diff consumes the already-extracted model). + # diff itself (the diff consumes the already-extracted model); validated at + # load (see validate_bazel_args) because anything that consumes it splices + # it into a bazel command line. bazel_args: tuple = () # Opt-in: also diff TEST targets (role=test). OFF by default because test # diffing requires BOTH models to be extracted with tests enabled and the @@ -161,6 +164,38 @@ def target_excluded(self, name: str) -> bool: return name in self.exclude_targets +def validate_bazel_args(args) -> tuple: + """Check that `bazel_args` is a list of flags, and return it as a tuple. + + Each entry must be a flag (starts with '-') or the value of the flag just + before it, when that flag has no '=' (so ["-c", "opt"] and + ["--config", "release"] are fine). Anything else -- most often a whole + command such as ["aquery", "mnemonic(...)", "--output=jsonproto"] -- is + refused, naming the entry: spliced into `bazel aquery ` it + becomes a second query expression and breaks the command. A boolean flag + followed by a stray word slips through; the check is a guard against the + command-in-the-field mistake, not a Bazel option parser.""" + if isinstance(args, str) or not isinstance(args, (list, tuple)): + raise ValueError( + f"bazel_args must be a list of flags, got {type(args).__name__}") + value_allowed = False + for a in args: + if not isinstance(a, str): + raise ValueError(f"bazel_args entry {a!r} is not a string") + if a.startswith("-"): + value_allowed = "=" not in a + continue + if value_allowed: + value_allowed = False + continue + raise ValueError( + f"bazel_args entry {a!r} is not a flag or a flag's value. " + "bazel_args holds only the flags appended to `bazel build` / " + "`bazel aquery` to mirror the real build (e.g. [\"--config=release\"] " + "or [\"-c\", \"opt\"]), never the command or query itself.") + return tuple(args) + + def load(path: str) -> MigrationConfig: """Load config from an explicit file path, or return empty if absent.""" if not path or not os.path.isfile(path): @@ -182,7 +217,7 @@ def load(path: str) -> MigrationConfig: source_map=tuple( (e["from"], e["to"]) for e in ig.get("source_map", [])), exclude_targets=set(obj.get("exclude_targets", [])), - bazel_args=tuple(obj.get("bazel_args", [])), + bazel_args=validate_bazel_args(obj.get("bazel_args", [])), include_tests=bool(obj.get("include_tests", False)), ) diff --git a/scripts/diff.py b/scripts/diff.py index d602dcb..6ffeb8f 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -519,5 +519,8 @@ def summarize(discs: List[Discrepancy], from serialize import load_model a = load_model(sys.argv[1]) b = load_model(sys.argv[2]) - cfg = config_mod.load(sys.argv[3]) if len(sys.argv) > 3 else MigrationConfig() + try: + cfg = config_mod.load(sys.argv[3]) if len(sys.argv) > 3 else MigrationConfig() + except ValueError as e: + sys.exit(f"diff.py: {sys.argv[3]}: {e}") print(json.dumps(summarize(diff_models(a, b, cfg), a, b, cfg), indent=2)) diff --git a/tests/test_engine.py b/tests/test_engine.py index f50ef31..3d28f8c 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -640,6 +640,62 @@ def test_config_loads_all_fields_from_json(): assert cfg.map_include("external/absl+/base") == "@absl/base" # rewrite +def test_bazel_args_accepts_flags_and_flag_values(): + import config as config_mod + for ok in ([], ["--config=release"], ["-c", "opt"], + ["--config", "release", "--copt=-O3"], + ["--compilation_mode=opt", "--cxxopt=-std=c++17", "--force_pic"]): + assert config_mod.validate_bazel_args(ok) == tuple(ok), ok + + +def test_bazel_args_rejects_a_command_line(): + # A whole aquery command in the field (seen in practice): spliced into + # `bazel aquery ` it yields two query expressions. + import config as config_mod + bad_lists = ( + ["aquery", 'mnemonic("CppCompile|CppLink|CppArchive", //...)', + "--output=jsonproto"], + ["--config=release", "build"], # '=' flag takes no separate value + ["-c", "opt", "//..."], # one value per flag + "--config=release", # a string, not a list + ) + for bad in bad_lists: + try: + config_mod.validate_bazel_args(bad) + except ValueError as e: + if isinstance(bad, list): + assert repr(bad[-1] if bad[0].startswith("-") else bad[0]) in str(e), e + else: + raise AssertionError(f"accepted {bad!r}") + + +def test_bazel_args_rejects_a_non_string_entry(): + import config as config_mod + try: + config_mod.validate_bazel_args(["-c", 3]) + except ValueError as e: + assert "3" in str(e) and "not a string" in str(e), e + else: + raise AssertionError("accepted a non-string entry") + + +def test_config_load_refuses_malformed_bazel_args(): + import json + import tempfile + import config as config_mod + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f: + json.dump({"bazel_args": ["aquery", "//...", "--output=jsonproto"]}, f) + path = f.name + try: + config_mod.load(path) + except ValueError as e: + assert "'aquery'" in str(e), e + else: + raise AssertionError("malformed bazel_args loaded without error") + finally: + os.unlink(path) + + def _test_models(cmake_test_srcs, bazel_test_srcs, n_cmake_bins=1, n_bazel_bins=1): """Build a/b models with a shared production lib plus test targets carrying the given sources, split across n_*_bins test executables.""" From 81b1ee5e1939cd774415296c741c94c7cd874a8b Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 18:52:57 -0400 Subject: [PATCH 02/11] 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 From 7bf197eec149a2c9db9c1a6d1ab69b2fe4c6cc5e Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:23:01 -0400 Subject: [PATCH 03/11] Compare every variant of a multi-variant source, deterministically _union_tus pooled library TUs into one source-keyed map with dict.setdefault while iterating a set of target names. When one source is compiled into several libraries with different flags (a full and a lite build of the same file, a hardened variant), only one variant per side was compared, and which one depended on set iteration order, that is, on PYTHONHASHSEED. The verdict was not reproducible. Measured in any2bazel-lab: the same MySQL Server models gave 4,715 to 4,817 errors over 10 hash seeds, a different error set each time, and the final models of a Blender migration converged under 2 of 8 seeds. Projects without multi-variant sources were unaffected (50 recorded verdicts identical under 20 seeds). Pooling now keeps every distinct variant per source, visiting targets in sorted order. Each CMake variant is compared with its best-matching Bazel variant: fewest errors, then warnings, then the smallest symmetric difference of the two TUs' define, flag and include sets, then a stable text order. Every CMake variant must be matched by some Bazel variant and nothing is suppressed; an extra Bazel-only variant is tolerated, like an extra Bazel flag. The distance term keeps a tie from choosing a variant that only adds flags (-w, -Wno-error=...) over an exact one. A source with one variant per side takes exactly the old path, and on every single-variant verdict the lab has recorded, the result is unchanged. Executables and the opt-in test union use the same pairing, and the Java source union is visited in sorted order too. One consequence for source_map users: a token that collapses several CMake TUs (an AUTOMOC bundle, say) now yields one comparison, and so possibly one discrepancy, per distinct CMake TU, so the worklist can grow. The source_map comment in config.py says so. In the lab's later QGIS run with this change, the differ gave the same verdict under three seeds. Lab-Proposal: P14 Co-Authored-By: Claude Opus 5.5 --- scripts/config.py | 7 ++-- scripts/diff.py | 88 +++++++++++++++++++++++++++++++++------ tests/test_engine.py | 99 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 15 deletions(-) diff --git a/scripts/config.py b/scripts/config.py index 4d2cbb2..ddf841e 100644 --- a/scripts/config.py +++ b/scripts/config.py @@ -88,9 +88,10 @@ class MigrationConfig: # bundles every moc_*.cpp into one mocs_compilation.cpp TU, while Bazel # compiles each moc_*.cpp separately. Mapping both spellings to one token # (the CMake bundle path AND the Bazel moc/ dir -> "@moc") reconciles them: - # the pooled TU maps collapse to the shared token, so presence matches and - # the representative TU's flags are still compared (extra Bazel defines stay - # benign WARN under the asymmetric-subset rule). Only ever use for codegen + # the pooled TU maps collapse to the shared token, so presence matches, and + # every mapped TU is still compared against its best-matching counterpart + # (diff._diff_variants; extra Bazel defines stay benign WARN under the + # asymmetric-subset rule). Only ever use for codegen # whose per-TU flags are uniform; never to paper over a real missing source. # Unlike include_map, the token REPLACES the whole path (the remainder is # dropped) so N generated files collapse to 1 -- see map_source(). diff --git a/scripts/diff.py b/scripts/diff.py index 9e927f0..20630ef 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -234,17 +234,79 @@ def _bazel_renames(a: CanonicalModel, b: CanonicalModel, TargetKind.INTERFACE} +def _add_variants(out: Dict[str, List[TranslationUnit]], tus, + cfg: "MigrationConfig") -> None: + """Add TUs to a source-keyed map of DISTINCT variants (see _union_tus).""" + for tu in tus: + variants = out.setdefault(cfg.map_source(tu.key()), []) + if tu not in variants: + variants.append(tu) + + def _union_tus(views: Dict[str, TargetView], names, - cfg: "MigrationConfig") -> Dict[str, TranslationUnit]: + cfg: "MigrationConfig") -> Dict[str, List[TranslationUnit]]: """Pool TUs of the given target views into one source-keyed map (the TU-SET comparison): grouping/renames/fold-ins don't matter -- every compiled source lands in one flat map keyed by repo-relative path. Keys are run through cfg.map_source so generated-source grouping asymmetries (e.g. CMake's single - AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token.""" - out: Dict[str, TranslationUnit] = {} - for n in names: - for tu in views[n].tus: - out.setdefault(cfg.map_source(tu.key()), tu) + AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token. + + Each key holds every DISTINCT variant of that source: one source compiled + into several libraries with different flags (a lite and a full build of the + same file, a hardened variant) is several TUs, and _diff_variants compares + all of them. Names are visited in sorted order so the result never depends + on set iteration order (i.e. on PYTHONHASHSEED).""" + out: Dict[str, List[TranslationUnit]] = {} + for n in sorted(names): + _add_variants(out, views[n].tus, cfg) + return out + + +def _tu_distance(a: TranslationUnit, b: TranslationUnit, + cfg: "MigrationConfig") -> int: + """Size of the symmetric difference of two TUs' define, flag and include + sets, after the reviewer's ignores. Unlike the discrepancies, this counts + flags only one side has, so an exact variant beats a noisy one (-w, + -Wno-error=...) that happens to produce the same discrepancies.""" + ad = {d for d in a.defines if not cfg.define_ignored(d)} + bd = {d for d in b.defines if not cfg.define_ignored(d)} + af = {f for f in a.flags if not cfg.flag_ignored(f)} + bf = {f for f in b.flags if not cfg.flag_ignored(f)} + ai = set(_norm_includes(a.includes, cfg)) + bi = set(_norm_includes(b.includes, cfg)) + return len(ad ^ bd) + len(af ^ bf) + len(ai ^ bi) + + +def _rank(discs: List[Discrepancy], distance: int) -> tuple: + """Order candidate pairings: fewest errors, then fewest warnings, then the + smallest distance between the two TUs (see _tu_distance), then a stable + text form so ties never depend on iteration order.""" + errors = sum(1 for d in discs if d.severity == Severity.ERROR.value) + return (errors, len(discs) - errors, distance, + json.dumps([asdict(d) for d in discs], sort_keys=True)) + + +def _best_match(target: str, a: TranslationUnit, b_vs: List[TranslationUnit], + cfg: "MigrationConfig"): + """(discrepancies, chosen Bazel TU) for CMake TU `a` against its + best-ranked Bazel variant (see _rank).""" + return min(((_diff_tu(target, a, b, cfg), b) for b in b_vs), + key=lambda pair: _rank(pair[0], _tu_distance(a, pair[1], cfg))) + + +def _diff_variants(target: str, a_vs: List[TranslationUnit], + b_vs: List[TranslationUnit], + cfg: "MigrationConfig") -> List[Discrepancy]: + """Compare every CMake variant of one source against its BEST-matching Bazel + variant (fewest errors, then warnings, then distance; see _rank). + Every CMake variant must be matched by some Bazel variant, so nothing is + suppressed: a variant Bazel lacks shows up as that variant's discrepancies + against its closest match. An extra Bazel-only variant is tolerated, like + any extra Bazel flag. With one variant per side this is exactly one + _diff_tu call.""" + out: List[Discrepancy] = [] + for a in a_vs: + out.extend(_best_match(target, a, b_vs, cfg)[0]) return out @@ -272,7 +334,7 @@ def _union_java_sources(views: Dict[str, TargetView], names) -> Dict[str, str]: (Maven's 2 groups vs Bazel's 1) is irrelevant; the SOURCE SET is what must match. First occurrence wins for the displayed original path.""" out: Dict[str, str] = {} - for n in names: + for n in sorted(names): for g in views[n].compile_groups: for src in g.sources: out.setdefault(_java_src_key(src), src) @@ -345,7 +407,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value, "", "source compiled in bazel but not cmake", tu=src)) for src in sorted(set(a_union) & set(b_union)): - out.extend(_diff_tu("", a_union[src], b_union[src], cfg)) + out.extend(_diff_variants("", a_union[src], b_union[src], cfg)) # ---- executables: identity-aligned, own TUs + own flags ---------------- for name in sorted(a_exes - b_exes): @@ -358,8 +420,10 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, ta, tb = a_views[name], b_views[name] # TU keys go through cfg.map_source (see _union_tus) so codegen grouping # asymmetries collapse to a shared token on both sides. - amap = {cfg.map_source(k): v for k, v in ta.tu_map().items()} - bmap = {cfg.map_source(k): v for k, v in tb.tu_map().items()} + amap: Dict[str, List[TranslationUnit]] = {} + bmap: Dict[str, List[TranslationUnit]] = {} + _add_variants(amap, ta.tus, cfg) + _add_variants(bmap, tb.tus, cfg) # Presence is judged against the whole other side (a_all/b_all), not just # this exe's own TUs: a source this exe compiles may live in a library on # the other side (or vice versa) -- that's a grouping difference, not a @@ -372,7 +436,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value, name, "source compiled in bazel but not cmake", tu=src)) for src in sorted(set(amap) & set(bmap)): - out.extend(_diff_tu(name, amap[src], bmap[src], cfg)) + out.extend(_diff_variants(name, amap[src], bmap[src], cfg)) # link flags: asymmetric subset, same policy as compile flags -- every # CMake link flag must be present on the Bazel side; extra Bazel link # flags tolerated (toolchain link noise already stripped). Reviewer @@ -468,7 +532,7 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], out.append(Discrepancy(Kind.EXTRA_TEST_TU.value, Severity.WARN.value, "", "test source compiled in bazel but not cmake", tu=src)) for src in sorted(set(a_union) & set(b_union)): - out.extend(_diff_tu("", a_union[src], b_union[src], cfg)) + out.extend(_diff_variants("", a_union[src], b_union[src], cfg)) # 2. test-binary existence by count if len(a_tests) != len(b_tests): diff --git a/tests/test_engine.py b/tests/test_engine.py index f21a261..95b3b5d 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -417,6 +417,105 @@ def test_source_map_reconciles_codegen_bundling_asymmetry(): assert summarize(diff_models(a, b, cfg))["converged"] +def _variant_models(cmake_libs, bazel_libs): + """One source (src/x.cc) compiled into several libraries per side, each + library a {name: [defines]} entry -- a multi-variant source.""" + def build(libs, is_bazel): + m = _bs(CanonicalModel(), is_bazel) + for name, defs in libs.items(): + raw = ["-D" + d for d in defs] + ["-std=c++17"] + m.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/x.cc", raw, is_bazel)])) + return m + return build(cmake_libs, False), build(bazel_libs, True) + + +def test_each_cmake_variant_is_paired_with_its_best_bazel_variant(): + # libprotobuf / libprotobuf-lite style: the same source, two flag sets, on + # both sides. Every variant has an exact counterpart -> converged. + a, b = _variant_models({"proto": ["FULL"], "proto_lite": ["LITE"]}, + {"pkg:proto": ["FULL"], "pkg:proto_lite": ["LITE"]}) + res = summarize(diff_models(a, b)) + assert res["converged"], res + + +def test_a_cmake_variant_missing_on_bazel_is_caught(): + a, b = _variant_models({"proto": ["FULL"], "proto_lite": ["LITE"]}, + {"pkg:proto_lite": ["LITE"]}) + discs = diff_models(a, b) + errs = [d for d in discs if d.severity == Severity.ERROR.value] + assert len(errs) == 1 and errs[0].kind == "defines_diff", errs + assert errs[0].cmake_only == ["FULL"], errs + + +def test_an_extra_bazel_variant_is_tolerated(): + a, b = _variant_models({"proto_lite": ["LITE"]}, + {"pkg:proto": ["FULL"], "pkg:proto_lite": ["LITE"]}) + discs = diff_models(a, b) + assert summarize(discs)["converged"], discs + assert discs == [], discs # the matching variant was chosen, not FULL + + +def test_tied_pairings_prefer_the_smaller_difference(): + # Three CMake bundles collapse to one source token via source_map, each + # with its own define; Bazel has a LIB0 TU and a TU with no define. LIB1 + # and LIB2 tie on errors against both; the pairing must be the one that + # does not report LIB0 (another variant's define) as bazel_only. + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw(f"gen/a{i}.cc", [f"-DLIB{i}"], False) for i in range(3)])) + b.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw("gen/b0.cc", ["-DLIB0"], True), + tu_from_raw("gen/b1.cc", [], True)])) + discs = diff_models(a, b, MigrationConfig(source_map=(("gen/", "@gen"),))) + assert [(d.cmake_only, d.bazel_only) for d in discs] == \ + [(["LIB1"], []), (["LIB2"], [])], discs + + +def test_tied_pairings_prefer_the_exact_variant_over_a_noisy_one(): + # Both Bazel variants produce no discrepancy against CMake {-std=c++17}; + # the one that only ADDS flags (-w, -Wno-error=foo) must not be chosen. + from diff import _best_match + from reconstruct import reconstruct + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x.cc", ["-std=c++17"], False)])) + for name, raw in (("a:noisy", ["-std=c++17", "-w", "-Wno-error=foo"]), + ("b:exact", ["-std=c++17"])): + b.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x.cc", raw, True)])) + ta = reconstruct(a)["l"].tus[0] + b_vs = [reconstruct(b)[n].tus[0] for n in ("a:noisy", "b:exact")] + discs, chosen = _best_match("", ta, b_vs, MigrationConfig()) + assert discs == [] and chosen.flags == ("-std=c++17",), chosen + + +def test_multi_variant_verdict_does_not_depend_on_hash_seed(): + # Regression: libraries used to be pooled with dict.setdefault while + # iterating a SET of names, so which variant got compared -- and the + # verdict -- changed with PYTHONHASHSEED. + import json + import subprocess + import tempfile + from serialize import dump_model + libs = {f"lib{i}": (["LITE"] if i % 2 else ["FULL"]) for i in range(8)} + a, b = _variant_models(libs, {"pkg:" + k: v for k, v in libs.items()}) + b.targets.pop("pkg:lib0") # FULL is still built by pkg:lib2, pkg:lib4, ... + with tempfile.TemporaryDirectory() as d: + pa, pb = os.path.join(d, "a.json"), os.path.join(d, "b.json") + dump_model(a, pa) + dump_model(b, pb) + script = os.path.join(os.path.dirname(__file__), "..", "scripts", "diff.py") + outs = set() + for seed in range(8): + env = dict(os.environ, PYTHONHASHSEED=str(seed)) + out = subprocess.run([sys.executable, script, pa, pb], env=env, + capture_output=True, text=True, check=True).stdout + outs.add(out) + assert len(outs) == 1, "diff output varies with PYTHONHASHSEED" + assert json.loads(outs.pop())["converged"] + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error. From 67b117f3c401947014de0b4ce8a400a7b9ee714e Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:24:24 -0400 Subject: [PATCH 04/11] Report Bazel-only flags beside the verdict The flag comparison is an asymmetric subset: a CMake flag missing on the Bazel side is an error, and a flag only Bazel has is tolerated. That is the right policy, since Bazel's toolchain injects flags no CMake build has, but nothing in the output said so. "0 errors" reads as "both builds compile the same way" when it means "no CMake flag is missing on the Bazel side", and a flag the Bazel side added appeared nowhere unless that TU also had a real discrepancy. Measured in any2bazel-lab: one migration's Bazel side carried 15 distinct compiler warning flags against CMake's one, including ten -Wno-error= demotions the agent added to get past -Werror, and the diff reported 0 errors and 0 warnings with an empty ignore block. Those demotions were justified; an unjustified -w would have looked the same. diff_models takes an optional DiffNotes collector. For every compared TU pair (the chosen variant pair) it counts the compile flags only Bazel has, after the ignore lists and minus the toolchain defaults already hidden from bazel_only; for every aligned executable, it does the same for link flags. summarize() reports them as "bazel_only_flags" (flag -> compared TU pairs, or executables) with a note that they are not compared; triage.py prints a one-line summary with the top entries; SKILL.md says so where diff.json is described. Severity and `converged` are unchanged. Not done: raising warning-control flags the Bazel side adds (-w, -Wno-*, -Wno-error=*) to WARN. That changes what the tool counts, so it is left for the maintainers to decide. In the lab's later graphicsgems run with this change, the agent's report named its Bazel-only flags; the earlier run's report had not. Lab-Proposal: P17 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 8 +++- scripts/diff.py | 88 +++++++++++++++++++++++++++++++++++--------- scripts/triage.py | 13 +++++++ tests/test_engine.py | 73 +++++++++++++++++++++++++++++++++++- tests/test_triage.py | 16 ++++++++ 5 files changed, 178 insertions(+), 20 deletions(-) diff --git a/SKILL.md b/SKILL.md index 9010834..f78cb02 100644 --- a/SKILL.md +++ b/SKILL.md @@ -58,7 +58,8 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ Done when both stages converge. The comparison is **asymmetric**: every correctness-relevant reference flag must be present on the Bazel side; extra - Bazel flags are tolerated. No artifact/symbol diff or test execution yet. + Bazel flags are tolerated (listed in `diff.json` as `bazel_only_flags`). No + artifact/symbol diff or test execution yet. ### How targets are matched @@ -286,7 +287,10 @@ python3 scripts/diff.py model..json model.bazel.json \ ``` `diff.json` has `converged` (⇔ zero `error` discrepancies), a `discrepancies` worklist (each with `kind`, `severity`, `target`, `tu`, `cmake_only`, -`bazel_only`), and an `excluded` map of non-participating targets by role. +`bazel_only`), an `excluded` map of non-participating targets by role, and +`bazel_only_flags`: flags only the Bazel side has. Those are never a +discrepancy (the comparison is one-directional), so a `-w` or `-Wno-error=` +you add converges silently; review the list before reporting. `cmake_only` is a historical name: for every frontend it means "present in the reference, absent in Bazel". Synthetic target names ``, `` (and `` when diff --git a/scripts/diff.py b/scripts/diff.py index 20630ef..02f7393 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -32,7 +32,8 @@ from __future__ import annotations import json -from dataclasses import asdict, dataclass +from collections import Counter +from dataclasses import asdict, dataclass, field from enum import Enum from typing import Dict, List, Optional @@ -91,6 +92,22 @@ class Discrepancy: bazel_only: Optional[list] = None # present in B, absent in A -> usually ok +@dataclass +class DiffNotes: + """What the diff observes but never counts as a discrepancy, reported in the + summary beside the verdict. Never affects `converged`. + + The flag comparison is one-directional (a CMake flag missing on Bazel is an + error; a flag only Bazel has is tolerated), so a converged diff can hide + flags the Bazel side ADDED -- a `-w` or `-Wno-error=` is never an error. + These counters make them visible: flag -> number of compared TU pairs + (compile; one pair per CMake TU variant, see _diff_variants) or aligned + executables (link) where Bazel has it and CMake does not, after the + reviewer's ignore lists and the toolchain's tolerated defaults.""" + bazel_only_flags: Counter = field(default_factory=Counter) + bazel_only_link_flags: Counter = field(default_factory=Counter) + + def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, cfg: "MigrationConfig") -> List[Discrepancy]: out: List[Discrepancy] = [] @@ -154,13 +171,17 @@ def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, def _diff_link_flags(name: str, ta: "TargetView", tb: "TargetView", - cfg: "MigrationConfig") -> List[Discrepancy]: + cfg: "MigrationConfig", + notes: Optional[DiffNotes] = None) -> List[Discrepancy]: """Asymmetric link-flag subset for one aligned executable/shared-lib: every CMake link flag must be present on the Bazel side; extra Bazel link flags - are tolerated. Reviewer-approved differences (cfg.link_flag_ignored) are - dropped from both sides first. Mirrors the compile-flag check.""" + are tolerated (and recorded in `notes`). Reviewer-approved differences + (cfg.link_flag_ignored) are dropped from both sides first. Mirrors the + compile-flag check.""" a_lf = {f for f in ta.link_flags if not cfg.link_flag_ignored(f)} b_lf = {f for f in tb.link_flags if not cfg.link_flag_ignored(f)} + if notes is not None: + notes.bazel_only_link_flags.update(b_lf - a_lf) if a_lf - b_lf: return [Discrepancy( kind=Kind.LINK_FLAGS_DIFF.value, @@ -295,18 +316,24 @@ def _best_match(target: str, a: TranslationUnit, b_vs: List[TranslationUnit], def _diff_variants(target: str, a_vs: List[TranslationUnit], - b_vs: List[TranslationUnit], - cfg: "MigrationConfig") -> List[Discrepancy]: + b_vs: List[TranslationUnit], cfg: "MigrationConfig", + notes: Optional[DiffNotes] = None) -> List[Discrepancy]: """Compare every CMake variant of one source against its BEST-matching Bazel variant (fewest errors, then warnings, then distance; see _rank). Every CMake variant must be matched by some Bazel variant, so nothing is suppressed: a variant Bazel lacks shows up as that variant's discrepancies against its closest match. An extra Bazel-only variant is tolerated, like any extra Bazel flag. With one variant per side this is exactly one - _diff_tu call.""" + _diff_tu call. The Bazel-only flags of each chosen pair are recorded in + `notes`.""" out: List[Discrepancy] = [] for a in a_vs: - out.extend(_best_match(target, a, b_vs, cfg)[0]) + discs, b = _best_match(target, a, b_vs, cfg) + out.extend(discs) + if notes is not None: + a_fl = {f for f in a.flags if not cfg.flag_ignored(f)} + b_fl = {f for f in b.flags if not cfg.flag_ignored(f)} + notes.bazel_only_flags.update(_display_bazel_only(b_fl - a_fl)) return out @@ -363,7 +390,11 @@ def _all_source_keys(views: Dict[str, TargetView], cfg: "MigrationConfig") -> se def diff_models(a: CanonicalModel, b: CanonicalModel, - cfg: Optional["MigrationConfig"] = None) -> List[Discrepancy]: + cfg: Optional["MigrationConfig"] = None, + notes: Optional[DiffNotes] = None) -> List[Discrepancy]: + """Compare the CMake model `a` with the Bazel model `b`. Returns the + discrepancy worklist; if `notes` is given, it is filled with what the diff + observed but does not count (see DiffNotes) for summarize() to report.""" out: List[Discrepancy] = [] if cfg is None: cfg = MigrationConfig() @@ -407,7 +438,8 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value, "", "source compiled in bazel but not cmake", tu=src)) for src in sorted(set(a_union) & set(b_union)): - out.extend(_diff_variants("", a_union[src], b_union[src], cfg)) + out.extend(_diff_variants("", a_union[src], b_union[src], + cfg, notes)) # ---- executables: identity-aligned, own TUs + own flags ---------------- for name in sorted(a_exes - b_exes): @@ -436,12 +468,12 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value, name, "source compiled in bazel but not cmake", tu=src)) for src in sorted(set(amap) & set(bmap)): - out.extend(_diff_variants(name, amap[src], bmap[src], cfg)) + out.extend(_diff_variants(name, amap[src], bmap[src], cfg, notes)) # link flags: asymmetric subset, same policy as compile flags -- every # CMake link flag must be present on the Bazel side; extra Bazel link # flags tolerated (toolchain link noise already stripped). Reviewer # ignores applied to both sides. - out.extend(_diff_link_flags(name, ta, tb, cfg)) + out.extend(_diff_link_flags(name, ta, tb, cfg, notes)) # ---- Java: project-wide source-SET comparison -------------------------- # Java compiles a whole source set per action; how those sources are grouped @@ -489,12 +521,13 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, # ---- tests (opt-in) ---------------------------------------------------- if cfg.include_tests: - out.extend(_diff_tests(a_views, b_views, cfg, a_all, b_all)) + out.extend(_diff_tests(a_views, b_views, cfg, a_all, b_all, notes)) return out def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], - cfg: "MigrationConfig", a_all: set, b_all: set) -> List[Discrepancy]: + cfg: "MigrationConfig", a_all: set, b_all: set, + notes: Optional[DiffNotes] = None) -> List[Discrepancy]: """Apply the COMPILE-PARITY stage to TEST targets. Two checks: 1. TU-SET union of all test sources (like libraries): same test .cc files @@ -532,7 +565,8 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], out.append(Discrepancy(Kind.EXTRA_TEST_TU.value, Severity.WARN.value, "", "test source compiled in bazel but not cmake", tu=src)) for src in sorted(set(a_union) & set(b_union)): - out.extend(_diff_variants("", a_union[src], b_union[src], cfg)) + out.extend(_diff_variants("", a_union[src], b_union[src], + cfg, notes)) # 2. test-binary existence by count if len(a_tests) != len(b_tests): @@ -574,10 +608,22 @@ def by_reason(model: CanonicalModel, renames: Dict[str, str]) -> dict: "bazel": by_reason(b, _bazel_renames(a, b, cfg))} +def _by_count(c: Counter) -> dict: + """Counter -> dict ordered by count (desc), then value: stable JSON.""" + return {k: n for k, n in sorted(c.items(), key=lambda kv: (-kv[1], kv[0]))} + + +BAZEL_ONLY_NOTE = ("Flag comparison is one-directional: every CMake flag must be " + "on the Bazel side, but these flags only the Bazel side has " + "are never a discrepancy. Review them; converged does not " + "cover them.") + + def summarize(discs: List[Discrepancy], a: Optional[CanonicalModel] = None, b: Optional[CanonicalModel] = None, - cfg: Optional["MigrationConfig"] = None) -> dict: + cfg: Optional["MigrationConfig"] = None, + notes: Optional[DiffNotes] = None) -> dict: errors = [d for d in discs if d.severity == Severity.ERROR.value] result = { "total": len(discs), @@ -588,6 +634,12 @@ def summarize(discs: List[Discrepancy], } if a is not None and b is not None: result["excluded"] = excluded_summary(a, b, cfg or MigrationConfig()) + if notes is not None: + result["bazel_only_flags"] = { + "note": BAZEL_ONLY_NOTE, + "compile": _by_count(notes.bazel_only_flags), # flag -> TU pairs + "link": _by_count(notes.bazel_only_link_flags), # flag -> executables + } return result @@ -605,4 +657,6 @@ def summarize(discs: List[Discrepancy], cfg = config_mod.load(sys.argv[3]) if len(sys.argv) > 3 else MigrationConfig() except ValueError as e: sys.exit(f"diff.py: {sys.argv[3]}: {e}") - print(json.dumps(summarize(diff_models(a, b, cfg), a, b, cfg), indent=2)) + notes = DiffNotes() + discs = diff_models(a, b, cfg, notes) + print(json.dumps(summarize(discs, a, b, cfg, notes), indent=2)) diff --git a/scripts/triage.py b/scripts/triage.py index 78729fd..4c76842 100644 --- a/scripts/triage.py +++ b/scripts/triage.py @@ -82,6 +82,7 @@ def triage(diff: dict, only_kind: Optional[str] = None) -> dict: "warnings": diff.get("warnings"), "kinds": out_kinds, "excluded": diff.get("excluded", {}), + "bazel_only_flags": diff.get("bazel_only_flags"), } @@ -95,6 +96,18 @@ def render(t: dict, top: int = 25) -> str: lines = [] status = "CONVERGED" if t["converged"] else "NOT converged" lines.append(f"{status} errors={t['errors']} warnings={t['warnings']}") + extra = t.get("bazel_only_flags") + if extra: + comp, link = extra.get("compile") or {}, extra.get("link") or {} + lines.append(f"bazel-only flags (never a discrepancy; review): " + f"{len(comp)} compile, {len(link)} link") + for label, hist in (("compile (TU pairs)", comp), ("link (executables)", link)): + if not hist: + continue + items = list(hist.items()) + lines.append(f" {label}: " + ", ".join( + f"{v} x{n}" for v, n in items[:top]) + + (f", ... +{len(items) - top} more" if len(items) > top else "")) for k in t["kinds"]: lines.append(f"\n[{k['severity']}] {k['kind']} x{k['count']}") for label, hist, cap in ( diff --git a/tests/test_engine.py b/tests/test_engine.py index 95b3b5d..bee9157 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -29,7 +29,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) from config import MigrationConfig -from diff import Severity, diff_models, summarize +from diff import DiffNotes, Severity, diff_models, summarize from model import (Action, BuildSystem, CanonicalModel, Dependency, Target, TargetKind, TargetRole) @@ -516,6 +516,77 @@ def test_multi_variant_verdict_does_not_depend_on_hash_seed(): assert json.loads(outs.pop())["converged"] +def test_bazel_only_flags_are_reported_but_never_block(): + # The comparison is one-directional: flags only Bazel has never produce a + # discrepancy, so a converged diff must still SAY they exist. + cmake_raw = ["-std=c++17", "-Werror"] + bazel_raw = ["-std=c++17", "-Werror", "-Wno-error=parentheses", "-w", + "-fstack-protector"] # the last: tolerated toolchain default + a = _model(tu_from_raw("src/a.cpp", cmake_raw, is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", bazel_raw, is_bazel=True), is_bazel=True) + notes = DiffNotes() + discs = diff_models(a, b, notes=notes) + res = summarize(discs, a, b, notes=notes) + assert res["converged"] and discs == [], res + assert res["bazel_only_flags"]["compile"] == {"-Wno-error=parentheses": 1, "-w": 1} + assert res["bazel_only_flags"]["link"] == {} + + +def test_bazel_only_flags_respect_ignore_and_count_per_tu(): + cmake_raw = ["-std=c++17"] + bazel_raw = ["-std=c++17", "-fno-strict-aliasing", "-Wno-unused"] + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + for m, raw, bz in ((a, cmake_raw, False), (b, bazel_raw, True)): + m.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw(f"src/{n}.cpp", raw, bz) + for n in ("a", "b", "c")])) + notes = DiffNotes() + summarize(diff_models(a, b, MigrationConfig(ignore_flags={"-Wno-unused"}), notes)) + assert dict(notes.bazel_only_flags) == {"-fno-strict-aliasing": 3} + + +def test_bazel_only_link_flags_are_reported(): + a = _bs(CanonicalModel(), is_bazel=False); a.add(_exe("app", ["-pthread"], False)) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(_exe("app", ["-pthread", "-Wl,--export-dynamic"], True)) + notes = DiffNotes() + res = summarize(diff_models(a, b, notes=notes), notes=notes) + assert res["converged"], res + assert res["bazel_only_flags"]["link"] == {"-Wl,--export-dynamic": 1} + + +def test_bazel_only_flags_are_collected_from_tests_when_opted_in(): + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("t_test", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("test/t.cpp", ["-std=c++17"], False)])) + b.add(Target("t:t_test", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("test/t.cpp", ["-std=c++17", "-w"], True)])) + notes = DiffNotes() + discs = diff_models(a, b, MigrationConfig(include_tests=True), notes) + assert summarize(discs)["converged"], discs + assert dict(notes.bazel_only_flags) == {"-w": 1} + + +def test_bazel_only_flags_come_from_the_exact_variant_when_one_exists(): + # Two Bazel variants of x.cc, one exact and one adding -w; the notes must + # reflect the chosen (exact) pair, not the noisy one. + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x.cc", ["-std=c++17"], False)])) + for name, raw in (("a:noisy", ["-std=c++17", "-w", "-Wno-error=foo"]), + ("b:exact", ["-std=c++17"])): + b.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x.cc", raw, True)])) + notes = DiffNotes() + assert diff_models(a, b, notes=notes) == [] + assert dict(notes.bazel_only_flags) == {} + + +def test_summary_without_notes_has_no_bazel_only_field(): + a = _model(tu_from_raw("src/a.cpp", ["-O2"], is_bazel=False), is_bazel=False) + assert "bazel_only_flags" not in summarize(diff_models(a, a)) + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error. diff --git a/tests/test_triage.py b/tests/test_triage.py index cb36bcb..1f41aef 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -75,6 +75,22 @@ def test_render_caps_bazel_only_tail(): assert "-std=gnu++17" in text +def test_render_shows_bazel_only_flags_even_when_converged(): + diff = {"converged": True, "errors": 0, "warnings": 0, "discrepancies": [], + "excluded": {}, + "bazel_only_flags": {"note": "...", + "compile": {"-Wno-error=parentheses": 24, "-w": 3}, + "link": {}}} + text = render(triage(diff)) + assert "bazel-only flags (never a discrepancy; review): 2 compile, 0 link" in text + assert "compile (TU pairs): -Wno-error=parentheses x24" in text + + +def test_render_without_bazel_only_flags_is_unchanged(): + # diff.json written before the field existed renders as before + assert "bazel-only" not in render(triage(DIFF)) + + if __name__ == "__main__": import traceback fns = [v for k, v in sorted(globals().items()) if k.startswith("test_")] From a3c8846305df94bf11ee16a36b7a6d0a2708d040 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:25:32 -0400 Subject: [PATCH 05/11] Report each side's optimization and debug level in the diff summary canonicalize.py drops -O* and -g* from both sides before comparing. That is the right rule for a per-TU diff, since the level is a build-wide setting, but nothing else looked at it, so the tool reported parity for builds compiled at a different optimization level than the reference, and SKILL.md told the agent the flags were "never your concern". Measured in any2bazel-lab across its first 35 judged migrations, with and without the skill: seven are of pono, whose reference compiles all 94 TUs at -O3, and six of those seven compile at -O2, each reported as 0 errors. lensfun's reference sets -g on all 40 TUs, and every migration lost it. On MySQL Server one migration flattened the reference's 201 -O3 TUs to -O2. summarize() now adds "levels": per side and per language (C, CXX, OBJC, OBJCXX, by source extension), histograms of the optimization level, the debug level and the debug format each compared compile action carries. The last flag of each kind wins, and "(none)" means absent. Level and format are kept apart because they vary independently: `-g3 -gdwarf-4` and `-g1 -gdwarf-4` share a format, and only -g3 emits macro information. A -gdwarf-N with no level flag counts as -g, since it enables debug info. "differs" is set when, for a language both sides compile, any of the three differs. It reads the raw argv the models already hold, over exactly the targets the diff compares (participating roles, minus exclude_targets on the name the diff compares under, minus Bazel header-processing actions), so no new extraction is needed. It never affects errors or `converged`. triage.py prints the levels, and SKILL.md says where to look instead of calling the flags no concern. In the lab's later runs with this series, pono kept -O3 (8 of 9 earlier pono migrations were at -O2), lensfun and openttd kept -g (no earlier lensfun migration had), and xbmc kept its reference's -O3. Lab-Proposal: P16 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 21 +++++----- scripts/diff.py | 81 +++++++++++++++++++++++++++++++++++++- scripts/triage.py | 14 +++++++ tests/test_engine.py | 92 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_triage.py | 14 +++++++ 5 files changed, 212 insertions(+), 10 deletions(-) diff --git a/SKILL.md b/SKILL.md index f78cb02..119c2fe 100644 --- a/SKILL.md +++ b/SKILL.md @@ -77,11 +77,13 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ ### Canonicalization `canonicalize.py` strips noise before comparing flags. Universal mechanics -(driver/wrapper paths, `-c`/`-o`, sysroot, reproducibility defines, `-O*`/`-g*`) -are dropped in code — never your concern. Judgment calls (warning-set or cosmetic -differences) go in `any2bazel.json`'s `ignore`. Correctness flags (`-std=*`, -`-fno-exceptions`, `-fno-rtti`, …) must never be ignored — they surface as hard -errors by design. +(driver/wrapper paths, `-c`/`-o`, sysroot, reproducibility defines) are dropped +in code — never your concern. `-O*`/`-g*` are dropped from the per-TU +comparison too, but the level still matters: `levels` in `diff.json` reports +each side's, and `differs: true` there is real. Judgment calls (warning-set or +cosmetic differences) go in `any2bazel.json`'s `ignore`. Correctness flags +(`-std=*`, `-fno-exceptions`, `-fno-rtti`, …) must never be ignored — they +surface as hard errors by design. ## The migration config: `any2bazel.json` @@ -287,10 +289,11 @@ python3 scripts/diff.py model..json model.bazel.json \ ``` `diff.json` has `converged` (⇔ zero `error` discrepancies), a `discrepancies` worklist (each with `kind`, `severity`, `target`, `tu`, `cmake_only`, -`bazel_only`), an `excluded` map of non-participating targets by role, and -`bazel_only_flags`: flags only the Bazel side has. Those are never a -discrepancy (the comparison is one-directional), so a `-w` or `-Wno-error=` -you add converges silently; review the list before reporting. +`bazel_only`), an `excluded` map of non-participating targets by role, +`levels` (optimization/debug level per side), and `bazel_only_flags`: flags +only the Bazel side has. Those are never a discrepancy (the comparison is +one-directional), so a `-w` or `-Wno-error=` you add converges silently; +review the list before reporting. `cmake_only` is a historical name: for every frontend it means "present in the reference, absent in Bazel". Synthetic target names ``, `` (and `` when diff --git a/scripts/diff.py b/scripts/diff.py index 02f7393..34c2aaa 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -32,6 +32,7 @@ from __future__ import annotations import json +import re from collections import Counter from dataclasses import asdict, dataclass, field from enum import Enum @@ -40,7 +41,8 @@ from canonicalize import BAZEL_TOLERATED_FLAG_PREFIXES from config import MigrationConfig from model import (CanonicalModel, TargetKind, TargetRole, TranslationUnit) -from reconstruct import TargetView, reconstruct +from reconstruct import (TargetView, _is_header_processing, + _source_from_compile_args, reconstruct) def _display_bazel_only(flags) -> list: @@ -608,6 +610,82 @@ def by_reason(model: CanonicalModel, renames: Dict[str, str]) -> dict: "bazel": by_reason(b, _bazel_renames(a, b, cfg))} +# Optimization / debug level flags. canonicalize.py drops -O*/-g* from the +# per-TU comparison (build type is a build-wide setting, and diffing it per TU +# would be noise), so levels_summary reads them from the RAW argv instead. +_OPT_LEVEL = re.compile(r"^-O([0-3sgz]|fast)?$") +# Debug LEVEL (how much is emitted: -g1 has no locals, -g3 adds macros) and +# debug FORMAT (-gdwarf-N) are separate: `-g3 -gdwarf-4` and `-g1 -gdwarf-4` +# share a format and differ in level. +_DEBUG_LEVEL = re.compile(r"^-g([0-3]|gdb[0-3]?|line-tables-only)?$") +_DEBUG_FORMAT = re.compile(r"^-gdwarf(-[0-9]+)?$") +_COMPILE_LEVEL_MNEMONICS = {"CppCompile", "ObjcCompile"} +_LANG_BY_EXT = {".c": "C", ".m": "OBJC", ".mm": "OBJCXX"} + + +def _compared_compile_args(model: CanonicalModel, cfg: "MigrationConfig", + renames: Optional[Dict[str, str]] = None): + """Yield (language, raw argv) for each compile action the diff compares: + participating roles, minus config-excluded targets (judged on the name + the diff compares under, see _bazel_renames), minus Bazel + header-processing actions.""" + renames = renames or {} + roles = set(PARTICIPATING_ROLES) + if cfg.include_tests: + roles.add(TargetRole.TEST) + for name, t in sorted(model.targets.items()): + if t.role not in roles or cfg.target_excluded(renames.get(name, name)): + continue + for act in t.actions: + if act.mnemonic not in _COMPILE_LEVEL_MNEMONICS: + continue + args = act.arguments + src = _source_from_compile_args(args) + if not src or _is_header_processing(args): + continue + ext = src[src.rfind("."):] if "." in src else "" + yield _LANG_BY_EXT.get(ext, "CXX"), args + + +def _compile_levels(model: CanonicalModel, cfg: "MigrationConfig", + renames: Optional[Dict[str, str]] = None) -> dict: + """language -> {"opt", "debug", "debug_format": {flag: n}} over the + compared compile actions. The LAST flag of each kind wins, as it does for + the compiler; "(none)" means no flag. A -gdwarf-N with no level flag + enables debug info at the default level, so it counts as "-g".""" + out: Dict[str, Dict[str, Counter]] = {} + for lang_name, args in _compared_compile_args(model, cfg, renames): + lang = out.setdefault(lang_name, {"opt": Counter(), "debug": Counter(), + "debug_format": Counter()}) + opt = [x for x in args if _OPT_LEVEL.match(x)] + dbg = [x for x in args if _DEBUG_LEVEL.match(x)] + fmt = [x for x in args if _DEBUG_FORMAT.match(x)] + lang["opt"][opt[-1] if opt else "(none)"] += 1 + lang["debug"][dbg[-1] if dbg else ("-g" if fmt else "(none)")] += 1 + lang["debug_format"][fmt[-1] if fmt else "(none)"] += 1 + return {lang: {k: _by_count(c) for k, c in h.items()} + for lang, h in sorted(out.items())} + + +LEVELS_NOTE = ("-O*/-g* are dropped from the per-TU comparison, so a different " + "optimization or debug level never shows up as a discrepancy. " + "This is what each side actually compiles at (TU counts).") + + +def levels_summary(a: CanonicalModel, b: CanonicalModel, + cfg: "MigrationConfig") -> dict: + """Optimization level, debug level and debug format of each side, per + language, with a `differs` flag: true when, for a language both sides + compile, the SET of values of any of the three differs (counts alone may + differ because TUs are missing). Only targets the diff compares count.""" + la = _compile_levels(a, cfg) + lb = _compile_levels(b, cfg, _bazel_renames(a, b, cfg)) + differs = any(set(la[lang][k]) != set(lb[lang][k]) + for lang in set(la) & set(lb) + for k in ("opt", "debug", "debug_format")) + return {"note": LEVELS_NOTE, "cmake": la, "bazel": lb, "differs": differs} + + def _by_count(c: Counter) -> dict: """Counter -> dict ordered by count (desc), then value: stable JSON.""" return {k: n for k, n in sorted(c.items(), key=lambda kv: (-kv[1], kv[0]))} @@ -634,6 +712,7 @@ def summarize(discs: List[Discrepancy], } if a is not None and b is not None: result["excluded"] = excluded_summary(a, b, cfg or MigrationConfig()) + result["levels"] = levels_summary(a, b, cfg or MigrationConfig()) if notes is not None: result["bazel_only_flags"] = { "note": BAZEL_ONLY_NOTE, diff --git a/scripts/triage.py b/scripts/triage.py index 4c76842..20554b3 100644 --- a/scripts/triage.py +++ b/scripts/triage.py @@ -83,6 +83,7 @@ def triage(diff: dict, only_kind: Optional[str] = None) -> dict: "kinds": out_kinds, "excluded": diff.get("excluded", {}), "bazel_only_flags": diff.get("bazel_only_flags"), + "levels": diff.get("levels"), } @@ -96,6 +97,19 @@ def render(t: dict, top: int = 25) -> str: lines = [] status = "CONVERGED" if t["converged"] else "NOT converged" lines.append(f"{status} errors={t['errors']} warnings={t['warnings']}") + lv = t.get("levels") + if lv: + def fmt(h): + return " ".join(f"{v} x{n}" for v, n in h.items()) + tag = "DIFFERS" if lv.get("differs") else "same" + lines.append(f"opt/debug levels ({tag}; not part of the diff):") + for lang in sorted(set(lv.get("cmake") or {}) | set(lv.get("bazel") or {})): + for side in ("cmake", "bazel"): + h = (lv.get(side) or {}).get(lang) + if h: + lines.append(f" {lang:6s} {side:5s} opt: {fmt(h['opt'])}" + f" | debug: {fmt(h['debug'])}" + f" | format: {fmt(h.get('debug_format', {}))}") extra = t.get("bazel_only_flags") if extra: comp, link = extra.get("compile") or {}, extra.get("link") or {} diff --git a/tests/test_engine.py b/tests/test_engine.py index bee9157..978c290 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -587,6 +587,98 @@ def test_summary_without_notes_has_no_bazel_only_field(): assert "bazel_only_flags" not in summarize(diff_models(a, a)) +def test_levels_reported_when_diff_is_blind_to_them(): + # CMake Release compiles at -O3; Bazel -c opt at -O2. -O* is dropped from + # the per-TU diff, so the diff converges -- the levels summary must not. + a = _model(tu_from_raw("src/a.cpp", ["-O3", "-DNDEBUG"], is_bazel=False), + is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-O2", "-g0", "-DNDEBUG"], is_bazel=True), + is_bazel=True) + res = summarize(diff_models(a, b), a, b) + assert res["converged"], res + lv = res["levels"] + assert lv["differs"] + assert lv["cmake"] == {"CXX": {"opt": {"-O3": 1}, "debug": {"(none)": 1}, + "debug_format": {"(none)": 1}}} + assert lv["bazel"] == {"CXX": {"opt": {"-O2": 1}, "debug": {"-g0": 1}, + "debug_format": {"(none)": 1}}} + + +def test_levels_last_flag_wins_and_split_by_language(): + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw("src/a.c", ["-g", "-O2", "-O0"], False), + tu_from_raw("src/b.cc", ["-g"], False)])) + b.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw("src/a.c", ["-O0", "-g"], True), + tu_from_raw("src/b.cc", ["-g"], True), + # a Bazel header-processing action is not a TU and is not counted + Action("CppCompile", ("-xc++-header", "-fsyntax-only", "-O2", + "-c", "src/b.h"))])) + lv = summarize(diff_models(a, b), a, b)["levels"] + assert lv["cmake"]["C"] == {"opt": {"-O0": 1}, "debug": {"-g": 1}, "debug_format": {"(none)": 1}} + assert lv["cmake"]["CXX"] == {"opt": {"(none)": 1}, "debug": {"-g": 1}, "debug_format": {"(none)": 1}} + assert lv["bazel"] == lv["cmake"] + assert not lv["differs"] + + +def test_levels_language_on_one_side_only_does_not_set_differs(): + # `differs` compares levels for languages BOTH sides compile; a language + # only one side has is a missing-TU matter, reported but not a level diff. + a = _model(tu_from_raw("src/a.c", ["-O2"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-O3"], is_bazel=True), is_bazel=True) + lv = summarize(diff_models(a, b), a, b)["levels"] + assert set(lv["cmake"]) == {"C"} and set(lv["bazel"]) == {"CXX"}, lv + assert lv["differs"] is False + + +def test_debug_level_and_format_are_tracked_separately(): + # -g3 emits macro information that -g1 does not; sharing -gdwarf-4 must + # not hide that. A format flag alone enables debug info at level -g. + a = _model(tu_from_raw("src/a.cpp", ["-g3", "-gdwarf-4"], is_bazel=False), + is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-g1", "-gdwarf-4"], is_bazel=True), + is_bazel=True) + lv = summarize(diff_models(a, b), a, b)["levels"] + assert lv["cmake"]["CXX"]["debug"] == {"-g3": 1} + assert lv["bazel"]["CXX"]["debug"] == {"-g1": 1} + assert lv["cmake"]["CXX"]["debug_format"] == {"-gdwarf-4": 1} + assert lv["differs"] + c = _model(tu_from_raw("src/a.cpp", ["-gdwarf-5"], is_bazel=True), is_bazel=True) + lc = summarize(diff_models(a, c), a, c)["levels"]["bazel"]["CXX"] + assert (lc["debug"], lc["debug_format"]) == ({"-g": 1}, {"-gdwarf-5": 1}) + + +def test_levels_skip_a_bazel_target_excluded_under_its_mapped_name(): + # target_map renames pkg:tool to tool, which is excluded: the diff skips + # it, so the levels must not count it either. + def m(name, raw, is_bazel): + model = _bs(CanonicalModel(), is_bazel) + model.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/a.cpp", ["-O2"], is_bazel)])) + model.add(Target(name, TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("tool/m.cpp", raw, is_bazel)])) + return model + a, b = m("tool", ["-O2"], False), m("pkg:tool", ["-O0"], True) + cfg = MigrationConfig(target_map={"tool": "pkg:tool"}, exclude_targets={"tool"}) + lv = summarize(diff_models(a, b, cfg), a, b, cfg)["levels"] + assert lv["bazel"]["CXX"]["opt"] == {"-O2": 1}, lv + assert not lv["differs"], lv + + +def test_levels_count_only_compared_targets(): + a = _bs(CanonicalModel(), False) + a.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/a.cpp", ["-O2"], False)])) + a.add(Target("vendored", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("third_party/z.c", ["-O3"], False)])) + a.add(Target("t_test", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("test/t.cpp", ["-O0"], False)])) + lv = summarize([], a, a, MigrationConfig(exclude_targets={"vendored"}))["levels"] + assert lv["cmake"] == {"CXX": {"opt": {"-O2": 1}, "debug": {"(none)": 1}, + "debug_format": {"(none)": 1}}} + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error. diff --git a/tests/test_triage.py b/tests/test_triage.py index 1f41aef..10ad42c 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -91,6 +91,20 @@ def test_render_without_bazel_only_flags_is_unchanged(): assert "bazel-only" not in render(triage(DIFF)) +def test_render_shows_levels(): + diff = {"converged": True, "errors": 0, "warnings": 0, "discrepancies": [], + "excluded": {}, + "levels": {"note": "...", "differs": True, + "cmake": {"CXX": {"opt": {"-O3": 94}, "debug": {"(none)": 94}, + "debug_format": {"(none)": 94}}}, + "bazel": {"CXX": {"opt": {"-O2": 94}, "debug": {"-g0": 94}, + "debug_format": {"(none)": 94}}}}} + text = render(triage(diff)) + assert "opt/debug levels (DIFFERS; not part of the diff):" in text + assert "opt: -O3 x94" in text and "opt: -O2 x94" in text + assert "format: (none) x94" in text + + if __name__ == "__main__": import traceback fns = [v for k, v in sorted(globals().items()) if k.startswith("test_")] From cd9878e06a10ac6562dd347c16c3e87945325766 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 15:27:15 -0400 Subject: [PATCH 06/11] Apply -U in flag canonicalization: the last -D/-U wins canonicalize_flags collected every -D into the define set and passed -U through as an ordinary flag, so an undefine never removed anything. A Bazel build that keeps assertions on with `build --copt=-UNDEBUG` after -c opt's -DNDEBUG was reported as defining NDEBUG (a WARN on every TU), and one that turned assertions back on against a reference compiled with -DNDEBUG converged with 0 errors. Later commits in this series report NDEBUG state and recommend exactly that -UNDEBUG, so it has to be read correctly. - canonicalize_flags applies `-U NAME` / `-UNAME` as the compiler does: it removes every NAME define collected so far, and a later -D defines it again. A -D likewise replaces every earlier definition of its name, so each side ends with the last word per macro: `-DF(x)=1 -DF(x)=2` and `-DF(x)=1 -UF -DF(x)=2` both leave F(x)=2 and compare equal. `-UNAME` is also kept as a comparable flag iff the -U is the last word on NAME (a later -D clears it), so whether it survives never depends on what either side's toolchain passed before it. Bazel's -c opt preamble (-D_FORTIFY_SOURCE=1 -DNDEBUG) followed by a project's `-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2` now compares equal to the reference (before this change it produced a discrepancy, in either direction). A last-word -U still compares a -U of a compiler-predefined macro (-U_FORTIFY_SOURCE alone). A define is matched by its macro name, the identifier before any `(` or `=`, so `-UF` cancels a function-like `-DF(x)=x`, as `gcc -E` shows. - A define with no value means value 1 to the compiler, so `-DX` is spelled `X=1` (and `-DF(x)` is `F(x)=1`); `-DX -DX=1` and `-DX=1 -DX` compare equal. Define values in diff.json now show that spelling. - extract_cmake.py synthesizes each compile argv in the order CMake's generators emit it ($DEFINES $INCLUDES $FLAGS, as in CMake 3.28's Makefile and Ninja output) instead of flags first, so a -UNDEBUG in CMAKE_CXX_FLAGS lands after, and cancels, an NDEBUG compile definition, as it does in the real command. It does not cancel Release's -DNDEBUG, which comes later in the same flags fragment; only a compile definition is cancelled. Include order changes too: structured include directories now precede any -I from the flags fragments. That has no verdict impact, since includes are compared for presence, not order. Checked against the preprocessor: a differential test generates fixed-seed random -D/-U sequences over object-like and function-like names (joined and split forms, with and without values, repeats) and requires two sequences to canonicalize to the same define set exactly when `cc -E` expands a probe source identically; it is skipped when no C compiler is installed. A larger run of the same check (3,000 sequences, 4.5 million pairs, three seeds) found no disagreement in either direction. One class is documented instead of fixed: a function-like macro that differs only by parameter names (F(x)=x vs F(y)=y) preprocesses the same but reports a define difference. Trade-off: a reference compiled with `-DNDEBUG -UNDEBUG` against a Bazel side with neither now shows a cmake-only `-UNDEBUG` flag error. It is benign (both sides compile without NDEBUG); copy the flag into copts, or add it to ignore.flags (ignore.defines does not reach it: it is a flag). Tests cover valueless defines, last-word -U in both orders, redefinition with and without an intervening -U, function-like macros in both orders, the predefined-macro case, the fortify override against Bazel's opt preamble in both directions, both NDEBUG directions through the diff, and the CMake argv order. Co-Authored-By: Claude Opus 5.5 --- scripts/canonicalize.py | 53 +++++++++++++++-- scripts/extract_cmake.py | 9 ++- tests/test_engine.py | 120 ++++++++++++++++++++++++++++++++++++++- tests/test_extractors.py | 20 +++++++ 4 files changed, 192 insertions(+), 10 deletions(-) diff --git a/scripts/canonicalize.py b/scripts/canonicalize.py index f2b8c66..a6f5552 100644 --- a/scripts/canonicalize.py +++ b/scripts/canonicalize.py @@ -183,12 +183,44 @@ def _matches_any(flag: str, prefixes: Iterable[str]) -> bool: return any(flag == p or flag.startswith(p) for p in prefixes) +def _macro_name(define: str) -> str: + """The macro a define names: the identifier before any `(` (a + function-like macro, `F(x)=x`) or `=` (a value).""" + ends = [i for i in (define.find("("), define.find("=")) if i >= 0] + return define[:min(ends)] if ends else define + + +def _undefine(defines: List[str], undefined: List[str], name: str) -> None: + """Apply `-U name` as the compiler does: it cancels every `name` define + that came before it. `name` is also recorded in `undefined`, the names + whose LAST word is a -U: those are emitted as `-Uname` flags and still + compared, because the -U may target a compiler-predefined macro (e.g. + -U_FORTIFY_SOURCE). A later -D on `name` clears it (see _redefine), so + whether a -U survives never depends on what came before it.""" + defines[:] = [d for d in defines if _macro_name(d) != name] + if name not in undefined: + undefined.append(name) + + +def _redefine(defines: List[str], undefined: List[str], d: str) -> None: + """Apply `-D d` as the compiler does: it replaces every earlier definition + of the same macro name (so each side ends with the last word per name, + whether or not a -U came between), and a -U on that name is no longer the + last word.""" + key = _macro_name(d) + defines[:] = [x for x in defines if _macro_name(x) != key] + if key in undefined: + undefined.remove(key) + + def _add_define(defines: List[str], d: str) -> None: """Append a define unless it's a hardcoded reproducibility injection - (__DATE__/__TIME__/__TIMESTAMP__), matched by key before '='.""" + (__DATE__/__TIME__/__TIMESTAMP__), matched by key before '='. A define + with no value means value 1 to the compiler (`-DX` is `-DX=1`, `-DF(x)` + is `-DF(x)=1`), so it is spelled that way and the two compare equal.""" key = d.split("=", 1)[0] if key not in _DROP_DEFINE_KEYS: - defines.append(d) + defines.append(d if "=" in d else d + "=1") def canonicalize_flags( @@ -199,7 +231,8 @@ def canonicalize_flags( ) -> Tuple[Tuple[str, ...], Tuple[str, ...], Tuple[str, ...]]: """Return (defines, includes, other_flags) in canonical form. - - defines: sorted, deduped, '-D' stripped to 'KEY=VAL' + - defines: sorted, deduped, '-D' stripped to 'KEY=VAL'; a later -U removes + them (see _undefine) - includes: repo-relative, ORDER PRESERVED, dedup-adjacent only - other_flags: correctness-relevant only (ignorable + bazel-default dropped), sorted for stable comparison @@ -207,6 +240,7 @@ def canonicalize_flags( defines: List[str] = [] includes: List[str] = [] other: List[str] = [] + undefined: List[str] = [] # names whose last word is -U (see _undefine) i = 0 n = len(raw) @@ -227,9 +261,16 @@ def canonicalize_flags( # -Dfoo / -D foo if tok == "-D" and i + 1 < n: - _add_define(defines, raw[i + 1]); i += 2; continue + _redefine(defines, undefined, raw[i + 1]); _add_define(defines, raw[i + 1]) + i += 2; continue if tok.startswith("-D"): - _add_define(defines, tok[2:]); i += 1; continue + _redefine(defines, undefined, tok[2:]); _add_define(defines, tok[2:]) + i += 1; continue + # -Ufoo / -U foo: cancels the defines collected so far (last wins) + if tok == "-U" and i + 1 < n: + _undefine(defines, undefined, raw[i + 1]); i += 2; continue + if tok.startswith("-U"): + _undefine(defines, undefined, tok[2:]); i += 1; continue # force-include flavors: -include FILE / -imacros FILE. The argument is # a FILE path (not a search dir), which each build system may spell @@ -260,6 +301,8 @@ def canonicalize_flags( other.append(tok); i += 1 + # a -U that is the last word on its name stays a comparable flag + other.extend("-U" + name for name in undefined) # defines: order-insensitive canon_defines = tuple(sorted(set(defines))) # includes: order preserved, drop only consecutive dups diff --git a/scripts/extract_cmake.py b/scripts/extract_cmake.py index f070f1f..3ee6fe7 100644 --- a/scripts/extract_cmake.py +++ b/scripts/extract_cmake.py @@ -146,17 +146,20 @@ def _parse_target(tobj: dict, repo_root: str) -> Target: # Synthesize one CppCompile Action per source: an argv the differ parses the # same way it parses Bazel's. CMake has no real command line, so we build the - # canonical compiler view: [, -D..., -I/-isystem ..., -c src]. + # canonical compiler view in the order CMake's generators emit it + # ($DEFINES $INCLUDES $FLAGS): [-D..., -I/-isystem ..., , + # -c src]. The order matters for -U: a -UNDEBUG in CMAKE_CXX_FLAGS comes + # after, and so cancels, a -DNDEBUG compile definition. actions: List[Action] = [] for cg in tobj.get("compileGroups", []): base: List[str] = [] - for frag in cg.get("compileCommandFragments", []): - base.extend(_split_fragment(frag.get("fragment", ""))) for d in cg.get("defines", []): base.append("-D" + d["define"]) for inc in cg.get("includes", []): base.append("-isystem" if inc.get("isSystem") else "-I") base.append(inc["path"]) + for frag in cg.get("compileCommandFragments", []): + base.extend(_split_fragment(frag.get("fragment", ""))) for idx in cg.get("sourceIndexes", []): src = sources[idx] argv = tuple(base + ["-c", src]) diff --git a/tests/test_engine.py b/tests/test_engine.py index 978c290..b836c8d 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -143,6 +143,122 @@ def test_missing_cmake_flag_is_caught(): assert "NEEDED=1" in dd.cmake_only +def test_undefine_cancels_earlier_defines_last_wins(): + from canonicalize import canonicalize_flags + def canon(raw): + d, _, f = canonicalize_flags(raw, REPO, is_bazel=False) + return d, f + # a -U cancels earlier defines, and stays a flag iff it is the last word + assert canon(["-DNDEBUG", "-UNDEBUG"]) == ((), ("-UNDEBUG",)) + assert canon(["-D", "X=1", "-U", "X", "-DY"]) == (("Y=1",), ("-UX",)) + assert canon(["-U_FORTIFY_SOURCE"]) == ((), ("-U_FORTIFY_SOURCE",)) + # a later -D clears it, whatever preceded the -U + assert canon(["-U_FORTIFY_SOURCE", "-D_FORTIFY_SOURCE=1"]) == \ + (("_FORTIFY_SOURCE=1",), ()) + assert canon(["-D_FORTIFY_SOURCE=1", "-U_FORTIFY_SOURCE", + "-D_FORTIFY_SOURCE=2"]) == (("_FORTIFY_SOURCE=2",), ()) + assert canon(["-UNDEBUG", "-DNDEBUG"]) == (("NDEBUG=1",), ()) + # function-like macros are matched by their name, in both orders + assert canon(["-DF(x)=x", "-UF"]) == ((), ("-UF",)) + assert canon(["-UF", "-DF(x)=x"]) == (("F(x)=x",), ()) + # a later -D replaces earlier definitions of the name, -U or not + assert canon(["-DF(x)=1", "-DF(x)=2"]) == (("F(x)=2",), ()) + assert canon(["-DF(x)=1", "-UF", "-DF(x)=2"]) == (("F(x)=2",), ()) + # a define with no value is value 1, for the compiler and for the diff + assert canon(["-DX=1", "-DX"]) == canon(["-DX", "-DX=1"]) == (("X=1",), ()) + assert canon(["-DF(x)"]) == (("F(x)=1",), ()) + assert canon(["-DX="]) == (("X=",), ()) # defined as empty: not 1 + + +def test_valueless_define_equals_value_one_across_sides(): + a = _model(tu_from_raw("src/a.cpp", ["-DX", "-DX=1"], is_bazel=False), + is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-DX=1", "-DX"], is_bazel=True), + is_bazel=True) + assert diff_models(a, b) == [] + + +def test_define_canonicalization_agrees_with_the_preprocessor(): + # Differential check against the C preprocessor, skipped when no compiler + # is installed: for fixed-seed random -D/-U sequences over object-like and + # function-like names (joined and split forms, with and without values, + # repeats), two sequences must canonicalize to the same define set exactly + # when `cc -E` expands a probe source identically. Known limitation, left + # out of the pool: a function-like macro that differs only by parameter + # names (F(x)=x vs F(y)=y) preprocesses the same but reports a difference. + import random + import shutil + import subprocess + from canonicalize import canonicalize_flags + cc = next((c for c in ("cc", "gcc", "clang") if shutil.which(c)), None) + if cc is None: + return + pool = [] + for n in ("A", "B"): + pool += [(f"-D{n}",), (f"-D{n}=1",), (f"-D{n}=2",), (f"-D{n}=",), + ("-D", n), ("-D", f"{n}=2"), (f"-U{n}",), ("-U", n)] + pool += [("-DF(x)",), ("-DF(x)=x",), ("-DF(x)=1",), ("-D", "F(x)=x+1"), + ("-DF=3",), ("-UF",), ("-U", "F")] + src = "".join(f"#ifdef {n}\n{n} is [{n}]\n#else\n{n} undef\n#endif\n" + for n in ("A", "B")) + src += "#ifdef F\nF is [F(7)] [F]\n#else\nF undef\n#endif\n" + rng = random.Random(7) + seqs = sorted({tuple(t for _ in range(rng.randint(0, 5)) + for t in rng.choice(pool)) for _ in range(300)}) + by_output, by_defines = {}, {} + for s in seqs: + out = subprocess.run([cc, "-E", "-P", "-x", "c", "-", *s], input=src, + capture_output=True, text=True, check=True).stdout + defines = canonicalize_flags(list(s), REPO, is_bazel=False)[0] + by_output.setdefault(" ".join(out.split()), set()).add(defines) + by_defines.setdefault(defines, set()).add(" ".join(out.split())) + split = {o: d for o, d in by_output.items() if len(d) > 1} + merged = {d: o for d, o in by_defines.items() if len(o) > 1} + assert not split, f"same preprocessing, different define sets: {split}" + assert not merged, f"same define set, different preprocessing: {merged}" + + +def test_redefinition_with_and_without_undefine_converges(): + # Both preprocess to F(x) == 2; only the Bazel side has the -U between. + a = _model(tu_from_raw("src/a.cpp", ["-DF(x)=1", "-DF(x)=2"], is_bazel=False), + is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-DF(x)=1", "-UF", "-DF(x)=2"], + is_bazel=True), is_bazel=True) + assert diff_models(a, b) == [] + + +def test_fortify_override_in_copts_converges_both_ways(): + # `-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2` copied into copts: under -c opt + # Bazel's toolchain has already passed -D_FORTIFY_SOURCE=1 -DNDEBUG. Both + # sides compile with exactly _FORTIFY_SOURCE=2, so nothing may differ. + override = ["-U_FORTIFY_SOURCE", "-D_FORTIFY_SOURCE=2"] + preamble = ["-D_FORTIFY_SOURCE=1", "-DNDEBUG"] + for cmake_raw, bazel_raw in ((["-DNDEBUG"] + override, preamble + override), + (preamble + override, ["-DNDEBUG"] + override)): + a = _model(tu_from_raw("src/a.cpp", cmake_raw, is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", bazel_raw, is_bazel=True), is_bazel=True) + assert diff_models(a, b) == [], (cmake_raw, bazel_raw) + + +def test_bazel_undefine_after_opt_ndebug_matches_a_reference_without_it(): + # `build --copt=-UNDEBUG` after -c opt's -DNDEBUG: assertions on, like a + # reference that never defines NDEBUG. No define differs. + a = _model(tu_from_raw("src/a.cpp", ["-O2"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-O2", "-DNDEBUG", "-UNDEBUG"], + is_bazel=True), is_bazel=True) + assert diff_models(a, b) == [] + + +def test_bazel_undefine_is_caught_against_a_reference_with_ndebug(): + # The reverse: the reference turns assertions off, Bazel turns them back on. + a = _model(tu_from_raw("src/a.cpp", ["-DNDEBUG"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-DNDEBUG", "-UNDEBUG"], is_bazel=True), + is_bazel=True) + discs = diff_models(a, b) + assert [(d.kind, d.severity, d.cmake_only) for d in discs] == \ + [("defines_diff", "error", ["NDEBUG=1"])], discs + + def test_extra_bazel_define_is_warn_not_error(): cmake_raw = ["-DFOO=1"] bazel_raw = ["-DFOO=1", "-DEXTRA=1"] @@ -445,7 +561,7 @@ def test_a_cmake_variant_missing_on_bazel_is_caught(): discs = diff_models(a, b) errs = [d for d in discs if d.severity == Severity.ERROR.value] assert len(errs) == 1 and errs[0].kind == "defines_diff", errs - assert errs[0].cmake_only == ["FULL"], errs + assert errs[0].cmake_only == ["FULL=1"], errs def test_an_extra_bazel_variant_is_tolerated(): @@ -469,7 +585,7 @@ def test_tied_pairings_prefer_the_smaller_difference(): tu_from_raw("gen/b1.cc", [], True)])) discs = diff_models(a, b, MigrationConfig(source_map=(("gen/", "@gen"),))) assert [(d.cmake_only, d.bazel_only) for d in discs] == \ - [(["LIB1"], []), (["LIB2"], [])], discs + [(["LIB1=1"], []), (["LIB2=1"], [])], discs def test_tied_pairings_prefer_the_exact_variant_over_a_noisy_one(): diff --git a/tests/test_extractors.py b/tests/test_extractors.py index fd282b7..7ee7613 100644 --- a/tests/test_extractors.py +++ b/tests/test_extractors.py @@ -139,6 +139,26 @@ def test_full_pipeline_converges(): assert res["errors"] == 0, json.dumps(res, indent=2) +def test_cmake_flags_come_after_definitions_so_undefine_applies(): + # CMake emits $DEFINES $INCLUDES $FLAGS: a -UNDEBUG in CMAKE_CXX_FLAGS + # cancels a NDEBUG compile definition. + import extract_cmake as ec + tobj = dict(TARGET_MYLIB, compileGroups=[dict( + TARGET_MYLIB["compileGroups"][0], + compileCommandFragments=[{"fragment": "-std=c++17 -UNDEBUG"}], + defines=[{"define": "NDEBUG"}, {"define": "FOO=1"}])]) + tu = reconstruct(_model_of(ec._parse_target(tobj, REPO)))["mylib"].tus[0] + assert tu.defines == ("FOO=1",), tu + assert "-UNDEBUG" in tu.flags, tu # the last word on NDEBUG: still compared + + +def _model_of(target): + from model import BuildSystem, CanonicalModel + m = CanonicalModel(build_system=BuildSystem.CMAKE, repo_root=REPO) + m.add(target) + return m + + def test_cmake_extracts_canonical_flags(): with tempfile.TemporaryDirectory() as root: build = _write_cmake_fixture(root) From ec84ad9ee111851ae011caa69e3774ab21a9ad24 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:59:21 -0400 Subject: [PATCH 07/11] Report Bazel-only defines and NDEBUG state beside the verdict Defines are compared both ways, but a define only the Bazel side has is a WARN on each TU, and no summary line showed it. The one that matters most is NDEBUG: Bazel's `-c opt` adds -DNDEBUG, so a migration that pins --compilation_mode=opt against a reference that keeps assertions turns them off everywhere and still converges. Measured in any2bazel-lab: four judged migrations compile with NDEBUG against a reference that passes none, and nothing in the differ's verdict or summary said so. Three migrations of CMake's own sources got it from `-c opt`, on every compile action; a GDAL migration passed -DNDEBUG itself, on 1,534 of 1,534 actions, at no optimization level. - bazel_only_flags gains "defines": each define only the Bazel side has, after ignore.defines, with the compared TU pairs it appears on. Its note says these are a WARN, not an error; triage prints the count and the top entries. - A new "ndebug" block sits next to "levels". Per side, it counts the compared compile actions that leave NDEBUG defined. With the diff's notes, "bazel_added" and "bazel_missing" count the compared TU pairs where only Bazel, or only CMake, defines it, and "differs" is true when either count is nonzero; a Bazel-only TU that is never compared cannot flip it. Both read the canonicalized defines, which apply -U in order (the previous commit), so a `build --copt=-UNDEBUG` after -c opt counts as assertions on. The counts ignore ignore.defines on purpose, so an accepted suppression cannot hide assertion state. triage.py prints one NDEBUG line after the levels. - SKILL.md mentions ndebug under "Diff against reference build". The guidance on opt's -DNDEBUG is in the build-type commit. Severity and `converged` are unchanged: a Bazel-only NDEBUG is still the existing WARN. Whether it should be an error, since it changes what the built program does, is a decision for the maintainers. In the lab's later CMake runs with this series, none added NDEBUG against the reference's none (three runs; two earlier runs had added it). Lab-Proposal: P17 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 6 ++- scripts/diff.py | 69 +++++++++++++++++++++++++-- scripts/triage.py | 19 +++++++- tests/test_engine.py | 110 +++++++++++++++++++++++++++++++++++++++++++ tests/test_triage.py | 15 ++++++ 5 files changed, 210 insertions(+), 9 deletions(-) diff --git a/SKILL.md b/SKILL.md index 119c2fe..c67c322 100644 --- a/SKILL.md +++ b/SKILL.md @@ -290,8 +290,10 @@ python3 scripts/diff.py model..json model.bazel.json \ `diff.json` has `converged` (⇔ zero `error` discrepancies), a `discrepancies` worklist (each with `kind`, `severity`, `target`, `tu`, `cmake_only`, `bazel_only`), an `excluded` map of non-participating targets by role, -`levels` (optimization/debug level per side), and `bazel_only_flags`: flags -only the Bazel side has. Those are never a discrepancy (the comparison is +`levels` (optimization/debug level per side), `ndebug` (assertion state per +side, and on how many compared TU pairs Bazel adds or lacks `NDEBUG`), and +`bazel_only_flags`: flags and defines only the Bazel side has. Those flags are +never a discrepancy and those defines only a WARN (the comparison is one-directional), so a `-w` or `-Wno-error=` you add converges silently; review the list before reporting. `cmake_only` is a historical name: for every frontend it means "present in the diff --git a/scripts/diff.py b/scripts/diff.py index 34c2aaa..3839bc2 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -38,9 +38,10 @@ from enum import Enum from typing import Dict, List, Optional -from canonicalize import BAZEL_TOLERATED_FLAG_PREFIXES +from canonicalize import BAZEL_TOLERATED_FLAG_PREFIXES, canonicalize_flags from config import MigrationConfig -from model import (CanonicalModel, TargetKind, TargetRole, TranslationUnit) +from model import (BuildSystem, CanonicalModel, TargetKind, TargetRole, + TranslationUnit) from reconstruct import (TargetView, _is_header_processing, _source_from_compile_args, reconstruct) @@ -105,9 +106,18 @@ class DiffNotes: These counters make them visible: flag -> number of compared TU pairs (compile; one pair per CMake TU variant, see _diff_variants) or aligned executables (link) where Bazel has it and CMake does not, after the - reviewer's ignore lists and the toolchain's tolerated defaults.""" + reviewer's ignore lists and the toolchain's tolerated defaults. Defines + only Bazel has are a WARN per TU, which no summary showed either; they are + counted the same way, per compared TU pair. + + NDEBUG is counted separately, in both directions and regardless of + ignore.defines, because it is assertion state: Bazel's `-c opt` adds it, so + pinning opt against a reference that keeps assertions turns them off.""" bazel_only_flags: Counter = field(default_factory=Counter) bazel_only_link_flags: Counter = field(default_factory=Counter) + bazel_only_defines: Counter = field(default_factory=Counter) + ndebug_bazel_added: int = 0 # compared TU pairs: Bazel defines, CMake not + ndebug_bazel_missing: int = 0 # compared TU pairs: CMake defines, Bazel not def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, @@ -336,9 +346,21 @@ def _diff_variants(target: str, a_vs: List[TranslationUnit], a_fl = {f for f in a.flags if not cfg.flag_ignored(f)} b_fl = {f for f in b.flags if not cfg.flag_ignored(f)} notes.bazel_only_flags.update(_display_bazel_only(b_fl - a_fl)) + a_def = {d for d in a.defines if not cfg.define_ignored(d)} + b_def = {d for d in b.defines if not cfg.define_ignored(d)} + notes.bazel_only_defines.update(b_def - a_def) + a_nd, b_nd = _defines_ndebug(a.defines), _defines_ndebug(b.defines) + notes.ndebug_bazel_added += b_nd and not a_nd + notes.ndebug_bazel_missing += a_nd and not b_nd return out +def _defines_ndebug(defines) -> bool: + """NDEBUG (any value) among canonicalized defines, which already apply -U + in order (the last -D/-U wins; see canonicalize._undefine).""" + return any(d.split("=", 1)[0] == "NDEBUG" for d in defines) + + # Java source roots: a .java path is keyed by its PACKAGE-rooted form (the path # below the source root), which is invariant across build systems that disagree # on the repo prefix / source-root layout (Bazel 'guava/src/...' vs Maven @@ -686,6 +708,41 @@ def levels_summary(a: CanonicalModel, b: CanonicalModel, return {"note": LEVELS_NOTE, "cmake": la, "bazel": lb, "differs": differs} +NDEBUG_NOTE = ("Assertion state. NDEBUG is part of what a build type means, and " + "Bazel's -c opt adds it: pinning opt against a reference without " + "NDEBUG turns assertions off. An extra Bazel define is only a WARN, " + "so this does not block convergence. Counts ignore ignore.defines.") + + +def ndebug_summary(a: CanonicalModel, b: CanonicalModel, cfg: "MigrationConfig", + notes: Optional[DiffNotes] = None) -> dict: + """NDEBUG on each side over the compared compile actions (their + canonicalized defines, so -U applies in order), plus, when `notes` is + given, the compared TU pairs where only Bazel defines it (`bazel_added`: + assertions off that the reference keeps) or only CMake does + (`bazel_missing`: assertions on that the reference turns off).""" + def count(model, renames=None): + c = Counter() + is_bazel = model.build_system == BuildSystem.BAZEL + for _, args in _compared_compile_args(model, cfg, renames): + defines = canonicalize_flags(list(args), model.repo_root, + is_bazel=is_bazel)[0] + c["NDEBUG" if _defines_ndebug(defines) else "(none)"] += 1 + return _by_count(c) + ca, cb = count(a), count(b, _bazel_renames(a, b, cfg)) + out = {"note": NDEBUG_NOTE, "cmake": ca, "bazel": cb} + if notes is not None: + # The compared pairs decide: a Bazel-only TU that is never compared + # must not flip the verdict while both pair counts are zero. + out["bazel_added"] = notes.ndebug_bazel_added + out["bazel_missing"] = notes.ndebug_bazel_missing + differs = bool(notes.ndebug_bazel_added or notes.ndebug_bazel_missing) + else: + differs = bool(ca) and bool(cb) and set(ca) != set(cb) + out["differs"] = differs + return out + + def _by_count(c: Counter) -> dict: """Counter -> dict ordered by count (desc), then value: stable JSON.""" return {k: n for k, n in sorted(c.items(), key=lambda kv: (-kv[1], kv[0]))} @@ -693,8 +750,8 @@ def _by_count(c: Counter) -> dict: BAZEL_ONLY_NOTE = ("Flag comparison is one-directional: every CMake flag must be " "on the Bazel side, but these flags only the Bazel side has " - "are never a discrepancy. Review them; converged does not " - "cover them.") + "are never a discrepancy, and these defines only it has are " + "only a WARN. Review them; converged does not cover them.") def summarize(discs: List[Discrepancy], @@ -713,11 +770,13 @@ def summarize(discs: List[Discrepancy], if a is not None and b is not None: result["excluded"] = excluded_summary(a, b, cfg or MigrationConfig()) result["levels"] = levels_summary(a, b, cfg or MigrationConfig()) + result["ndebug"] = ndebug_summary(a, b, cfg or MigrationConfig(), notes) if notes is not None: result["bazel_only_flags"] = { "note": BAZEL_ONLY_NOTE, "compile": _by_count(notes.bazel_only_flags), # flag -> TU pairs "link": _by_count(notes.bazel_only_link_flags), # flag -> executables + "defines": _by_count(notes.bazel_only_defines), # define -> TU pairs } return result diff --git a/scripts/triage.py b/scripts/triage.py index 20554b3..b479ec8 100644 --- a/scripts/triage.py +++ b/scripts/triage.py @@ -84,6 +84,7 @@ def triage(diff: dict, only_kind: Optional[str] = None) -> dict: "excluded": diff.get("excluded", {}), "bazel_only_flags": diff.get("bazel_only_flags"), "levels": diff.get("levels"), + "ndebug": diff.get("ndebug"), } @@ -110,12 +111,26 @@ def fmt(h): lines.append(f" {lang:6s} {side:5s} opt: {fmt(h['opt'])}" f" | debug: {fmt(h['debug'])}" f" | format: {fmt(h.get('debug_format', {}))}") + nd = t.get("ndebug") + if nd: + def side(h): + return " ".join(f"{v} x{n}" for v, n in (h or {}).items()) or "-" + tag = "DIFFERS" if nd.get("differs") else "same" + line = (f"NDEBUG / assertions ({tag}): cmake {side(nd.get('cmake'))}" + f" | bazel {side(nd.get('bazel'))}") + if "bazel_added" in nd: + line += (f"; bazel adds it on {nd['bazel_added']} compared TU pairs," + f" lacks it on {nd['bazel_missing']}") + lines.append(line) extra = t.get("bazel_only_flags") if extra: comp, link = extra.get("compile") or {}, extra.get("link") or {} + defs = extra.get("defines") or {} lines.append(f"bazel-only flags (never a discrepancy; review): " - f"{len(comp)} compile, {len(link)} link") - for label, hist in (("compile (TU pairs)", comp), ("link (executables)", link)): + f"{len(comp)} compile, {len(link)} link; " + f"bazel-only defines (WARN only): {len(defs)}") + for label, hist in (("compile (TU pairs)", comp), ("link (executables)", link), + ("defines (TU pairs)", defs)): if not hist: continue items = list(hist.items()) diff --git a/tests/test_engine.py b/tests/test_engine.py index b836c8d..e4185c1 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -795,6 +795,116 @@ def test_levels_count_only_compared_targets(): "debug_format": {"(none)": 1}}} +def _ndebug_pair(cmake_raw, bazel_raw, cfg=None): + a = _model(tu_from_raw("src/a.cpp", cmake_raw, is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", bazel_raw, is_bazel=True), is_bazel=True) + notes = DiffNotes() + discs = diff_models(a, b, cfg, notes) + return summarize(discs, a, b, cfg, notes) + + +def test_ndebug_added_by_bazel_opt_is_reported_though_converged(): + # -c opt adds -DNDEBUG; the reference keeps assertions. An extra Bazel + # define is only a WARN, so the diff converges -- the summary must say it. + res = _ndebug_pair(["-O2"], ["-O2", "-DNDEBUG"]) + assert res["converged"], res + nd = res["ndebug"] + assert nd["cmake"] == {"(none)": 1} and nd["bazel"] == {"NDEBUG": 1}, nd + assert (nd["bazel_added"], nd["bazel_missing"], nd["differs"]) == (1, 0, True) + assert res["bazel_only_flags"]["defines"] == {"NDEBUG=1": 1} + + +def test_ndebug_missing_on_bazel_is_reported_in_the_other_direction(): + res = _ndebug_pair(["-O3", "-DNDEBUG"], ["-O3"]) + assert not res["converged"] # a missing CMake define stays an error + nd = res["ndebug"] + assert (nd["bazel_added"], nd["bazel_missing"], nd["differs"]) == (0, 1, True) + + +def test_ndebug_undefined_again_counts_as_absent(): + # `build --copt=-UNDEBUG` beside -c opt keeps assertions on: last one wins. + res = _ndebug_pair([], ["-DNDEBUG", "-UNDEBUG"]) + nd = res["ndebug"] + assert nd["bazel"] == {"(none)": 1}, nd + assert (nd["bazel_added"], nd["differs"]) == (0, False), nd + assert res["bazel_only_flags"]["defines"] == {}, res # agrees with ndebug + + +def test_ndebug_pair_counts_honour_argv_order(): + # `build --copt=-UNDEBUG` followed by a rule-level -DNDEBUG leaves NDEBUG + # defined. The pair counts must agree with the per-side counts. + res = _ndebug_pair(["-DNDEBUG"], ["-UNDEBUG", "-DNDEBUG"]) + nd = res["ndebug"] + assert nd["cmake"] == {"NDEBUG": 1} and nd["bazel"] == {"NDEBUG": 1}, nd + assert (nd["bazel_added"], nd["bazel_missing"], nd["differs"]) == (0, 0, False), nd + + +def test_ndebug_turned_back_on_is_missing_and_an_error(): + # Reference -DNDEBUG; Bazel -DNDEBUG then -UNDEBUG: assertions on. + res = _ndebug_pair(["-DNDEBUG"], ["-DNDEBUG", "-UNDEBUG"]) + assert not res["converged"], res + nd = res["ndebug"] + assert nd["bazel"] == {"(none)": 1}, nd + assert (nd["bazel_added"], nd["bazel_missing"], nd["differs"]) == (0, 1, True) + + +def test_ndebug_and_defines_collected_through_executables(): + # The executable path pairs TUs per target, not through . + def exe(raw, is_bazel): + m = _bs(CanonicalModel(), is_bazel) + m.add(Target("app", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("tool/m.cpp", raw, is_bazel)])) + return m + a, b = exe(["-O2"], False), exe(["-O2", "-DNDEBUG", "-DEXTRA"], True) + notes = DiffNotes() + res = summarize(diff_models(a, b, notes=notes), a, b, notes=notes) + assert res["converged"], res + assert res["bazel_only_flags"]["defines"] == {"EXTRA=1": 1, "NDEBUG=1": 1} + assert (res["ndebug"]["bazel_added"], res["ndebug"]["bazel"]) == (1, {"NDEBUG": 1}) + + +def test_ndebug_counts_ignore_the_ignore_list(): + # An accepted suppression may hide the WARN and the bazel-only define, but + # never the assertion-state line. + cfg = MigrationConfig(ignore_defines={"NDEBUG"}) + res = _ndebug_pair([], ["-DNDEBUG", "-DEXTRA=1"], cfg) + assert res["converged"] and res["warnings"] == 1, res # EXTRA=1 only + assert res["bazel_only_flags"]["defines"] == {"EXTRA=1": 1} + assert res["ndebug"]["bazel_added"] == 1 and res["ndebug"]["differs"] + + +def test_ndebug_differs_follows_the_compared_pairs(): + # An extra Bazel-only TU without NDEBUG changes the per-side key sets but + # is never compared; both pair counts are 0, so nothing differs. + a = _model(tu_from_raw("src/a.cpp", ["-DNDEBUG"], is_bazel=False), is_bazel=False) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw("src/a.cpp", ["-DNDEBUG"], True), + tu_from_raw("src/extra.cpp", [], True)])) + notes = DiffNotes() + nd = summarize(diff_models(a, b, notes=notes), a, b, notes=notes)["ndebug"] + assert nd["bazel"] == {"NDEBUG": 1, "(none)": 1}, nd + assert (nd["bazel_added"], nd["bazel_missing"], nd["differs"]) == (0, 0, False) + + +def test_ndebug_skips_a_bazel_target_excluded_under_its_mapped_name(): + a = _model(tu_from_raw("src/a.cpp", [], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", [], is_bazel=True), is_bazel=True) + a.add(Target("tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("tool/m.cpp", [], False)])) + b.add(Target("pkg:tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("tool/m.cpp", ["-DNDEBUG"], True)])) + cfg = MigrationConfig(target_map={"tool": "pkg:tool"}, exclude_targets={"tool"}) + nd = summarize(diff_models(a, b, cfg), a, b, cfg)["ndebug"] + assert nd["bazel"] == {"(none)": 1}, nd + + +def test_ndebug_without_notes_reports_sides_only(): + a = _model(tu_from_raw("src/a.cpp", ["-DNDEBUG"], is_bazel=False), is_bazel=False) + nd = summarize(diff_models(a, a), a, a)["ndebug"] + assert "bazel_added" not in nd and nd["differs"] is False, nd + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error. diff --git a/tests/test_triage.py b/tests/test_triage.py index 10ad42c..6ae065e 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -105,6 +105,21 @@ def test_render_shows_levels(): assert "format: (none) x94" in text +def test_render_shows_ndebug_and_bazel_only_defines(): + diff = {"converged": True, "errors": 0, "warnings": 1534, "discrepancies": [], + "excluded": {}, + "ndebug": {"note": "...", "cmake": {"(none)": 1534}, + "bazel": {"NDEBUG": 1534}, "bazel_added": 1534, + "bazel_missing": 0, "differs": True}, + "bazel_only_flags": {"note": "...", "compile": {}, "link": {}, + "defines": {"NDEBUG": 1534}}} + text = render(triage(diff)) + assert ("NDEBUG / assertions (DIFFERS): cmake (none) x1534 | bazel NDEBUG x1534;" + " bazel adds it on 1534 compared TU pairs, lacks it on 0") in text, text + assert "bazel-only defines (WARN only): 1" in text + assert "defines (TU pairs): NDEBUG x1534" in text + + if __name__ == "__main__": import traceback fns = [v for k, v in sorted(globals().items()) if k.startswith("test_")] From bf6401d679ebaf35542b5a4a2e84f13cbd61b5a3 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:26:22 -0400 Subject: [PATCH 08/11] Warn in diff.json when configure-time generated files are not compared A configure_file() output never reaches a command line, so a value that differs only inside one (a config.h define, a version string) is invisible to the diff. The skill said so in a note beside the CMake extraction commands, but diff.json, the artifact people act on, said nothing, and `converged: true` read as covering them. Measured in any2bazel-lab: lensfun's generated config.h differed from the reference in 3 of 7 migrations with the skill, each with a converged diff. configured_files does populate (the lab saw it on another project); the empty lists on lensfun, which spells the command CONFIGURE_FILE, come from the uppercase bug fixed earlier in this series. diff_models now appends one WARN discrepancy, kind configured_files_not_compared, target "", when the CMake model records configured files: cmake_only lists them, and the detail gives the count and how many land on a compile include path. It never blocks `converged`. One aggregated entry rather than one per file keeps a large project's worklist readable (MySQL Server has about 200). All project-owned configured files are listed, not only those flagged as compile inputs, because that flag depends on the output sitting under a known include directory and can miss a header reached another way. A new end-to-end test covers the path from a trace through extract_cmake and the model to the diff. In the lab's later runs with this series, agents byte-compared the listed files on lensfun, CMake and QGIS; the QGIS agent wrote eleven Bazel tests that do it, and CMake's three configure headers that had differed from the reference came out identical in all three CMake runs. Lab-Proposal: P2 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 4 +++- docs/REFERENCE-cmake.md | 2 ++ scripts/diff.py | 23 +++++++++++++++++++++++ tests/test_engine.py | 17 +++++++++++++++++ tests/test_extractors.py | 26 ++++++++++++++++++++++++++ 5 files changed, 71 insertions(+), 1 deletion(-) diff --git a/SKILL.md b/SKILL.md index c67c322..a50e24b 100644 --- a/SKILL.md +++ b/SKILL.md @@ -312,7 +312,9 @@ are easiest to reason about once the TUs underneath agree. also emits **WARN-only** kinds that don't block `converged` — `extra_tu` / `extra_target` (compiled/built on the Bazel side but not the reference) and `extra_test_tu` — note them but they need no action unless they point to -something you didn't intend to add. +something you didn't intend to add. The WARN `configured_files_not_compared` +does need action: byte-compare each listed file against the one your Bazel +build generates. #### Triage findings {#triage} diff --git a/docs/REFERENCE-cmake.md b/docs/REFERENCE-cmake.md index 49ed665..35c6f80 100644 --- a/docs/REFERENCE-cmake.md +++ b/docs/REFERENCE-cmake.md @@ -40,6 +40,8 @@ python3 "$SKILL_DIR/scripts/extract_cmake.py" model.cmake.js > the content differ are TODO (see > [TODO-configure-time-generation.md](TODO-configure-time-generation.md)). This > is distinct from build-time codegen (genrules), which is also unmodeled. +> `diff.py` lists them in a `configured_files_not_compared` warning (see +> [Diff against reference build](../SKILL.md#diff)). The project flags passed to `cmake` define the platform/options the Bazel aquery must mirror. diff --git a/scripts/diff.py b/scripts/diff.py index 3839bc2..94ab1cb 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -82,6 +82,8 @@ class Kind(str, Enum): MISSING_TEST_TU = "missing_test_tu" # test source compiled in cmake, not bazel EXTRA_TEST_TU = "extra_test_tu" # test source compiled in bazel, not cmake TEST_BINARY_COUNT = "test_binary_count" # differing number of test executables + # configure-time generated files exist but their CONTENTS are not compared: + CONFIGURED_FILES = "configured_files_not_compared" @dataclass @@ -546,9 +548,30 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, # ---- tests (opt-in) ---------------------------------------------------- if cfg.include_tests: out.extend(_diff_tests(a_views, b_views, cfg, a_all, b_all, notes)) + + # ---- configure-time generated files: recorded, NOT compared ------------- + # configure_file() outputs never reach a command line, so a value that + # differs only inside one (a config.h define, a version string) is + # invisible to everything above. Say so in the worklist itself, where the + # reader of diff.json will see it, rather than only in the docs. + out.extend(_configured_files_warning(a)) return out +def _configured_files_warning(a: CanonicalModel) -> List[Discrepancy]: + files = a.configured_files + if not files: + return [] + names = sorted(files) + n_inputs = sum(1 for c in files.values() if c.is_compile_input) + return [Discrepancy( + Kind.CONFIGURED_FILES.value, Severity.WARN.value, "", + f"{len(names)} configure-time generated file(s), {n_inputs} on a compile " + "include path, are not covered by this comparison: byte-compare each " + "against the file the Bazel build generates", + cmake_only=names)] + + def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], cfg: "MigrationConfig", a_all: set, b_all: set, notes: Optional[DiffNotes] = None) -> List[Discrepancy]: diff --git a/tests/test_engine.py b/tests/test_engine.py index e4185c1..05eddea 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -905,6 +905,23 @@ def test_ndebug_without_notes_reports_sides_only(): assert "bazel_added" not in nd and nd["differs"] is False, nd +def test_configured_files_raise_a_non_blocking_warning(): + from model import ConfiguredFile + a = _model(tu_from_raw("src/a.cpp", ["-DX"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-DX"], is_bazel=True), is_bazel=True) + assert diff_models(a, b) == [] # nothing configured -> no warning + a.add_configured_file(ConfiguredFile("config.h", "/b/config.h", + is_compile_input=True)) + a.add_configured_file(ConfiguredFile("foo.pc", "/b/foo.pc")) + discs = diff_models(a, b) + res = summarize(discs) + assert res["converged"] and res["warnings"] == 1, res + w = discs[0] + assert w.kind == "configured_files_not_compared" and w.severity == "warn" + assert w.cmake_only == ["config.h", "foo.pc"] + assert w.detail.startswith("2 configure-time generated file(s), 1 on a") + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error. diff --git a/tests/test_extractors.py b/tests/test_extractors.py index 7ee7613..b5ce4e9 100644 --- a/tests/test_extractors.py +++ b/tests/test_extractors.py @@ -159,6 +159,32 @@ def _model_of(target): return m +def test_configured_files_reach_the_diff_end_to_end(): + # The trace -> extract_cmake -> model JSON -> diff path, with the command + # spelled in capitals as large projects write it. + from serialize import dump_model, load_model + with tempfile.TemporaryDirectory() as root: + build = _write_cmake_fixture(root) + trace = os.path.join(root, "trace.jsonl") + with open(trace, "w") as f: + f.write(json.dumps({"version": {"major": 1, "minor": 2}}) + "\n") + f.write(json.dumps({"cmd": "CONFIGURE_FILE", "file": REPO + "/CMakeLists.txt", + "line": 3, "args": [REPO + "/config.h.in", + build + "/config.h"]}) + "\n") + path = os.path.join(root, "model.cmake.json") + dump_model(extract_cmake.extract(build, REPO, trace), path) + a = load_model(path) + assert list(a.configured_files) == ["config.h"], a.configured_files + + aq_path = os.path.join(root, "aquery.json") + with open(aq_path, "w") as f: + json.dump(AQUERY, f) + res = summarize(diff_models(a, extract_bazel.extract(aq_path, REPO))) + assert res["converged"], res + kinds = [d["kind"] for d in res["discrepancies"]] + assert kinds == ["configured_files_not_compared"], kinds + + def test_cmake_extracts_canonical_flags(): with tempfile.TemporaryDirectory() as root: build = _write_cmake_fixture(root) From 95162917e43e08caa83cf8775d31c2c3052d2da7 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:29:21 -0400 Subject: [PATCH 09/11] State the convergence boundary where convergence is defined SKILL.md said that configure_file outputs are "recorded but not yet diffed" only in a note beside the CMake extraction commands. The paragraph that defines done ("Done when both stages converge") carried no such caveat, so a reader who lands there reads converged: true as covering the whole build. Measured in any2bazel-lab: a lensfun migration converged with its generated config.h differing from the reference (a GLIB_VERSION_MIN_REQUIRED value that appears in no command line), and the same divergence recurred in later runs, each alongside a converged diff. Adds a short "what converged does not cover" list directly under the definition of done, starting with generated file contents and the instruction to byte-compare them. Later commits add the other boundaries to the same list. Lab-Proposal: P1 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/SKILL.md b/SKILL.md index a50e24b..9b0c4d5 100644 --- a/SKILL.md +++ b/SKILL.md @@ -61,6 +61,12 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ Bazel flags are tolerated (listed in `diff.json` as `bazel_only_flags`). No artifact/symbol diff or test execution yet. + **`converged: true` is parity of command lines, nothing more.** Check + separately what it cannot see: + - **Generated file contents.** A value that reaches only a generated file + (`configure_file` output, genrule output) never appears on a command line. + Byte-compare each generated file against the reference build's. + ### How targets are matched - **Roles:** each target gets a `role` (`production`, `test`, `dashboard`, From 6d06f857e69ea91f61c98b936638722b566b18b7 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:29:40 -0400 Subject: [PATCH 10/11] Say that shared-library link lines are not compared SKILL.md described link consistency as covering "every name-aligned executable/shared-lib". diff.py does not do that: shared libraries are in the library TU union, only executables reach the link-flag comparison, and the external-dependency check is a project-wide union, so a dependency one .so is missing passes if any other target links it. A clean diff says nothing about how a shared library links. Measured in any2bazel-lab: a pono migration converged while libpono.so lacked its MPFR link, and the Bazel-built binary crashed at load. The agent found it only by running the binary. Corrects the two places in SKILL.md and the four comments in scripts/diff.py (the module docstring, LINK_FLAGS_DIFF, _diff_link_flags and _diff_tests) that claimed shared-library link parity, and adds the boundary to the "what converged does not cover" list with the remedy: run a binary that loads each shared library. Comparing shared libraries' link lines is a larger design change (they would need identity alignment like executables) and is left to the maintainers. No later run exercised this change on the project that motivated it. On lensfun, an agent following it loaded the built shared library, found it missing a GLib symbol, and fixed the link. Lab-Proposal: P8 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 9 ++++++--- scripts/diff.py | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/SKILL.md b/SKILL.md index 9b0c4d5..75ae9a4 100644 --- a/SKILL.md +++ b/SKILL.md @@ -52,7 +52,7 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ - **Compile parity** — every TU compiled with equivalent flags/defines/ includes (project-wide TU-set), and the external link-dependency closure matches. - - **Link consistency** — every name-aligned executable/shared-lib links with + - **Link consistency** — every name-aligned executable links with equivalent link flags. The per-binary stage: the *artifacts* agree, not just the TUs. @@ -66,6 +66,9 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ - **Generated file contents.** A value that reaches only a generated file (`configure_file` output, genrule output) never appears on a command line. Byte-compare each generated file against the reference build's. + - **How shared libraries link.** A shared library is compared by its TUs + only; its link flags and the libraries it links are not checked (the + external-dep check is project-wide). Run a binary that loads each one. ### How targets are matched @@ -311,8 +314,8 @@ comparisons. The diff reports both parity stages at once: **compile-parity** kinds (`missing_tu`, `defines_diff`, `includes_diff`, `flags_diff`, `missing_dep`, `missing_target`) and the **link-consistency** kind (`link_flags_diff`, one per -name-aligned executable/shared library). Fix compile parity first — link flags -are easiest to reason about once the TUs underneath agree. +name-aligned executable). Fix compile parity first — link flags are easiest to +reason about once the TUs underneath agree. `error`-severity kinds (above) block convergence and are what you fix. The diff also emits **WARN-only** kinds that don't block `converged` — `extra_tu` / diff --git a/scripts/diff.py b/scripts/diff.py index 94ab1cb..ab98cbd 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -19,8 +19,9 @@ Parity has two stages, both reported here: COMPILE PARITY -- per-TU flag/define/include equivalence (project-wide TU-set) + external link-dependency closure. - LINK CONSISTENCY -- per name-aligned executable/shared-lib, equivalent link - flags (link_flags_diff). + LINK CONSISTENCY -- per name-aligned executable, equivalent link flags + (link_flags_diff). Shared libraries are compared by + their TUs only; their link lines are not checked. ASYMMETRY: a flag present in CMake but missing in Bazel is a discrepancy (under-specified Bazel target -> likely wrong build). A flag present only in @@ -73,7 +74,7 @@ class Kind(str, Enum): DEFINES_DIFF = "defines_diff" INCLUDES_DIFF = "includes_diff" FLAGS_DIFF = "flags_diff" - LINK_FLAGS_DIFF = "link_flags_diff" # executable/shared-lib link flags + LINK_FLAGS_DIFF = "link_flags_diff" # executable link flags MISSING_DEP = "missing_dep" # Java (CompileGroup) source-set comparison: MISSING_JAVA_SRC = "missing_java_src" # .java compiled in A but not B @@ -187,7 +188,7 @@ def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, def _diff_link_flags(name: str, ta: "TargetView", tb: "TargetView", cfg: "MigrationConfig", notes: Optional[DiffNotes] = None) -> List[Discrepancy]: - """Asymmetric link-flag subset for one aligned executable/shared-lib: every + """Asymmetric link-flag subset for one aligned executable: every CMake link flag must be present on the Bazel side; extra Bazel link flags are tolerated (and recorded in `notes`). Reviewer-approved differences (cfg.link_flag_ignored) are dropped from both sides first. Mirrors the @@ -589,12 +590,12 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], sources compile but are never linked into runnable tests. SCOPE: tests get the COMPILE-PARITY stage only. LINK CONSISTENCY (per-binary - link flags and link-dep closure) runs for PRODUCTION executables/shared - libs, not for test binaries: tests align by source-set union, not by binary - identity, so there's no per-binary link command to compare. So with tests - on, "converged" means production artifacts are link-consistent AND all test - SOURCES compile equivalently -- not that each test binary links identically. - Extending link consistency to per-test-binary is a natural future increment. + link flags) runs for PRODUCTION executables, not for test binaries: tests + align by source-set union, not by binary identity, so there's no + per-binary link command to compare. So with tests on, "converged" means + production artifacts are link-consistent AND all test SOURCES compile + equivalently -- not that each test binary links identically. Extending + link consistency to per-test-binary is a natural future increment. """ out: List[Discrepancy] = [] a_tests = {n for n, t in a_views.items() From d6b9e6a40e69e809efa5fadfb40cb60e38376530 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:27:14 -0400 Subject: [PATCH 11/11] Align executables by unambiguous label basename The CMake model keys executables by bare target name (BinRec), the Bazel model by package-qualified label (gems:BinRec). Executables align by exact name, so every executable in a non-root package needed a target_map entry even when nothing was renamed, which buries the entries that record real renames. Measured in any2bazel-lab: two graphicsgems migrations wrote 32 and 26 target_map entries, none of them a rename; every entry bridged a bare CMake name to its package-qualified Bazel label. After target_map is applied, each CMake executable that still has no exact-name counterpart aligns with the Bazel executable whose label basename equals its name, when exactly one unaligned Bazel executable has that basename. Two packages sharing the basename are left unaligned (missing_target plus extra_target, as before) for target_map to resolve; libraries, external-repo labels, and CMake names that are target_map keys (even when the mapped Bazel target does not exist) never align this way. The pairs are reported in diff.json as "aligned_by_basename" so the implicit matches stay reviewable. The alignment is part of _bazel_renames, the one rename decision the diff and every summary share, so an exclude_targets entry for an auto-aligned executable is honoured the same way by the comparison, the excluded list, the levels and the NDEBUG counts. SKILL.md now reserves target_map for renames and ambiguous basenames. In the lab's later graphicsgems run with this change, target_map was empty (the earlier run had 30 entries). Lab-Proposal: P3 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 12 ++++-- scripts/diff.py | 54 +++++++++++++++++++++--- tests/test_engine.py | 98 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 9 deletions(-) diff --git a/SKILL.md b/SKILL.md index 75ae9a4..627fbf8 100644 --- a/SKILL.md +++ b/SKILL.md @@ -80,8 +80,10 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ path**, so names and grouping don't matter — a rename or object-library fold-in converges with no mapping. Only **external** link deps are checked; internal dep names are noise. -- **Executables** align by **name** (a missing exe is a real gap); use - `target_map` for renames. +- **Executables** align by **name** (a missing exe is a real gap), or with the + one Bazel executable whose label basename matches (`BinRec` ↔ `gems:BinRec`; + see `aligned_by_basename` in `diff.json`). Use `target_map` for renames and + for a basename two packages share. ### Canonicalization @@ -131,8 +133,10 @@ Fields: reproducible; `diff.py` refuses an entry that is neither a flag nor a flag's value. - **`target_map`** — `reference_name → bazel_name` for intentionally renamed - **executables**. Bazel targets are keyed by full label, so the value is - usually `:name` (e.g. `"bssl": ":bssl"`). Libraries need no mapping. + **executables**, or when two Bazel packages share the basename. The value is + the label without `//`: `:name` at the root, `pkg:name` elsewhere (e.g. + `"bssl": "tool:bssl"`). Same-named executables align without an entry; + libraries need no mapping. - **`dep_map`** — `reference_dep_name → bazel_dep_name` for an **external link dep** spelled differently per build (e.g. `Catch2Main` vs `catch2_main`). An explicit, recorded rename — not a fuzzy match — so a residual `missing_dep` is diff --git a/scripts/diff.py b/scripts/diff.py index ab98cbd..3d434cc 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -121,6 +121,8 @@ class DiffNotes: bazel_only_defines: Counter = field(default_factory=Counter) ndebug_bazel_added: int = 0 # compared TU pairs: Bazel defines, CMake not ndebug_bazel_missing: int = 0 # compared TU pairs: CMake defines, Bazel not + # cmake_name -> bazel_name, executables aligned by unambiguous basename + aligned_by_basename: Dict[str, str] = field(default_factory=dict) def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, @@ -256,14 +258,50 @@ def _apply_target_map(views: Dict[str, TargetView], return renamed +def _label_basename(name: str) -> str: + """'gems:BinRec' -> 'BinRec'; ':bssl' -> 'bssl' (Bazel names are labels).""" + return name.rsplit(":", 1)[-1] + + +def _align_by_basename(a_views: dict, b_views: dict, + mapped: frozenset = frozenset()) -> Dict[str, str]: + """cmake_name -> bazel_name for every CMake executable with no exact-name + counterpart whose name is the basename of EXACTLY ONE unaligned Bazel + executable (CMake keys executables by bare name, Bazel by package-qualified + label, so without this every executable needed a target_map entry). Two + Bazel executables sharing the basename are ambiguous and left alone: that + case, and genuine renames, still need target_map. External-repo labels + ('@...') never align, and neither does a CMake name in `mapped` (a + target_map key): its explicit entry decides, even if it names a Bazel + target that does not exist. Values need only a `.kind`: model targets + and reconstructed views both work.""" + by_base: Dict[str, List[str]] = {} + for name, v in b_views.items(): + if (v.kind == TargetKind.EXECUTABLE and name not in a_views + and not name.startswith("@")): + by_base.setdefault(_label_basename(name), []).append(name) + return {name: by_base[name][0] for name, v in sorted(a_views.items()) + if v.kind == TargetKind.EXECUTABLE and name not in b_views + and name not in mapped and len(by_base.get(name, ())) == 1} + + 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} + diff renames: target_map entries first, then the automatic alignment of + executables by unambiguous label basename (see _align_by_basename), which + never overrides a target_map key. 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.""" + renames = {bazel: cmake for cmake, bazel in cfg.target_map.items() + if bazel in b.targets} + renamed = {renames.get(label, label): t for label, t in b.targets.items()} + label_of = {name: label for label, name in renames.items()} + for cmake, name in _align_by_basename(a.targets, renamed, + frozenset(cfg.target_map)).items(): + renames[label_of.get(name, name)] = cmake + return renames _LIBRARY_KINDS = {TargetKind.STATIC, TargetKind.SHARED, TargetKind.OBJECT, @@ -434,6 +472,11 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, 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) + if notes is not None: + explicit = set(cfg.target_map.values()) + notes.aligned_by_basename.update( + {name: label for label, name in renames.items() + if label not in explicit}) a_names = _participating(a_views, cfg) b_names = _participating(b_views, cfg) @@ -802,6 +845,7 @@ def summarize(discs: List[Discrepancy], "link": _by_count(notes.bazel_only_link_flags), # flag -> executables "defines": _by_count(notes.bazel_only_defines), # define -> TU pairs } + result["aligned_by_basename"] = dict(sorted(notes.aligned_by_basename.items())) return result diff --git a/tests/test_engine.py b/tests/test_engine.py index 05eddea..4b83922 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -976,6 +976,104 @@ def test_executables_still_aligned_by_identity(): assert any(d.kind == "missing_target" for d in discs) +def test_executable_aligns_by_unique_label_basename(): + # CMake keys executables by bare name, Bazel by package-qualified label; + # an unambiguous basename needs no target_map entry. + a = _bs(CanonicalModel(), is_bazel=False) + b = _bs(CanonicalModel(), is_bazel=True) + for name in ("BinRec", "Hash3D"): + a.add(Target(name, TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw(f"gems/{name}.c", ["-DX"], False)])) + b.add(Target("gems:BinRec", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("gems/BinRec.c", ["-DX"], True)])) + b.add(Target(":Hash3D", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("gems/Hash3D.c", ["-DX"], True)])) + notes = DiffNotes() + discs = diff_models(a, b, notes=notes) + res = summarize(discs, notes=notes) + assert res["converged"] and discs == [], discs + assert res["aligned_by_basename"] == {"BinRec": "gems:BinRec", "Hash3D": ":Hash3D"} + + +def test_ambiguous_basename_is_not_aligned(): + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x/tool.c", [], False)])) + b = _bs(CanonicalModel(), is_bazel=True) + for pkg in ("x", "y"): + b.add(Target(f"{pkg}:tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x/tool.c", [], True)])) + kinds = sorted(d.kind for d in diff_models(a, b)) + assert kinds == ["extra_target", "extra_target", "missing_target"], kinds + # target_map resolves the ambiguity + discs = diff_models(a, b, MigrationConfig(target_map={"tool": "x:tool"})) + assert [d.kind for d in discs] == ["extra_target"], discs + + +def test_target_map_wins_over_basename_alignment(): + # A target_map entry consumes the Bazel executable, so it is no longer a + # basename candidate for the same-named CMake executable. + a = _bs(CanonicalModel(), is_bazel=False) + for name in ("tool", "tool_v2"): + a.add(Target(name, TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x/tool.c", [], False)])) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target("x:tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x/tool.c", [], True)])) + notes = DiffNotes() + discs = diff_models(a, b, MigrationConfig(target_map={"tool_v2": "x:tool"}), + notes) + assert [(d.kind, d.target) for d in discs] == [("missing_target", "tool")], discs + assert notes.aligned_by_basename == {} + + +def test_a_target_map_key_never_aligns_by_basename(): + # An entry naming a Bazel target that does not exist consumes nothing; + # the CMake name must still not fall back to basename alignment. + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x/tool.c", [], False)])) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target("x:tool", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x/tool.c", [], True)])) + notes = DiffNotes() + cfg = MigrationConfig(target_map={"tool": "y:tool_renamed"}) + kinds = sorted(d.kind for d in diff_models(a, b, cfg, notes)) + assert kinds == ["extra_target", "missing_target"], kinds + assert notes.aligned_by_basename == {} + + +def test_auto_aligned_exclusion_is_shared_by_diff_and_summaries(): + # CMake app, Bazel pkg:app, aligned by basename; exclude_targets names + # app. The diff skips it, and so must the excluded list and the levels. + def m(name, raw, is_bazel): + model = _bs(CanonicalModel(), is_bazel) + model.add(Target("lib", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/a.cpp", ["-O2"], is_bazel)])) + model.add(Target(name, TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("app/m.cpp", raw, is_bazel)])) + return model + a, b = m("app", ["-O2"], False), m("pkg:app", ["-O0"], True) + cfg = MigrationConfig(exclude_targets={"app"}) + notes = DiffNotes() + res = summarize(diff_models(a, b, cfg, notes), a, b, cfg, notes) + assert res["discrepancies"] == [], res + assert res["aligned_by_basename"] == {"app": "pkg:app"}, res + assert res["excluded"]["bazel"] == {"config_excluded": ["pkg:app"]}, res + assert not res["levels"]["differs"], res["levels"] + assert res["ndebug"]["bazel"] == {"(none)": 1}, res["ndebug"] + + +def test_basename_never_aligns_a_library_or_an_exact_match(): + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("app", TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("app.c", [], False)])) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target("pkg:app", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("app.c", [], True)])) + assert any(d.kind == "missing_target" for d in diff_models(a, b)) + + def _exe(name, link_raw, is_bazel, src="tool/m.cpp"): return Target(name, TargetKind.EXECUTABLE, role=TargetRole.PRODUCTION, actions=[tu_from_raw(src, ["-DFOO=1"], is_bazel=is_bazel),