Skip to content

Commit c6f8f8a

Browse files
authored
Tests require compiler extensions (#1193)
**Issue:** aws-crt-cpp fails while building test files, if user sets `CXXFLAGS=-std=c++11`. This was happening in [aws-crt-builder's CI](https://github.com/awslabs/aws-crt-builder/blob/160150b3abcbfad9a21641ace57e68e0185329ad/.github/workflows/sanity-test.yml#L229), and [this PR](awslabs/aws-crt-builder#319) started failing because of it. **Diagnosis:** Here's what I found: * Our tests rely on the [`## __VA_ARGS__`](https://gcc.gnu.org/onlinedocs/gcc/Variadic-Macros.html) compiler extension, due to its usage within our [`ASSERT(...)`](https://github.com/awslabs/aws-c-common/blob/d80b00560f0ebb441538b3ab40192a242afeaa80/include/aws/testing/aws_test_harness.h#L96) macros. * We use it so that custom failure messages are optional * Apparently, this compiler extension is widely supported, because this code has been here for 7 years, and we never realized it was problematic * CMake defaults to building with compiler extensions ON * With extensions ON, the `-std=gnu99` compiler flag will be used, instead of `-std=c99` flag * Our tests fail to compile if user changes does something that makes compiler extensions OFF by default. This can be done in a variety of ways, including... * `CXXFLAGS=-std=c++11` * `CFLAGS=-std=c99` * `CMAKE_C_EXTENSIONS_DEFAULT=OFF` This leaves us with two options: 1) Ensure compiler extensions are enabled when building tests 2) Change the ASSERT(...) macros, so they don't rely on the extension. I investigated changing the ASSERT(...) macros, having differently named ASSERTF(...) macros for custom messages, but this was a massive breaking change for our downstream libraries that use these macros. I'll leave it in this [less-macro-magic](https://github.com/awslabs/aws-c-common/tree/less-macro-magic) branch, that we can pick up if we ever need to do this in the future. I'm going to go with the less disruptive option 1 fix for now... **Description of changes:** * Ensure compiler extensions are turned ON for tests * Ensure public headers are not relying on compiler extensions
1 parent d80b005 commit c6f8f8a

File tree

3 files changed

+25
-3
lines changed

3 files changed

+25
-3
lines changed

CMakeLists.txt

+10-1
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,16 @@ aws_set_common_properties(${PROJECT_NAME} NO_WEXTRA)
189189
aws_prepare_symbol_visibility_args(${PROJECT_NAME} "AWS_COMMON")
190190
target_compile_options(${PROJECT_NAME} PUBLIC ${PLATFORM_CFLAGS})
191191

192-
aws_check_headers(${PROJECT_NAME} ${AWS_COMMON_HEADERS})
192+
# Check public headers, to ensure they're safe for any 3rdparty to include
193+
set(HEADERS_TO_CHECK ${AWS_COMMON_HEADERS})
194+
195+
# HACK: don't check rw_lock.h, which can fail in esoteric circumstances
196+
# (building without compiler extensions, and without defining _POSIX_C_SOURCE or _XOPEN_SOURCE).
197+
# Currently, this file is only used by aws-c-*** libs, which are fine.
198+
get_filename_component(NAUGHTY_HEADER "include/aws/common/rw_lock.h" ABSOLUTE)
199+
list(REMOVE_ITEM HEADERS_TO_CHECK ${NAUGHTY_HEADER})
200+
201+
aws_check_headers(${PROJECT_NAME} ${HEADERS_TO_CHECK})
193202

194203
#apple source already includes the definitions we want, and setting this posix source
195204
#version causes it to revert to an older version. So don't turn it on there, we don't need it.

cmake/AwsCheckHeaders.cmake

+3
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ function(aws_check_headers_internal target std is_cxx)
6161
CXX_STANDARD ${std}
6262
CXX_STANDARD_REQUIRED 0
6363
C_STANDARD 99
64+
# Forbid public headers from relying on compiler extensions
65+
CXX_EXTENSIONS OFF
66+
C_EXTENSIONS OFF
6467
)
6568

6669
# Ensure our headers can be included by an application with its warnings set very high.

cmake/AwsTestHarness.cmake

+12-2
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,13 @@ function(generate_test_driver driver_exe_name)
5353

5454
target_link_libraries(${driver_exe_name} PRIVATE ${PROJECT_NAME})
5555

56-
set_target_properties(${driver_exe_name} PROPERTIES LINKER_LANGUAGE C C_STANDARD 99)
56+
set_target_properties(${driver_exe_name} PROPERTIES
57+
LINKER_LANGUAGE C
58+
C_STANDARD 99
59+
# Our ASSERT(...) macros rely on (widely supported) ##__VA_ARGS__ extension
60+
C_EXTENSIONS ON
61+
)
62+
5763
target_compile_definitions(${driver_exe_name} PRIVATE AWS_UNSTABLE_TESTING_API=1)
5864
target_include_directories(${driver_exe_name} PRIVATE ${CMAKE_CURRENT_LIST_DIR})
5965

@@ -75,7 +81,11 @@ function(generate_cpp_test_driver driver_exe_name)
7581
add_executable(${driver_exe_name} ${CMAKE_CURRENT_BINARY_DIR}/test_runner.cpp ${TESTS})
7682
target_link_libraries(${driver_exe_name} PRIVATE ${PROJECT_NAME})
7783

78-
set_target_properties(${driver_exe_name} PROPERTIES LINKER_LANGUAGE CXX)
84+
set_target_properties(${driver_exe_name} PROPERTIES
85+
LINKER_LANGUAGE CXX
86+
# Our ASSERT(...) macros rely on (widely supported) ##__VA_ARGS__ extension
87+
CXX_EXTENSIONS ON
88+
)
7989
if (MSVC)
8090
if(AWS_STATIC_MSVC_RUNTIME_LIBRARY OR STATIC_CRT)
8191
target_compile_options(${driver_exe_name} PRIVATE "/MT$<$<CONFIG:Debug>:d>")

0 commit comments

Comments
 (0)