clang-tidy tooling - #563
Draft
michalhosna wants to merge 27 commits into
Draft
Conversation
The pin is the one thing every other piece reads: the prebuilt fetcher, the superbuild, and the moqx build all resolve moxygen from it. MOXYGEN_REV starts where deps/moxygen points, not at whatever the branch was cut from.
Inert until CMakeLists.txt includes it. The rev-to-release resolution and its consistency checks are documented in the file header.
- The from-source alternative to the prebuilt, for revs and platforms with no published tarball and for instrumented sanitizer builds. - moqx itself is not built here; only the origin of the moxygen prefix differs between the two modes. - Inert until scripts/configure.sh --moxygen from-source drives it.
- Pins live in cmake/dependencies.cmake. - moxygen is consumed as an installed CMake package: the published prebuilt or a superbuild prefix. - binaryDir is build/<presetName> so profiles coexist. - No .a preference in CMAKE_FIND_LIBRARY_SUFFIXES. folly-targets.cmake hardcodes libglog.so, so an .a-preferring Glog resolution loads both copies and glog's double flag registration crashes under ASan. - GFLAGS_SHARED=ON retires the tests' whole-archive workaround, which only existed to force flag registration out of the static gflags. - The module path and the policy floor are build-level requirements, not per-preset choices: a bare `cmake -S .` needs them too. - compile_commands.json is exported because lint needs it and Ninja does not write it unasked. - A missing system -dev package surfaced deep inside the folly stack rather than as the install-system-deps.sh hint.
Only the build lifecycle scripts, install-system-deps.sh and moqx-run.sh are daily entry points. The rest move next to their audience, and every caller moves with them.
- With CPM the build dir's CMakeCache is the only state, so the setup-deps dispatcher and its marker files go. - --moxygen is explicit: a default would silently pick between an hours-long stack build and an uninstrumented prebuilt. - prebuilt-with-fallback is the entry point. It probes the fetcher directly rather than reading a failed moqx configure as "no prebuilt". - A sanitizer preset refuses any moxygen not instrumented to match. ASan deps under a TSan moqx link two clashing runtimes. - The variables --moxygen and --moxygen-dir own are refused as -D arguments; last-wins would defeat the interlock above. - Job count is -j, then MOQX_BUILD_JOBS, then a RAM-derated default for sanitizer profiles; an explicit value is never clamped (distcc).
- Dependency caches are content-keyed on the pins and restored by prefix: a bump would otherwise cold-download the full set in every concurrent job. - Every build path installs system libraries through install-system-deps.sh. - version-release resolves the release from MOXYGEN_REV, not moxygen's newest tag, which would strand the release as a draft. - The release job stops compiling tests: MOQX_BUILD_TESTS defaults ON, so it built every test executable and never ran ctest. - The releases API is read with a token; anonymous is 60/hour/IP, shared across runner egress.
- The build context no longer carries deps/, so .dockerignore drops the submodule excludes with it. - The moxygen stage is keyed on the pin rather than on src/, so a source-only push reuses the prefix. - It falls back to the superbuild when the pin publishes no prebuilt, the same ladder the build lanes use. - One prefix feeds both targets, so relay and interop-client cannot carry different moxygen builds.
The PR and main lanes build moqx's own TUs under ASan against uninstrumented prebuilt deps. A real ASan/TSan run needs the whole stack instrumented, which only the from-source superbuild produces, so it runs nightly rather than per push.
MOQBIN follows whatever moxygen the build resolved. ctest passes it per test, and scripts outside ctest source the moqx-tools.env the configure writes.
deps/moxygen is going away. The source comes from --moxygen-dir, then CPM_moxygen_SOURCE, then what the build already fetched, then a clone of the pinned rev.
Dead weight once CPM supplies both.
deps/moxygen/LICENSE is gone with the submodule; the fork's copy is the durable target. sync-relay.sh emits the same text it writes into the ported files, so the next sync is a no-op.
- The superbuild and the moqx build share the CPM clones, so a from-source moqx compiles against the exact source its moxygen prefix was built from, and a raw `cmake --preset` build reuses them instead of re-fetching into its own _deps. - The extracted prebuilt installs hang off the same root, so MOQX_DEPS_CACHE moves everything at once on a host with a small home partition.
- CI runs scripts/dev/format.sh; lint.sh is a manual entry point. - The targets' tools/*.cc globs never matched anything.
The static libs are not consumable without installed headers or an install(EXPORT), and they bloat the release tarballs.
- Nothing built or registered changes. - The load test stays unregistered: it needs a live relay. - The shell-test blocks stay explicit: their properties vary.
folly's static_assert on syscall addresses (NetOps.cpp) is not constant under -fsanitize=function, which `,undefined` pulls in. ASan's ABI still matches across the boundary, so moqx keeps both. One SanitizerFlags.cmake now holds the sets the moqx build and the superbuild share.
Only the env spelling was validated. -DBOOST_USE_STATIC_LIBS=auto, which superbuild/README.md advertises as the knob, passed straight through to Boost_USE_STATIC_LIBS where CMake reads it as truthy — forcing static Boost on the distros that package none, the exact failure the probe exists to avoid.
An INTERFACE target linked PRIVATE, so the warning set reaches moqx's own translation units — src/, test/ and benchmark/ — and propagates to no consumer. -Wmissing-field-initializers is off: GCC fires it on C++20 designated initializers that omit members carrying default member initializers, the idiom throughout the tree and correct. Clang does not warn there.
Most of compile_commands.json is CPM-fetched dependency code. Making .clang-tidy load surfaces thousands of warnings in it; that is #518.
Two equal dependency modes, the pin table, and the scripts that drive them.
- README drops the submodule bootstrap and `build.sh setup` for the trilogy. - ci-architecture documents the shared ~/.cache/moqx cache and how it is keyed. - release replaces the `.moxygen-release` tag file with the MOXYGEN_REV pin, so a release/* branch must freeze on a v*-tagged rev. - Repo paths are linked from the root so they resolve wherever they are rendered.
GitHub deprecated the Node 20 runtime: node20 actions still run, but on Node 24, and warn on every job. Each action goes to its lowest major that declares runs.using: node24, not to latest — the jumps are runtime-only, so this stays a version bump rather than a behaviour change. Two majors carry breaking notes that do not apply here: create-github-app-token v3 drops custom proxy handling (no HTTP(S)_PROXY anywhere in .github/), and setup-buildx-action v4 removes deprecated inputs (it is used with none). upload-pages-artifact is composite and has no runtime of its own; v5 is the first to pin a node24 upload-artifact internally. All of these need Actions Runner >= 2.327.1. The self-hosted linode runner reports 2.336.0.
clangd's default database search walks a source file's ancestors and a literal build/ under each; it never descends into a named build/<profile>/. Every translation unit therefore got an empty compilation database, with no error surfaced anywhere — only `clangd --check` showed the cascade of unresolved standard headers. Symlinking from configure.sh and not build.sh makes "last configured profile wins" fall out for free: build.sh runs on every incremental compile and would flap the link for reasons unrelated to which profile you work against.
Checks: * silently fell back to clang-tidy's default check set (a dead AnalyzeTemporaryDtors key made every run reject the config and continue anyway), so nothing in .clang-tidy actually applied. With the key dropped, Checks: * pulls in ~18k warnings, two-thirds of them vendor families for projects moqx isn't (LLVM's own libc, Fuchsia, Altera FPGA, Zircon, Android, ObjC, OpenMP, MPI, Darwin, Abseil, the Linux kernel). .clang-tidy now denies those families explicitly and gates CI on the categories worth failing a PR over (bugprone, performance, clang-analyzer, concurrency, cert). This makes the gate real against the existing codebase, which has ~375 pre-existing findings in that set not yet addressed — the next commit marks them, so this one alone does not pass. lint.sh also gained its own pass/fail logic instead of trusting run-clang-tidy's exit code: clang-analyzer-* diagnostics ignore HeaderFilterRegex and --exclude-header-filter (verified against 19.1.7), so a prebuilt dependency header's -isystem status doesn't stop the analyzer from reporting on it. Four such findings in moxygen's prebuilt folly headers are unreachable by NOLINT and uninteresting to us either way, so lint.sh now greps clang-tidy's output for errors located inside the checkout before deciding pass/fail. It also resolves clang-tidy-19/run-clang-tidy-19 explicitly rather than assuming bare names on PATH: Debian symlinks them, apt.llvm.org's packages (what CI installs) don't. clang-analyzer-* is most of a full run's cost (its symbolic execution vs the AST-matcher checks): ~13min without it vs ~35min with, across this checkout. lint.sh takes an explicit --mode=full|without-static-analyzer (defaulting to without-static-analyzer, and erroring on an unrecognized value) rather than a bare boolean, so a later flag added alongside it has something unambiguous to combine with instead of overloading a boolean's meaning. ci-pr.yml's PR-gating lint job installs clang-tidy 19 via apt.llvm.org (jammy's own repo only has 14), configures without building — clang-tidy needs the compilation database and fetched headers, not compiled objects — and runs the fast default. clang-tidy full runs the full --mode=full lint nightly, or on dispatch to vet a PR before merge, mirroring sanitizers.yml's existing nightly/dispatch pattern for other heavy checks. Even the fast default sweeps every TU, which is still most of a PR's build time even without the analyzer. lint.sh --changed scopes it to what the diff actually affects instead: a changed .cpp directly, or a changed .h resolved to every TU that transitively includes it, walking the #include graph by header basename (find-affected-tus.py) rather than trying to resolve exact include paths — over-inclusive on a basename collision, never under-inclusive, since missing a real dependent would silently defeat the point of gating this at all. Falls back to a full sweep if anything outside src/test/benchmark changed, or any .clang-tidy did, since either can affect files the diff doesn't touch. ci-pr.yml's lint job now runs scoped to the PR's diff against its base branch (needs fetch-depth: 0 — the default shallow clone has no origin/<base> to diff against).
375 pre-existing findings under the newly-gated categories (bugprone, performance, clang-analyzer, concurrency, cert), marked // NOLINTNEXTLINE(check) rather than fixed, so the gate the previous commit added starts green instead of blocking on a backlog nobody has triaged yet. NOLINTNEXTLINE, not a trailing // NOLINT on the flagged line itself: a trailing comment that pushes a line over the 100-column limit forces clang-format to rewrap the statement, which shifts every later finding in the file and cascades into spurious re-runs. A comment on its own line above leaves the flagged line untouched. 14 of those findings — all of them cert-err58-cpp, all in test/ — are global const TrackNamespace/FullTrackName/TrackAlias fixtures built from string literals at namespace scope. Their constructors are non-trivial enough to trip the check, but a throwing one here fails every run of the binary immediately rather than lurking as a latent bug, and the pattern is uniform across every site. Rather than 14 near-identical NOLINTNEXTLINEs, test/.clang-tidy drops cert-err58-cpp from WarningsAsErrors for the whole directory (inheriting everything else from the root config), leaving the remaining 361 as per-site markers. Each remaining suppression is either a real bug for a follow-up pass or a documented false positive; see the issue for the proposed triage order. None of this is a judgment call that the flagged code is fine — it's an inventory.
michalhosna
marked this pull request as draft
August 19, 2026 14:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First part of #518.
Does not fix anything at this point. Just enables working clang-tidy and ignores all current finds. So that we are not adding new finds. I will post another PRs down the line, fixing at least some of our finds.
clang-tidy is fairly slow & heavy → Run clang-tidy only on changed TUs in the PR, mostly optimalization for small PRs.
The heaviest part is clang-analyzer → Do not run clang-analyzer on PRs by default. Only nightly on main, and on request on PR.
- This breaks "Main always green", but the same problem is with full-instrumented asan at #519
Based on #519
This change is