Skip to content

cmake: force-disable unity for clang-tidied build targets only#20670

Closed
vszakats wants to merge 5 commits intocurl:masterfrom
vszakats:cminternallibunity
Closed

cmake: force-disable unity for clang-tidied build targets only#20670
vszakats wants to merge 5 commits intocurl:masterfrom
vszakats:cminternallibunity

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented 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:

Closes #20670


https://github.com/curl/curl/pull/20670/files?w=1

  • consider enabling unity for libcurlu and libcurltool permanently (as in 'initial idea' below). [OTHER PR] → cmake: always build curlu and curltool test libs in unity mode #20677

  • additional opportunity is to selectively disable unity for the shared libcurl only, when building both shared and static. This has no downside AFAICS.

  • initial idea: always enable unity for internal libs curlu and curltool:

    • + unity tested even more, better perf by def when building tests, better perf in cases needing no-unity.
    • - units/tunits always test against unity-made libs with cmake, libcurlu and libcurltool is no longer clang-tidy tested (curlu has a bunch of UNITTEST-only function, curltool does not [edit: this is moot: clang-tidy is already explicitly disabled for curlu and curltool since ccb6564 cmake: omit clang-tidy on internal libs curlu and curltool #17693).
  • alternative take: consider force-disabling unity only for targets checked with clang-tidy when clang-tidy is enabled. (More complex, more limited, possibly safer, more flexible?) → went with this one.

@vszakats
Copy link
Member Author

vszakats commented Feb 22, 2026

Fun fact: clang-tidy may still check into system headers without the --system-headers option set
(or explicitly disabled via SystemHeaders: false config), this, combined with --fix (or
--fix-error or a combination of them) may result in editing and silently breaking for example
Homebrew OpenSSL headers. Causing surprise and fatal clang-diagnostic-error errors on next runs.

I find it unpredictable which included file / header is checked by clang-tidy.

E.g. the manual page https://clang.llvm.org/extra/clang-tidy/index.html says --header-filter=<string>
has the default value of .*, yet, with the default, it doesn't check local/private headers included via
#include "name.h". When setting --header-filter=.* (or HeaderFilterRegex: '.*' via config), it
does, but then it also looks into macOS SDK and libssh2 headers. Both included with <>, the former
presumably via -isysroot (implicitly by clang), the latter on an explicit -isystem header path.

Correction: The v23 (dev) docs mention these as defaults. The v21 (stable, as used in these tests)
one https://releases.llvm.org/21.1.0/tools/clang/tools/extra/docs/clang-tidy/index.html does not say
say about defaults.

Hard to say if this is a documentation fix, or maybe .* becomes the default on the next release,
and/or maybe this means it got fixes which will have made this safe. edit: Indeed, .* became the default in v22.1.0: https://releases.llvm.org/22.1.0/tools/clang/tools/extra/docs/ReleaseNotes.html#improvements-to-clang-tidy

edit: fix → #20724

@vszakats vszakats marked this pull request as draft February 22, 2026 14:21
Instead of blanked disabling it for all targets. In practice it means
`CMAKE_UNITY_BUILD=ON` is honored for libcurlu and libcurltool internal
libraries after this patch, making clang-tidy builds faster, by
avoiding checking
@vszakats vszakats changed the title [TEST] cmake: always enable unity for internal libs curlu and curltool cmake: disable unity for clang-tidied build targets only Feb 22, 2026
@vszakats vszakats marked this pull request as ready for review February 22, 2026 15:29
@github-actions github-actions bot added the CI Continuous Integration label Feb 22, 2026
This reverts commit 0312564.
@vszakats vszakats changed the title cmake: disable unity for clang-tidied build targets only cmake: force-disable unity for clang-tidied build targets only Feb 22, 2026
@vszakats vszakats closed this in fff9905 Feb 22, 2026
@vszakats vszakats deleted the cminternallibunity branch February 22, 2026 18:16
vszakats added a commit to vszakats/curl that referenced this pull request Feb 22, 2026
To speed up building tests by default.

Follow-up to fff9905 curl#20670
@vszakats
Copy link
Member Author

vszakats commented Feb 25, 2026

I find it unpredictable which included file / header is checked by clang-tidy.

It turned out to be my mistake. System headers were picked accidentally
when using the manual clang-tidy method used for tests with cmake. This
needs manually reconstructing the C compiler command-line, including
headers path. When doing this, my code added all of them with -I, meaning
system (and 3rd-party) ones too. This made them appear as local headers
for clang-tidy.

Testing and fixing in #20720, and it'll become a separate PR. → #20724

vszakats added a commit that referenced this pull request Feb 25, 2026
Pass system directories with `-isystem` to avoid clang-tidy parsing
3rd-party and system headers with `HeaderFilterRegex: '.*' enabled.

Also:
- drop rule exception no longer necessary.
- sync normal vs. system header path order with compiler invocation.
- tidy up `set()` syntax.
- clear a temporary variable.

Bug: #20670 (comment)
Follow-up to e088e10 #17705
Cherry-picked from: #20720

Closes #20724
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

1 participant