-
Notifications
You must be signed in to change notification settings - Fork 543
CXX-3094 Upgrade Catch2 from v2 to v3 #1200
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
Conversation
Although not directly related, given the Catch2 migration improves trailing semicolon handling + relocation/modification of the newly-named |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! The faster compile and reduced warnings are much appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only minor remarks
string(REPLACE " -Werror" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}") | ||
string(REPLACE " -Werror" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should /WX
also be removed? (for MSVC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not pass /WX
in any of our build or CI scripts, so it is not necessary.
cmake/FetchMnmlstcCore.cmake
Outdated
@@ -7,6 +7,11 @@ set(core-subbuild "${CMAKE_CURRENT_BINARY_DIR}/_deps/core-subbuild") | |||
set(core-build "${CMAKE_CURRENT_BINARY_DIR}/_deps/core-build") | |||
set(core-install "${CMAKE_CURRENT_BINARY_DIR}/_deps/core-install") | |||
|
|||
set(fetch_args "") | |||
if ("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.25.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.25.0") | |
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.25.0") |
cmake/FetchMongoC.cmake
Outdated
@@ -4,11 +4,18 @@ include(FetchContent) | |||
|
|||
message(STATUS "Download and configure C driver version ${LIBMONGOC_DOWNLOAD_VERSION} ... begin") | |||
|
|||
set(fetch_args "") | |||
if ("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.25.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.25.0") | |
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.25.0") |
Summary
Resolves CXX-3094. Verified by this patch.
Upgrades the Catch2 library used by the test suite from the v2 standalone header to the v3 library obtained via FetchContent.
This bumps the minimum required C++ standard to build tests to C++14 as reflected in the new changelog entry. This does not bump the minimum required C++ standard to build the bsoncxx or mongocxx libraries.
ClangFormat
The ClangFormat config file is updated to separate bsoncxx/mongocxx headers from test and third party headers. Note the order of the Regex entries is significant (first match from top to bottom), hence the regex for config and test headers must (confusingly) come before the regex driver and third party headers even though they are prioritized to come after in formatted source code.
FetchCatch2 Module
This PR adds a new
FetchCatch2.cmake
module used to obtain the Catch2 library via FetchContent in a manner similar toFetchMongoC.cmake
. The only notable difference toFetchMongoC.cmake
is that Catch2 cache variables are set viaset_property()
instead of viaset()
(normal variable overrides) due to CMP0077, as Catch2's CMake config does not set its CMake policy to 3.13 or newer.Catch2 v2 -> v3 Migration
Due to the new minimum C++ standard requirement of C++14 or newer by Catch2 v3 (notably the use of extended
constexpr
functions), tests are no longer built or run on VS 2015 distros, as "extended constexpr" requires VS 2017 and newer. Instead, they now only run thecompile_without_tests
EVG task on VS 2015 distros to preserve build coverage + uninstall check tasks.The
cxx_std_14
CMake compile feature is used to build bsoncxx and mongocxx tests with C++14 via the(bsoncxx|mongocxx)_test_properties
interface library. Note the(bsoncxx|mongocxx)_testing
libraries themselves do not require C++14 (nor do the normal static/shared libraries); they are still compiled with C++11 whenCMAKE_CXX_STANDARD=11
, which matches the standard used for normal library compilation.Note
Only the test executables require C++14 or newer. Users whose compilers do not have C++14 support can set
ENABLE_TESTS=OFF
to avoid the C++14 requirement while still being able to build the bsoncxx/mongocxx libraries.The
<bsoncxx/test/catch.hh>
header is still used as the primary method to import Catch2 test macros, thus it is the header by which<catch2/catch_test_macros.hpp>
is included by most test files. Several test files needed to be updated to include the now separate Catch2 headers for matcher, generators, and etc. as needed. Otherwise, the migration mostly involved the namespace-scoping of matcher-related entities:Catch::Matches
->Catch::Matchers::Matches
Catch::Contains
->Catch::Matchers::ContainsSubstring
Catch::MatcherBase
->Catch::Matchers::MatcherBase
The
catch/include/helpers.hpp
header was moved tomongocxx/test/catch_helpers.hh
where it is more appropriate + formatted.The
test.sh
script required the addition of the Catch2 binary directory to$PATH
find Catch2 DLLs on Windows.Although improving compilation times isn't the goal of this PR, it is considered a major feature of Catch2 v3 (in large part due to no longer being a header-only library), so here are some crude benchmarks comparing the old vs. new compilation times to build
test_bson
from scratch (rounded to nearest second):-j 16
for parallel benchmarks)Miscellaneous
Drive-by improvements include:
SYSTEM
toFetchContent_Declare()
with CMake 3.25+, which reduces the amount of noisy warnings emitted when compiling imported library sources or when including imported library headers. CMake 3.28 additionally adds support forEXCLUDE_FROM_ALL
, but setting the directory property directly seems more convenient for backward compatibility purposes.MONGO_CXX_DRIVER_COMPILING
preprocessor macro definition which was obsoleted by CXX-2748 Disable generation of built tree targets file #1024.CHECK_OPTIONAL_ARGUMENT
macro.