Skip to content
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

build: make CMake test flags more consistent with make #4392

Merged
merged 11 commits into from
Feb 9, 2024

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jan 31, 2024

Description of changes:

  1. synchronize CMake test build flags with make build flags
    The default make flags contain things like -Wunused which will error on unused variables (good!) but since I don't develop with make I don't see these errors until they run in CI (not good!)

  2. add an option to error on warnings in tests
    The CMake build does not currently expose a way to error on compiler warnings in tests. S2N_WERROR_ALL sets the -Werror setting to public on libs2n, so anything that links against libs2n will automatically get the werror setting applied by CMake.

  3. fix existing warnings.
    There were enum versions and cast alignment warnings. I fixed those.

/home/ec2-user/workspace/s2n-tls/tests/unit/s2n_signature_algorithms_test.c:242:68: warning: implicit conversion from enumeration type 's2n_authentication_method' to different enumeration type 's2n_pkey_type' [-Wenum-conversion]
                    conn->handshake_params.client_cert_pkey_type = S2N_AUTHENTICATION_ECDSA;
                                                                 ~ ^~~~~~~~~~~~~~~~~~~~~~~~
/home/ec2-user/workspace/s2n-tls/tests/unit/s2n_signature_algorithms_test.c:259:68: warning: implicit conversion from enumeration type 's2n_authentication_method' to different enumeration type 's2n_pkey_type' [-Wenum-conversion]
                    conn->handshake_params.client_cert_pkey_type = S2N_AUTHENTICATION_RSA;
                                                                 ~ ^~~~~~~~~~~~~~~~~~~~~~

I also had to change the s2n_build_test to remove the use of strcasestr, which was causing an implicit-declaration warning on certain platforms like macOS. This is because I removed the -Wno-implicit-function-declaration setting on our unit tests, which will prevent odd kinds of errors like this.

/Users/runner/work/s2n-tls/s2n-tls/tests/unit/s2n_build_test.c:87:29: error: implicit declaration of function 'strcasestr' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            EXPECT_NOT_NULL(strcasestr(ssleay_version_text, name));
                            ^
/Users/runner/work/s2n-tls/s2n-tls/tests/unit/s2n_build_test.c:87:29: note: did you mean 'strcasecmp'?

Testing:

removed an EXPECT_OK in a test, and then confirmed that the following build failed

alias strict='
rm -rf build
cmake -B build . -D CMAKE_C_COMPILER=clang -D CMAKE_BUILD_TYPE=RelWithDebInfo -D S2N_WERROR_ALL=ON &&
cmake --build ./build -j $(nproc) &&
CTEST_PARALLEL_LEVEL=$(nproc) make -C build test ARGS="--output-on-failure"
'

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jan 31, 2024
@jmayclin jmayclin changed the title build: allow -Werror to be toggled for tests build: make CMake test flags more consistent with make Jan 31, 2024
@jmayclin jmayclin requested a review from goatgoose January 31, 2024 23:10
UNSAFE_TREAT_WARNINGS_AS_ERRORS is on by default, which is going to make
it incredibly hard to roll out new changes.
- clang format
@jmayclin jmayclin requested a review from camshaft February 2, 2024 23:10
@jmayclin jmayclin marked this pull request as ready for review February 2, 2024 23:11
@@ -24,6 +24,8 @@ otherwise a crypto target needs to be defined." ON)
option(UNSAFE_TREAT_WARNINGS_AS_ERRORS "Compiler warnings are treated as errors. Warnings may
indicate danger points where you should verify with the S2N-TLS developers that the security of
the library is not compromised. Turn this OFF to ignore warnings." ON)
option(S2N_WERROR_ALL "This option will cause all artifacts linked to libs2n to use the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be set in s2n_codebuild.sh for unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that is a much larger project unfortunately. That was my initial approach, but I encountered some of the following problems

  • 32 bit build has a rollover warning in the 2050 detection function (which will never be called, but the time_t definition has that problem regardless
  • individual librypto builds have problems with unused variable/unused function warnings, because there are some functions that never get called under certain if/def conditions.

@jmayclin jmayclin enabled auto-merge (squash) February 8, 2024 17:08
@jmayclin jmayclin merged commit ec6ca6e into aws:main Feb 9, 2024
31 checks passed
@jmayclin jmayclin deleted the werror-for-tests branch June 15, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants