diff --git a/SKILL.md b/SKILL.md index 62ad5c0..627fbf8 100644 --- a/SKILL.md +++ b/SKILL.md @@ -52,13 +52,23 @@ 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. 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. + + **`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 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 @@ -70,17 +80,21 @@ 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 `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` @@ -112,12 +126,17 @@ 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. + **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 @@ -283,7 +302,13 @@ 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, +`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 reference, absent in Bazel". Synthetic target names ``, `` (and `` when @@ -293,14 +318,16 @@ 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` / `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/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/config.py b/scripts/config.py index fd17cbd..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(). @@ -100,9 +101,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 +165,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 +218,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..3d434cc 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 @@ -32,14 +33,18 @@ from __future__ import annotations import json -from dataclasses import asdict, dataclass +import re +from collections import Counter +from dataclasses import asdict, dataclass, field 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 reconstruct import TargetView, reconstruct +from model import (BuildSystem, CanonicalModel, TargetKind, TargetRole, + TranslationUnit) +from reconstruct import (TargetView, _is_header_processing, + _source_from_compile_args, reconstruct) def _display_bazel_only(flags) -> list: @@ -69,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 @@ -78,6 +83,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 @@ -91,6 +98,33 @@ 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. 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 + # 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, cfg: "MigrationConfig") -> List[Discrepancy]: out: List[Discrepancy] = [] @@ -154,13 +188,17 @@ def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, def _diff_link_flags(name: str, ta: "TargetView", tb: "TargetView", - cfg: "MigrationConfig") -> List[Discrepancy]: - """Asymmetric link-flag subset for one aligned executable/shared-lib: every + cfg: "MigrationConfig", + notes: Optional[DiffNotes] = None) -> List[Discrepancy]: + """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. 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, @@ -220,24 +258,150 @@ 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 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, 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", + 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. The Bazel-only flags of each chosen pair are recorded in + `notes`.""" + out: List[Discrepancy] = [] + for a in a_vs: + 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)) + 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 @@ -262,7 +426,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) @@ -291,14 +455,28 @@ 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() # 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) + 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) @@ -330,7 +508,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_tu("", 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): @@ -343,8 +522,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 @@ -357,12 +538,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_tu(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 @@ -410,12 +591,34 @@ 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)) + + # ---- 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) -> 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 @@ -430,12 +633,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() @@ -453,7 +656,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_tu("", 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): @@ -481,21 +685,147 @@ 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))} + + +# 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} + + +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]))} + + +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, and these defines only it has are " + "only a WARN. 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), @@ -506,6 +836,16 @@ 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 + } + result["aligned_by_basename"] = dict(sorted(notes.aligned_by_basename.items())) return result @@ -519,5 +859,10 @@ 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() - print(json.dumps(summarize(diff_models(a, b, cfg), a, b, cfg), indent=2)) + 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}") + notes = DiffNotes() + discs = diff_models(a, b, cfg, notes) + print(json.dumps(summarize(discs, a, b, cfg, notes), indent=2)) 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/scripts/triage.py b/scripts/triage.py index 78729fd..b479ec8 100644 --- a/scripts/triage.py +++ b/scripts/triage.py @@ -82,6 +82,9 @@ 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"), + "levels": diff.get("levels"), + "ndebug": diff.get("ndebug"), } @@ -95,6 +98,45 @@ 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', {}))}") + 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; " + 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()) + 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 f50ef31..4b83922 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) @@ -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"] @@ -417,6 +533,395 @@ 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=1"], 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=1"], []), (["LIB2=1"], [])], 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_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_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 _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_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. @@ -471,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), @@ -600,6 +1203,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 @@ -640,6 +1269,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.""" diff --git a/tests/test_extractors.py b/tests/test_extractors.py index fd282b7..b5ce4e9 100644 --- a/tests/test_extractors.py +++ b/tests/test_extractors.py @@ -139,6 +139,52 @@ 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_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) diff --git a/tests/test_triage.py b/tests/test_triage.py index cb36bcb..6ae065e 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -75,6 +75,51 @@ 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)) + + +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 + + +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_")]