diff --git a/SKILL.md b/SKILL.md index 1b5af20..bcefaf1 100644 --- a/SKILL.md +++ b/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_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 `_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`) | diff --git a/scripts/canonicalize.py b/scripts/canonicalize.py index f2b8c66..5491051 100644 --- a/scripts/canonicalize.py +++ b/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 ` 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` + root looks present on the Bazel side while `` still cannot find it + (zlib's `#include `). 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: diff --git a/scripts/diff.py b/scripts/diff.py index d602dcb..2761c94 100644 --- a/scripts/diff.py +++ b/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 `, Bazel's -iquote does not (Bazel puts the + # workspace and genfiles roots on -iquote for every compile, so a + # reference `-I` 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 (`_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, "", "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_tu_unions("", 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, "", "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_tu_unions("", a_union, b_union, cfg)) # 2. test-binary existence by count if len(a_tests) != len(b_tests): diff --git a/scripts/extract_cmake.py b/scripts/extract_cmake.py index f070f1f..78ea77e 100644 --- a/scripts/extract_cmake.py +++ b/scripts/extract_cmake.py @@ -139,10 +139,94 @@ def _target_id_to_name(codemodel: dict) -> Dict[str, str]: return out -def _parse_target(tobj: dict, repo_root: str) -> Target: +def _source_path(path: str, source_dir: Optional[str], repo_root: str) -> str: + """Key a codemodel source by its path relative to repo_root. + + The File API spells a source relative to the CMake *top-level source dir* + (absolute only when it lives outside it). When the CMake project is not the + repo root -- a package vendored under third_party/, a monorepo with several + CMake projects -- that spelling never matches the Bazel side, which keys the + same file workspace-relative. Anchor it on the codemodel's `paths.source` + and re-relativize against repo_root; a source outside repo_root stays + absolute (the differ leaves those alone).""" + if source_dir and not os.path.isabs(path): + path = os.path.normpath(os.path.join(source_dir, path)) + if os.path.isabs(path) and repo_root: + rel = os.path.relpath(path, repo_root) + if not rel.startswith(".."): + return rel.replace(os.sep, "/") + return path.replace(os.sep, "/") + + +# Include-root flags whose argument is a DIRECTORY that the compiler resolves +# against its CWD. Joined (-I..) or split (-I ..) forms both occur in fragments. +_INCLUDE_FLAGS = ("-I", "-isystem", "-iquote", "-idirafter") + + +def _include_root(path: str, target_source_dir: Optional[str], + repo_root: str) -> str: + """Spell a RELATIVE include root the way the Bazel side spells it. + + A subdirectory CMakeLists that does INCLUDE_DIRECTORIES(..) or + ADD_DEFINITIONS(-I..) yields a root spelled relative to that directory. + CMake defines a relative include dir as relative to + CMAKE_CURRENT_SOURCE_DIR; the raw -I.. flag is the same intent spelled by + hand (it only ever names a real header root for in-source builds, where + the compiler's CWD is the source tree -- OpenWrt's default). Stored + verbatim it can never match Bazel's workspace-relative `third_party/`, + and every such package needed an include_map entry for `..`. Resolve it + against the target's source dir, normalize (`lua/..` -> package root) and + re-relativize against repo_root, exactly as sources are keyed. Absolute + roots are left alone: canonicalize.py already normalizes those (and + `examples/..` collapses there).""" + if os.path.isabs(path) or not target_source_dir: + return path + return _source_path(path, target_source_dir, repo_root) + + +def _resolve_include_tokens(tokens: List[str], target_source_dir: Optional[str], + repo_root: str) -> List[str]: + """Rewrite the path of every -I/-isystem/-iquote/-idirafter token in a + compileCommandFragments argv through _include_root; everything else is kept + verbatim (the model stores raw argv -- only the SPELLING of a CWD-relative + path is fixed, since that path is meaningless outside its CWD).""" + out: List[str] = [] + i, n = 0, len(tokens) + while i < n: + tok = tokens[i] + if tok in _INCLUDE_FLAGS and i + 1 < n: + out.append(tok) + out.append(_include_root(tokens[i + 1], target_source_dir, repo_root)) + i += 2 + continue + for flag in _INCLUDE_FLAGS: + if tok.startswith(flag) and len(tok) > len(flag): + tok = flag + _include_root(tok[len(flag):], target_source_dir, + repo_root) + break + out.append(tok) + i += 1 + return out + + +def _target_source_dir(tobj: dict, source_dir: Optional[str]) -> Optional[str]: + """The directory whose CMakeLists defines this target: the codemodel's + top-level `paths.source` joined with the target's own `paths.source` + (`.` for the top level, `lua` for lua/CMakeLists.txt). None when the reply + carries no `paths` (older fixtures), which keeps relative roots verbatim.""" + if not source_dir: + return None + sub = (tobj.get("paths") or {}).get("source") or "." + return os.path.normpath(os.path.join(source_dir, sub)) + + +def _parse_target(tobj: dict, repo_root: str, + source_dir: Optional[str] = None) -> Target: name = tobj["name"] kind = _KIND.get(tobj.get("type", ""), TargetKind.UNKNOWN) - sources = [s["path"] for s in tobj.get("sources", [])] + sources = [_source_path(s["path"], source_dir, repo_root) + for s in tobj.get("sources", [])] + target_source_dir = _target_source_dir(tobj, source_dir) # 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 @@ -151,12 +235,14 @@ def _parse_target(tobj: dict, repo_root: str) -> Target: for cg in tobj.get("compileGroups", []): base: List[str] = [] for frag in cg.get("compileCommandFragments", []): - base.extend(_split_fragment(frag.get("fragment", ""))) + base.extend(_resolve_include_tokens( + _split_fragment(frag.get("fragment", "")), + target_source_dir, repo_root)) 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"]) + base.append(_include_root(inc["path"], target_source_dir, repo_root)) for idx in cg.get("sourceIndexes", []): src = sources[idx] argv = tuple(base + ["-c", src]) @@ -230,6 +316,8 @@ def extract(build_dir: str, repo_root: str, reply_dir = os.path.join(build_dir, ".cmake", "api", "v1", "reply") codemodel = _find_codemodel(reply_dir) id_to_name = _target_id_to_name(codemodel) + # top-level CMake source dir; sources are spelled relative to it + source_dir = (codemodel.get("paths") or {}).get("source") model = CanonicalModel(build_system=BuildSystem.CMAKE, repo_root=repo_root) include_dirs = set() @@ -237,7 +325,7 @@ def extract(build_dir: str, repo_root: str, for tref in cfg["targets"]: with open(os.path.join(reply_dir, tref["jsonFile"])) as f: tobj = json.load(f) - target = _parse_target(tobj, repo_root) + target = _parse_target(tobj, repo_root, source_dir) _attach_deps(target, tobj, id_to_name) model.add(target) for cg in tobj.get("compileGroups", []): diff --git a/scripts/model.py b/scripts/model.py index 11c8cad..929c575 100644 --- a/scripts/model.py +++ b/scripts/model.py @@ -122,6 +122,11 @@ class TranslationUnit: includes: Tuple[str, ...] = () # repo-relative, ORDER PRESERVED (search order) flags: Tuple[str, ...] = () # other copts, canonicalized language: str = "CXX" + # Roots of `includes` this TU reaches ONLY through -iquote (quoted + # includes): Bazel puts the workspace and genfiles roots there for every + # compile, so a reference -I root can look present while `` still + # fails. Subset of `includes`; empty for a CMake TU in practice. + quote_only: Tuple[str, ...] = () def key(self) -> str: return self.source diff --git a/scripts/reconstruct.py b/scripts/reconstruct.py index 8e9a606..ff6cdd5 100644 --- a/scripts/reconstruct.py +++ b/scripts/reconstruct.py @@ -36,7 +36,7 @@ from dataclasses import dataclass, field from typing import List, Optional, Tuple -from canonicalize import canonicalize_flags, canonicalize_link_flags +from canonicalize import canonicalize_compile, canonicalize_link_flags from model import (Action, BuildSystem, CanonicalModel, Dependency, Target, TargetKind, TranslationUnit) @@ -238,11 +238,11 @@ def reconstruct_target(t: Target, build_system: BuildSystem, src = _source_from_compile_args(act.arguments) if not src: continue - cdef, cinc, cfl = canonicalize_flags( + cdef, cinc, cfl, cquote = canonicalize_compile( list(act.arguments), repo_root, is_bazel=is_bazel) view.tus.append(TranslationUnit( source=_rel(src, repo_root), defines=cdef, - includes=cinc, flags=cfl)) + includes=cinc, flags=cfl, quote_only=cquote)) elif act.mnemonic in _LINK_MNEMONICS: # CppArchive (static lib) has no meaningful link flags; only CppLink. if act.mnemonic == "CppLink": diff --git a/tests/test_engine.py b/tests/test_engine.py index c8a83ea..612cdc9 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -690,6 +690,275 @@ def test_nonparticipating_roles_are_excluded_not_diffed(): assert "Nightly" in res["excluded"]["cmake"]["dashboard"] +_TWIN_SCRIPT = r""" +import json, os, sys +sys.path.insert(0, sys.argv[1]) +from diff import diff_models, summarize +from model import Action, BuildSystem, CanonicalModel, Target, TargetKind, TargetRole +def tu(raw): + return Action(mnemonic="CppCompile", arguments=tuple(raw + ["-c", "foo/a.cpp"])) +a = CanonicalModel(build_system=BuildSystem.CMAKE, repo_root="/work/proj") +a.add(Target("foo", TargetKind.SHARED, role=TargetRole.PRODUCTION, + actions=[tu(["-Dfoo_EXPORTS", "-DX=1"])])) +a.add(Target("foo-static", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu(["-DX=1"])])) +b = CanonicalModel(build_system=BuildSystem.BAZEL, repo_root="/work/proj") +b.add(Target(":foo", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu(["-DX=1"])])) +print(json.dumps(summarize(diff_models(a, b)), sort_keys=True)) +""" + + +def test_shared_static_twin_representative_is_deterministic(): + # CMake builds the same source twice: as a SHARED lib (-Dfoo_EXPORTS) and + # as its STATIC twin (no define). The library TU-union keeps ONE + # representative per source; which one must not depend on target-name + # enumeration order (a set, so hash-seeded per process) or the reported + # defines_diff flips between runs. Run the diff under several hash seeds. + import subprocess + scripts = os.path.join(os.path.dirname(__file__), "..", "scripts") + outs = set() + for seed in range(8): + env = dict(os.environ, PYTHONHASHSEED=str(seed)) + outs.add(subprocess.check_output( + [sys.executable, "-c", _TWIN_SCRIPT, scripts], env=env).decode()) + assert len(outs) == 1, outs + + + +# ---- shared/static twins: a source compiled several ways matches ANY variant + +def _twin_models(shared_raw, static_raw, bazel_raws, src="foo/a.cpp"): + """CMake's shared/static twin: add_library(foo SHARED ${SOURCES}) + + add_library(foo-static STATIC ${SOURCES}), each compiling `src` with its + own argv. Bazel: one cc_library per entry of bazel_raws (label, argv).""" + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("foo", TargetKind.SHARED, role=TargetRole.PRODUCTION, + actions=[tu_from_raw(src, shared_raw, is_bazel=False)])) + a.add(Target("foo-static", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw(src, static_raw, is_bazel=False)])) + b = _bs(CanonicalModel(), is_bazel=True) + for label, raw in bazel_raws: + b.add(Target(label, TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw(src, raw, is_bazel=True)])) + return a, b + + +def test_twin_bazel_tu_matching_the_static_variant_converges(): + # The SHARED twin carries -Dfoo_EXPORTS and -fPIC; the STATIC twin + # neither. Bazel's non-PIC cc_library matches the static twin exactly. + # With one representative per source (`foo` sorts before `foo-static`) + # this was a defines_diff unless *_EXPORTS was suppressed. + a, b = _twin_models(["-Dfoo_EXPORTS", "-DX=1", "-fPIC"], ["-DX=1"], + [(":foo", ["-DX=1"])]) + discs = diff_models(a, b) + assert discs == [], discs + assert summarize(discs)["converged"] + + +def test_twin_bazel_tu_matching_the_shared_variant_converges(): + # The mirror: Bazel builds the PIC object with the shared twin's argv. + a, b = _twin_models(["-Dfoo_EXPORTS", "-DX=1", "-fPIC"], ["-DX=1"], + [(":foo", ["-Dfoo_EXPORTS", "-DX=1", "-fPIC"])]) + assert diff_models(a, b) == [] + + +def test_twin_pic_only_bazel_tu_converges_without_ignoring_exports(): + # Bazel compiles PIC (-fPIC) but has no EXPORTS marker. The static twin + # is satisfied (extra Bazel flags are tolerated), so no ignore.defines + # entry for foo_EXPORTS is needed. + a, b = _twin_models(["-Dfoo_EXPORTS", "-DX=1", "-fPIC"], ["-DX=1"], + [(":foo", ["-DX=1", "-fPIC"])]) + assert diff_models(a, b) == [] + + +def test_twin_bazel_matches_neither_variant_reports_closest(): + # Bazel drops a define both twins carry (-DX=1): neither variant is + # satisfied. The report is against the CLOSEST variant (fewest missing + # tokens: the static twin, 1 token vs 2) and names both the chosen CMake + # target and the Bazel target. + a, b = _twin_models(["-Dfoo_EXPORTS", "-DX=1", "-fPIC"], ["-DX=1"], + [(":foo", ["-DY=2"])]) + discs = diff_models(a, b) + errs = [d for d in discs if d.severity == "error"] + assert len(errs) == 1 and errs[0].kind == "defines_diff", discs + assert errs[0].cmake_only == ["X=1"], errs[0] + assert errs[0].bazel_only == ["Y=2"], errs[0] + assert errs[0].target == "" and errs[0].tu == "foo/a.cpp" + assert "foo-static" in errs[0].detail and "static_library" in errs[0].detail, errs[0].detail + assert "bazel TU from :foo" in errs[0].detail, errs[0].detail + assert not summarize(discs)["converged"] + + +def test_twin_closest_tie_prefers_the_same_kind(): + # Equally far from both twins (one missing token each): a PIC Bazel TU is + # reported against the SHARED twin, a non-PIC one against the STATIC twin. + shared, static = ["-Dfoo_EXPORTS", "-DX=1", "-fPIC"], ["-DSTATIC_ONLY", "-DX=1"] + a, b = _twin_models(shared, static, [(":foo", ["-DX=1", "-fPIC"])]) + d = [d for d in diff_models(a, b) if d.severity == "error"] + assert len(d) == 1 and d[0].cmake_only == ["foo_EXPORTS"], d + assert "cmake variants: foo [shared_library]" in d[0].detail, d[0].detail + a, b = _twin_models(shared, static, [(":foo", ["-DX=1"])]) + d = [d for d in diff_models(a, b) if d.severity == "error"] + assert len(d) == 1 and d[0].cmake_only == ["STATIC_ONLY"], d + assert "cmake variants: foo-static [static_library]" in d[0].detail, d[0].detail + + +def test_twin_every_bazel_variant_must_match_one_cmake_variant(): + # Bazel mirrors the twin (two cc_library targets): each Bazel TU is + # checked on its own. Both fine -> converged; one wrong -> exactly that + # one is reported, naming its Bazel target. + shared, static = ["-Dfoo_EXPORTS", "-DX=1", "-fPIC"], ["-DX=1"] + a, b = _twin_models(shared, static, [(":foo", shared), (":foo_static", static)]) + assert diff_models(a, b) == [] + a, b = _twin_models(shared, static, [(":foo", shared), (":foo_static", ["-DZ"])]) + errs = [d for d in diff_models(a, b) if d.severity == "error"] + assert len(errs) == 1 and "bazel TU from :foo_static" in errs[0].detail, errs + + +def test_single_variant_detail_is_unchanged(): + # No twin on either side: the detail text stays exactly as before. + a = _model(tu_from_raw("src/a.cpp", ["-DFOO=1"], is_bazel=False)) + b = _model(tu_from_raw("src/a.cpp", [], is_bazel=True), is_bazel=True) + d = diff_models(a, b) + assert len(d) == 1 and d[0].detail == "defines differ", d + + +def test_twin_ignore_defines_lever_still_applies(): + # A shared-only library (no static twin) whose Bazel cc_library omits the + # *_EXPORTS marker still needs ignore.defines. + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("plugin", TargetKind.SHARED, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("v.c", ["-Dplugin_EXPORTS", "-fPIC"], is_bazel=False)])) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target(":plugin", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("v.c", ["-fPIC"], is_bazel=True)])) + d = diff_models(a, b) + assert [x.kind for x in d] == ["defines_diff"] and d[0].cmake_only == ["plugin_EXPORTS"], d + # a single variant: no variant suffix -- but the marker is named, with the + # two fixes, since every shared-only library meets exactly this finding + assert d[0].detail.startswith("defines differ ["), d[0].detail + assert "plugin_EXPORTS is the DEFINE_SYMBOL marker" in d[0].detail + assert "local_defines" in d[0].detail and "ignore.defines" in d[0].detail + cfg = MigrationConfig(ignore_defines={"plugin_EXPORTS"}) + assert summarize(diff_models(a, b, cfg))["converged"] + + +def test_test_tu_union_matches_any_variant_when_opted_in(): + # The test-TU union shares the code path: a test source compiled in two + # test binaries with different defines is matched by either. + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("t1", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("t/x.cc", ["-DA"], is_bazel=False)])) + a.add(Target("t2", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("t/x.cc", ["-DB"], is_bazel=False)])) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target("t1", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("t/x.cc", ["-DB"], is_bazel=True)])) + b.add(Target("t2", TargetKind.EXECUTABLE, role=TargetRole.TEST, + actions=[tu_from_raw("t/x.cc", ["-DB"], is_bazel=True)])) + cfg = MigrationConfig(include_tests=True) + discs = [d for d in diff_models(a, b, cfg) if d.kind != "test_binary_count"] + assert discs == [], discs + + + +# ---- plain host builds: absolute driver path, -std= twice, -iquote roots ---- + +def _view_of(model, name): + from reconstruct import reconstruct + return reconstruct(model)[name] + + +def test_bazel_tool_path_is_never_a_flag(): + # the auto-configured host toolchain spells the driver absolutely; it + # is argv[0] mechanics on both the compile and the link, never a flag + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target("pkg:foo", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[Action(mnemonic="CppCompile", arguments=( + "/usr/bin/gcc", "-std=c++17", "-c", "pkg/a.cpp"))])) + b.add(_exe("app", ["/usr/bin/gcc", "-o", "bazel-out/bin/app", + "-pass-exit-codes", "-Wl,--gc-sections"], True)) + assert _view_of(b, "pkg:foo").tus[0].flags == ("-std=c++17",) + assert _view_of(b, "app").link_flags == ("-Wl,--gc-sections",) + # a CMake fragment never carries a tool; a leading flag is kept + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("foo", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("pkg/a.cpp", ["-std=c++17"], is_bazel=False)])) + assert _view_of(a, "foo").tus[0].flags == ("-std=c++17",) + + +def test_std_flag_last_spelling_wins(): + # the toolchain's -std=c++17 followed by the rule's -std=c++11: the + # compiler honours the last one, so that is the Bazel side's standard + a = _model(tu_from_raw("src/a.cpp", ["-std=c++11"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-std=c++17", "-Wall", "-std=c++11"], is_bazel=True), + is_bazel=True) + assert summarize(diff_models(a, b))["converged"] + # the other way round the reference's standard is NOT what Bazel compiles + b = _model(tu_from_raw("src/a.cpp", ["-std=c++11", "-Wall", "-std=c++17"], is_bazel=True), + is_bazel=True) + d = diff_models(a, b) + assert [x.kind for x in d] == ["flags_diff"] and d[0].cmake_only == ["-std=c++11"], d + + +def test_quote_only_root_does_not_satisfy_a_reference_include_root(): + # zlib: CMake passes -I; Bazel passes -iquote . for every compile, + # which looks like the same root but does not serve `#include ` + a = _model(tu_from_raw("a.c", ["-I", "/work/proj", "-DZLIB_BUILD"], is_bazel=False), + is_bazel=False) + b = _model(tu_from_raw("a.c", ["-iquote", ".", "-iquote", "bazel-out/k8-fastbuild/bin", + "-DZLIB_BUILD"], is_bazel=True), is_bazel=True) + d = diff_models(a, b) + assert [x.kind for x in d] == ["includes_diff"], d + assert d[0].cmake_only == ["."] and "-iquote" in d[0].detail, d[0] + assert d[0].severity == Severity.ERROR.value + # `includes = ["."]` (-I / -isystem on the same root) satisfies it + for extra in (["-I."], ["-isystem", "."]): + b = _model(tu_from_raw("a.c", ["-iquote", "."] + extra + ["-DZLIB_BUILD"], is_bazel=True), + is_bazel=True) + assert summarize(diff_models(a, b))["converged"], extra + # a reference root that is itself -iquote only asks for nothing more + a = _model(tu_from_raw("a.c", ["-iquote", "/work/proj"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("a.c", ["-iquote", "."], is_bazel=True), is_bazel=True) + assert summarize(diff_models(a, b))["converged"] + + + +def test_pic_and_non_pic_compiles_of_one_source_are_judged_as_one(): + # Bazel builds fmt's format.cc twice under -c opt: `.pic.o` for the + # cc_shared_library and `.o` for the archive. The reference (shared-only) + # compiles it once, with -fPIC. The non-PIC twin must not be a + # `-fPIC missing` error, and a finding both twins share is reported once. + a = _bs(CanonicalModel(), is_bazel=False) + a.add(Target("fmt", TargetKind.SHARED, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/format.cc", ["-fPIC", "-DFOO=1"], is_bazel=False)])) + b = _bs(CanonicalModel(), is_bazel=True) + b.add(Target(":fmt", TargetKind.STATIC, role=TargetRole.PRODUCTION, + actions=[tu_from_raw("src/format.cc", ["-fPIC", "-DFOO=1"], is_bazel=True), + tu_from_raw("src/format.cc", ["-DFOO=1"], is_bazel=True)])) + assert summarize(diff_models(a, b))["converged"] + # both twins lack FOO: one finding, from the PIC twin, not two + b.targets[":fmt"].actions = [tu_from_raw("src/format.cc", ["-fPIC"], is_bazel=True), + tu_from_raw("src/format.cc", [], is_bazel=True)] + d = diff_models(a, b) + assert [x.kind for x in d] == ["defines_diff"] and d[0].cmake_only == ["FOO=1"], d + # a second compile that differs by MORE than the PIC flag is not the + # twin: it is judged on its own, -fPIC included + b.targets[":fmt"].actions = [tu_from_raw("src/format.cc", ["-fPIC", "-DFOO=1"], is_bazel=True), + tu_from_raw("src/format.cc", ["-DFOO=1", "-DBAR"], is_bazel=True)] + d = diff_models(a, b) + assert sorted(x.kind for x in d) == ["defines_diff", "flags_diff"], d + assert [x for x in d if x.kind == "flags_diff"][0].cmake_only == ["-fPIC"] + + +def test_exports_marker_hint_uses_cmake_identifier_mangling(): + from diff import _export_macro + assert _export_macro("fmt-c") == "fmt_c_EXPORTS" + assert _export_macro("jansson") == "jansson_EXPORTS" + assert _export_macro("lua5.3") == "lua5_3_EXPORTS" + + if __name__ == "__main__": import traceback fns = [v for k, v in sorted(globals().items()) if k.startswith("test_")] diff --git a/tests/test_extractors.py b/tests/test_extractors.py index fd282b7..735affd 100644 --- a/tests/test_extractors.py +++ b/tests/test_extractors.py @@ -139,6 +139,82 @@ def test_full_pipeline_converges(): assert res["errors"] == 0, json.dumps(res, indent=2) +def test_cmake_project_below_repo_root_keys_sources_repo_relative(): + """A CMake project vendored under the repo root (third_party/foo) must key + its sources the way the Bazel side does: relative to the REPO root, not to + the CMake source dir. The File API spells sources relative to `paths.source`.""" + with tempfile.TemporaryDirectory() as root: + build = _write_cmake_fixture(root) + reply = os.path.join(build, ".cmake", "api", "v1", "reply") + cm = dict(CODEMODEL) + cm["paths"] = {"source": os.path.join(REPO, "third_party", "proj"), + "build": build} + with open(os.path.join(reply, "codemodel.json"), "w") as f: + json.dump(cm, f) + a = extract_cmake.extract(build, REPO) + tu = _view(a, "mylib").tus[0] + assert tu.source == "third_party/proj/src/a.cpp", tu.source + + +def test_cmake_subdir_relative_include_roots_are_resolved(): + """A subdirectory CMakeLists (lua/) doing ADD_DEFINITIONS(-I..) or + INCLUDE_DIRECTORIES(..) yields roots that only mean something from the + compile's CWD. The File API hands them over verbatim -- `-I..` in a + compileCommandFragment, or as an `includes[]` path -- so stored as-is the + CMake side says `..` while the Bazel side says `third_party/proj`, and each + package needed an include_map entry to converge. Resolve them + against the target's source dir and re-relativize against the repo root.""" + with tempfile.TemporaryDirectory() as root: + build = _write_cmake_fixture(root) + reply = os.path.join(build, ".cmake", "api", "v1", "reply") + cm = dict(CODEMODEL) + cm["paths"] = {"source": os.path.join(REPO, "third_party", "proj"), + "build": build} + with open(os.path.join(reply, "codemodel.json"), "w") as f: + json.dump(cm, f) + tgt = json.loads(json.dumps(TARGET_MYLIB)) + tgt["paths"] = {"source": "lua", "build": "lua"} + tgt["sources"] = [{"path": "lua/a.cpp"}] + cg = tgt["compileGroups"][0] + cg["compileCommandFragments"] = [ + {"fragment": "-std=c++17 -I.. -isystem ../vendor -I ."}] + cg["includes"] = [{"path": ".."}, # relative + {"path": "/work/proj/third_party/proj/lua/.."}, + {"path": "/opt/sdk/include", "isSystem": True}] + with open(os.path.join(reply, "target-mylib.json"), "w") as f: + json.dump(tgt, f) + a = extract_cmake.extract(build, REPO) + # the raw argv is rewritten only in the SPELLING of the relative roots + raw = a.targets["mylib"].actions[0].arguments + assert "-Ithird_party/proj" in raw, raw + assert ("-isystem", "third_party/proj/vendor") in zip(raw, raw[1:]), raw + assert ".." not in raw and "." not in raw, raw + assert "-std=c++17" in raw + tu = _view(a, "mylib").tus[0] + assert tu.source == "third_party/proj/lua/a.cpp", tu.source + assert "third_party/proj" in tu.includes, tu.includes # -I.. / [..] + assert "third_party/proj/lua" in tu.includes, tu.includes # -I . + assert "third_party/proj/vendor" in tu.includes, tu.includes + assert "/opt/sdk/include" in tu.includes, tu.includes # abs, kept + assert not any(i.startswith(".") for i in tu.includes), tu.includes + + +def test_cmake_relative_include_roots_kept_without_paths(): + """A reply with no codemodel `paths` (older fixtures) has nothing to anchor + on; a relative root stays verbatim rather than being resolved against a + guessed directory.""" + with tempfile.TemporaryDirectory() as root: + build = _write_cmake_fixture(root) + reply = os.path.join(build, ".cmake", "api", "v1", "reply") + tgt = json.loads(json.dumps(TARGET_MYLIB)) + tgt["compileGroups"][0]["compileCommandFragments"] = [ + {"fragment": "-std=c++17 -I.."}] + with open(os.path.join(reply, "target-mylib.json"), "w") as f: + json.dump(tgt, f) + a = extract_cmake.extract(build, REPO) + assert "-I.." in a.targets["mylib"].actions[0].arguments + + def test_cmake_extracts_canonical_flags(): with tempfile.TemporaryDirectory() as root: build = _write_cmake_fixture(root)