Compare every variant of a multi-variant source, deterministically

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().

scripts/diff.py

@@ -234,17 +234,79 @@ def _bazel_renames(a: CanonicalModel, b: CanonicalModel,
                   TargetKind.INTERFACE}
 
 
+def _add_variants(out: Dict[str, List[TranslationUnit]], tus,
+                  cfg: "MigrationConfig") -> None:
+    """Add TUs to a source-keyed map of DISTINCT variants (see _union_tus)."""
+    for tu in tus:
+        variants = out.setdefault(cfg.map_source(tu.key()), [])
+        if tu not in variants:
+            variants.append(tu)
+
+
 def _union_tus(views: Dict[str, TargetView], names,
-               cfg: "MigrationConfig") -> Dict[str, TranslationUnit]:
+               cfg: "MigrationConfig") -> Dict[str, List[TranslationUnit]]:
     """Pool TUs of the given target views into one source-keyed map (the TU-SET
     comparison): grouping/renames/fold-ins don't matter -- every compiled source
     lands in one flat map keyed by repo-relative path. Keys are run through
     cfg.map_source so generated-source grouping asymmetries (e.g. CMake's single
-    AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token."""
-    out: Dict[str, TranslationUnit] = {}
-    for n in names:
-        for tu in views[n].tus:
-            out.setdefault(cfg.map_source(tu.key()), tu)
+    AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token.
+
+    Each key holds every DISTINCT variant of that source: one source compiled
+    into several libraries with different flags (a lite and a full build of the
+    same file, a hardened variant) is several TUs, and _diff_variants compares
+    all of them. Names are visited in sorted order so the result never depends
+    on set iteration order (i.e. on PYTHONHASHSEED)."""
+    out: Dict[str, List[TranslationUnit]] = {}
+    for n in sorted(names):
+        _add_variants(out, views[n].tus, cfg)
+    return out
+
+
+def _tu_distance(a: TranslationUnit, b: TranslationUnit,
+                 cfg: "MigrationConfig") -> int:
+    """Size of the symmetric difference of two TUs' define, flag and include
+    sets, after the reviewer's ignores. Unlike the discrepancies, this counts
+    flags only one side has, so an exact variant beats a noisy one (-w,
+    -Wno-error=...) that happens to produce the same discrepancies."""
+    ad = {d for d in a.defines if not cfg.define_ignored(d)}
+    bd = {d for d in b.defines if not cfg.define_ignored(d)}
+    af = {f for f in a.flags if not cfg.flag_ignored(f)}
+    bf = {f for f in b.flags if not cfg.flag_ignored(f)}
+    ai = set(_norm_includes(a.includes, cfg))
+    bi = set(_norm_includes(b.includes, cfg))
+    return len(ad ^ bd) + len(af ^ bf) + len(ai ^ bi)
+
+
+def _rank(discs: List[Discrepancy], distance: int) -> tuple:
+    """Order candidate pairings: fewest errors, then fewest warnings, then the
+    smallest distance between the two TUs (see _tu_distance), then a stable
+    text form so ties never depend on iteration order."""
+    errors = sum(1 for d in discs if d.severity == Severity.ERROR.value)
+    return (errors, len(discs) - errors, distance,
+            json.dumps([asdict(d) for d in discs], sort_keys=True))
+
+
+def _best_match(target: str, a: TranslationUnit, b_vs: List[TranslationUnit],
+                cfg: "MigrationConfig"):
+    """(discrepancies, chosen Bazel TU) for CMake TU `a` against its
+    best-ranked Bazel variant (see _rank)."""
+    return min(((_diff_tu(target, a, b, cfg), b) for b in b_vs),
+               key=lambda pair: _rank(pair[0], _tu_distance(a, pair[1], cfg)))
+
+
+def _diff_variants(target: str, a_vs: List[TranslationUnit],
+                   b_vs: List[TranslationUnit],
+                   cfg: "MigrationConfig") -> List[Discrepancy]:
+    """Compare every CMake variant of one source against its BEST-matching Bazel
+    variant (fewest errors, then warnings, then distance; see _rank).
+    Every CMake variant must be matched by some Bazel variant, so nothing is
+    suppressed: a variant Bazel lacks shows up as that variant's discrepancies
+    against its closest match. An extra Bazel-only variant is tolerated, like
+    any extra Bazel flag. With one variant per side this is exactly one
+    _diff_tu call."""
+    out: List[Discrepancy] = []
+    for a in a_vs:
+        out.extend(_best_match(target, a, b_vs, cfg)[0])
     return out
 
 
@@ -272,7 +334,7 @@ def _union_java_sources(views: Dict[str, TargetView], names) -> Dict[str, str]:
     (Maven's 2 groups vs Bazel's 1) is irrelevant; the SOURCE SET is what must
     match. First occurrence wins for the displayed original path."""
     out: Dict[str, str] = {}
-    for n in names:
+    for n in sorted(names):
         for g in views[n].compile_groups:
             for src in g.sources:
                 out.setdefault(_java_src_key(src), src)
@@ -345,7 +407,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel,
         out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value,
                                "<libraries>", "source compiled in bazel but not cmake", tu=src))
     for src in sorted(set(a_union) & set(b_union)):
-        out.extend(_diff_tu("<libraries>", a_union[src], b_union[src], cfg))
+        out.extend(_diff_variants("<libraries>", a_union[src], b_union[src], cfg))
 
     # ---- executables: identity-aligned, own TUs + own flags ----------------
     for name in sorted(a_exes - b_exes):
@@ -358,8 +420,10 @@ def diff_models(a: CanonicalModel, b: CanonicalModel,
         ta, tb = a_views[name], b_views[name]
         # TU keys go through cfg.map_source (see _union_tus) so codegen grouping
         # asymmetries collapse to a shared token on both sides.
-        amap = {cfg.map_source(k): v for k, v in ta.tu_map().items()}
-        bmap = {cfg.map_source(k): v for k, v in tb.tu_map().items()}
+        amap: Dict[str, List[TranslationUnit]] = {}
+        bmap: Dict[str, List[TranslationUnit]] = {}
+        _add_variants(amap, ta.tus, cfg)
+        _add_variants(bmap, tb.tus, cfg)
         # Presence is judged against the whole other side (a_all/b_all), not just
         # this exe's own TUs: a source this exe compiles may live in a library on
         # the other side (or vice versa) -- that's a grouping difference, not a
@@ -372,7 +436,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel,
             out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value,
                                    name, "source compiled in bazel but not cmake", tu=src))
         for src in sorted(set(amap) & set(bmap)):
-            out.extend(_diff_tu(name, amap[src], bmap[src], cfg))
+            out.extend(_diff_variants(name, amap[src], bmap[src], cfg))
         # link flags: asymmetric subset, same policy as compile flags -- every
         # CMake link flag must be present on the Bazel side; extra Bazel link
         # flags tolerated (toolchain link noise already stripped). Reviewer
@@ -468,7 +532,7 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView],
         out.append(Discrepancy(Kind.EXTRA_TEST_TU.value, Severity.WARN.value,
                                "<tests>", "test source compiled in bazel but not cmake", tu=src))
     for src in sorted(set(a_union) & set(b_union)):
-        out.extend(_diff_tu("<tests>", a_union[src], b_union[src], cfg))
+        out.extend(_diff_variants("<tests>", a_union[src], b_union[src], cfg))
 
     # 2. test-binary existence by count
     if len(a_tests) != len(b_tests):

tests/test_engine.py

@@ -417,6 +417,105 @@ def test_source_map_reconciles_codegen_bundling_asymmetry():
     assert summarize(diff_models(a, b, cfg))["converged"]
 
 
+def _variant_models(cmake_libs, bazel_libs):
+    """One source (src/x.cc) compiled into several libraries per side, each
+    library a {name: [defines]} entry -- a multi-variant source."""
+    def build(libs, is_bazel):
+        m = _bs(CanonicalModel(), is_bazel)
+        for name, defs in libs.items():
+            raw = ["-D" + d for d in defs] + ["-std=c++17"]
+            m.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION,
+                         actions=[tu_from_raw("src/x.cc", raw, is_bazel)]))
+        return m
+    return build(cmake_libs, False), build(bazel_libs, True)
+
+
+def test_each_cmake_variant_is_paired_with_its_best_bazel_variant():
+    # libprotobuf / libprotobuf-lite style: the same source, two flag sets, on
+    # both sides. Every variant has an exact counterpart -> converged.
+    a, b = _variant_models({"proto": ["FULL"], "proto_lite": ["LITE"]},
+                           {"pkg:proto": ["FULL"], "pkg:proto_lite": ["LITE"]})
+    res = summarize(diff_models(a, b))
+    assert res["converged"], res
+
+
+def test_a_cmake_variant_missing_on_bazel_is_caught():
+    a, b = _variant_models({"proto": ["FULL"], "proto_lite": ["LITE"]},
+                           {"pkg:proto_lite": ["LITE"]})
+    discs = diff_models(a, b)
+    errs = [d for d in discs if d.severity == Severity.ERROR.value]
+    assert len(errs) == 1 and errs[0].kind == "defines_diff", errs
+    assert errs[0].cmake_only == ["FULL"], errs
+
+
+def test_an_extra_bazel_variant_is_tolerated():
+    a, b = _variant_models({"proto_lite": ["LITE"]},
+                           {"pkg:proto": ["FULL"], "pkg:proto_lite": ["LITE"]})
+    discs = diff_models(a, b)
+    assert summarize(discs)["converged"], discs
+    assert discs == [], discs  # the matching variant was chosen, not FULL
+
+
+def test_tied_pairings_prefer_the_smaller_difference():
+    # Three CMake bundles collapse to one source token via source_map, each
+    # with its own define; Bazel has a LIB0 TU and a TU with no define. LIB1
+    # and LIB2 tie on errors against both; the pairing must be the one that
+    # does not report LIB0 (another variant's define) as bazel_only.
+    a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True)
+    a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[
+        tu_from_raw(f"gen/a{i}.cc", [f"-DLIB{i}"], False) for i in range(3)]))
+    b.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[
+        tu_from_raw("gen/b0.cc", ["-DLIB0"], True),
+        tu_from_raw("gen/b1.cc", [], True)]))
+    discs = diff_models(a, b, MigrationConfig(source_map=(("gen/", "@gen"),)))
+    assert [(d.cmake_only, d.bazel_only) for d in discs] == \
+        [(["LIB1"], []), (["LIB2"], [])], discs
+
+
+def test_tied_pairings_prefer_the_exact_variant_over_a_noisy_one():
+    # Both Bazel variants produce no discrepancy against CMake {-std=c++17};
+    # the one that only ADDS flags (-w, -Wno-error=foo) must not be chosen.
+    from diff import _best_match
+    from reconstruct import reconstruct
+    a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True)
+    a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION,
+                 actions=[tu_from_raw("x.cc", ["-std=c++17"], False)]))
+    for name, raw in (("a:noisy", ["-std=c++17", "-w", "-Wno-error=foo"]),
+                      ("b:exact", ["-std=c++17"])):
+        b.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION,
+                     actions=[tu_from_raw("x.cc", raw, True)]))
+    ta = reconstruct(a)["l"].tus[0]
+    b_vs = [reconstruct(b)[n].tus[0] for n in ("a:noisy", "b:exact")]
+    discs, chosen = _best_match("<libraries>", ta, b_vs, MigrationConfig())
+    assert discs == [] and chosen.flags == ("-std=c++17",), chosen
+
+
+def test_multi_variant_verdict_does_not_depend_on_hash_seed():
+    # Regression: libraries used to be pooled with dict.setdefault while
+    # iterating a SET of names, so which variant got compared -- and the
+    # verdict -- changed with PYTHONHASHSEED.
+    import json
+    import subprocess
+    import tempfile
+    from serialize import dump_model
+    libs = {f"lib{i}": (["LITE"] if i % 2 else ["FULL"]) for i in range(8)}
+    a, b = _variant_models(libs, {"pkg:" + k: v for k, v in libs.items()})
+    b.targets.pop("pkg:lib0")  # FULL is still built by pkg:lib2, pkg:lib4, ...
+    with tempfile.TemporaryDirectory() as d:
+        pa, pb = os.path.join(d, "a.json"), os.path.join(d, "b.json")
+        dump_model(a, pa)
+        dump_model(b, pb)
+        script = os.path.join(os.path.dirname(__file__), "..", "scripts", "diff.py")
+        outs = set()
+        for seed in range(8):
+            env = dict(os.environ, PYTHONHASHSEED=str(seed))
+            out = subprocess.run([sys.executable, script, pa, pb], env=env,
+                                 capture_output=True, text=True, check=True).stdout
+            outs.add(out)
+    assert len(outs) == 1, "diff output varies with PYTHONHASHSEED"
+    assert json.loads(outs.pop())["converged"]
+
+
 def test_ignore_flags_and_defines_suppress_diffs():
     # A reviewer-approved flag/define difference is suppressed via config and
     # the diff converges; without the config it's an error.