diff: fixes for shared/static twins, -iquote-only roots, -std= and PIC duplicates

SKILL.md

@@ -76,6 +76,12 @@ 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.
+- **Every variant of a source counts.** When the reference compiles one source
+  several ways (a shared/static twin: the shared objects carry
+  `-D<target>_EXPORTS` and `-fPIC`), a Bazel TU converges if it satisfies
+  **any** variant. Otherwise it is reported against the closest one, and
+  `detail` names that CMake target and the Bazel target. Never rename a Bazel
+  target to steer which twin it is compared with.
 - **Executables** align by **name** (a missing exe is a real gap); use
   `target_map` for renames.
 
@@ -312,8 +318,8 @@ correctness flags).
 |------------------|-----|
 | `missing_target` | add the missing `cc_binary`, or `target_map` a renamed exe, or `exclude_targets` if out of scope |
 | `missing_tu`     | add the source to some library's `srcs`, or `exclude_targets` if it's a vendored/out-of-scope subtree |
-| `defines_diff`   | add each `cmake_only` define to `defines`, or `ignore.defines` it |
-| `includes_diff`  | add the missing CMake include root to `includes`; for a dep whose root is spelled differently each side, `ignore.include_map` it (preferred) or `ignore.include_prefixes` it |
+| `defines_diff`   | add each `cmake_only` define to `defines`, or `ignore.defines` it. A `<target>_EXPORTS` marker of a shared/static twin needs neither (the static twin already matches); only a shared-only library needs `ignore.defines` for it |
+| `includes_diff`  | add the missing CMake include root to `includes`; for a dep whose root is spelled differently each side, `ignore.include_map` it (preferred) or `ignore.include_prefixes` it. Detail `present on bazel side only as -iquote`: the root is there for `#include "..."` only (Bazel's workspace root), so `#include <...>` will not find it; put it in `includes` |
 | `flags_diff`     | add each `cmake_only` flag to `copts`, or `ignore.flags` it |
 | `link_flags_diff`| add each `cmake_only` flag to the target's `linkopts`, or `ignore.link_flags` it if benign (e.g. a compile flag CMake repeats at link) |
 | `missing_dep`    | add the missing external/system dep to the target's `deps`/`linkopts`; if it's just a name spelled differently per build (`Catch2Main` vs `catch2_main`, `OpenSSL::SSL` vs `ssl`), add a `dep_map` entry. External deps are captured from both `-l` flags and archive-file inputs (e.g. `external/catch2+/libcatch2_main.a`) |

scripts/canonicalize.py

@@ -113,8 +113,21 @@
     "-fexperimental-",
     "-Wl,-install_name",
     "-Wl,-rpath,__BAZEL",       # bazel sandbox rpaths
+    "-pass-exit-codes",         # gcc driver mechanics (unix toolchain link_flags)
 )
 
+# Compile flags with LAST-WINS semantics in the driver: a project copt
+# `-std=c++11` after a toolchain `-std=c++17` is what the compiler honours,
+# so only the last spelling on a side is its language standard; keeping both
+# would let a Bazel side that carries the reference's -std= *and* a later,
+# different one pass as equivalent.
+_LAST_WINS_PREFIXES = ("-std=",)
+
+# Position-independence flags: the one difference between the PIC and
+# non-PIC compile of a source Bazel builds for both an archive and a shared
+# library (`.pic.o` / `.o`). diff.py's variant matching reads this set.
+PIC_FLAGS = frozenset({"-fPIC", "-fpic", "-fPIE", "-fpie"})
+
 # Link flags with no correctness meaning on either side (build-type / debug).
 IGNORABLE_LINK_PREFIXES = (
     "-g",
@@ -203,16 +216,37 @@ def canonicalize_flags(
     - includes: repo-relative, ORDER PRESERVED, dedup-adjacent only
     - other_flags: correctness-relevant only (ignorable + bazel-default dropped),
       sorted for stable comparison
+    (canonicalize_compile also reports which roots are quote-only.)
     """
+    return canonicalize_compile(raw, repo_root, is_bazel=is_bazel)[:3]
+
+
+def canonicalize_compile(
+    raw: List[str],
+    repo_root: str,
+    *,
+    is_bazel: bool,
+) -> Tuple[Tuple[str, ...], Tuple[str, ...], Tuple[str, ...], Tuple[str, ...]]:
+    """canonicalize_flags plus a fourth element: the include roots this argv
+    reaches ONLY through `-iquote`. `-I`, `-isystem` and `-idirafter` roots
+    serve `#include <x.h>` and `#include "x.h"` alike; an `-iquote` root
+    serves quoted includes only. Bazel puts the workspace root and the
+    genfiles root on `-iquote` for every compile, so a reference `-I<src>`
+    root looks present on the Bazel side while `<x.h>` still cannot find it
+    (zlib's `#include <zconf.h>`). The differ reports such a root as missing
+    rather than present."""
     defines: List[str] = []
     includes: List[str] = []
+    angle: set = set()
+    quote: set = set()
     other: List[str] = []
 
     i = 0
     n = len(raw)
     # On the Bazel side the first argv token is the compiler/wrapper path
-    # (driver mechanics). CMake File-API fragments don't include it.
-    if is_bazel and n and _is_driver_token(raw[0]):
+    # (driver mechanics), whatever its spelling: `/usr/bin/gcc`, a relative
+    # wrapper script, a bare `clang`. CMake File-API fragments never carry it.
+    if is_bazel and n and not raw[0].startswith("-"):
         i = 1
     while i < n:
         tok = raw[i]
@@ -242,10 +276,13 @@ def canonicalize_flags(
 
         # include flavors: -I, -isystem, -iquote, -idirafter (split or joined)
         if tok in ("-I", "-isystem", "-iquote", "-idirafter") and i + 1 < n:
-            includes.append(_to_repo_relative(raw[i + 1], repo_root))
+            root = _to_repo_relative(raw[i + 1], repo_root)
+            includes.append(root)
+            (quote if tok == "-iquote" else angle).add(root)
             i += 2; continue
         if tok.startswith("-I"):
-            includes.append(_to_repo_relative(tok[2:], repo_root)); i += 1; continue
+            root = _to_repo_relative(tok[2:], repo_root)
+            includes.append(root); angle.add(root); i += 1; continue
 
         # drop pure-noise bazel toolchain flags on the bazel side only. NOTE:
         # tolerated-default flags (-fstack-protector, -fdiagnostics-color, ...)
@@ -267,10 +304,16 @@ def canonicalize_flags(
     for inc in includes:
         if not canon_includes or canon_includes[-1] != inc:
             canon_includes.append(inc)
+    # last-wins flags: only the last spelling on this side counts
+    for prefix in _LAST_WINS_PREFIXES:
+        hits = [f for f in other if f.startswith(prefix)]
+        if len(hits) > 1:
+            other = [f for f in other if not f.startswith(prefix)] + [hits[-1]]
     # other flags: order-insensitive for parity
     canon_other = tuple(sorted(set(other)))
+    quote_only = tuple(r for r in canon_includes if r in quote and r not in angle)
 
-    return canon_defines, tuple(canon_includes), canon_other
+    return canon_defines, tuple(canon_includes), canon_other, quote_only
 
 
 def canonicalize_link_flags(raw: List[str], *, is_bazel: bool) -> Tuple[str, ...]:
@@ -290,8 +333,8 @@ def canonicalize_link_flags(raw: List[str], *, is_bazel: bool) -> Tuple[str, ...
     out: List[str] = []
     i = 0
     n = len(raw)
-    if is_bazel and n and _is_link_input(raw[0]):
-        i = 1  # leading linker/wrapper path
+    if is_bazel and n and not raw[0].startswith("-"):
+        i = 1  # leading linker/wrapper path, whatever its spelling
     while i < n:
         tok = raw[i]
         if tok in _LINK_PAIR_FLAGS and i + 1 < n:

scripts/diff.py

@@ -18,7 +18,10 @@
 migrate loop calls this; an empty worklist (for a full round) means "done".
 Parity has two stages, both reported here:
   COMPILE PARITY    -- per-TU flag/define/include equivalence (project-wide
-                       TU-set) + external link-dependency closure.
+                       TU-set; a source the reference compiles several ways,
+                       e.g. in a shared/static twin, is matched against ANY of
+                       its variants -- see _union_tus) + external
+                       link-dependency closure.
   LINK CONSISTENCY  -- per name-aligned executable/shared-lib, equivalent link
                        flags (link_flags_diff).
 
@@ -32,11 +35,12 @@
 from __future__ import annotations
 
 import json
+import re
 from dataclasses import asdict, dataclass
 from enum import Enum
-from typing import Dict, List, Optional
+from typing import Dict, List, Optional, Tuple
 
-from canonicalize import BAZEL_TOLERATED_FLAG_PREFIXES
+from canonicalize import BAZEL_TOLERATED_FLAG_PREFIXES, PIC_FLAGS
 from config import MigrationConfig
 from model import (CanonicalModel, TargetKind, TargetRole, TranslationUnit)
 from reconstruct import TargetView, reconstruct
@@ -136,6 +140,27 @@ def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit,
             cmake_only=missing,
             bazel_only=[i for i in b_inc if i not in set(a_inc)],
         ))
+    # A root present on both sides but reachable on the Bazel side ONLY
+    # through -iquote is not the same search directory: the reference's -I
+    # serves `#include <x.h>`, Bazel's -iquote does not (Bazel puts the
+    # workspace and genfiles roots on -iquote for every compile, so a
+    # reference `-I<src>` root always *looks* present). Reported apart from
+    # `missing` so the fix reads: make the root angle-capable (`includes`).
+    b_quote = set(_norm_includes(b.quote_only, cfg))
+    a_quote = set(_norm_includes(a.quote_only, cfg))
+    quote_only = [i for i in a_inc
+                  if i in set(b_inc) and i in b_quote and i not in a_quote]
+    if quote_only:
+        out.append(Discrepancy(
+            kind=Kind.INCLUDES_DIFF.value,
+            severity=Severity.ERROR.value,
+            target=target, tu=a.source,
+            detail="cmake include root present on bazel side only as -iquote "
+                   "(quoted includes): `#include <...>` cannot find it there; "
+                   "put it in `includes` or an -I copt",
+            cmake_only=quote_only,
+            bazel_only=[],
+        ))
 
     # other flags: asymmetric subset -- A must be subset of B.
     # Reviewer-approved ignores are dropped from both sides first.
@@ -224,17 +249,158 @@ def _apply_target_map(views: Dict[str, TargetView],
                   TargetKind.INTERFACE}
 
 
+@dataclass
+class TUVariant:
+    """One compilation of a source by one target: the TU plus who compiled it.
+    A source compiled by several targets of a side (CMake's shared/static
+    twins; a Bazel cc_library built both PIC and non-PIC) has several."""
+    tu: TranslationUnit
+    target: str
+    kind: str          # TargetKind value of the owning target
+
+    def signature(self):
+        return (self.tu.defines, self.tu.includes, self.tu.flags)
+
+
 def _union_tus(views: Dict[str, TargetView], names,
-               cfg: "MigrationConfig") -> Dict[str, TranslationUnit]:
+               cfg: "MigrationConfig") -> Dict[str, List[TUVariant]]:
     """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:
+    AUTOMOC bundle vs Bazel's per-header moc_*.cpp) collapse to one token.
+
+    EVERY VARIANT of a source is kept, in sorted-target order (sorted: `names`
+    is usually a set, and the report must not depend on the hash seed).
+    CMake's shared/static twin (add_library(foo SHARED) + add_library(
+    foo-static STATIC) from one source list) compiles each file twice with
+    different argv (-Dfoo_EXPORTS, -fPIC, sometimes project defines), and a
+    Bazel cc_library compiles it once, PIC or not as the toolchain decides --
+    or twice when both a binary and a cc_shared_library consume it. Keeping
+    one representative would compare the Bazel TU against an arbitrary twin,
+    so migrations would rename targets or suppress *_EXPORTS markers to land
+    on the twin they match. Instead the comparison (_diff_tu_variants)
+    accepts a Bazel TU that satisfies ANY CMake variant. Two targets
+    compiling a source with identical canonical argv contribute one variant
+    (the first in sorted order names it)."""
+    out: Dict[str, List[TUVariant]] = {}
+    seen: Dict[str, set] = {}
+    for n in sorted(names):
         for tu in views[n].tus:
-            out.setdefault(cfg.map_source(tu.key()), tu)
+            key = cfg.map_source(tu.key())
+            v = TUVariant(tu, n, views[n].kind.value)
+            sig = v.signature()
+            if sig in seen.setdefault(key, set()):
+                continue
+            seen[key].add(sig)
+            out.setdefault(key, []).append(v)
+    return out
+
+
+def _pic(v: TUVariant) -> bool:
+    """Is this variant the position-independent one? A CMake SHARED target's
+    objects are PIC by construction; otherwise the argv says (-fPIC/-fpic).
+    Used only as a tie-break when two variants are equally close."""
+    return (v.kind == TargetKind.SHARED.value
+            or any(f in PIC_FLAGS for f in v.tu.flags))
+
+
+def _variant_distance(discs: List[Discrepancy]) -> Tuple[int, int]:
+    """How far a Bazel TU is from one CMake variant, from _diff_tu's findings:
+    (tokens the Bazel side is MISSING -- the errors -- , tokens it has EXTRA at
+    warn level). An error without a cmake_only list counts as one token."""
+    missing = sum(len(d.cmake_only or []) or 1 for d in discs
+                  if d.severity == Severity.ERROR.value)
+    extra = sum(len(d.bazel_only or []) for d in discs
+                if d.severity != Severity.ERROR.value)
+    return missing, extra
+
+
+def _diff_tu_variants(target: str, a_vars: List[TUVariant], b_var: TUVariant,
+                      cfg: "MigrationConfig", name_variants: bool) -> List[Discrepancy]:
+    """Compare ONE Bazel TU against every CMake variant of the same source.
+
+    The Bazel TU converges if it satisfies the asymmetric checks (_diff_tu)
+    against AT LEAST ONE variant: nothing is reported then, whichever twin it
+    was. Otherwise the findings against the CLOSEST variant are reported --
+    the one with the fewest missing tokens, then the fewest extra ones, then
+    the one of the same kind as the Bazel TU (PIC objects belong to the shared
+    twin, non-PIC to the static one), then the first in sorted-target order,
+    so the report is deterministic. With `name_variants` (either side has
+    more than one variant of this source) the detail names both the chosen
+    CMake target and the Bazel target the TU came from, so the reader knows
+    which argv pair to look at; a plain one-to-one source keeps the bare
+    detail text.
+    """
+    best = None
+    for av in a_vars:
+        discs = _diff_tu(target, av.tu, b_var.tu, cfg)
+        if not discs:
+            return []
+        missing, extra = _variant_distance(discs)
+        key = (missing, extra, _pic(av) != _pic(b_var))
+        if best is None or key < best[0]:
+            best = (key, av, discs)
+    _, av, discs = best
+    if name_variants:
+        which = (f"closest of {len(a_vars)} cmake variants" if len(a_vars) > 1
+                 else "cmake variant")
+        suffix = (f" ({which}: {av.target} [{av.kind}]; "
+                  f"bazel TU from {b_var.target})")
+        for d in discs:
+            d.detail += suffix
+    # CMake's DEFINE_SYMBOL marker (`<target>_EXPORTS`, on every SHARED/MODULE
+    # target) is the one define a migration meets on every shared-only
+    # library, and the fix is always the same; say so where it is reported
+    marker = _export_macro(av.target)
+    for d in discs:
+        if d.kind == Kind.DEFINES_DIFF.value and marker in (d.cmake_only or []):
+            d.detail += (f" [{marker} is the DEFINE_SYMBOL marker CMake adds to "
+                         f"every SHARED/MODULE target and no static twin matches: "
+                         f"carry it in local_defines, or record it in ignore.defines]")
+    return discs
+
+
+def _export_macro(target: str) -> str:
+    """CMake's default DEFINE_SYMBOL for a target: the name as a C identifier
+    (`fmt-c` -> `fmt_c`) plus `_EXPORTS`."""
+    return re.sub(r"[^A-Za-z0-9_]", "_", target) + "_EXPORTS"
+
+
+def _sans_pic(v: TUVariant):
+    """A variant's signature with the position-independence flags removed:
+    the PIC and non-PIC compiles of one Bazel cc_library are equal under it."""
+    d, i, f = v.signature()
+    return d, i, tuple(x for x in f if x not in PIC_FLAGS)
+
+
+def _diff_tu_unions(target: str, a_union: Dict[str, List[TUVariant]],
+                    b_union: Dict[str, List[TUVariant]],
+                    cfg: "MigrationConfig") -> List[Discrepancy]:
+    """The TU-set comparison for the sources both sides compile: every Bazel
+    variant must match some CMake variant (_diff_tu_variants). A CMake
+    variant no Bazel TU matches is not a finding on its own -- Bazel builds
+    a source once (PIC or not) where CMake's twin builds it twice."""
+    out: List[Discrepancy] = []
+    for src in sorted(set(a_union) & set(b_union)):
+        multi = len(a_union[src]) > 1 or len(b_union[src]) > 1
+        results = [(bv, _diff_tu_variants(target, a_union[src], bv, cfg, multi))
+                   for bv in b_union[src]]
+        # Bazel compiles a source twice when both an archive and a shared
+        # library consume it (`.o` and `.pic.o`), and the two compiles
+        # differ ONLY by the PIC flag. Which ones the toolchain builds is
+        # not a BUILD-file decision, so such PIC twins are judged as ONE
+        # compile: the twin closest to the reference speaks for both (a
+        # shared-only reference, every TU -fPIC, gets no `-fPIC missing`
+        # error for the non-PIC archive compile, and no second copy of a
+        # finding both twins share). The PIC twin wins a tie: it is the one
+        # the shared library links.
+        by_twin: Dict[tuple, List[tuple]] = {}
+        for bv, discs in results:
+            by_twin.setdefault(_sans_pic(bv), []).append((bv, discs))
+        for twins in by_twin.values():
+            twins.sort(key=lambda r: (_variant_distance(r[1]), not _pic(r[0])))
+            out.extend(twins[0][1])
     return out
 
 
@@ -321,6 +487,8 @@ def diff_models(a: CanonicalModel, b: CanonicalModel,
     b_all = _all_source_keys(b_views, cfg)
 
     # ---- libraries: project-wide TU-set comparison -------------------------
+    # Each side's union keeps every variant of a source (shared/static twins);
+    # a Bazel TU passes if it matches ANY CMake variant (_diff_tu_variants).
     a_union = _union_tus(a_views, a_libs, cfg)
     b_union = _union_tus(b_views, b_libs, cfg)
     for src in sorted(set(a_union) - b_all):
@@ -329,8 +497,7 @@ def diff_models(a: CanonicalModel, b: CanonicalModel,
     for src in sorted(set(b_union) - a_all):
         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_tu_unions("<libraries>", a_union, b_union, cfg))
 
     # ---- executables: identity-aligned, own TUs + own flags ----------------
     for name in sorted(a_exes - b_exes):
@@ -452,8 +619,7 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView],
     for src in sorted(set(b_union) - a_all):
         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_tu_unions("<tests>", a_union, b_union, cfg))
 
     # 2. test-binary existence by count
     if len(a_tests) != len(b_tests):