cmake: add native clang-tidy support for tests, with concatenated sources#20667
Closed
vszakats wants to merge 12 commits intocurl:masterfrom
Closed
cmake: add native clang-tidy support for tests, with concatenated sources#20667vszakats wants to merge 12 commits intocurl:masterfrom
vszakats wants to merge 12 commits intocurl:masterfrom
Conversation
1 task
384f5ca to
9af829b
Compare
vszakats
added a commit
that referenced
this pull request
Feb 22, 2026
Instead of globally disabling unity for all targets when clang-tidy is enabled. After this patch `CMAKE_UNITY_BUILD=ON` is honored for: - static libcurl when building both static and shared separately. - libcurlu and libcurltool internal libraries when building the test target. While keeping unity disabled for the libcurl build pass running clang-tidy, and the curl tool, also running clang-tidy. To make clang-tidy-enabled builds finish faster when unity mode is enabled, yet performs the same clang-tidy checks as before this patch. Effect on: - GHA/macos: core build: same, buils tests 5-12 seconds faster, with steps going down from 259 to 25. 52s: https://github.com/curl/curl/actions/runs/22279958340/job/64448913325 -> 47s: https://github.com/curl/curl/actions/runs/22279873606/job/64448710743 - GHA/windows (not enabled): it'd save about 1 minute, bringing total time barely below 10m, still one of the slowest jobs overall. (#20667 is trying a way for 4x speed-up (with a drawback)). 5m21s: https://github.com/curl/curl/actions/runs/22222907068/job/64284556852 -> 4m26s: https://github.com/curl/curl/actions/runs/22281033369/job/64451601548 Closes #20670
9af829b to
4a2a722
Compare
vszakats
added a commit
that referenced
this pull request
Feb 23, 2026
Backtrack on previous change that aimed to solve the wrong `share.h` being included. It turns out it did not fix this issue. At the same time it introduced relative header filenames and the need to include the same headers differently depending on the source files' location, reducing readability and editability. Replace this method by re-adding curl's lib source directory to the header path and addressing headers by the their full, relative name to that base directory. Aligning with this method already used in src and tests. With these advantages: - makes includes easier to read, recognize, grep, sort, write, and copy between sources, - syncs the way these headers are included across curl components, - avoids the ambiguity between system `schannel.h`, `rustls.h` vs. local headers using the same names in `lib/vtls`, - silences clang-tidy `readability-duplicate-include` checker, which detects the above issue, Ref: https://clang.llvm.org/extra/clang-tidy/checks/readability/duplicate-include.html - possibly silences TIOBE coding standard warnings: `6.10.2.a: Don't use relative paths in #include statements.` - long shot: it works well with concatenated test sources, for clang-tidy-friendly custom unity builds. Ref: #20667 Slight downside: it's not enforced. If there happens to be a collision between a local `lib/*.h` header and a system one, the solution is to rename (possibly with its `.c` counterpart) into the `curl_` namespace. This is also the method used by curl in the past. Also: - curlx/inet_pton: reduce scope of an include. - toolx/tool_time: apply this to an include, and update VS project files accordingly. Also dropping unnecessary lib/curlx header path. - clang-tidy: enable `readability-duplicate-include`. Follow-up to 3887069 #19676 Follow-up to 625f2c1 #16991 #16949 Closes #20623
4a2a722 to
0973858
Compare
clang-tidy is missing `#line` support: llvm/llvm-project#62405 llvm/llvm-project#83025 (dupe)
This reverts commit 8e1b9c9.
0973858 to
c116cf8
Compare
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
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.
Tests are build in "unity"-style, by including sources into an umbrella
C files (similar to how CMake unity works). This does not play well with
clang-tidy, which seems to unconditionally ignore C sources included
like this. To fix it, curl's CMake implements a manual clang-tidy
support for tests, which compiles sources one-by-one, while also making
sure sources compile cleanly standalone (e.g. all sources need to
include
first.h). The manual clang-tidy implementation is fragile, andperformance, in particular when targeting Windows, is abysmal.
This patch introduces an alternate solution, enabled by the
_CURL_TESTS_CONCAT=ONoption. In this mode, umbrella sources includethe actual sources instead of
#includingthem. Allowing to use CMake'sbuilt-in clang-tidy support to compile them, with clang-tidy actually
checking the sources. Making the manual clang-tidy support unnecessary.
In the Windows CI job it results in a 4x performance improvement (4m →
1m), making it practical to run clang-tidy on tests on Windows, in CI.
The main downside is that clang-tidy doesn't understand the
#linedirective. Meaning issues found show the wrong filename and line number
next to them. It's not impossible to locate errors this way, but also
not convenient.
Minor/potential downside is that the concatenated source needs to be
reassembled each time an original source is updated. This may result in
more copying on the disk when used in local development. The largest
source is 1.4MB, so probably not a show-stopper on most machines.
Another is the complexity of maintaining two methods in parallel, which
may be necessary till clang-tidy understands
#line:llvm/llvm-project#62405
This solution may in theory also enable adding clang-tidy support for
tests in autotools, though I haven't tried.
Targeted for curl CI for now, and used in a GHA/windows job. 100%
experimental, not recommended outside these.
https://github.com/curl/curl/pull/20667/files?w=1
the fragile workaround trying to emulate it manually.
(via
curl_add_clang_tidy_test_target())This possibly makes it work in more cross-build scenarios
(and possibly other corner-cases), though I haven't stress-tested
it yet.
plays well with normal complation.
(minor downside: umbrealla file needs to be regenerated
on source changes.)
Ref: https://github.com/curl/curl/actions/runs/22267951773/job/64417510535?pr=20667
Still slow partly due to curlu and curltool libs. → down to 1m8 with unity force-enabled for curlu and curltool ref: https://github.com/curl/curl/actions/runs/22270239494/job/64432225756?pr=20667. 5x speedup. also 1m8 with alternate, better patch: https://github.com/curl/curl/actions/runs/22280029021/job/64449085738?pr=20667