Fix unit test build errors under -Werror#5686
Conversation
|
@maddeleine |
tests/unit/s2n_x509_validator_test.c
Outdated
There was a problem hiding this comment.
Add a note for the CI failure. The range of time_t is platform-specific:
tests/unit/s2n_x509_validator_test.c:130:24: error: implicit conversion from 'long long' to 'time_t' (aka 'long') changes value from 2524608000 to -1770359296 [-Werror,-Wconstant-conversion]
There was a problem hiding this comment.
Thanks — understood. Just to confirm: should I add a comment explaining the platform-specific time_t behavior, or guard/skip this test on platforms where time_t is 32-bit?
There was a problem hiding this comment.
I think we need both :) just adding a comment can't fix the CI
There was a problem hiding this comment.
Added a note and guarded the 2050 check so it only runs on platforms where time_t can represent it safely. Also added a null-check for the ASN1 time allocation.
There was a problem hiding this comment.
I changed return false to return true to fix the CI. Seems like there are two different things going on here: whether the year 2050 can be represented by time_t depends on the OS system; the result of X509_cmp_time() depends on the libcrypto.
32BitBuildAndUnit runs on a 32-bit system using linux-gnu as libcrypto. We do have another job testing Openssl102 on a 64-bit system. Now all the CI checks passed. Let's see if Madeleine has other feedback 😆
There was a problem hiding this comment.
Thanks for the explanation — that makes sense now.
I hadn’t fully separated the time_t range issue from the libcrypto behavior before, so thanks for clarifying that and adjusting the return value accordingly.
Glad to see all the CI checks are green now 😄 — will wait to see if Madeleine has any other feedback.
maddeleine
left a comment
There was a problem hiding this comment.
Thanks for tackling this, there were a bit more build errors in our tests than I expected, but that means that we really needed this turned on! Great!
|
Thanks @maddeleine and @CarolYeh910 for the guidance and reviews - really helpful! |
Goal
Enable
-Werrorfor unit test builds and fix all warnings that appear when compiling unit tests under strict warning settings.Why
Unit tests were previously built without
-Werror, allowing compiler warnings to accumulate over time.This caused build failures on stricter platforms such as macOS, FreeBSD, and OpenBSD once warnings were treated as errors.
Aligning unit test builds with the main library’s warning strictness improves build hygiene and prevents regressions.
How
CMakeLists.txtto:-Wno-missing-braces-Werrorto unit test targets whenUNSAFE_TREAT_WARNINGS_AS_ERRORS=ONS2N_CLONE_SUPPORTEDS2N_OCSP_STAPLING_SUPPORTEDssize_tCallouts
UNSAFE_TREAT_WARNINGS_AS_ERRORS, consistent with existing build behaviorTesting
Built and tested with:
Related
Resolves #5650
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.