Skip to content

GHA: add a checksrc job#14625

Closed
bagder wants to merge 4 commits intomasterfrom
bagder/checksrc-ci
Closed

GHA: add a checksrc job#14625
bagder wants to merge 4 commits intomasterfrom
bagder/checksrc-ci

Conversation

@bagder
Copy link
Member

@bagder bagder commented Aug 21, 2024

This job unconditionally runs checksrc on ALL .c and .h files present in git.

checksrc.pl: fixed to look for ".checksrc" in the same directory from where it loads the file to check

@bagder bagder added the CI Continuous Integration label Aug 21, 2024
@bagder
Copy link
Member Author

bagder commented Aug 21, 2024

Because of #14624

@github-actions github-actions bot added the tests label Aug 21, 2024
@bagder bagder force-pushed the bagder/checksrc-ci branch from 7971abd to d3efa88 Compare August 21, 2024 07:50
@bagder
Copy link
Member Author

bagder commented Aug 21, 2024

It looks like old-mingw is somehow not understanding the CURL_TEMP_PRINTF() macro in curl/mprintf.h correctly.

@bagder
Copy link
Member Author

bagder commented Aug 21, 2024

@vszakats what does "old-mingw" mean in this CI context? It looks like I might need to add some additional conditions.

vszakats
vszakats reviewed Aug 21, 2024
View reviewed changes
vszakats
vszakats reviewed Aug 21, 2024
View reviewed changes
@vszakats
Copy link
Member

vszakats commented Aug 21, 2024 •
edited
Loading

@vszakats what does "old-mingw" mean in this CI context? It looks like I might need to add some additional conditions.

It's a shortcut for old-mingw-w64, meaning old, standalone builds of mingw-w64. (not the original "old-mingw")

edit: Thinking to rename to mingw-old..

@bagder

This comment was marked as resolved.

Sign in to view
@bagder

This comment was marked as outdated.

Sign in to view
@vszakats
Copy link
Member

vszakats commented Aug 21, 2024

I'll look into this, thanks!

@vszakats
Copy link
Member

vszakats commented Aug 21, 2024 •
edited
Loading

[edit: RESOLVED, but unhidden for future reference.]

I couldn't confirm that the tests/http/client programs pick up the wrong curl headers. This part looks fine to me. If it was wrong, the 9.5.0 job would also break the same way. If you have logs that suggest otherwise, let me know and I'm happy to look again.

Adding more mingw-w64 variants revealed that the cutoff point was mingw-w64 8.0.0, which made things work out of the box, and mingw-w64 7.0.0 and older are breaking by default. (#14633)

Example toolchains:
mingw-w64 7.0.0: https://github.com/brechtsanders/winlibs_mingw/releases/tag/8.4.0-7.0.0-r1
mingw-w64 8.0.0: https://github.com/brechtsanders/winlibs_mingw/releases/tag/8.4.1-snapshot20210401-r1
CI run:
https://github.com/curl/curl/actions/runs/10490818399

This is not bound to the GCC version, but the mingw-w64 package version which pulls the CRT headers, implibs and other things. At build-time this value is in __MINGW64_VERSION_MAJOR (needs _mingw.h.)

The relevant line in this announcement:
https://sourceforge.net/p/mingw-w64/mailman/message/37111166/ via https://www.mingw-w64.org/changelog/
is "__USE_MINGW_ANSI_STDIO now automatically enabled in C99 and C11 mode when not using UCRT by Pali Rohár"

Setting the CPPFLAGS -D__USE_MINGW_ANSI_STDIO=1 fixes it for affected older versions:
https://github.com/curl/curl/actions/runs/10492129092/job/29062925504#step:12:438

External builds (=someone using this code pattern with these mingw-w64 distros) remain affacted, unless they use the same workaround.

Here's the answer why lib/src aren't affected:

curl/lib/curl_setup.h

Lines 320 to 329 in 9fff074

/* curl uses its own printf() function internally. It understands the GNU
* format. Use this format, so that is matches the GNU format attribute we
* use with the MinGW compiler, allowing it to verify them at compile-time.
*/
#ifdef __MINGW32__
# undef CURL_FORMAT_CURL_OFF_T
# undef CURL_FORMAT_CURL_OFF_TU
# define CURL_FORMAT_CURL_OFF_T "lld"
# define CURL_FORMAT_CURL_OFF_TU "llu"
#endif

I'm not sure, but in case curl publishes the CURL_FORMAT_CURL_OFF_T[U] macros to be used solely with curl_*printf() functions, I think the above could be promoted to include/curl/system.h, which would fix this problem for tests, lib, src and everyone using this pattern in their own code with the affected toolchains.

@dfandrich
Copy link
Contributor

dfandrich commented Aug 21, 2024

Analysis of PR #14625:

Test 291 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Generated by Testclutch

@vszakats vszakats mentioned this pull request Aug 21, 2024
Closed
2 tasks
vszakats
vszakats reviewed Aug 21, 2024
View reviewed changes
vszakats
vszakats reviewed Aug 21, 2024
View reviewed changes
vszakats added a commit that referenced this pull request Aug 22, 2024
@vszakats
build: make CURL_FORMAT_CURL_OFF_T[U] work with mingw-w64 <=7.0.0
c730c85 
Add tweak for mingw-w64 when building tests/http/client programs to
avoid a bogus `-Wformat` warning when using mingw-w64 v7.0.0 or older.
The warning is bogus because these programs use curl's `printf()`
implementation that is guaranteed to support that format spec.

Add this for both CMake and autotools. (But only CMake is CI tested with
an old toolchain.)

Apply the workaround to `docs/examples`, and fix an example to use
curl's `printf()` with `CURL_FORMAT_CURL_OFF_T`.

Reintroduce curl `printf()` calls into `tests/http/client`, via #14625.
Also restore large number masks to a printf, changed earlier in #14382.

Follow-up to 232302f #14382
Ref: #14625 (comment)

Closes #14640
@bagder bagder force-pushed the bagder/checksrc-ci branch from 4edfcd2 to 51baa06 Compare August 22, 2024 08:59
bagder added 3 commits August 22, 2024 11:04
@bagder
misc: general C style cleanups
c747131 
... in code that previously was not checksrc'ed
@bagder
projects/wolfssl_options.h: shorten long URL in comment
683e50c
@bagder
GHA: add a checksrc job
b7bdc77 
This job unconditionally runs checksrc on ALL .c and .h files present in
git.

checksrc.pl: fixed to look for ".checksrc" in the same directory from
where it loads the file to check so that it an be invoked like this

Closes #14625
@bagder bagder force-pushed the bagder/checksrc-ci branch from 51baa06 to b7bdc77 Compare August 22, 2024 09:06
@bagder bagder closed this in 99ba50d Aug 22, 2024
bagder added a commit that referenced this pull request Aug 22, 2024
@bagder
GHA: add a checksrc job
23749bf 
This job unconditionally runs checksrc on ALL .c and .h files present in
git.

checksrc.pl: fixed to look for ".checksrc" in the same directory from
where it loads the file to check so that it an be invoked like this

Closes #14625
@bagder bagder deleted the bagder/checksrc-ci branch August 22, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vszakats vszakats vszakats left review comments

Assignees

No one assigned

Labels

CI Continuous Integration libcurl API tests

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bagder @vszakats @dfandrich