build: Don't apply clang-tidy fixes twice via header symlinks (#8636)

.clang-tidy

@@ -140,5 +140,9 @@ CheckOptions:
   readability-identifier-naming.GlobalFunctionIgnoredRegexp: "^(to_string|hash_append|tuple_hash)$"
 
 HeaderFilterRegex: '^.*/(tests?|xrpl|xrpld)/.*\.(h|hpp|ipp)$'
-ExcludeHeaderFilterRegex: '^.*/protocol_autogen/.*\.(h|hpp)$|^.*\.pb\.h$'
+# Skip headers reached via add_module's symlinks (<build-dir>/modules/xrpl.lib*/...).
+# verify_headers already checks each one under its real path,
+# so reporting it twice would also apply its fixes twice.
+# .ipp files are kept: they are only ever reached via the symlink.
+ExcludeHeaderFilterRegex: '^.*/protocol_autogen/.*\.(h|hpp)$|^.*\.pb\.h$|^.*/modules/xrpl\.lib[^/]+/.*\.(h|hpp)$'
 WarningsAsErrors: "*"

CONTRIBUTING.md

@@ -365,14 +365,15 @@ With this set, the hook will run as part of `git commit` alongside the other pre
 Then run clang-tidy on your local changes:
 
 ```
-run-clang-tidy -p build -allow-no-checks src tests
+run-clang-tidy -p build -allow-no-checks include src tests
 ```
 
-This will check all source files in the `src`, `include` and `tests` directories using the compile commands from your `build` directory.
+This will check all source files in the `include`, `src` and `tests` directories using the compile commands from your `build` directory.
+Headers are checked on their own, which requires the `verify_headers` CMake option (ON by default).
 If you wish to automatically fix whatever clang-tidy finds _and_ is capable of fixing, add `-fix -format` to the above command:
 
 ```
-run-clang-tidy -p build -quiet -fix -format -allow-no-checks src tests
+run-clang-tidy -p build -quiet -fix -format -allow-no-checks include src tests
 ```
 
 `-format` reformats the fixed code with [`.clang-format`](./.clang-format); without it the fixes are inserted in LLVM style and the `clang-format` hook rewrites them afterwards.

bin/pre-commit/clang_tidy_check.py

@@ -5,15 +5,6 @@ The script determines the staged files itself (see `pass_filenames: false` in
 .pre-commit-config.yaml) so run-clang-tidy is run once and handles parallelism
 internally: pre-commit would otherwise split the files across parallel hook
 invocations that race when fixes edit a shared header.
-
-Fixes are collected with `-export-fixes` and applied by clang-apply-replacements
-in a separate step rather than with run-clang-tidy's `-fix`. The `add_module`
-build isolates each module's headers behind a per-module symlink directory
-(build/modules/<module>/...), so a header reachable from several translation
-units is referenced through different paths that all resolve to the same source
-file. clang-apply-replacements deduplicates identical replacements by their
-literal path, so those paths must be canonicalised to the real source path
-first; otherwise the same fix is applied once per path and corrupts the header.
 """
 
 from __future__ import annotations
@@ -23,7 +14,6 @@ import re
 import shutil
 import subprocess
 import sys
-import tempfile
 from pathlib import Path
 
 # Extensions run-clang-tidy can analyse: `.cpp` translation units and, thanks to
@@ -31,11 +21,6 @@ from pathlib import Path
 # compile_commands.json entry). `.ipp` fragments have no entry and are skipped.
 TIDY_EXTENSIONS = {".cpp", ".h", ".hpp"}
 
-# A single-quoted `FilePath:` entry in an -export-fixes YAML file, allowing the
-# `- ` marker that precedes it inside a `Replacements:` sequence. clang-tidy
-# emits paths single-quoted and doubles any embedded quote per YAML rules.
-FILEPATH_RE = re.compile(r"^(\s*(?:-\s+)?FilePath:\s*)'((?:[^']|'')*)'\s*$")
-
 
 def clang_tidy_major() -> str | None:
     """Major version of the `clang-tidy` on PATH, which run-clang-tidy invokes."""
@@ -79,25 +64,6 @@ def staged_files(repo_root: Path) -> list[Path]:
     return [repo_root / rel for rel in output.splitlines() if rel]
 
 
-def canonicalize_fix_paths(fixes_dir: Path) -> None:
-    """Rewrite every `FilePath` in the exported fixes to its real source path.
-
-    A header included through a module's isolation symlink is recorded under that
-    symlink's path; collapsing all paths to the same real file lets
-    clang-apply-replacements recognise the per-translation-unit duplicates and
-    apply each fix once.
-    """
-    for yaml in fixes_dir.glob("*.yaml"):
-        lines = []
-        for line in yaml.read_text().splitlines():
-            if m := FILEPATH_RE.match(line):
-                path = m.group(2).replace("''", "'")
-                real = os.path.realpath(path).replace("'", "''")
-                line = f"{m.group(1)}'{real}'"
-            lines.append(line)
-        yaml.write_text("\n".join(lines) + "\n")
-
-
 def main():
     if not os.environ.get("TIDY"):
         return 0
@@ -116,19 +82,10 @@ def main():
 
     version = clang_tidy_major()
     run_clang_tidy = find_tool("run-clang-tidy", version)
-    clang_apply_replacements = find_tool("clang-apply-replacements", version)
-    missing = [
-        name
-        for name, path in (
-            ("run-clang-tidy", run_clang_tidy),
-            ("clang-apply-replacements", clang_apply_replacements),
-        )
-        if not path
-    ]
-    if missing:
+    if not run_clang_tidy:
         tried = f" (tried the '-{version}' suffix too)" if version else ""
         print(
-            f"clang-tidy check failed: TIDY is enabled but {' and '.join(missing)} "
+            f"clang-tidy check failed: TIDY is enabled but run-clang-tidy "
             f"was not found in PATH{tried}.",
             file=sys.stderr,
         )
@@ -143,27 +100,19 @@ def main():
         )
         return 1
 
-    with tempfile.TemporaryDirectory() as fixes_dir:
-        result = subprocess.run(
-            [
-                run_clang_tidy,
-                "-quiet",
-                "-p",
-                build_dir,
-                "-export-fixes",
-                fixes_dir,
-                "-allow-no-checks",
-            ]
-            + files
-        )
-        canonicalize_fix_paths(Path(fixes_dir))
-        # `FormatStyle` in .clang-tidy does not reach this path,
-        # so ask for the repository style here.
-        applied = subprocess.run(
-            [clang_apply_replacements, "--format", "--style=file", fixes_dir]
-        )
-
-    return result.returncode or applied.returncode
+    result = subprocess.run(
+        [
+            run_clang_tidy,
+            "-quiet",
+            "-p",
+            build_dir,
+            "-fix",
+            "-format",
+            "-allow-no-checks",
+        ]
+        + files
+    )
+    return result.returncode
 
 
 if __name__ == "__main__":

cmake/isolate_headers.cmake

@@ -34,11 +34,12 @@ include(create_symbolic_link)
 # sometimes `PUBLIC` and sometimes `PRIVATE`.
 # The Cs are all guaranteed to be unique.
 # We can guarantee a unique X per target by using
-# `${CMAKE_CURRENT_BINARY_DIR}/include/${target}`.
+# `${CMAKE_CURRENT_BINARY_DIR}/modules/${target}`.
 #
 # isolate_headers(target A B scope)
 function(isolate_headers target A B scope)
     file(RELATIVE_PATH C "${A}" "${B}")
+    # Keep in sync with ExcludeHeaderFilterRegex in .clang-tidy.
     set(X "${CMAKE_CURRENT_BINARY_DIR}/modules/${target}")
     set(Y "${X}/${C}")
     cmake_path(GET Y PARENT_PATH parent)

nix/packages.nix

@@ -62,8 +62,8 @@ let
   # version-suffixed name first and fall back to a system binary on the PATH
   # when Nix doesn't provide it:
   #   - Conan's Boost recipe looks up `g++-<major>` before plain `g++`.
-  #   - bin/pre-commit/clang_tidy_check.py looks up `run-clang-tidy-<v>` and
-  #     `clang-apply-replacements-<v>` before the unsuffixed names.
+  #   - bin/pre-commit/clang_tidy_check.py looks up `run-clang-tidy-<v>`
+  #     before the unsuffixed name.
   # On a host that also has the matching system binary
   # (e.g. Ubuntu's `/usr/bin/g++-<v>` or `clang-tidy-<v>`),
   # the probe escapes Nix and mixes a system tool into the Nix environment.

src/test/app/ValidatorSite_test.cpp

@@ -132,21 +132,8 @@ private:
         std::chrono::seconds effectiveOverlap = detail::kDefaultEffectiveOverlap;
         int expectedRefreshMin = 0;
     };
-
-    /**
-     * Serve each configured list from its own publisher server, fetch them all
-     * through one ValidatorSite and check what it reports for each site.
-     *
-     * @param good Directory the cache files are written to.
-     * @param paths One server per entry, with what to serve and what to expect.
-     * @param requestTimeout How long the ValidatorSite waits for a response
-     * before it reports that the request took too long.
-     */
     void
-    testFetchList(
-        detail::DirGuard const& good,
-        std::vector<FetchListConfig> const& paths,
-        std::chrono::seconds requestTimeout = std::chrono::seconds{12})
+    testFetchList(detail::DirGuard const& good, std::vector<FetchListConfig> const& paths)
     {
         testcase << "Fetch list - "
                  << boost::algorithm::join(
@@ -224,10 +211,10 @@ private:
 
         BEAST_EXPECT(trustedKeys.load({}, emptyCfgKeys, cfgPublishers));
 
-        // Normally, tests will only need a fraction of the default timeout,
-        // but sometimes DNS resolution takes an inordinate amount of time, so
-        // the test will just wait.
-        auto sites = std::make_unique<ValidatorSite>(env.app(), journal, requestTimeout);
+        // Normally, tests will only need a fraction of this time,
+        // but sometimes DNS resolution takes an inordinate amount
+        // of time, so the test will just wait.
+        auto sites = std::make_unique<ValidatorSite>(env.app(), journal, 12s);
 
         std::vector<std::string> uris;
         for (auto const& u : servers)
@@ -599,19 +586,18 @@ public:
                         .failApply = true,
                     },
                 });
-            // timeout: the server answers 1s after the request times out
+            // timeout
             testFetchList(
                 good,
                 {
                     {
-                        .path = "/sleep/2",
+                        .path = "/sleep/13",
                         .msg = "took too long",
                         .ssl = ssl,
                         .failFetch = true,
                         .failApply = true,
                     },
-                },
-                std::chrono::seconds{1});
+                });
             // bad manifest format using known versions
             // * Retrieves a v1 formatted list claiming version 2
             testFetchList(