From bcc2d437d379caad07c176708597de408f5ec643 Mon Sep 17 00:00:00 2001 From: helly25 <6420169+helly25@users.noreply.github.com> Date: Sun, 9 Aug 2026 15:58:57 +0100 Subject: [PATCH] Extract the compile DB in the configuration we develop in The DB was extracted in the DEFAULT build configuration and only had its compiler substituted afterwards (`--bcce-compiler`). Everything else - the standard library, include paths, feature macros - therefore came from whatever toolchain bazel picked by default, which differs per platform. That is what made clang-tidy report differently on macOS and Linux. Extract with `--config=clang-tidy` instead, a new .bazelrc config that is `--config=clang` plus an explicit `-stdlib=libc++`. The stdlib stays explicit on purpose: development targets libc++, and the DB should say so rather than inherit the driver default (libc++ on macOS, libstdc++ on Linux). `--bcce-compiler` is now redundant - the extracted commands already name the hermetic toolchain - and is dropped along with the compiler-resolution block that supported it. `--bcce-copt=-isysroot` stays: macOS still needs the SDK for system C headers. This also fixes the missing sources. aquery expands `//...` with `manual` targets already removed, so the benchmarks, the differential test and show_compiler were never in the DB at all and were being linted with flags interpolated from unrelated siblings. `//bazelmod:refresh_compile_commands` now names them explicitly. It is a SINGLE targets entry: the key is interpolated into `deps(...)`, so it may be a query expression, and a union needs no per-target flags duplication. The `clang-tidy` tag on those targets states the intent, and `clang-tidy-targets-match-tag` fails if the tag and the explicit list ever disagree - verified to fail on a removed entry and to name exactly which target drifted. A tag alone cannot do the job, because aquery's exclusion happens before any `attr(tags, ...)` filter can see the target. The refresh target lives in //bazelmod because the extractor is a dev_dependency: the root package defines //:clang-format and //:is_clang, which library targets reference, so a module consumer would load it and fail on the missing dep. Nothing outside MODULE.bazel references //bazelmod. The `repo_name` alias is dropped because the extractor's macro emits a raw `@hedron_compile_commands//...` label, which resolves in the CALLER's repo context and so breaks under a rename; that is fixed upstream in helly25/bazel-compile-commands-extractor#30. compile_commands.json: 2034 entries, every bazel-built source covered. bazel test --config=clang //... - 109/109 pass. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com> --- .bazelrc | 10 ++++ .pre-commit-config.yaml | 12 ++++ .pre-commit/check_clang_tidy_targets.sh | 57 +++++++++++++++++++ bazelmod/BUILD.bazel | 27 +++++++++ bazelmod/clang_tidy_targets.bzl | 44 +++++++++++++++ bazelmod/dev.MODULE.bazel | 2 +- compile_commands-update.sh | 75 ++++++------------------- mbo/container/BUILD.bazel | 5 +- mbo/diff/BUILD.bazel | 5 +- mbo/hash/BUILD.bazel | 10 +++- tools/BUILD.bazel | 5 +- 11 files changed, 188 insertions(+), 64 deletions(-) create mode 100755 .pre-commit/check_clang_tidy_targets.sh create mode 100644 bazelmod/clang_tidy_targets.bzl diff --git a/.bazelrc b/.bazelrc index d95031be..7e9342a1 100644 --- a/.bazelrc +++ b/.bazelrc @@ -60,6 +60,16 @@ common:clang --define is_clang=true # https://github.com/llvm/llvm-project/issues/100909 common:clang --cxxopt=-Wno-unused-command-line-argument +# The configuration the code is INDEXED in: `compile_commands.json` (see +# `compile_commands-update.sh`) and therefore clangd and clang-tidy. It is +# `--config=clang` plus an explicit libc++, because that is what development +# actually targets and the compile DB must say so rather than inherit whatever +# the driver defaults to - libc++ on macOS but libstdc++ on Linux, which made +# the same clang-tidy check report differently per platform. +common:clang-tidy --config=clang +common:clang-tidy --cxxopt=-stdlib=libc++ +common:clang-tidy --linkopt=-stdlib=libc++ + common:symbolizer --strip=never common:symbolizer --run_under=//tools:run_under_symbolizer diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d025a1a7..c4888cdb 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -83,6 +83,18 @@ repos: entry: .pre-commit/check_version.sh pass_filenames: False types: [text] + - id: clang-tidy-targets-match-tag + name: clang-tidy targets match the tag + description: | + `CLANG_TIDY_MANUAL_TARGETS` in bazelmod/clang_tidy_targets.bzl must equal the set of + targets tagged `clang-tidy`. The list exists because aquery expands `//...` with + `manual` targets already removed, so //bazelmod:refresh_compile_commands has to name + them explicitly or they never reach compile_commands.json. The tag states the intent; + this stops the two drifting apart. Skips when bazel is unavailable. + language: system + entry: .pre-commit/check_clang_tidy_targets.sh + pass_filenames: false + files: ^(.*/BUILD\.bazel|bazelmod/clang_tidy_targets\.bzl)$ - id: no-do-not-merge name: No 'DO NOT MERGE' description: | diff --git a/.pre-commit/check_clang_tidy_targets.sh b/.pre-commit/check_clang_tidy_targets.sh new file mode 100755 index 00000000..92932242 --- /dev/null +++ b/.pre-commit/check_clang_tidy_targets.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash + +# SPDX-FileCopyrightText: Copyright (c) The helly25 authors (helly25.com) +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Verifies that `CLANG_TIDY_MANUAL_TARGETS` in bazelmod/clang_tidy_targets.bzl is +# exactly the set of targets tagged `clang-tidy`. +# +# The list has to exist because aquery expands `//...` with `manual` targets +# already removed, so `//bazelmod:refresh_compile_commands` must name them +# explicitly to get them into `compile_commands.json`. The tag stays the +# statement of intent; this check stops the two from drifting apart when a target +# is added, removed or retagged. +# +# Skips when bazel is unavailable rather than failing, so a checkout without a +# toolchain can still commit. + +set -euo pipefail + +if ! command -v bazel >/dev/null 2>&1; then + echo "clang-tidy-targets: skipped (no bazel on PATH)." 1>&2 + exit 0 +fi + +readonly TARGETS_BZL="bazelmod/clang_tidy_targets.bzl" + +# Tagged targets, as bazel sees them. `bazel query` (unlike aquery) does report +# `manual` targets, which is what makes this check possible at all. Strip the +# `@@//` canonical-repo prefix so both sides compare as `//pkg:target`. +TAGGED="$(bazel query 'attr(tags, "clang-tidy", //...)' 2>/dev/null | sed 's|^@@||' | sort)" + +# The list the build actually uses. +LISTED="$(sed -n 's|^ *"\(//[^"]*\)",$|\1|p' "${TARGETS_BZL}" | sort)" + +if [ "${TAGGED}" = "${LISTED}" ]; then + exit 0 +fi + +echo "ERROR: ${TARGETS_BZL} and the 'clang-tidy' tag disagree." 1>&2 +echo " Tagged but not listed (these would be MISSING from compile_commands.json):" 1>&2 +comm -23 <(printf '%s\n' "${TAGGED}") <(printf '%s\n' "${LISTED}") | sed 's/^/ /' 1>&2 +echo " Listed but not tagged (stale entries):" 1>&2 +comm -13 <(printf '%s\n' "${TAGGED}") <(printf '%s\n' "${LISTED}") | sed 's/^/ /' 1>&2 +echo " Fix by editing ${TARGETS_BZL}, or by adding/removing the 'clang-tidy' tag." 1>&2 +exit 1 diff --git a/bazelmod/BUILD.bazel b/bazelmod/BUILD.bazel index 476669a4..fe78ab94 100644 --- a/bazelmod/BUILD.bazel +++ b/bazelmod/BUILD.bazel @@ -13,9 +13,36 @@ # See the License for the specific language governing permissions and # limitations under the License. +load("@hedron_compile_commands//:refresh_compile_commands.bzl", "refresh_compile_commands") +load(":clang_tidy_targets.bzl", "CLANG_TIDY_MANUAL_TARGETS") + package(default_visibility = []) exports_files([ "dev.MODULE.bazel", "llvm.MODULE.bazel", ]) + +# Generates `compile_commands.json`; run it through `//compile_commands-update.sh`. +# +# Lives here rather than in the root package because the extractor is a +# `dev_dependency`: the root package defines `//:clang-format` and `//:is_clang`, +# which library targets reference, so a module consumer would load it and fail on +# the missing dev dep. Nothing outside MODULE.bazel references `//bazelmod`. +# +# One entry, because the key is interpolated into `deps(...)` and so may be a +# query EXPRESSION rather than a single pattern. The union is required: aquery +# expands `//...` with `manual` targets already excluded, so the benchmarks and +# other manual targets we still want indexed have to be named explicitly (a +# `attr(tags, "clang-tidy", //...)` filter returns nothing extra, because the +# exclusion happens before the filter). `//bazelmod:clang_tidy_manual_targets` +# below keeps that list honest against the tag. +refresh_compile_commands( + name = "refresh_compile_commands", + tags = ["manual"], + targets = { + " + ".join([ + "@//...", + ] + CLANG_TIDY_MANUAL_TARGETS): "--config=clang-tidy", + }, +) diff --git a/bazelmod/clang_tidy_targets.bzl b/bazelmod/clang_tidy_targets.bzl new file mode 100644 index 00000000..d128cc5c --- /dev/null +++ b/bazelmod/clang_tidy_targets.bzl @@ -0,0 +1,44 @@ +# SPDX-FileCopyrightText: Copyright (c) The helly25 authors (helly25.com) +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Manual targets that must still be indexed for clang-tidy. + +A `manual` target is excluded from wildcard patterns, and aquery applies that +exclusion while expanding `//...` - before any `attr(tags, ...)` filter can see +it. So a tag alone cannot pull these into `compile_commands.json`; the labels +have to be named explicitly in `//bazelmod:refresh_compile_commands`. + +The `clang-tidy` tag on each of these targets remains the source of truth for +INTENT ("this manual target is meant to be linted"), and the +`clang-tidy-targets-match-tag` pre-commit hook fails if this list and the tag +query disagree - so the list cannot silently drift as targets are added, +removed or retagged. + +Note this covers everything bazel builds. The one repository source that is not +in `compile_commands.json`, `mbo/hash/measurements/smhasher3/mbohash.cpp`, cannot +be: it is a plugin for SMHasher3, copied into that project by +`mbo/hash/measurements/build_smhasher3.sh` and compiled by ITS cmake. There is no +BUILD file for it here and thus nothing to tag, and linting it against this +repository's flags would only produce bogus errors for the SMHasher3 headers +(`Platform.h`, `Hashlib.h`) it includes. +""" + +CLANG_TIDY_MANUAL_TARGETS = [ + "//mbo/container:limited_set_benchmark", + "//mbo/diff:diff_benchmark", + "//mbo/hash:hash_benchmark", + "//mbo/hash:hash_differential_test", + "//tools:show_compiler", +] diff --git a/bazelmod/dev.MODULE.bazel b/bazelmod/dev.MODULE.bazel index 5f9e6f60..391a3c72 100644 --- a/bazelmod/dev.MODULE.bazel +++ b/bazelmod/dev.MODULE.bazel @@ -15,7 +15,7 @@ """Extra development modules.""" -bazel_dep(name = "hedron_compile_commands", dev_dependency = True, repo_name = "bazel_compile_commands_extractor") +bazel_dep(name = "hedron_compile_commands", dev_dependency = True) git_override( module_name = "hedron_compile_commands", commit = "6eb3ff1de6445355552ef4230be0caf3e1dd27e5", diff --git a/compile_commands-update.sh b/compile_commands-update.sh index 5c54b237..68fb1869 100755 --- a/compile_commands-update.sh +++ b/compile_commands-update.sh @@ -15,13 +15,18 @@ # See the License for the specific language governing permissions and # limitations under the License. -# Generate `compile_commands.json` describing the HERMETIC clang, so that clangd -# and `tools/clang_tidy.sh` parse with the same compiler the `--config=clang` -# builds use. Note that `--config=clang` cannot do this: it only configures the -# build of the extractor tool itself, and never reaches the `aquery` the tool -# runs internally, so the recorded commands still named the autodetected local -# (Apple) clang. The fork's runtime flags below are the supported override; they -# must follow `--`, or `bazel run` hands them to bazel rather than to the tool. +# Generate `compile_commands.json` so that clangd and `tools/clang_tidy.sh` parse +# in the configuration development actually targets. +# +# The extraction runs through `//bazelmod:refresh_compile_commands`, whose target +# list carries `--config=clang-tidy` (`.bazelrc`: `--config=clang` plus an +# explicit libc++). That is what puts the flags right at the source, so no +# compiler override is needed: passing `--config=clang` on THIS command line +# would not work, as it only configures the build of the extractor tool itself +# and never reaches the `aquery` the tool runs internally. +# +# The runtime `--bcce-*` flags below must follow `--`, or `bazel run` hands them +# to bazel rather than to the tool. set -euo pipefail @@ -30,67 +35,21 @@ function die() { exit 1 } -OUTPUT_BASE="$(bazel info output_base 2>/dev/null || true)" -[ -n "${OUTPUT_BASE}" ] || die "'bazel info output_base' failed; is bazel on PATH?" - -# The hermetic LLVM install (headers + libs) lives in the `_llvm_llvm` repo; the -# plain `_llvm` repo only re-exports clang-format/clang-tidy/clangd. -declare -a CLANG_LOCS=( - "${OUTPUT_BASE}/external/toolchains_llvm++llvm+llvm_toolchain_llvm_llvm/bin/clang++" - "${OUTPUT_BASE}/external/toolchains_llvm~~llvm~llvm_toolchain_llvm_llvm/bin/clang++" - "${OUTPUT_BASE}/external/llvm_toolchain_llvm/bin/clang++" -) - -CLANG="" -function resolve_clang() { - for LOC in "${CLANG_LOCS[@]}"; do - if [ -x "${LOC}" ]; then - CLANG="${LOC}" - return - fi - done -} - -resolve_clang -if [ -z "${CLANG}" ]; then - # A fresh checkout (or CI runner) has not materialized the toolchain yet: it is - # only fetched once something is actually built with `--config=clang`. Build the - # smallest cc target there is to trigger that, then look again. - echo "Hermetic clang++ not present; fetching the toolchain via a probe build ..." 1>&2 - bazel build --config=clang //tools:show_compiler >/dev/null \ - || die "probe build '//tools:show_compiler --config=clang' failed; cannot fetch the LLVM toolchain" - resolve_clang -fi - -[ -n "${CLANG}" ] || die "Cannot find the hermetic clang++ even after a '--config=clang' build" - # Sources reachable both normally and through a build-machine tool are compiled # twice (target + exec configuration), and both commands would be emitted. Keep # only the target-configuration one: clang-tidy works per entry, so the exec copy # is duplicate linting for a near-identical result. Files compiled ONLY in the # exec configuration keep their command, so nothing leaves the compile DB. -declare -a BCCE_ARGS=("--bcce-compiler=${CLANG}" "--bcce-prefer-target-config") - -# Parse against libc++ on EVERY platform, so clang-tidy sees the same standard -# library everywhere. The extracted commands come from the default build -# configuration, not `--config=clang`, so they carry no `-stdlib`; the hermetic -# clang then falls back to each platform's default - libc++ on macOS, libstdc++ -# on Linux. That made the same check report differently per platform: a finding -# fixed on a Mac could be absent on the Linux CI runner and the reverse. The -# hermetic toolchain ships its own libc++ on both, so asking for it is enough -# (a no-op on macOS, the actual switch on Linux). -BCCE_ARGS+=("--bcce-copt=-stdlib=libc++") +declare -a BCCE_ARGS=("--bcce-prefer-target-config") -# The hermetic clang carries its own libc++ but no system C headers: without the -# SDK sysroot its support headers fail on `'time.h' file not found`. -# The bazel `--config=clang` toolchain supplies this itself; the extracted -# commands come from the autodetected toolchain, which relies on Apple clang's -# built-in default, so it has to be made explicit here. Linux needs no such flag. +# macOS only: the hermetic clang carries its own libc++ but no system C headers, +# so without the SDK sysroot its support headers fail on +# `'time.h' file not found`. Linux needs no such flag. if [ "$(uname -s)" = "Darwin" ]; then SDKROOT_PATH="$(xcrun --show-sdk-path 2>/dev/null || true)" [ -n "${SDKROOT_PATH}" ] || die "'xcrun --show-sdk-path' failed; install the Xcode command line tools" BCCE_ARGS+=("--bcce-copt=-isysroot${SDKROOT_PATH}") fi -bazel run @bazel_compile_commands_extractor//:refresh_all -- "${BCCE_ARGS[@]}" +bazel run //bazelmod:refresh_compile_commands -- "${BCCE_ARGS[@]}" echo "OK" diff --git a/mbo/container/BUILD.bazel b/mbo/container/BUILD.bazel index 3b22ff7f..5ebef4f9 100644 --- a/mbo/container/BUILD.bazel +++ b/mbo/container/BUILD.bazel @@ -174,7 +174,10 @@ cc_binary( name = "limited_set_benchmark", testonly = 1, srcs = ["limited_set_benchmark_main.cc"], - tags = ["manual"], + tags = [ + "clang-tidy", + "manual", + ], visibility = ["//visibility:private"], deps = [ ":limited_options_cc", diff --git a/mbo/diff/BUILD.bazel b/mbo/diff/BUILD.bazel index 1e0ccdb1..55c93174 100644 --- a/mbo/diff/BUILD.bazel +++ b/mbo/diff/BUILD.bazel @@ -131,7 +131,10 @@ cc_binary( name = "diff_benchmark", testonly = True, srcs = ["diff_benchmark.cc"], - tags = ["manual"], + tags = [ + "clang-tidy", + "manual", + ], deps = [ ":diff_cc", "//mbo/file:artefact_cc", diff --git a/mbo/hash/BUILD.bazel b/mbo/hash/BUILD.bazel index 365fed02..18cde3b8 100644 --- a/mbo/hash/BUILD.bazel +++ b/mbo/hash/BUILD.bazel @@ -190,7 +190,10 @@ cc_test( # module is loaded (bazelmod/dev.MODULE.bazel) - i.e. when helly25_mbo is # the root module. Wildcards used by consumers must not analyze it; CI runs # it explicitly (see .github/workflows/test.yml). - tags = ["manual"], + tags = [ + "clang-tidy", + "manual", + ], deps = [ ":hash_cc", ":hash_test_util", @@ -204,7 +207,10 @@ cc_binary( name = "hash_benchmark", testonly = True, srcs = ["hash_benchmark.cc"], - tags = ["manual"], + tags = [ + "clang-tidy", + "manual", + ], deps = [ ":hash_test_util", "@abseil-cpp//absl/strings", diff --git a/tools/BUILD.bazel b/tools/BUILD.bazel index a1723b69..11d77ead 100644 --- a/tools/BUILD.bazel +++ b/tools/BUILD.bazel @@ -60,5 +60,8 @@ sh_binary( cc_binary( name = "show_compiler", srcs = ["show_compiler.cc"], - tags = ["manual"], + tags = [ + "clang-tidy", + "manual", + ], )