Skip to content

build: drop explicit curlx from hdr paths, refer headers with curlx/ prefix #17680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 23 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jun 20, 2025

To make all src and test code refer to curlx headers the same way.

Also:

  • src: move curlx.h include to tool_setup.h.
  • src/tool_setup.h: drop stray curlx/timeval.h.
  • servers: de-duplicate curlx.h and curl_setup.h includes.
  • libtests, units: drop stray curlx sub-headers in favor of
    <curlx/curlx.h>.
  • tests: include curlx.h with <> instead of "". To match
    other parts of the codebase.

/Users/runner/work/curl/curl/bld/tests/client/clients.c:38:10: error: '../curl_setup.h' file not found, did you mean 'curl_setup.h'? [clang-diagnostic-error]
   38 | #include "../curl_setup.h"
      |          ^~~~~~~~~~~~~~~~~
      |          "curl_setup.h"
/Users/runner/work/curl/curl/bld/tests/client/clients.c:391:10: error: 'timediff.h' file not found [clang-diagnostic-error]
  391 | #include "timediff.h"
      |          ^~~~~~~~~~~~
2 errors generated.

https://github.com/curl/curl/actions/runs/15774892957/job/44467063782?pr=17680#step:14:250

@vszakats vszakats marked this pull request as draft June 20, 2025 08:27
@vszakats vszakats changed the title build: drop explicit curlx from hdr path, refer to headers with curlx/ prefix. build: try dropping explicit curlx from hdr path, refer to headers with curlx/ prefix. Jun 20, 2025
@vszakats vszakats changed the title build: try dropping explicit curlx from hdr path, refer to headers with curlx/ prefix. build: try dropping explicit curlx from hdr path, refer to headers with curlx/ prefix Jun 20, 2025
@vszakats vszakats force-pushed the t-drop-curlx-hdr-path branch from fcd5cd4 to c234533 Compare June 20, 2025 11:35
@vszakats
Copy link
Member Author

vszakats commented Jun 20, 2025

Noting that the clang-tidy oddities seen earlier are still here to see:

clang-tidy is saying: llvm/llvm-project#67550

edit: This was fixed by making sure that each test C files compile as individual units, then refactoring clang-tidy for tests, by doing it manually per-file, instead of running it on the bundle C files via the CMake built-in clang-tidy facility. → #17703 #17705

vszakats added a commit that referenced this pull request Jun 21, 2025
Skip clang-tidy while compiling curlu and curltool internal libraries.
To save about 1 minute per run. These libraries compile the lib and src
sources a second time, with the `UNITTESTS` macro enabled, which makes
tiny difference, for internal use. I figure it's not worth the extra CI
(and local) time because finding extra issues in these passes is
unlikely, and if found, not critical.

autotools also doesn't check curlu and curltool with clang-tidy.

Ref: #17680 (comment)
Ref: https://stackoverflow.com/questions/61867616/ignore-certain-files-when-using-clang-tidy
Ref: https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html

Follow-up to fabfa8e #15825

Closes #17693
@vszakats vszakats force-pushed the t-drop-curlx-hdr-path branch from c234533 to a95e313 Compare June 21, 2025 09:20
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
Skip clang-tidy while compiling curlu and curltool internal libraries.
To save about 1 minute per run. These libraries compile the lib and src
sources a second time, with the `UNITTESTS` macro enabled, which makes
tiny difference, for internal use. I figure it's not worth the extra CI
(and local) time because finding extra issues in these passes is
unlikely, and if found, not critical.

autotools also doesn't check curlu and curltool with clang-tidy.

Ref: curl#17680 (comment)
Ref: https://stackoverflow.com/questions/61867616/ignore-certain-files-when-using-clang-tidy
Ref: https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html

Follow-up to fabfa8e curl#15825

Closes curl#17693
vszakats added a commit that referenced this pull request Jun 22, 2025
Tidy up headers and includes to ensure all individual test source
compile cleanly (but not link). To allow running clang-tidy (and
possibly other static analyzers) on them. It also improves readability
and allows to verify them locally, without the bundle logic.

clang-tidy ignores #included C files, so it's blind to bundle C files
the include these tests. The current workaround of embedding has
a couple of downsides:. meaningless filenames and line numbers,
missing issues, messing up self header paths. Thus, running it on
individual sources would be beneficial.

Also:
- de-duplicate includes.
- untangle some includes.
- formatting/indentation fixes.
- merge `getpart.h` into `first.h`.

Ref: #17680 (comment)

Closes #17703
vszakats added a commit that referenced this pull request Jun 22, 2025
Replace existing `mk-unity.pl` `--embed` workaround with running
`clang-tidy` manually on individual test source instead. This aligns
with how clang-tidy works and removes `mk-unity.pl` from the solution.

Also:
- mqttd: fix potentially uninitialized buffer by zero filling it.
  ```
  tests/server/mqttd.c:484:41: error: The left operand of '<<' is a garbage value
    [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors]
    484 |       payload_len = (size_t)(buffer[10] << 8) | buffer[11];
        |                                         ^
  [...]
  tests/server/mqttd.c:606:45: error: The left operand of '<<' is a garbage value
    [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors]
    606 |       topiclen = (size_t)(buffer[1 + bytes] << 8) | buffer[2 + bytes];
        |                                             ^
  ```
- sockfilt: fix potential out-of-bound pointer:
  ```
  tests/server/sockfilt.c:1128:33: error: The 2nd argument to 'send' is a buffer
     with size 17010 but should be a buffer with size equal to or greater than
     the value of the 3rd argument (which is 18446744073709551615)
     [clang-analyzer-unix.StdCLibraryFunctions,-warnings-as-errors]
   1128 |         ssize_t bytes_written = swrite(sockfd, buffer, buffer_len);
        |                                 ^
  ```
- clang-tidy: suppress bogus `bzero()` warnings that happens
  inside the notorious `FD_ZERO()` macros, on macOS.

Ref: #17680 (comment)

Closes #17705
@vszakats vszakats force-pushed the t-drop-curlx-hdr-path branch 3 times, most recently from 564c606 to 2ae78e0 Compare June 22, 2025 21:46
@vszakats vszakats changed the title build: try dropping explicit curlx from hdr path, refer to headers with curlx/ prefix build: drop explicit curlx from hdr paths, refer headers with curlx/ prefix Jun 23, 2025
@vszakats vszakats marked this pull request as ready for review June 23, 2025 06:49
@vszakats vszakats force-pushed the t-drop-curlx-hdr-path branch from 6748564 to 5f4748b Compare June 23, 2025 11:55
@vszakats vszakats closed this in 1a70977 Jun 23, 2025
@vszakats vszakats deleted the t-drop-curlx-hdr-path branch June 23, 2025 15:03
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