Skip to content

CXX-2577 compile and test with c++20 #968

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

Merged
merged 15 commits into from
May 30, 2023
Merged

Conversation

rcsanchez97
Copy link
Contributor

I split my changes into separate commits so that it was clear where I needed a supporting change (newer CMake), where I was dealing with somewhat unrelated bugs (the output paths in case of failure and the incorrect CMake variables), the new tasks, and the syntax changes needed to make sure that the new tasks are successful.

I also want add an entirely new RHEL9 variant (that being the RHEL version with a new enough toolchain that supports c++20), but I need to investigate if the server is available for RHEL9 and I need to update the download script in DET. It seemed better to get these changes going for review as they currently are and then to either push additional commits on this PR or follow-up with a separate PR depending on what the RHEL9 variant ends up requiring.

Full Evergreen patch build: https://spruce.mongodb.com/version/6466d6f03e8e86357cad069e/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

@rcsanchez97
Copy link
Contributor Author

I was able to figure out a simple way to make the RHEL9 build work. I have pushed an additional commit with the new build variants. Here is a patch build with the new variants: https://spruce.mongodb.com/version/6469400b7742aea9a520062d/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

@rcsanchez97
Copy link
Contributor Author

Update patch build, to reflect the last few commits I made: https://spruce.mongodb.com/version/646bb408e3c331f46b5e294e/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

Use of CMake versions newer than 3.15.0 with Boost polyfill enabled exposes a missing find_dependency call in the bsoncxx package config file, which is causing example project compilation failures on MacOS due to the imported Boost target being missing (library is known to link with target Boost::boost, but the target is not defined).

Suggest using a pattern similar to what is used for the bsoncxx_pkg_dep variable -> BSONCXX_PKG_DEP parameter -> expansion in config file to inject a find_dependency(Boost 1.56.0 REQUIRED) when BSONCXX_POLY_USE_BOOST is set.

See this patch and corresponding changes for reference:

@eramongodb
Copy link
Contributor

eramongodb commented May 24, 2023

Alternatively, if the Boost dependency does not require link dependencies (which may be the case given only target_include_directories is used given CMake versions prior to 3.15.0), the changes proposed above to add find_dependency can be ignored, and instead the Boost library dependency can be hidden from package consumers via:

target_link_libraries(${TARGET} PRIVATE $<BUILD_INTERFACE:Boost::boost>)

However, I do not think this solution is recommended (even if it is the status quo) given the high potential for ODR violations due to the use of polyfills in the ABI (e.g. bsoncxx/mongocxx built with optional -> boost::optional, user builds with optional -> core::optional, user invokes bsoncxx::validate() which returns an optional -> <??? ODR violation>).

Another workaround (to minimize changes to the config) may be to ensure find_package(Boost 1.56.0 REQUIRED) is invoked before find_package(bsoncxx REQUIRED) in all example projects' CMake config files so that the Boost::boost target is known. find_package for Boost is currently only done on Windows (by requirement). This may be a bit tricky given it should only be done/required if bsoncxx was built with BSONCXX_POLY_USE_BOOST, which is info that is currently not being forwarded to example projects by build_example_projects.sh.

Among the options listed, I'm in favor of fixing the missing find_dependency in the package config file.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

LGTM, other than the same comment from Ezra

@rcsanchez97
Copy link
Contributor Author

Updated Evergreen patch build: https://spruce.mongodb.com/version/646f8ff52a60ed20864822fa/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

This fixes the failure resulting from not being able to find Boost on macOS.

@rcsanchez97 rcsanchez97 requested a review from eramongodb May 25, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants