From ddb5b91dea9367e6863a116cbd148c3c68ce1118 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:23:01 -0400 Subject: [PATCH] Compare every variant of a multi-variant source, deterministically _union_tus pooled library TUs into one source-keyed map with dict.setdefault while iterating a set of target names. When one source is compiled into several libraries with different flags (a full and a lite build of the same file, a hardened variant), only one variant per side was compared, and which one depended on set iteration order, that is, on PYTHONHASHSEED. The verdict was not reproducible. Measured in any2bazel-lab: the same MySQL Server models gave 4,715 to 4,817 errors over 10 hash seeds, a different error set each time, and the final models of a Blender migration converged under 2 of 8 seeds. Projects without multi-variant sources were unaffected (50 recorded verdicts identical under 20 seeds). Pooling now keeps every distinct variant per source, visiting targets in sorted order. Each CMake variant is compared with its best-matching Bazel variant: fewest errors, then warnings, then the smallest symmetric difference of the two TUs' define, flag and include sets, then a stable text order. Every CMake variant must be matched by some Bazel variant and nothing is suppressed; an extra Bazel-only variant is tolerated, like an extra Bazel flag. The distance term keeps a tie from choosing a variant that only adds flags (-w, -Wno-error=...) over an exact one. A source with one variant per side takes exactly the old path, and on every single-variant verdict the lab has recorded, the result is unchanged. Executables and the opt-in test union use the same pairing, and the Java source union is visited in sorted order too. One consequence for source_map users: a token that collapses several CMake TUs (an AUTOMOC bundle, say) now yields one comparison, and so possibly one discrepancy, per distinct CMake TU, so the worklist can grow. The source_map comment in config.py says so. In the lab's later QGIS run with this change, the differ gave the same verdict under three seeds. Lab-Proposal: P14 Co-Authored-By: Claude Opus 5.5 --- scripts/config.py | 7 ++-- scripts/diff.py | 88 +++++++++++++++++++++++++++++++++------ tests/test_engine.py | 99 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 15 deletions(-) diff --git a/scripts/config.py b/scripts/config.py index 4d2cbb2..ddf841e 100644 --- a/scripts/config.py +++ b/scripts/config.py @@ -88,9 +88,10 @@ class MigrationConfig: # bundles every moc_*.cpp into one mocs_compilation.cpp TU, while Bazel # compiles each moc_*.cpp separately. Mapping both spellings to one token # (the CMake bundle path AND the Bazel moc/ dir -> "@moc") reconciles them: - # the pooled TU maps collapse to the shared token, so presence matches and - # the representative TU's flags are still compared (extra Bazel defines stay - # benign WARN under the asymmetric-subset rule). Only ever use for codegen + # the pooled TU maps collapse to the shared token, so presence matches, and + # every mapped TU is still compared against its best-matching counterpart + # (diff._diff_variants; extra Bazel defines stay benign WARN under the + # asymmetric-subset rule). Only ever use for codegen # whose per-TU flags are uniform; never to paper over a real missing source. # Unlike include_map, the token REPLACES the whole path (the remainder is # dropped) so N generated files collapse to 1 -- see map_source(). diff --git a/scripts/diff.py b/scripts/diff.py index 9e927f0..20630ef 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -234,17 +234,79 @@ def _bazel_renames(a: CanonicalModel, b: CanonicalModel, TargetKind.INTERFACE} +def _add_variants(out: Dict[str, List[TranslationUnit]], tus, + cfg: "MigrationConfig") -> None: + """Add TUs to a source-keyed map of DISTINCT variants (see _union_tus).""" + for tu in tus: + variants = out.setdefault(cfg.map_source(tu.key()), []) + if tu not in variants: + variants.append(tu) + + def _union_tus(views: Dict[str, TargetView], names, - cfg: "MigrationConfig") -> Dict[str, TranslationUnit]: + cfg: "MigrationConfig") -> Dict[str, List[TranslationUnit]]: """Pool TUs of the given target views into one source-keyed map (the TU-SET comparison): grouping/renames/fold-ins don't matter -- every compiled source lands in one flat map keyed by repo-relative path. Keys are run through cfg.map_source so generated-source grouping asymmetries (e.g. CMake's single - AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token.""" - out: Dict[str, TranslationUnit] = {} - for n in names: - for tu in views[n].tus: - out.setdefault(cfg.map_source(tu.key()), tu) + AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token. + + Each key holds every DISTINCT variant of that source: one source compiled + into several libraries with different flags (a lite and a full build of the + same file, a hardened variant) is several TUs, and _diff_variants compares + all of them. Names are visited in sorted order so the result never depends + on set iteration order (i.e. on PYTHONHASHSEED).""" + out: Dict[str, List[TranslationUnit]] = {} + for n in sorted(names): + _add_variants(out, views[n].tus, cfg) + return out + + +def _tu_distance(a: TranslationUnit, b: TranslationUnit, + cfg: "MigrationConfig") -> int: + """Size of the symmetric difference of two TUs' define, flag and include + sets, after the reviewer's ignores. Unlike the discrepancies, this counts + flags only one side has, so an exact variant beats a noisy one (-w, + -Wno-error=...) that happens to produce the same discrepancies.""" + ad = {d for d in a.defines if not cfg.define_ignored(d)} + bd = {d for d in b.defines if not cfg.define_ignored(d)} + af = {f for f in a.flags if not cfg.flag_ignored(f)} + bf = {f for f in b.flags if not cfg.flag_ignored(f)} + ai = set(_norm_includes(a.includes, cfg)) + bi = set(_norm_includes(b.includes, cfg)) + return len(ad ^ bd) + len(af ^ bf) + len(ai ^ bi) + + +def _rank(discs: List[Discrepancy], distance: int) -> tuple: + """Order candidate pairings: fewest errors, then fewest warnings, then the + smallest distance between the two TUs (see _tu_distance), then a stable + text form so ties never depend on iteration order.""" + errors = sum(1 for d in discs if d.severity == Severity.ERROR.value) + return (errors, len(discs) - errors, distance, + json.dumps([asdict(d) for d in discs], sort_keys=True)) + + +def _best_match(target: str, a: TranslationUnit, b_vs: List[TranslationUnit], + cfg: "MigrationConfig"): + """(discrepancies, chosen Bazel TU) for CMake TU `a` against its + best-ranked Bazel variant (see _rank).""" + return min(((_diff_tu(target, a, b, cfg), b) for b in b_vs), + key=lambda pair: _rank(pair[0], _tu_distance(a, pair[1], cfg))) + + +def _diff_variants(target: str, a_vs: List[TranslationUnit], + b_vs: List[TranslationUnit], + cfg: "MigrationConfig") -> List[Discrepancy]: + """Compare every CMake variant of one source against its BEST-matching Bazel + variant (fewest errors, then warnings, then distance; see _rank). + Every CMake variant must be matched by some Bazel variant, so nothing is + suppressed: a variant Bazel lacks shows up as that variant's discrepancies + against its closest match. An extra Bazel-only variant is tolerated, like + any extra Bazel flag. With one variant per side this is exactly one + _diff_tu call.""" + out: List[Discrepancy] = [] + for a in a_vs: + out.extend(_best_match(target, a, b_vs, cfg)[0]) return out @@ -272,7 +334,7 @@ def _union_java_sources(views: Dict[str, TargetView], names) -> Dict[str, str]: (Maven's 2 groups vs Bazel's 1) is irrelevant; the SOURCE SET is what must match. First occurrence wins for the displayed original path.""" out: Dict[str, str] = {} - for n in names: + for n in sorted(names): for g in views[n].compile_groups: for src in g.sources: out.setdefault(_java_src_key(src), src) @@ -345,7 +407,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value, "", "source compiled in bazel but not cmake", tu=src)) for src in sorted(set(a_union) & set(b_union)): - out.extend(_diff_tu("", a_union[src], b_union[src], cfg)) + out.extend(_diff_variants("", a_union[src], b_union[src], cfg)) # ---- executables: identity-aligned, own TUs + own flags ---------------- for name in sorted(a_exes - b_exes): @@ -358,8 +420,10 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, ta, tb = a_views[name], b_views[name] # TU keys go through cfg.map_source (see _union_tus) so codegen grouping # asymmetries collapse to a shared token on both sides. - amap = {cfg.map_source(k): v for k, v in ta.tu_map().items()} - bmap = {cfg.map_source(k): v for k, v in tb.tu_map().items()} + amap: Dict[str, List[TranslationUnit]] = {} + bmap: Dict[str, List[TranslationUnit]] = {} + _add_variants(amap, ta.tus, cfg) + _add_variants(bmap, tb.tus, cfg) # Presence is judged against the whole other side (a_all/b_all), not just # this exe's own TUs: a source this exe compiles may live in a library on # the other side (or vice versa) -- that's a grouping difference, not a @@ -372,7 +436,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, out.append(Discrepancy(Kind.EXTRA_TU.value, Severity.WARN.value, name, "source compiled in bazel but not cmake", tu=src)) for src in sorted(set(amap) & set(bmap)): - out.extend(_diff_tu(name, amap[src], bmap[src], cfg)) + out.extend(_diff_variants(name, amap[src], bmap[src], cfg)) # link flags: asymmetric subset, same policy as compile flags -- every # CMake link flag must be present on the Bazel side; extra Bazel link # flags tolerated (toolchain link noise already stripped). Reviewer @@ -468,7 +532,7 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], out.append(Discrepancy(Kind.EXTRA_TEST_TU.value, Severity.WARN.value, "", "test source compiled in bazel but not cmake", tu=src)) for src in sorted(set(a_union) & set(b_union)): - out.extend(_diff_tu("", a_union[src], b_union[src], cfg)) + out.extend(_diff_variants("", a_union[src], b_union[src], cfg)) # 2. test-binary existence by count if len(a_tests) != len(b_tests): diff --git a/tests/test_engine.py b/tests/test_engine.py index f21a261..95b3b5d 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -417,6 +417,105 @@ def test_source_map_reconciles_codegen_bundling_asymmetry(): assert summarize(diff_models(a, b, cfg))["converged"] +def _variant_models(cmake_libs, bazel_libs): + """One source (src/x.cc) compiled into several libraries per side, each + library a {name: [defines]} entry -- a multi-variant source.""" + def build(libs, is_bazel): + m = _bs(CanonicalModel(), is_bazel) + for name, defs in libs.items(): + raw = ["-D" + d for d in defs] + ["-std=c++17"] + m.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/x.cc", raw, is_bazel)])) + return m + return build(cmake_libs, False), build(bazel_libs, True) + + +def test_each_cmake_variant_is_paired_with_its_best_bazel_variant(): + # libprotobuf / libprotobuf-lite style: the same source, two flag sets, on + # both sides. Every variant has an exact counterpart -> converged. + a, b = _variant_models({"proto": ["FULL"], "proto_lite": ["LITE"]}, + {"pkg:proto": ["FULL"], "pkg:proto_lite": ["LITE"]}) + res = summarize(diff_models(a, b)) + assert res["converged"], res + + +def test_a_cmake_variant_missing_on_bazel_is_caught(): + a, b = _variant_models({"proto": ["FULL"], "proto_lite": ["LITE"]}, + {"pkg:proto_lite": ["LITE"]}) + discs = diff_models(a, b) + errs = [d for d in discs if d.severity == Severity.ERROR.value] + assert len(errs) == 1 and errs[0].kind == "defines_diff", errs + assert errs[0].cmake_only == ["FULL"], errs + + +def test_an_extra_bazel_variant_is_tolerated(): + a, b = _variant_models({"proto_lite": ["LITE"]}, + {"pkg:proto": ["FULL"], "pkg:proto_lite": ["LITE"]}) + discs = diff_models(a, b) + assert summarize(discs)["converged"], discs + assert discs == [], discs # the matching variant was chosen, not FULL + + +def test_tied_pairings_prefer_the_smaller_difference(): + # Three CMake bundles collapse to one source token via source_map, each + # with its own define; Bazel has a LIB0 TU and a TU with no define. LIB1 + # and LIB2 tie on errors against both; the pairing must be the one that + # does not report LIB0 (another variant's define) as bazel_only. + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw(f"gen/a{i}.cc", [f"-DLIB{i}"], False) for i in range(3)])) + b.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, actions=[ + tu_from_raw("gen/b0.cc", ["-DLIB0"], True), + tu_from_raw("gen/b1.cc", [], True)])) + discs = diff_models(a, b, MigrationConfig(source_map=(("gen/", "@gen"),))) + assert [(d.cmake_only, d.bazel_only) for d in discs] == \ + [(["LIB1"], []), (["LIB2"], [])], discs + + +def test_tied_pairings_prefer_the_exact_variant_over_a_noisy_one(): + # Both Bazel variants produce no discrepancy against CMake {-std=c++17}; + # the one that only ADDS flags (-w, -Wno-error=foo) must not be chosen. + from diff import _best_match + from reconstruct import reconstruct + a, b = _bs(CanonicalModel(), False), _bs(CanonicalModel(), True) + a.add(Target("l", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x.cc", ["-std=c++17"], False)])) + for name, raw in (("a:noisy", ["-std=c++17", "-w", "-Wno-error=foo"]), + ("b:exact", ["-std=c++17"])): + b.add(Target(name, TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("x.cc", raw, True)])) + ta = reconstruct(a)["l"].tus[0] + b_vs = [reconstruct(b)[n].tus[0] for n in ("a:noisy", "b:exact")] + discs, chosen = _best_match("", ta, b_vs, MigrationConfig()) + assert discs == [] and chosen.flags == ("-std=c++17",), chosen + + +def test_multi_variant_verdict_does_not_depend_on_hash_seed(): + # Regression: libraries used to be pooled with dict.setdefault while + # iterating a SET of names, so which variant got compared -- and the + # verdict -- changed with PYTHONHASHSEED. + import json + import subprocess + import tempfile + from serialize import dump_model + libs = {f"lib{i}": (["LITE"] if i % 2 else ["FULL"]) for i in range(8)} + a, b = _variant_models(libs, {"pkg:" + k: v for k, v in libs.items()}) + b.targets.pop("pkg:lib0") # FULL is still built by pkg:lib2, pkg:lib4, ... + with tempfile.TemporaryDirectory() as d: + pa, pb = os.path.join(d, "a.json"), os.path.join(d, "b.json") + dump_model(a, pa) + dump_model(b, pb) + script = os.path.join(os.path.dirname(__file__), "..", "scripts", "diff.py") + outs = set() + for seed in range(8): + env = dict(os.environ, PYTHONHASHSEED=str(seed)) + out = subprocess.run([sys.executable, script, pa, pb], env=env, + capture_output=True, text=True, check=True).stdout + outs.add(out) + assert len(outs) == 1, "diff output varies with PYTHONHASHSEED" + assert json.loads(outs.pop())["converged"] + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error.