Enable strict compiler warnings for C and Cython levels#9718
Conversation
|
Hi @ThomasWaldmann! I've opened this PR to resolve #9140 by adding stricter build-level warnings. I ended up implementing changes across both the C flag array and the Cython compiler options. Please let me know if you'd like me to add or modify any specific flags, or if there are any further adjustments needed to get this ready for a merge. Looking forward to your feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9718 +/- ##
==========================================
+ Coverage 83.55% 84.71% +1.15%
==========================================
Files 93 92 -1
Lines 15681 14959 -722
Branches 2353 2231 -122
==========================================
- Hits 13102 12672 -430
+ Misses 1838 1590 -248
+ Partials 741 697 -44 ☔ View full report in Codecov by Harness. |
|
@Tawfeeqshaik Thanks for the PR. Wondering about platform compatibility of such options: are they the same on every platform, linux, bsd, macOS, omniOS, haiku - but not on windows? |
@ThomasWaldmann, thanks for taking a look at this! Yep, they're fully compatible. Since Linux, macOS, the BSDs, omniOS, and Haiku all rely on GCC or Clang as their default compilers, they'll handle As for the Cython directives ( |
|
https://github.com/borgbackup/borg/actions/runs/26938203010/job/79473456121?pr=9718 Quite a lot of warnings now. Guess we need to evaluate what's actually useful for borg and what is just spam (because it is either harmless or up to another project to fix). |
Thanks for checking it further! Yep, that makes total sense , enabling strict warnings can definitely surface a lot of existing noise across dependencies and older code paths. It’s cool to see the compiler digging deep, but we definitely want to avoid cluttering the build logs with unactionable noise. Just let me know which specific flags you'd like me to drop or adjust, and I'll update the PR right away! |
|
There are tons of "implicit declaration of '...'" warnings. E.g.: warning: src/borg/chunkers/buzhash.pyx:6:7: implicit declaration of 'time' Line 6 is just: So, what does it even want from us? :-) |
Ah, got it! That's Cython's code generation triggering a C-level warning because it's missing the explicit header includes (like I can fix those cleanly by adding |
|
Hi @ThomasWaldmann, just wanted to let you know that the latest changes have successfully passed all of the CI linting and platform test suites! The build logs look crisp and focused now. Whenever you have a moment, it should be fully ready for your final review and merge. Thanks again for the helpful guidance through this! |
|
Well, the "implicit declaration" stuff still looks a bit spammy, did you see that? |
|
Did you just |
Ah, you're right I accidentally committed my local venv files while fixing the formatting issue. I've cleaned up the PR and pushed only the relevant changes now. |
| @@ -48,6 +48,9 @@ | |||
| # Extra cflags for all extensions, usually just warnings we want to enable explicitly | |||
| cflags = ["-Wall", "-Wextra", "-Wpointer-arith", "-Wno-unreachable-code-fallthrough"] | |||
There was a problem hiding this comment.
From the logs:
cc1: note: unrecognized command-line option ‘-Wno-unreachable-code-fallthrough’ may have been intended to silence earlier diagnostics
That seems to be a pre-existing issue, but maybe you could please check that also? Was that option removed? Is the option name different?
There was a problem hiding this comment.
Ya . -Wno-unreachable-code-fallthrough isn't actually a valid flag combination for GCC or Clang, which is why the compiler was scratching its head and throwing that diagnostic note.
I just went ahead and swapped it out for the correct flag, -Wno-implicit-fallthrough. That should completely clean up that pre-existing noise from the logs on this run!
|
@ThomasWaldmann is it possible to approve the workflow? |
|
Hi @ThomasWaldmann, you're exactly right. The I have updated |
|
The omniOS issue is already fixed in master. |
Got it, thanks for the update! Let me know if you'd like me to rebase/merge master to clear the CI status, or if it's good to go from your end. |
ThomasWaldmann
left a comment
There was a problem hiding this comment.
After changing this, maybe collapse all commits into one and give it a good commit comment.
|
|
||
| # Extra cflags for all extensions, usually just warnings we want to enable explicitly | ||
| cflags = ["-Wall", "-Wextra", "-Wpointer-arith", "-Wno-unreachable-code-fallthrough"] | ||
| cflags = ["-Wall", "-Wextra", "-Wpointer-arith", "-Wno-implicit-fallthrough"] |
There was a problem hiding this comment.
I just removed that "no-unreachable-code-fallthrough" from the original code and didn't see any such warnings in the output. So there is no need for this option, neither for the new one, the related warning also did not appear in the log output.
There was a problem hiding this comment.
@ThomasWaldmann I have completely removed the unnecessary flag from cflags as requested.
I have also reset the branch history and squashed all changes into a single clean commit with a clear description. The PR is ready for your final review.
2e00621 to
7fe2d56
Compare
|
Thanks! |
Description
Fixes #9140
This PR addresses the issue by implementing stricter compiler warnings and checks across both the C and Cython build layers:
-Wpedanticand-Wstrict-prototypesto the centralcflagsarray (safely wrapping it to skip Windows/MSVC compiler configurations).warn.undeclaredandwarn.unreachableto thecompiler_directivesdictionary inside thecythonizeblock to catch un-declared variables and dead code early.Checklist
master(or maintenance branch if only applicable there)toxor the relevant test subset)