Skip to content

build: tidy up compiler definition for tests#17768

Closed
vszakats wants to merge 17 commits intocurl:masterfrom
vszakats:cm-compopt
Closed

build: tidy up compiler definition for tests#17768
vszakats wants to merge 17 commits intocurl:masterfrom
vszakats:cm-compopt

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Jun 28, 2025 •
edited
Loading

  • tests: merge cmake commands.
  • tests: use target_compile_definitions().
  • tests/server: use generator expression for platform-specific macro.
  • tests/unit: sync Makefile.am comment with cmake.
  • tests/unit: merge two AM_CPPFLAGS lines to keep synced with cmake.
  • tests: move macro definitions to first.h headers from build level.
    CURL_NO_OLDIES, CURL_DISABLE_DEPRECATION, WITHOUT_LIBCURL,
    CURL_STATICLIB (for servers).
    To share more logic.
    Pass CURL_STATICLIB in server on all platforms for simplicity.
    (On non-Windows, it's a no-op. It's already done like this with curlu
    and libcurltool.)

Also for lib:

  • lib: merge commands.
  • lib: sync macro order with tests (also in Makefile.am).
  • perhaps leave CURL_NO_OLDIES, CURL_DISABLE_DEPRECATION
    on build level for better readability?
    No, this seems fine on a second before/after review.

@github-actions github-actions bot added the tests label Jun 28, 2025
@vszakats vszakats marked this pull request as draft June 28, 2025 00:20
@vszakats vszakats changed the title cmake: tidy up compiler definition commands build: tidy up compiler definition for tests Jun 28, 2025
vszakats added a commit to vszakats/curl that referenced this pull request Jun 28, 2025
@vszakats
curlx: replace WITHOUT_LIBCURL with BUILDING_LIBCURL
f5dd573 
Cherry-picked from curl#17768
@vszakats vszakats mentioned this pull request Jun 28, 2025
Closed
vszakats added a commit to vszakats/curl that referenced this pull request Jul 4, 2025
@vszakats
cmake: fix curl_add_clang_tidy_test_target for targets with no -D…
2f3219c 
… option

Current build doesn't hit this case, but a pending PR does.

Fixing:
```
[...] -Ilib -Itests/client -DCURL_HIDDEN_SYMBOLS -DHAVE_CONFIG_H -D_definitions_t-NOTFOUND
```
error: ISO C99 requires whitespace after the macro name [clang-diagnostic-c99-extensions,-warnings-as-errors]

Ref: curl#17768
This was referenced Jul 4, 2025
Closed
Closed
vszakats added a commit to vszakats/curl that referenced this pull request Jul 4, 2025
@vszakats
cmake: fix curl_add_clang_tidy_test_target when no -D option
42fdc65 
Fix `curl_add_clang_tidy_test_target` generating an invalid option for
`clang-tidy` if the tested target has no custom macro definition.

Current build doesn't hit this case, but a pending PR does.

Fixing:
```
[...] -Ilib -Itests/client -DCURL_HIDDEN_SYMBOLS -DHAVE_CONFIG_H -D_definitions_t-NOTFOUND
```
error: ISO C99 requires whitespace after the macro name [clang-diagnostic-c99-extensions,-warnings-as-errors]

Cherry-picked from curl#17768
Closes curl#17813
vszakats added a commit to vszakats/curl that referenced this pull request Jul 4, 2025
@vszakats
cmake: curl_add_clang_tidy_test_target tidy-up
78b29f3 
Factor out two local variables.

Cherry-picked from curl#17768

Closes curl#17814
vszakats added a commit to vszakats/curl that referenced this pull request Jul 4, 2025
@vszakats
cmake: curl_add_clang_tidy_test_target tidy-up
70c5eab 
Factor out two local variables.

Cherry-picked from curl#17768

Closes curl#17814
vszakats added a commit to vszakats/curl that referenced this pull request Jul 4, 2025
@vszakats
cmake: curl_add_clang_tidy_test_target tidy-ups
3238ba5 
- simplify gathering header directories and compiler definitions
  recursively.

- handle the case when the cmake directory doesn't define header
  directories or compiler definitions.

- honor more corners cases:
  - `INTERFACE_INCLUDE_DIRECTORIES` of the initial target.
  - handle no header directory for initial target.

- de-duplicate header directories and compiler redefinitions to mimic
  CMake.

- drop unnecessary `unset()`s.

Note that the order of header directories remains different compared to
how CMake passes them to the compiler when building tests. The order is
already skewed in the test target `INCLUDE_DIRECTORIES` property,
preventing to reproduce the CMake order. The distinction between `-I`
and `-isystem` is also missing from target properties.

Cherry-picked from curl#17768

Closes curl#17814
vszakats added a commit to vszakats/curl that referenced this pull request Jul 4, 2025
@vszakats
cmake: curl_add_clang_tidy_test_target tidy-ups
8a948af 
- simplify gathering header directories and compiler definitions
  recursively.

- handle the case when the cmake directory doesn't define header
  directories or compiler definitions.

- honor more corners cases:
  - `INTERFACE_INCLUDE_DIRECTORIES` of the initial target.
  - handle no header directory for initial target.

- de-duplicate header directories and compiler redefinitions to mimic
  CMake.

- drop unnecessary `unset()`s.

Note that the order of header directories remains different compared to
how CMake passes them to the compiler when building tests. The order is
already skewed in the test target `INCLUDE_DIRECTORIES` property,
preventing to reproduce the CMake order. The distinction between `-I`
and `-isystem` is also missing from target properties.

Cherry-picked from curl#17768

Closes curl#17814
vszakats added a commit that referenced this pull request Jul 4, 2025
@vszakats
cmake: curl_add_clang_tidy_test_target tidy-ups
e54c80b 
- simplify gathering header directories and compiler definitions
  recursively.

- handle the case when the cmake directory object doesn't define header
  directories or compiler definitions.

- honor more corners cases:
  - `INTERFACE_INCLUDE_DIRECTORIES` of the initial target.
  - handle no header directory for initial target.

- de-duplicate header directories and compiler redefinitions to mimic
  CMake.

- drop unnecessary `unset()`s.

Note that the order of header directories remains different compared to
how CMake passes them to the compiler when building tests. The order is
already different in the test target `INCLUDE_DIRECTORIES` property,
preventing to reproduce the exact CMake order. The distinction between
`-I` and `-isystem` is also missing from target properties.

Cherry-picked from #17768

Closes #17814
@vszakats vszakats added the feature-window A merge of this requires an open feature window label Jul 7, 2025
@vszakats vszakats mentioned this pull request Jul 9, 2025
Closed
@vszakats vszakats marked this pull request as ready for review July 28, 2025 09:32
vszakats added 14 commits July 28, 2025 11:33
@vszakats
cleanup
7ea51a1
@vszakats
cleanup-2
1891eef
@vszakats
cleanup-3
7273ad3
@vszakats
cleanup-4
249b6ce
@vszakats
sync am
6a76d67
@vszakats
fixup
50b9895
@vszakats
fixup-gen-exp
2f8eacb
@vszakats
tests/server: always pass CURL_STATICLIB (also on non-Windows)
3b8364e 
To avoid generator expressions. Generator expresson do not work with
curl_clang_tidy_tests() macro, resulting in an empty `-D` option and
breaking clang-tidy:

```
error: macro name must be an identifier [clang-diagnostic-error]
```
https://github.com/curl/curl/actions/runs/15938223758/job/44962176793?pr=17768#step:14:337

But, curl/curl.h behaves the same on non-Windows with tests/server with
and without CURL_STATICLIB, making it safe to pass for all platforms.
@vszakats
tests/server: define CURL_STATICLIB in first.h instead of cm/am
f5906fa
@vszakats
tests/server: move WITHOUT_LIBCURL from build to first.h
819f636
@vszakats
tests: move CURL_NO_OLDIES from build to first.h
96dcf33
@vszakats
tests: move CURL_DISABLE_DEPRECATION from build to first.h
795b243
@vszakats
lib sync macro order with tests/unit
ddf5da0
@vszakats
tests/server: define CURL_STATICLIB for all targets (curlu does it al…
7d5ebb1 
…ready)
@vszakats vszakats closed this in 2c90c3a Jul 28, 2025
@vszakats vszakats deleted the cm-compopt branch July 28, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build cmake feature-window A merge of this requires an open feature window tests tidy-up

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vszakats

Comments