From 67b117f3c401947014de0b4ce8a400a7b9ee714e Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:24:24 -0400 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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_")]