-
Notifications
You must be signed in to change notification settings - Fork 543
CXX-2749 Use FetchContent and find_package() to obtain mnmlstc/core #1019
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
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Infrastructure as Code | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Vulnerabilities | ![]() ![]() ![]() ![]() |
View in Orca |
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
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.
Fix is much appreciated. Suggested changes for supporting CMake 3.11.
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.
I concur with Kevin's comments, but otherwise LGTM.
Raised the bumped-to minimum CMake version to 3.15 for consistency with the C Driver (already required for the auto-download configuration). This addresses both issues raised concerning use of |
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. Happy to see the sudo
instruction is removed from the install instructions.
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.
Sorry to delay. A very welcome upgrade! One small comment that isn't important.
Resolves CXX-2749. Verified by this patch. (Note: Debian packaging tasks are failing due to patch-specific code paths; expected to pass on waterfall once merged).
This PR proposes using FetchContent instead of ExternalProject to acquire the downloaded mnmlstc/core library when mnmlstc/core is selected as the polyfill library. This moves the download+configure+build of mnmlstc/core from the build step to the configure step for the CXX Driver. Additionally, this PR removes the patching of include directives in mnmlstc/core header files, opting to add the
third_party
include path to targets/flags instead. The use of the CMake FetchContent module requires bumping the minimum CMake version to 3.11.These changes were motivated by the issue of mnmlstc/core files being prematurely installed during the CXX Driver CMake build step due to the ExternalProject custom fix-includes step depending on the install step. This PR proposes instead installing mnmlstc/core to a local install directory (in the CXX Driver binary directory) and importing CMake targets via
find_package()
to maintain isolation of CMake project configurations. The local install directory is added to the CXX Driver's installation rules viainstall(DIRECTORY)
. (Note: mnmlstc/core'sshare
files are currently excluded, but can be added back in if necessary.)The result is that the installed CXX Driver files look identical to the target branch, but with the following exceptions:
bsoncxx/third_party/mnmlstc/
prefix (unmodified).${CMAKE_INSTALL_INCLUDEDIR}/bsoncxx/v_noabi/bsoncxx/third_party/mnmlstc
directory is now added to the set of include directories for the bsoncxx library via CMake target properties, CMake variables, and pkg-config cflags as appropriate.As a drive-by improvement,
build_example_projects.sh
now print the name of the example project on failure so it is clear which CMake project configuration+build+test attempt caused the failure.