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.

Sign up for GitHub

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

Jump to bottom

refactor: clean up CMakelists.txt #4779

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+60 −20
Open

refactor: clean up CMakelists.txt #4779

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f4ee06
refactor: clean up CMakelists.txt
jmayclin Sep 18, 2024
d5cb871
address ci failure
jmayclin Sep 18, 2024
9fea9b2
address ci failure
jmayclin Sep 18, 2024
1002b60
try removing POSIX_SOURCE compile definition
jmayclin Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 60 additions & 20 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,30 @@ set_target_properties(${PROJECT_NAME} PROPERTIES SOVERSION ${VERSION_MAJOR})

set(CMAKE_C_FLAGS_DEBUGOPT "")

target_compile_options(${PROJECT_NAME} PRIVATE -pedantic -std=gnu99 -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts
-Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security
-Wno-missing-braces -Wsign-compare -Wno-strict-prototypes -Wa,--noexecstack
target_compile_options(${PROJECT_NAME} PRIVATE
-pedantic
-std=gnu99
-Wall
-Wcast-align
-Wchar-subscripts
-Wcomment
-Wformat-security
-Wimplicit
-Wshadow
-Wsign-compare
-Wuninitialized
-Wunused
-Wwrite-strings

# Assembler Options
-Wa,--noexecstack

# Suppressed Warnings
-Wno-deprecated-declarations
# GCC 4 fails to parse our macros with a "missing-braces" error
-Wno-missing-braces
Comment on lines +158 to +159
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have an issue for this? ARE macros missing braces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ARE macros missing braces?

My understanding is that the answer is "no" otherwise things would actually be broken.

/codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/tests/unit/s2n_x509_validator_time_verification_test.c: In function 'main':
/codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/tests/unit/s2n_x509_validator_time_verification_test.c:116:34: warning: missing braces around initializer [-Wmissing-braces]
             DEFER_CLEANUP(struct s2n_stuffer cert_chain_stuffer = { 0 }, s2n_stuffer_free);
                                  ^
/codebuild/output/src491586333/src/github.com/aws/s2n-tls/s2n_head/./utils/s2n_safety.h:62:43: note: in definition of macro 'DEFER_CLEANUP'
     __attribute__((cleanup(_thecleanup))) _thealloc
                                           ^

console link

Other people have run into similar false positives with old versions of GCC.
OP-TEE/optee_os#1998

I did find a bug report that seems related, but it says the fix was backported to GCC 4.8.3 and our CI version is 4.8.5

-Wno-strict-prototypes
-Wno-unknown-pragmas
)

if (S2N_WERROR_ALL)
Expand Down Expand Up @@ -445,11 +466,14 @@ target_include_directories(${PROJECT_NAME} PUBLIC $<BUILD_INTERFACE:${CMAKE_CURR
if (BUILD_TESTING)
enable_testing()

############################################################################
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
################### build testlib (utility library) ########################
############################################################################

file(GLOB TESTLIB_SRC "tests/testlib/*.c")
file(GLOB EXAMPLES_SRC "docs/examples/*.c")
file(GLOB TESTLIB_HEADERS "tests/testlib/*.h" "tests/s2n_test.h")

add_library(testss2n STATIC ${TESTLIB_HEADERS} ${TESTLIB_SRC} ${EXAMPLES_SRC})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Headers shouldn't be included as part of the add_library directive. Generally the add_library directive is just used for things that generate object files. Header should then be added through target_include_directories, but that is already configured to add the tests/ path.

add_library(testss2n STATIC ${TESTLIB_SRC} ${EXAMPLES_SRC})
target_include_directories(testss2n PUBLIC tests)
target_compile_options(testss2n PRIVATE -std=gnu99)
target_link_libraries(testss2n PUBLIC ${PROJECT_NAME})
Expand All @@ -460,17 +484,18 @@ if (BUILD_TESTING)
endif()

if (S2N_INTERN_LIBCRYPTO)
# if libcrypto was interned, rewrite libcrypto symbols so use of internal functions will link correctly
# if libcrypto was interned, rewrite libcrypto symbols so use of internal
# functions will link correctly
add_custom_command(
TARGET testss2n POST_BUILD
COMMAND
objcopy --redefine-syms libcrypto.symbols lib/libtestss2n.a
)
endif()

#run unit tests
file (GLOB TEST_LD_PRELOAD "tests/LD_PRELOAD/*.c")
Copy link
Contributor Author

@jmayclin jmayclin Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Removing this because it is unused. I searched the entire codebase (not just CMakeLists.txt) and found that this library is only used when unit tests are run through make. See the PR overview for my thoughts on why we should totally delete this library.

add_library(allocator_overrides SHARED ${TEST_LD_PRELOAD})
############################################################################
########################## configure unit tests ############################
############################################################################

set(UNIT_TEST_ENVS S2N_DONT_MLOCK=1)
if (TSAN OR ASAN)
Expand Down Expand Up @@ -500,12 +525,16 @@ if (BUILD_TESTING)
endif()
message(STATUS "Running tests with environment: ${UNIT_TEST_ENVS}")

############################################################################
############################ build unit tests ##############################
############################################################################

file(GLOB UNITTESTS_SRC "tests/unit/*.c")
foreach(test_case ${UNITTESTS_SRC})
string(REGEX REPLACE ".+\\/(.+)\\.c" "\\1" test_case_name ${test_case})
# NAME_WE: name without extension
get_filename_component(test_case_name ${test_case} NAME_WE)
Comment on lines +534 to +535
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat, a very cmake way to do it!


add_executable(${test_case_name} ${test_case})
target_include_directories(${test_case_name} PRIVATE ./)
target_link_libraries(${test_case_name} PRIVATE testss2n)
Comment on lines -508 to 538
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea what these target_include_directories are trying to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

target_include_directories is used to indicate the header file locations that are necessary for that specific target.

This is an odd directive to use for an individual test binary, because those test binaries don't have associated header files.

I think the generic thought was "test binary needs access to the test headers" but those test headers are already publicly associated with testss2n. So when testss2n is linked to the test binary, the test binary will also get the test headers.

s2n-tls/CMakeLists.txt

Line 453 in 2467416

target_include_directories(testss2n PUBLIC tests)

if (S2N_INTERN_LIBCRYPTO)
# if libcrypto was interned, rewrite libcrypto symbols so use of internal functions will link correctly
Expand All @@ -516,19 +545,28 @@ if (BUILD_TESTING)
)
endif()
target_compile_options(${test_case_name} PRIVATE
-Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized
-Wshadow -Wcast-align -Wwrite-strings -Wformat-security
-Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated
-fPIC -D_POSIX_C_SOURCE=200809L -std=gnu99)
-Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized
-Wshadow -Wcast-align -Wwrite-strings -Wformat-security
-Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated -std=gnu99
Comment on lines +548 to +550
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably out of scope of this refactor, but it'd be nice if we had a list of common warnings we want enabled. It's not clear whether this matches the warnings on the main library, and whether the differences are intentional or just a mistake because someone forgot to update everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I think the proper way to do this is to define

S2N_COMMON_WARNINGS

And then if S2N_WERROR_ALL is set then you would just make the compile options on libs2n public, ensuring that all of the associated items (test utility library, fuzz tests, unit tests) all just get them by default. I'll open an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an action item for this in #4792

)
if (S2N_LTO)
target_compile_options(${test_case_name} PRIVATE -flto)
endif()
add_test(NAME ${test_case_name} COMMAND $<TARGET_FILE:${test_case_name}> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/tests/unit)

add_test(
NAME ${test_case_name}
COMMAND $<TARGET_FILE:${test_case_name}>
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/tests/unit
)
set_property(TEST ${test_case_name} PROPERTY LABELS "unit")
set_property(TEST ${test_case_name} PROPERTY ENVIRONMENT ${UNIT_TEST_ENVS})

endforeach(test_case)

############################################################################
######################### build utility binaries ###########################
############################################################################

add_executable(s2nc "bin/s2nc.c" "bin/echo.c" "bin/https.c" "bin/common.c")
target_link_libraries(s2nc ${PROJECT_NAME})
target_compile_options(s2nc PRIVATE -std=gnu99)
Expand Down Expand Up @@ -603,7 +641,9 @@ if (BUILD_TESTING)
file(GLOB TESTLIB_SRC "tests/testlib/*.c")
file(GLOB TESTLIB_HEADERS "tests/testlib/*.h" "tests/s2n_test.h")

add_library(fuzztest SHARED ${TESTLIB_HEADERS} ${TESTLIB_SRC})
# This must be a shared object so that symbols can be overridden by the
# fuzz test specific LD_PRELOAD libraries.
add_library(fuzztest SHARED ${TESTLIB_SRC})
target_include_directories(fuzztest PUBLIC tests)
target_link_libraries(fuzztest PUBLIC ${PROJECT_NAME})

Expand Down Expand Up @@ -650,14 +690,15 @@ if (BUILD_TESTING)
get_filename_component(TEST_NAME ${src} NAME_WE)

add_executable(${TEST_NAME} ${src})
target_include_directories(${TEST_NAME} PRIVATE ./)

target_compile_options(${TEST_NAME} PRIVATE
-g -Wno-unknown-pragmas -Wno-unused-result
)
target_link_options(${TEST_NAME} PRIVATE
-fsanitize=fuzzer -lstdc++
)
target_link_libraries(${TEST_NAME} PRIVATE
fuzztest
-fsanitize=fuzzer -lstdc++
)

# Set the output directory for the fuzzing binaries
Expand Down Expand Up @@ -701,7 +742,6 @@ install(
RUNTIME DESTINATION bin COMPONENT Runtime
)


configure_file("cmake/${PROJECT_NAME}-config.cmake"
"${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config.cmake"
@ONLY)
Expand Down
Loading