From 70fcf71204b2c0f49e3b62abc63284ac2239118e Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:26:22 -0400 Subject: [PATCH 1/3] Warn in diff.json when configure-time generated files are not compared A configure_file() output never reaches a command line, so a value that differs only inside one (a config.h define, a version string) is invisible to the diff. The skill said so in a note beside the CMake extraction commands, but diff.json, the artifact people act on, said nothing, and `converged: true` read as covering them. Measured in any2bazel-lab: lensfun's generated config.h differed from the reference in 3 of 7 migrations with the skill, each with a converged diff. configured_files does populate (the lab saw it on another project); the empty lists on lensfun, which spells the command CONFIGURE_FILE, come from the uppercase bug fixed earlier in this series. diff_models now appends one WARN discrepancy, kind configured_files_not_compared, target "", when the CMake model records configured files: cmake_only lists them, and the detail gives the count and how many land on a compile include path. It never blocks `converged`. One aggregated entry rather than one per file keeps a large project's worklist readable (MySQL Server has about 200). All project-owned configured files are listed, not only those flagged as compile inputs, because that flag depends on the output sitting under a known include directory and can miss a header reached another way. A new end-to-end test covers the path from a trace through extract_cmake and the model to the diff. In the lab's later runs with this series, agents byte-compared the listed files on lensfun, CMake and QGIS; the QGIS agent wrote eleven Bazel tests that do it, and CMake's three configure headers that had differed from the reference came out identical in all three CMake runs. Lab-Proposal: P2 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 4 +++- docs/REFERENCE-cmake.md | 2 ++ scripts/diff.py | 23 +++++++++++++++++++++++ tests/test_engine.py | 17 +++++++++++++++++ tests/test_extractors.py | 26 ++++++++++++++++++++++++++ 5 files changed, 71 insertions(+), 1 deletion(-) diff --git a/SKILL.md b/SKILL.md index c67c322..a50e24b 100644 --- a/SKILL.md +++ b/SKILL.md @@ -312,7 +312,9 @@ are easiest to reason about once the TUs underneath agree. also emits **WARN-only** kinds that don't block `converged` — `extra_tu` / `extra_target` (compiled/built on the Bazel side but not the reference) and `extra_test_tu` — note them but they need no action unless they point to -something you didn't intend to add. +something you didn't intend to add. The WARN `configured_files_not_compared` +does need action: byte-compare each listed file against the one your Bazel +build generates. #### Triage findings {#triage} diff --git a/docs/REFERENCE-cmake.md b/docs/REFERENCE-cmake.md index 49ed665..35c6f80 100644 --- a/docs/REFERENCE-cmake.md +++ b/docs/REFERENCE-cmake.md @@ -40,6 +40,8 @@ python3 "$SKILL_DIR/scripts/extract_cmake.py" model.cmake.js > the content differ are TODO (see > [TODO-configure-time-generation.md](TODO-configure-time-generation.md)). This > is distinct from build-time codegen (genrules), which is also unmodeled. +> `diff.py` lists them in a `configured_files_not_compared` warning (see +> [Diff against reference build](../SKILL.md#diff)). The project flags passed to `cmake` define the platform/options the Bazel aquery must mirror. diff --git a/scripts/diff.py b/scripts/diff.py index 3839bc2..94ab1cb 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -82,6 +82,8 @@ class Kind(str, Enum): MISSING_TEST_TU = "missing_test_tu" # test source compiled in cmake, not bazel EXTRA_TEST_TU = "extra_test_tu" # test source compiled in bazel, not cmake TEST_BINARY_COUNT = "test_binary_count" # differing number of test executables + # configure-time generated files exist but their CONTENTS are not compared: + CONFIGURED_FILES = "configured_files_not_compared" @dataclass @@ -546,9 +548,30 @@ def diff_models(a: CanonicalModel, b: CanonicalModel, # ---- tests (opt-in) ---------------------------------------------------- if cfg.include_tests: out.extend(_diff_tests(a_views, b_views, cfg, a_all, b_all, notes)) + + # ---- configure-time generated files: recorded, NOT compared ------------- + # configure_file() outputs never reach a command line, so a value that + # differs only inside one (a config.h define, a version string) is + # invisible to everything above. Say so in the worklist itself, where the + # reader of diff.json will see it, rather than only in the docs. + out.extend(_configured_files_warning(a)) return out +def _configured_files_warning(a: CanonicalModel) -> List[Discrepancy]: + files = a.configured_files + if not files: + return [] + names = sorted(files) + n_inputs = sum(1 for c in files.values() if c.is_compile_input) + return [Discrepancy( + Kind.CONFIGURED_FILES.value, Severity.WARN.value, "", + f"{len(names)} configure-time generated file(s), {n_inputs} on a compile " + "include path, are not covered by this comparison: byte-compare each " + "against the file the Bazel build generates", + cmake_only=names)] + + def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], cfg: "MigrationConfig", a_all: set, b_all: set, notes: Optional[DiffNotes] = None) -> List[Discrepancy]: diff --git a/tests/test_engine.py b/tests/test_engine.py index e4185c1..05eddea 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -905,6 +905,23 @@ def test_ndebug_without_notes_reports_sides_only(): assert "bazel_added" not in nd and nd["differs"] is False, nd +def test_configured_files_raise_a_non_blocking_warning(): + from model import ConfiguredFile + a = _model(tu_from_raw("src/a.cpp", ["-DX"], is_bazel=False), is_bazel=False) + b = _model(tu_from_raw("src/a.cpp", ["-DX"], is_bazel=True), is_bazel=True) + assert diff_models(a, b) == [] # nothing configured -> no warning + a.add_configured_file(ConfiguredFile("config.h", "/b/config.h", + is_compile_input=True)) + a.add_configured_file(ConfiguredFile("foo.pc", "/b/foo.pc")) + discs = diff_models(a, b) + res = summarize(discs) + assert res["converged"] and res["warnings"] == 1, res + w = discs[0] + assert w.kind == "configured_files_not_compared" and w.severity == "warn" + assert w.cmake_only == ["config.h", "foo.pc"] + assert w.detail.startswith("2 configure-time generated file(s), 1 on a") + + def test_ignore_flags_and_defines_suppress_diffs(): # A reviewer-approved flag/define difference is suppressed via config and # the diff converges; without the config it's an error. diff --git a/tests/test_extractors.py b/tests/test_extractors.py index 7ee7613..b5ce4e9 100644 --- a/tests/test_extractors.py +++ b/tests/test_extractors.py @@ -159,6 +159,32 @@ def _model_of(target): return m +def test_configured_files_reach_the_diff_end_to_end(): + # The trace -> extract_cmake -> model JSON -> diff path, with the command + # spelled in capitals as large projects write it. + from serialize import dump_model, load_model + with tempfile.TemporaryDirectory() as root: + build = _write_cmake_fixture(root) + trace = os.path.join(root, "trace.jsonl") + with open(trace, "w") as f: + f.write(json.dumps({"version": {"major": 1, "minor": 2}}) + "\n") + f.write(json.dumps({"cmd": "CONFIGURE_FILE", "file": REPO + "/CMakeLists.txt", + "line": 3, "args": [REPO + "/config.h.in", + build + "/config.h"]}) + "\n") + path = os.path.join(root, "model.cmake.json") + dump_model(extract_cmake.extract(build, REPO, trace), path) + a = load_model(path) + assert list(a.configured_files) == ["config.h"], a.configured_files + + aq_path = os.path.join(root, "aquery.json") + with open(aq_path, "w") as f: + json.dump(AQUERY, f) + res = summarize(diff_models(a, extract_bazel.extract(aq_path, REPO))) + assert res["converged"], res + kinds = [d["kind"] for d in res["discrepancies"]] + assert kinds == ["configured_files_not_compared"], kinds + + def test_cmake_extracts_canonical_flags(): with tempfile.TemporaryDirectory() as root: build = _write_cmake_fixture(root) From 9ca97b60bc052de928a64c82bd342c3d7b20ffd3 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:29:21 -0400 Subject: [PATCH 2/3] State the convergence boundary where convergence is defined SKILL.md said that configure_file outputs are "recorded but not yet diffed" only in a note beside the CMake extraction commands. The paragraph that defines done ("Done when both stages converge") carried no such caveat, so a reader who lands there reads converged: true as covering the whole build. Measured in any2bazel-lab: a lensfun migration converged with its generated config.h differing from the reference (a GLIB_VERSION_MIN_REQUIRED value that appears in no command line), and the same divergence recurred in later runs, each alongside a converged diff. Adds a short "what converged does not cover" list directly under the definition of done, starting with generated file contents and the instruction to byte-compare them. Later commits add the other boundaries to the same list. Lab-Proposal: P1 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/SKILL.md b/SKILL.md index a50e24b..9b0c4d5 100644 --- a/SKILL.md +++ b/SKILL.md @@ -61,6 +61,12 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ Bazel flags are tolerated (listed in `diff.json` as `bazel_only_flags`). No artifact/symbol diff or test execution yet. + **`converged: true` is parity of command lines, nothing more.** Check + separately what it cannot see: + - **Generated file contents.** A value that reaches only a generated file + (`configure_file` output, genrule output) never appears on a command line. + Byte-compare each generated file against the reference build's. + ### How targets are matched - **Roles:** each target gets a `role` (`production`, `test`, `dashboard`, From 7ff21fecddc4bdf72e7ec812e177ac83160b59c4 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Sat, 26 Sep 2026 02:29:40 -0400 Subject: [PATCH 3/3] Say that shared-library link lines are not compared SKILL.md described link consistency as covering "every name-aligned executable/shared-lib". diff.py does not do that: shared libraries are in the library TU union, only executables reach the link-flag comparison, and the external-dependency check is a project-wide union, so a dependency one .so is missing passes if any other target links it. A clean diff says nothing about how a shared library links. Measured in any2bazel-lab: a pono migration converged while libpono.so lacked its MPFR link, and the Bazel-built binary crashed at load. The agent found it only by running the binary. Corrects the two places in SKILL.md and the four comments in scripts/diff.py (the module docstring, LINK_FLAGS_DIFF, _diff_link_flags and _diff_tests) that claimed shared-library link parity, and adds the boundary to the "what converged does not cover" list with the remedy: run a binary that loads each shared library. Comparing shared libraries' link lines is a larger design change (they would need identity alignment like executables) and is left to the maintainers. No later run exercised this change on the project that motivated it. On lensfun, an agent following it loaded the built shared library, found it missing a GLib symbol, and fixed the link. Lab-Proposal: P8 Co-Authored-By: Claude Opus 5.5 --- SKILL.md | 9 ++++++--- scripts/diff.py | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/SKILL.md b/SKILL.md index 9b0c4d5..75ae9a4 100644 --- a/SKILL.md +++ b/SKILL.md @@ -52,7 +52,7 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ - **Compile parity** — every TU compiled with equivalent flags/defines/ includes (project-wide TU-set), and the external link-dependency closure matches. - - **Link consistency** — every name-aligned executable/shared-lib links with + - **Link consistency** — every name-aligned executable links with equivalent link flags. The per-binary stage: the *artifacts* agree, not just the TUs. @@ -66,6 +66,9 @@ Bazel aquery jsonproto ──extract_bazel.py───────┘ - **Generated file contents.** A value that reaches only a generated file (`configure_file` output, genrule output) never appears on a command line. Byte-compare each generated file against the reference build's. + - **How shared libraries link.** A shared library is compared by its TUs + only; its link flags and the libraries it links are not checked (the + external-dep check is project-wide). Run a binary that loads each one. ### How targets are matched @@ -311,8 +314,8 @@ comparisons. The diff reports both parity stages at once: **compile-parity** kinds (`missing_tu`, `defines_diff`, `includes_diff`, `flags_diff`, `missing_dep`, `missing_target`) and the **link-consistency** kind (`link_flags_diff`, one per -name-aligned executable/shared library). Fix compile parity first — link flags -are easiest to reason about once the TUs underneath agree. +name-aligned executable). Fix compile parity first — link flags are easiest to +reason about once the TUs underneath agree. `error`-severity kinds (above) block convergence and are what you fix. The diff also emits **WARN-only** kinds that don't block `converged` — `extra_tu` / diff --git a/scripts/diff.py b/scripts/diff.py index 94ab1cb..ab98cbd 100644 --- a/scripts/diff.py +++ b/scripts/diff.py @@ -19,8 +19,9 @@ Parity has two stages, both reported here: COMPILE PARITY -- per-TU flag/define/include equivalence (project-wide TU-set) + external link-dependency closure. - LINK CONSISTENCY -- per name-aligned executable/shared-lib, equivalent link - flags (link_flags_diff). + LINK CONSISTENCY -- per name-aligned executable, equivalent link flags + (link_flags_diff). Shared libraries are compared by + their TUs only; their link lines are not checked. ASYMMETRY: a flag present in CMake but missing in Bazel is a discrepancy (under-specified Bazel target -> likely wrong build). A flag present only in @@ -73,7 +74,7 @@ class Kind(str, Enum): DEFINES_DIFF = "defines_diff" INCLUDES_DIFF = "includes_diff" FLAGS_DIFF = "flags_diff" - LINK_FLAGS_DIFF = "link_flags_diff" # executable/shared-lib link flags + LINK_FLAGS_DIFF = "link_flags_diff" # executable link flags MISSING_DEP = "missing_dep" # Java (CompileGroup) source-set comparison: MISSING_JAVA_SRC = "missing_java_src" # .java compiled in A but not B @@ -187,7 +188,7 @@ def _diff_tu(target: str, a: TranslationUnit, b: TranslationUnit, def _diff_link_flags(name: str, ta: "TargetView", tb: "TargetView", cfg: "MigrationConfig", notes: Optional[DiffNotes] = None) -> List[Discrepancy]: - """Asymmetric link-flag subset for one aligned executable/shared-lib: every + """Asymmetric link-flag subset for one aligned executable: every CMake link flag must be present on the Bazel side; extra Bazel link flags are tolerated (and recorded in `notes`). Reviewer-approved differences (cfg.link_flag_ignored) are dropped from both sides first. Mirrors the @@ -589,12 +590,12 @@ def _diff_tests(a_views: Dict[str, TargetView], b_views: Dict[str, TargetView], sources compile but are never linked into runnable tests. SCOPE: tests get the COMPILE-PARITY stage only. LINK CONSISTENCY (per-binary - link flags and link-dep closure) runs for PRODUCTION executables/shared - libs, not for test binaries: tests align by source-set union, not by binary - identity, so there's no per-binary link command to compare. So with tests - on, "converged" means production artifacts are link-consistent AND all test - SOURCES compile equivalently -- not that each test binary links identically. - Extending link consistency to per-test-binary is a natural future increment. + link flags) runs for PRODUCTION executables, not for test binaries: tests + align by source-set union, not by binary identity, so there's no + per-binary link command to compare. So with tests on, "converged" means + production artifacts are link-consistent AND all test SOURCES compile + equivalently -- not that each test binary links identically. Extending + link consistency to per-test-binary is a natural future increment. """ out: List[Discrepancy] = [] a_tests = {n for n, t in a_views.items()