CXX-2767 Add macro guards tests for bsoncxx and mongocxx #1043

eramongodb · 2023-10-19T21:43:42Z

Description

This PR introduces "macro guard tests" as a preemptive and precautionary measure against introducing preprocessor macro leaks from config headers in upcoming refactors, as well as to validate correctness of the current headers. Tracked by CXX-2767. Verified by this patch (note the new compile_macro_guard_tests tasks).

Macro Guard Tests

The prelude.hpp and postlude.hpp headers in both bsoncxx and mongocxx (as well as their private counterparts) are intended to ensure preprocessor macros defined in the config headers (i.e. version.hpp), henceforth referred to as "config macros", neither conflict with nor leak into user code. As demonstrated by changes in this PR, the application of these "macro guard" headers has not been perfect throughout the evolution of the CXX Driver codebase.

Therefore, this PR adds automated "macro guard tests" in the form of two CMake ~~custom targets~~ static libraries: test_bsoncxx_macro_guards and test_mongocxx_macro_guards. ~~Each target in turn has dependencies on test-macro_guards-<...> static library targets~~ Each library is composed of generated C++ source files, each corresponding to an individual header file in the CXX Driver repository (obtained via file(GLOB_RECURSE); CONFIGURE_DEPENDS is currently omitted but can be added on request):

test_bsoncxx_macro_guards
├── .../include-bsoncxx-v_noabi-document-view.cpp
├── .../include-bsoncxx-v_noabi-document-value.cpp
└── ...

Due to the volume of CMake targets that are introduced by these tests (one for every tested header), the generation of these targets are guarded by an ENABLE_MACRO_GUARD_TESTS CMake option, which is disabled by default to avoid penalizing regular builds of the CXX Driver.

Because the guard tests depend on the individual header file as well as the set of macros expected to be guarded, these tests are all generated by CMake using configure_file() using a dummy test_macro_guards.cpp.in file. These source files are generated under a macro_guards subdirectory in the current binary directory for a bit nicer-to-navigate binary directory layout.

Some headers should not be tested, such as the guarded config headers or X macro headers. These headers are explicitly excluded from the list of headers to be tested using EXCLUDE_REGEXES (see the new add_macro_guard_test() function defined in MacroGuardTest.cmake).

Missing Macro Guard Inclusion

(Reverted; see below) The following public headers currently include a prelude header without the corresponding include of the postlude header, thus leaking config macros into user code:

bsoncxx/builder/list.hpp
mongocxx/gridfs/downloader.hpp
mongocxx/gridfs/uploader.hpp
mongocxx/options/auto_encryption.hpp
mongocxx/options/data_key.hpp
mongocxx/options/range.hpp

Adding the missing postlude header inclusion may break downstream code that depends on the leaked macros. This is arguably not intended behavior and should be considered a bugfix (users should be including the prelude header instead), but I do not think this is clearly communicated in documentation, tutorials, or examples. Therefore, if we want to acknowledge this as a potentially source-breaking API change, this will require an API major version bump.

Addressing these missing postlude header inclusions revealed some source files of our own that were failing to include the prelude header as they should have been. This PR addresses those as well.

Other Potential Source-Breaking Changes

In addition to the above, there is a small risk that some of these changes may break source compatibility for downstream users. Same as above: if we want to acknowledge any of these risks as a potentially source-breaking API change, this will require an API major version bump.

These risks are:

(Partially reverted; see below) A -Wmacro-redefined error for a user that expects #include <bsoncxx/config/prelude.hpp> to undef an already-defined BSONCXX_ENUM and depends on this behavior for correctness, e.g.:

#define BSONCXX_ENUM "old-value"

#include <bsoncxx/config/prelude.hpp>

// Expectation: no -Wmacro-redefined error.
#define BSONCXX_ENUM "new-value"

This change is motivated by the fact that BSONCXX_ENUM is not a preprocessor macro related to the config headers. It is an X-macro intended to be used with the bsoncxx/enums/*.hpp headers. The "ensure BSONCXX_ENUM was undefined" check is inherited by bsoncxx/types.hpp instead.

Addition of ${CMAKE_INSTALL_INCLUDEDIR} breaks header search behavior for a user due to the directory being populated by existing (unrelated) header files that may conflict with the user's expected include directories search order, e.g.:

...

# Current: this flag already exists.
-I<cxx-driver-install-prefix>/bsoncxx/v_noabi

# New: this flag may preempt later flags.
# e.g. <cxx-driver-install-prefix> may contain `<external/header.hpp>`.
-I<cxx-driver-install-prefix>

# User may expect <external/header.hpp> to be found here instead.
-I<external-lib-install-prefix>

...

This change is motivated by the upcoming addition of stable ABI header files under the ${CMAKE_INSTALL_INCLUDEDIR}/bsoncxx/v1 alongside existing unstable ABI headers under ${CMAKE_INSTALL_INCLUDEDIR}/bsoncxx/v_noabi. -I<includedir>/bsoncxx/v_noabi must remain for backward source compatibility (e.g. #include <bsoncxx/document/view.hpp>) and cannot be removed at this time.

Therefore, the intent is to allow inclusion of stable ABI headers via #include <bsoncxx/v1/bsoncxx/document/view.hpp>. This would allow symmetry with inclusion of unstable ABI headers via #include <bsoncxx/v_noabi/bsoncxx/document/view.hpp> while still allowing existing code to use #include <bsoncxx/document/view.hpp>. (See rationale in #1026 for more info.)

A user declared the bsoncxx::stdx namespace themselves before including <mongocxx/stdx.hpp> in a manner that conflicts with the new namespace declaration added for standalone header inclusion compatibility. However, users should not be forward-declaring entities that they do not own (see CXX-2288), so this change should fall under our rights as library maintainers.

This change is motivated by the macro guard tests including each tested header on its own. This naturally also tests the header's compatibility with standalone inclusion. This is a property we should be supporting in all our public header files (excluding the guarded config headers, the prelude/postlude headers themselves, and the X macro headers). A using-declaration cannot refer to a namespace that has not yet been declared; therefore, the bsoncxx::stdx namespace must be forward-declared by mongocxx/stdx.hpp to support standalone inclusion.

Miscellaneous

To avoid the mass recompilation of the catch/main.cpp component for every test executable and macro guard test target, the object was split into a <project>_catch_main object library that is included in a new <project>_test_properties_with_main target. The macro guard tests are only compiled as static libraries and do not require a main() function, so they are linked with <project>_test_properties instead.

vector-of-bool · 2023-10-20T19:26:19Z

cmake/MacroGuardTest.cmake

+    string(APPEND MACRO_GUARD_TEST_PRELUDE
+        "static constexpr bool compare_equal(const char* lhs, const char* rhs) {\n"
+        "    return (*lhs == *rhs) && (*lhs == '\\0' || compare_equal(lhs + 1, rhs + 1));\n"
+        "}\n"
+        "\n"
+        "static_assert(compare_equal(\"abc\", \"abc\"), \"compare_equal() sanity check failed\");\n"
+        "static_assert(!compare_equal(\"abc\", \"def\"), \"compare_equal() sanity check failed\");\n"
+        "\n"
+        "#define _TO_STR(x) #x\n"
+        "#define TO_STR(x) _TO_STR(x)\n"
+        "\n"
+    )


Use a [[bracket string]] to avoid the quoting and escaping.

I am preferential towards being consistent with the other quoted arguments that require variable expansions (incompatible with bracket arguments) rather than using a bracket argument that requires unusual indentation to avoid it being reflected in the generated source file:

# Check and set initial state. foreach(macro ${PARSED_GUARDED_MACROS}) string(APPEND MACRO_GUARD_TEST_PRELUDE "#if defined(${macro})\n" "#error \"${macro} is already defined\"\n" "#endif\n" "#define ${macro} macro guard test\n" "\n" ) endforeach() # Implement as recursive algorithm for C++11 compatibility. string(APPEND MACRO_GUARD_TEST_PRELUDE [[ static constexpr bool compare_equal(const char* lhs, const char* rhs) { return (*lhs == *rhs) && (*lhs == '\0' || compare_equal(lhs + 1, rhs + 1)); } static_assert(compare_equal("abc", "abc"), "compare_equal() sanity check failed"); static_assert(!compare_equal("abc", "def"), "compare_equal() sanity check failed"); #define _TO_STR(x) #x #define TO_STR(x) _TO_STR(x) ]])

Would you rather we still use a bracket argument here?

I'll leave it up to you. In such cases I don't usually mind odd indentation in generated sources, and I use string(CONFIGURE) to do variable substitutions within large bracket strings.

vector-of-bool · 2023-10-20T19:28:27Z

cmake/MacroGuardTest.cmake

+        # e.g. bsoncxx/v_noabi/bsoncxx/document/view.hpp -> bsoncxx-v_noabi-bsoncxx-document-view
+        string(REPLACE "/" "-" test_name "${header}")
+        string(REGEX REPLACE "^(.*)\\.(hh|hpp)$" "\\1" test_name "${test_name}")
+        configure_file(test_macro_guards.cpp.in macro_guards/${subdir}/${test_name}.cpp)


Since the file only expands to a single string, can this be a file(WRITE) instead?

I considered file(WRITE), but it doesn't seem to support "smart" target dependency handling. It's an unconditional write regardless whether the file is already generated and/or no changes have been made to its contents, so I went with configure_file() as recommended in the docs:

If the file is a build input, use the configure_file() command to update the file only when its content changes.

I was unaware of this feature of configure_file. Perhaps file(GENERATE ... CONTENT ...) instead?

file(CONFIGURE ... CONTENT ...) does the same thing, but isn't available until CMake 3.18.

vector-of-bool · 2023-10-20T19:31:57Z

cmake/MacroGuardTest.cmake

+        configure_file(test_macro_guards.cpp.in macro_guards/${subdir}/${test_name}.cpp)
+
+        # Prefix library name to reduce potential for target conflicts.
+        add_library(test-macro_guards-${subdir}-${test_name} STATIC EXCLUDE_FROM_ALL ${CMAKE_CURRENT_BINARY_DIR}/macro_guards/${subdir}/${test_name}.cpp)


Does this need an individual target for every header? Could it be a single target with multiple source files attached?

Now that you mention it, that makes a lot of sense! 😅 Will do.

vector-of-bool · 2023-10-20T19:36:12Z

src/bsoncxx/test/CMakeLists.txt

+        "include/**/*.hpp"
+
+        # Private headers.
+        "lib/**/*.hh"
+
+        # Test headers.
+        "test/**/*.hh"


AFAIK, CMake does not recognize the ** directory pattern as anything other than two adjacent wildcards (I wish it did, though). include/*.hpp etc will be sufficient with GLOB_RECURSE.

Good catch. Will do.

vector-of-bool · 2023-10-20T19:41:21Z

src/mongocxx/test/CMakeLists.txt

+target_link_libraries(test_driver PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_logging PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_instance PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_client_side_encryption_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_crud_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_gridfs_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_command_monitoring_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_transactions_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_retryable_reads_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_read_write_concern_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_mongohouse_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_unified_format_spec PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_versioned_api PRIVATE mongocxx_test_properties_with_main)


Drive-by cleanup:

Suggested change

target_link_libraries(test_driver PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_logging PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_instance PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_client_side_encryption_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_crud_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_gridfs_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_command_monitoring_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_transactions_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_retryable_reads_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_read_write_concern_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_mongohouse_specs PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_unified_format_spec PRIVATE mongocxx_test_properties_with_main)

target_link_libraries(test_versioned_api PRIVATE mongocxx_test_properties_with_main)

set_property(

TARGET

test_driver

test_logging

test_instance

test_client_side_encryption_specs

test_crud_specs

test_gridfs_specs

test_command_monitoring_specs

test_transactions_specs

test_retryable_reads_specs

test_read_write_concern_specs

test_mongohouse_specs

test_unified_format_spec

test_versioned_api

APPEND PROPERTY LINK_LIBRARIES

mongocxx_test_properties_with_main

)

Note: this suggestion emphasized to me the importance of indented arguments, so I have set the allowOptionalArgumentIndentation format option for CMake Language Support to true going forward (which I admittedly should have already set in #1037).

However, the way it handles inline comments within argument lists is a bit quirky. Perhaps switching to https://github.com/cheshirekow/cmake_format may be preferable down the line. 🤔

vector-of-bool · 2023-10-20T19:56:04Z

cmake/MacroGuardTest.cmake

+# PROJECT_TEST_PROPERTIES: the (bsoncxx|mongocxx)_test_properties target to be linked with.
+function(add_macro_guard_test)
+    set(opt_args "")
+    set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")


Suggested change

set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")

set(single_args "PROJECT_NAME;TEST_LINK_LIBRARIES")

or

Suggested change

set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")

set(single_args "PROJECT_NAME;TEST_USE_LIBRARIES")

The name *_PROPERTIES makes me expect that it wants a property-pair list a-la set_target_properties()

a la set_target_properties()

That's more or less the purpose that the (bsoncxx|mongocxx)_test_properties interface library is fulfilling, just in a convenient form. This variable was named to be symmetrical with that intended target. Renamed to PROJECT_TEST_PROPERTIES_TARGET to more explicitly express that it's a target, not a list of properties.

vector-of-bool · 2023-10-20T20:03:59Z

cmake/MacroGuardTest.cmake

+# PROJECT_NAME: bsoncxx or mongocxx.
+# GUARDED_MACROS: list of macros that should be guarded by prelude/postlude headers.
+# INCLUDE_FILTERS: list of regex filters for headers to be added to the tests (must be relative to PROJECT_SOURCE_DIR).
+# EXCLUDE_FILTERS: list of regex filters for headers to be excluded (applied after INCLUDE_FILTERS).
+# PROJECT_TEST_PROPERTIES: the (bsoncxx|mongocxx)_test_properties target to be linked with.


Use a bracket-comment for easier reading+writing:

Suggested change

# PROJECT_NAME: bsoncxx or mongocxx.

# GUARDED_MACROS: list of macros that should be guarded by prelude/postlude headers.

# INCLUDE_FILTERS: list of regex filters for headers to be added to the tests (must be relative to PROJECT_SOURCE_DIR).

# EXCLUDE_FILTERS: list of regex filters for headers to be excluded (applied after INCLUDE_FILTERS).

# PROJECT_TEST_PROPERTIES: the (bsoncxx|mongocxx)_test_properties target to be linked with.

#[==[

Define a build-time test that asserts that the given macros are maintained across

the inclusion of the header files that match the given pattern

add_macro_guard_test(

PROJECT_NAME <name>

INCLUDE_FILTERS [pattern...]

[EXCLUDE_FILTERS [pattern...]]

GUARDED_MACROS [macro...]

PROJECT_TEST_PROPERTIES <target>

)

PROJECT_NAME

bsoncxx or mongocxx.

GUARDED_MACROS

list of macros that should be guarded by prelude/postlude headers.

INCLUDE_FILTERS

list of regex filters for headers to be added to the tests (must be relative to PROJECT_SOURCE_DIR).

EXCLUDE_FILTERS

list of regex filters for headers to be excluded (applied after INCLUDE_FILTERS).

PROJECT_TEST_PROPERTIES

the (bsoncxx|mongocxx)_test_properties target to be linked with.

]==]

vector-of-bool · 2023-10-20T20:09:31Z

cmake/MacroGuardTest.cmake

+        if("${PARSED_${required_arg}}" STREQUAL "")
+            message(FATAL_ERROR "missing required argument: ${required_arg}")
+        endif()


Suggested change

if("${PARSED_${required_arg}}" STREQUAL "")

message(FATAL_ERROR "missing required argument: ${required_arg}")

endif()

if(NOT DEFINED "PARSED_${required_arg}")

message(FATAL_ERROR "missing required argument: ${required_arg}")

endif()

This permits empty strings as values, but that may be part of the check. Alternatively, tweak the error message:

Suggested change

if("${PARSED_${required_arg}}" STREQUAL "")

message(FATAL_ERROR "missing required argument: ${required_arg}")

endif()

if("${PARSED_${required_arg}}" STREQUAL "")

message(FATAL_ERROR "Required argument ${required_arg} was not provided or was provided with an empty value")

endif()

Yes, an empty argument is considered invalid input for these parameters. Your suggestion pointed me to PARSED_KEYWORDS_MISSING_VALUES, which seems to better fulfill the intended purpose here. Tweaked the error message accordingly.

vector-of-bool · 2023-10-20T20:22:09Z

cmake/MacroGuardTest.cmake

+    foreach(filter ${PARSED_INCLUDE_FILTERS})
+        file(GLOB_RECURSE headers
+            LIST_DIRECTORIES false
+            RELATIVE ${PROJECT_SOURCE_DIR}
+            "${PROJECT_SOURCE_DIR}/${filter}"
+        )
+        list(APPEND GUARDED_HEADERS ${headers})
+    endforeach()


You can pass multiple glob patterns to file(GLOB) in a single call:

Suggested change

foreach(filter ${PARSED_INCLUDE_FILTERS})

file(GLOB_RECURSE headers

LIST_DIRECTORIES false

RELATIVE ${PROJECT_SOURCE_DIR}

"${PROJECT_SOURCE_DIR}/${filter}"

)

list(APPEND GUARDED_HEADERS ${headers})

endforeach()

# Make the include patterns absolute from the project's source dir:

list(TRANSFORM PARSED_INCLUDE_FILTERS PREPEND "${PROJECT_SOURCE_DIR}/")

# Collect included headers:

file(GLOB_RECURSE headers CONFIGURE_DEPENDS RELATIVE "${PROJECT_SOURCE_DIR}" ${PARSED_INCLUDE_FILTERS})

vector-of-bool · 2023-10-20T20:38:13Z

cmake/MacroGuardTest.cmake

+function(add_macro_guard_test)
+    set(opt_args "")
+    set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")
+    set(multi_args "GUARDED_MACROS;INCLUDE_FILTERS;EXCLUDE_FILTERS")


Suggested change

set(multi_args "GUARDED_MACROS;INCLUDE_FILTERS;EXCLUDE_FILTERS")

set(multi_args "GUARDED_MACROS;INCLUDE_PATTERNS;EXCLUDE_REGEX")

Rename suggested since they accept different syntaxes

kevinAlbs

The tests and header hygiene improvements are much appreciated.

I consider the potentially source-breaking API low-risk enough not to require a major API version bump.

src/mongocxx/test/test_macro_guards.cpp.in

src/bsoncxx/test/test_macro_guards.cpp.in

eramongodb

Latest changes verified by this patch.

eramongodb · 2023-10-25T15:16:40Z

cmake/MacroGuardTest.cmake

+    string(APPEND MACRO_GUARD_TEST_PRELUDE
+        "static constexpr bool compare_equal(const char* lhs, const char* rhs) {\n"
+        "    return (*lhs == *rhs) && (*lhs == '\\0' || compare_equal(lhs + 1, rhs + 1));\n"
+        "}\n"
+        "\n"
+        "static_assert(compare_equal(\"abc\", \"abc\"), \"compare_equal() sanity check failed\");\n"
+        "static_assert(!compare_equal(\"abc\", \"def\"), \"compare_equal() sanity check failed\");\n"
+        "\n"
+        "#define _TO_STR(x) #x\n"
+        "#define TO_STR(x) _TO_STR(x)\n"
+        "\n"
+    )


I am preferential towards being consistent with the other quoted arguments that require variable expansions (incompatible with bracket arguments) rather than using a bracket argument that requires unusual indentation to avoid it being reflected in the generated source file:

# Check and set initial state. foreach(macro ${PARSED_GUARDED_MACROS}) string(APPEND MACRO_GUARD_TEST_PRELUDE "#if defined(${macro})\n" "#error \"${macro} is already defined\"\n" "#endif\n" "#define ${macro} macro guard test\n" "\n" ) endforeach() # Implement as recursive algorithm for C++11 compatibility. string(APPEND MACRO_GUARD_TEST_PRELUDE [[ static constexpr bool compare_equal(const char* lhs, const char* rhs) { return (*lhs == *rhs) && (*lhs == '\0' || compare_equal(lhs + 1, rhs + 1)); } static_assert(compare_equal("abc", "abc"), "compare_equal() sanity check failed"); static_assert(!compare_equal("abc", "def"), "compare_equal() sanity check failed"); #define _TO_STR(x) #x #define TO_STR(x) _TO_STR(x) ]])

Would you rather we still use a bracket argument here?

eramongodb · 2023-10-25T15:22:36Z

cmake/MacroGuardTest.cmake

+        # e.g. bsoncxx/v_noabi/bsoncxx/document/view.hpp -> bsoncxx-v_noabi-bsoncxx-document-view
+        string(REPLACE "/" "-" test_name "${header}")
+        string(REGEX REPLACE "^(.*)\\.(hh|hpp)$" "\\1" test_name "${test_name}")
+        configure_file(test_macro_guards.cpp.in macro_guards/${subdir}/${test_name}.cpp)


I considered file(WRITE), but it doesn't seem to support "smart" target dependency handling. It's an unconditional write regardless whether the file is already generated and/or no changes have been made to its contents, so I went with configure_file() as recommended in the docs:

If the file is a build input, use the configure_file() command to update the file only when its content changes.

eramongodb · 2023-10-25T15:25:40Z

cmake/MacroGuardTest.cmake

+        configure_file(test_macro_guards.cpp.in macro_guards/${subdir}/${test_name}.cpp)
+
+        # Prefix library name to reduce potential for target conflicts.
+        add_library(test-macro_guards-${subdir}-${test_name} STATIC EXCLUDE_FROM_ALL ${CMAKE_CURRENT_BINARY_DIR}/macro_guards/${subdir}/${test_name}.cpp)


Now that you mention it, that makes a lot of sense! 😅 Will do.

eramongodb · 2023-10-25T15:33:12Z

cmake/MacroGuardTest.cmake

+# PROJECT_TEST_PROPERTIES: the (bsoncxx|mongocxx)_test_properties target to be linked with.
+function(add_macro_guard_test)
+    set(opt_args "")
+    set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")


a la set_target_properties()

That's more or less the purpose that the (bsoncxx|mongocxx)_test_properties interface library is fulfilling, just in a convenient form. This variable was named to be symmetrical with that intended target. Renamed to PROJECT_TEST_PROPERTIES_TARGET to more explicitly express that it's a target, not a list of properties.

eramongodb · 2023-10-25T15:45:12Z

cmake/MacroGuardTest.cmake

+        if("${PARSED_${required_arg}}" STREQUAL "")
+            message(FATAL_ERROR "missing required argument: ${required_arg}")
+        endif()


Yes, an empty argument is considered invalid input for these parameters. Your suggestion pointed me to PARSED_KEYWORDS_MISSING_VALUES, which seems to better fulfill the intended purpose here. Tweaked the error message accordingly.

eramongodb · 2023-10-25T16:06:40Z

src/mongocxx/test/CMakeLists.txt

+target_link_libraries(test_driver PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_logging PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_instance PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_client_side_encryption_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_crud_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_gridfs_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_command_monitoring_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_transactions_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_retryable_reads_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_read_write_concern_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_mongohouse_specs PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_unified_format_spec PRIVATE mongocxx_test_properties_with_main)
+target_link_libraries(test_versioned_api PRIVATE mongocxx_test_properties_with_main)


Note: this suggestion emphasized to me the importance of indented arguments, so I have set the allowOptionalArgumentIndentation format option for CMake Language Support to true going forward (which I admittedly should have already set in #1037).

However, the way it handles inline comments within argument lists is a bit quirky. Perhaps switching to https://github.com/cheshirekow/cmake_format may be preferable down the line. 🤔

eramongodb · 2023-10-25T16:20:23Z

src/bsoncxx/test/CMakeLists.txt

+        "include/**/*.hpp"
+
+        # Private headers.
+        "lib/**/*.hh"
+
+        # Test headers.
+        "test/**/*.hh"


Good catch. Will do.

eramongodb · 2023-10-25T16:32:47Z

src/bsoncxx/test/CMakeLists.txt

+# Avoid redundant recompilation of catch/main.cpp.
+add_library(bsoncxx_catch_main OBJECT ${THIRD_PARTY_SOURCE_DIR}/catch/main.cpp)
+target_link_libraries(bsoncxx_catch_main PRIVATE bsoncxx_test_properties)
+add_library(bsoncxx_test_properties_with_main INTERFACE)
+target_sources(bsoncxx_test_properties_with_main INTERFACE $<TARGET_OBJECTS:bsoncxx_catch_main>)
+target_link_libraries(bsoncxx_test_properties_with_main INTERFACE bsoncxx_test_properties)
+
+target_link_libraries(test_bson PRIVATE bsoncxx_test_properties_with_main)


CMake mandates that a name with :: be a target without falling back

That is very good to remember! Will do.

kevinAlbs · 2023-10-25T17:25:03Z

cmake/MacroGuardTest.cmake

+	    List of macros that should be guarded by prelude/postlude headers.
+
+	INCLUDE_PATTERNS
+	    List of regex filters for headers to be added to the tests. Must be


Suggested change

List of regex filters for headers to be added to the tests. Must be

List of glob expressions for headers to be added to the tests. Must be

…ders

eramongodb · 2023-10-26T14:49:32Z

After further consideration and discussion, reverted the potential API breaking changes in favor of backward compatible workarounds and documenting the issues in CXX-2769 and CXX-2770. The include directories concern will be considered a potentially breaking change to the build system, not the public API.

Notably, a test macro is used to patch the missing postlude header includes for macro guard tests, as otherwise the config macro leaks pollute any/all headers that include the problematic headers, rendering the macro guard tests mostly useless. This test macro is not documented and is not intended to be used by downstream users. Verified by this patch.

eramongodb added 9 commits October 19, 2023 15:39

Move BSONCXX_ENUM macro guard into types.hpp

ba2b9b4

Use object library for catch/main.cpp

5e86e60

Add include/ and lib/ to target include directories

9fa9fb9

Add macro guard tests

fcf4d0f

Fix issues preventing standalone header inclusion

f27c87f

Fix missing postlude header include directives

5af1cd0

Fix missing prelude header include directives

ea89972

Add macro guards tests to EVG config

307a0c3

Refactor macro guard test commands into MacroGuardTest

e08ca0f

eramongodb requested review from kevinAlbs, kkloberdanz and vector-of-bool October 19, 2023 21:43

eramongodb self-assigned this Oct 19, 2023

eramongodb changed the title ~~Add macro guards tests for bsoncxx and mongocxx~~ CXX-2767 Add macro guards tests for bsoncxx and mongocxx Oct 20, 2023

eramongodb mentioned this pull request Oct 20, 2023

CXX-2762 Ensure ABI namespace is included in API documentation #1039

Merged

vector-of-bool reviewed Oct 20, 2023

View reviewed changes

kevinAlbs reviewed Oct 25, 2023

View reviewed changes

src/mongocxx/test/test_macro_guards.cpp.in Outdated Show resolved Hide resolved

src/bsoncxx/test/test_macro_guards.cpp.in Outdated Show resolved Hide resolved

eramongodb added 13 commits October 25, 2023 11:33

Update substituted variable name to match MacroGuardTest refactor

0227c6a

Rename PROJECT_TEST_PROPERTIES -> PROJECT_TEST_PROPERTIES_TARGET

617c5dc

Use bracket comment for documentation

f024bef

Use PARSED_KEYWORDS_MISSING_VALUES instead of string comparison

9a3c573

Take advantage of multi-glob support

7a02a2a

Rename include/exclude parameters to distinguish expected use

2bee306

Fix exclude regexes with incorrect path prefix

21e506f

Reduce verbosity of linking with test properties target

508fa2a

Reformat argument lists with indentation

20c5f0e

Reduce macro guard targets to a single static library

28addeb

Add missing include for to_json() in to_string.hh

097aeea

Fix glob expression for included headers

a1bcba8

Simplify and improve robustness of test library targets

d87a87d

eramongodb commented Oct 25, 2023

View reviewed changes

eramongodb requested review from vector-of-bool and kevinAlbs October 25, 2023 16:51

kevinAlbs approved these changes Oct 25, 2023

View reviewed changes

vector-of-bool approved these changes Oct 25, 2023

View reviewed changes

kkloberdanz approved these changes Oct 25, 2023

View reviewed changes

eramongodb added 3 commits October 26, 2023 09:08

Merge remote-tracking branch 'upstream/master' into HEAD

f271cb1

CXX-2769 Document out-of-place BSONCXX_ENUM guards in macro guard hea…

506ee47

…ders

CXX-2770 Workaround missing postlude headers via a test macro

c1b2fc6

eramongodb requested a review from kevinAlbs October 26, 2023 14:49

kevinAlbs approved these changes Oct 26, 2023

View reviewed changes

eramongodb merged commit 8a568e0 into mongodb:master Oct 26, 2023

eramongodb deleted the cxx-macro-guards branch October 26, 2023 20:20

eramongodb mentioned this pull request Dec 8, 2023

[CXX-2625] Bring our very own string_view (plus: Some iterators + ranges backports) #1062

Merged

This was referenced Nov 1, 2024

CXX-2770 Add missing postlude header include directives #1249

Merged

CXX-2769 Remove out-of-place BSONCXX_ENUM macro guards #1252

Merged

	set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")
	set(single_args "PROJECT_NAME;TEST_LINK_LIBRARIES")

	set(single_args "PROJECT_NAME;PROJECT_TEST_PROPERTIES")
	set(single_args "PROJECT_NAME;TEST_USE_LIBRARIES")

	set(multi_args "GUARDED_MACROS;INCLUDE_FILTERS;EXCLUDE_FILTERS")
	set(multi_args "GUARDED_MACROS;INCLUDE_PATTERNS;EXCLUDE_REGEX")

	List of regex filters for headers to be added to the tests. Must be
	List of glob expressions for headers to be added to the tests. Must be

CXX-2767 Add macro guards tests for bsoncxx and mongocxx #1043

CXX-2767 Add macro guards tests for bsoncxx and mongocxx #1043

Uh oh!

Conversation

eramongodb commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Macro Guard Tests

Missing Macro Guard Inclusion

Other Potential Source-Breaking Changes

Miscellaneous

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vector-of-bool Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

eramongodb commented Oct 19, 2023 •

edited

Loading

vector-of-bool Oct 25, 2023 •

edited

Loading

eramongodb commented Oct 26, 2023 •

edited

Loading