Enable -Werror for unit tests and fix resulting compiler warnings#5668
Enable -Werror for unit tests and fix resulting compiler warnings#5668thulasiramk-2310 wants to merge 23 commits intoaws:mainfrom
Conversation
|
Looks like there's also a failure on mac builds. I think you probably just need to add an if-def S2N_CLONE_SUPPORTED around the s2n_unit_test_clone_child_process function to get those builds to pass. |
Co-authored-by: maddeleine <59030281+maddeleine@users.noreply.github.com>
|
Looks like there's more mac build errors. If you have a mac handy that you can build this code change on, that would be the fastest way to resolve this. Otherwise we'll just have to go one by one through the errors. |
|
@maddeleine I’ve pushed a fix for the macOS issue. If CI still fails, I’ll try reproducing it on macOS. |
|
Yep another error. Looks like we did the same thing with the s2n_random_test. You'll need to add the S2N_CLONE_SUPPORTED to that test as well. |
|
I’m fixing the remaining macOS/FreeBSD/OpenBSD failures incrementally. Any guidance on what to prioritize or common pitfalls to check next would be appreciated. |
This reverts commit 0e60205.
|
Adding the ifdef S2N_CLONE_SUPPORTED macro in the s2n_random_test will fix the current failure. |
|
Yes — I've wrapped the clone helper with #if S2N_CLONE_SUPPORTED in s2n_random_test.c , and it fixes the unused-function error locally. |
| @@ -581,6 +581,9 @@ if (BUILD_TESTING) | |||
| -Wshadow -Wcast-align -Wwrite-strings -Wformat-security | |||
| -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated -std=gnu99 | |||
| ) | |||
There was a problem hiding this comment.
You're going to have to add -Wno-missing-braces to this list on line 582. I don't see any other warnings that need to be suppressed in the build output, so maybe this is the last fix?
|
Ah actually there's two more fixes that we need to make:
|
|
@maddeleine ,I’ve fixed the reported errors and updated the Pull Request. |
|
@maddeleine This PR ended up with a noisy commit history due to some accidental clang-format runs. To keep things clean, I’m going to close this PR and open a fresh one with the same fixes but a clean commit history. I’ll link the new PR here shortly. |
Goal
Enable
-Werrorfor all unit test builds and fix the warnings that appear when compiling tests with this flag enabled.Why
Unit tests previously did not compile with
-Werror, which allowed several warnings to accumulate unnoticed.Applying warnings-as-errors improves build hygiene, enforces consistency with the main library build, and prevents future regressions.
How
Build System
CMakeLists.txtto apply-Werrorto all unit test targets whenUNSAFE_TREAT_WARNINGS_AS_ERRORS=ONis set.Warning Fixes
tests/unit/s2n_safety_test.c
tests/unit/s2n_tls12_handshake_test.c
memcmp().tests/unit/s2n_tls13_handshake_state_machine_test.c
memcmp()checks.Verification
Callouts
UNSAFE_TREAT_WARNINGS_AS_ERRORSmechanism to keep the change opt-in, as requested in the issue discussion.Testing
Built and tested with:
Verified:
Related
Resolves #5650
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.