-
Notifications
You must be signed in to change notification settings - Fork 543
CXX-2797 Remove support for external polyfill libraries #1257
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.
Exciting!
Remove polyfill related Evergreen config. Changes may have been overwritten by #1244. (e.g. packaging-debian-mnmlstc
is still present)
Good catch, thank you. Reapplied the config changes in terms of the EVG config generator. |
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.
Huzzah!
Ack. I have it on my todo list to look at and fix both the RPM and DEB packaging tasks. |
Summary
Resolves CXX-2797. Verified by this patch.
This PR is the long-awaited followup to #1062, #1075, and #1086 which fully removes support for external polyfill libraries, namely mnmlstc/core and Boost).
ENABLE_BSONCXX_POLY_USE_IMPLS=ON
is now implicit behavior, which selects bsoncxx impls for pre-C++17 and the standard library otherwise.Note
The
stdx::make_unique*
polyfills are not configuration-dependent as they use their own feature detection routines (following #1028 and #1086) and do not affect either the API (just a simple function template interface compatible with the stdlib) or ABI (no exported symbols), so they are not included or mentioned in these changes.CMake Configuration
All references to mnmlstc/core and Boost are removed from the CMake configuration, including related variables, options, and files. These include:
ENABLE_BSONCXX_POLY_USE_IMPLS
(now implicit behavior).BSONCXX_POLY_USE_IMPLS=ON
can be used to explicitly opt-out of using stdlib with C++17 or newer (hopefully no one needs to do this?).BSONCXX_POLY_USE_STD=ON
can be used to explicitly opt-out of using bsoncxx impls with pre-C++17 (probably pointless?).BSONCXX_POLY_USE_MNMLSTC
BSONCXX_POLY_USE_SYSTEM_MNMLSTC
BSONCXX_POLY_MNMLSTC_DEPRECATED_INCLUDE_DIRS
(used for legacy CMake package config files; obsoleted by CXX-2753 Refactor CMake config files to reflect new directory structure #1037)BSONCXX_POLY_MNMLSTC_PKGCONFIG_INCLUDE_DIRS
(used for pkg-config config files)BSONCXX_POLY_USE_BOOST
BOOST_ROOT
BSONCXX_BOOST_PKG_DEP
(used for pkg-config config files)FetchMnmlstcCore.cmake
file.src/bsoncxx/third_party
directory and related files.Source Files
BSONCXX_POLY_USE_*
preprocessor macros for mnmlstc/core and Boost are removed, including relevant conditional compilation (i.e. in stdx headers), macro guards, and documentation.noexcept
-ness inbsoncxx/types/bson_value/view.cpp
is removed.THIRD-PARTY-NOTICES
as it is no longer used.Evergreen Config and Scripts
CMAKE_CXX_STANDARD
).compile.sh
no longer defaults to C++17 for VS 2017 or newer.run-clang-tidy.sh
still compiles with C++17, but the note regarding mnmlstc/core is removed.$generator
and$platform
) and avoid redundant variables (e.g.$example_*
).Packaging Files and Scripts
debian-package-build-mnmlstc
task is removed.build_snapshot_rpm.sh
anddebian_package_build.sh
scripts remove all mentions of mnmlstc/core or Boost.mongo-cxx-driver.spec
andspec.patch
files are deliberately left untouched; appropriate changes are deferred to a followup PR to properly sync these updates with thedebian/unstable
branch (CC @rcsanchez97).debian-package-build
task failure:dpkg-checkbuilddeps: error: Unmet build dependencies: libboost-dev
. Attempts to resolved this error failed, likely due to requiring corresponding changes to thedebian/unstable
branch.Catch2 and VS 2017
Test cases for
stdx::optional<T>
were failing to compile on VS 2017 due to numerous errors of the form:This appears to be specifically due to poor compatibility between Catch2's expression decomposition implementation (for purposes of expressive error message construction) and VS 2017 concerning friend function/operator overload templates, which are heavily used by
stdx::optional<T>
viaequality_operators
andtag_invoke
inoperators.hpp
.Because this appears to be specifically a Catch2 + VS 2017 compatibility issue rather than an implementation correctness issue, the optional test cases were refactored to avoid directly using comparison operators in its assertion expressions (see: new
*_VS2017
macros) to avoid expression decomposition.Although the compiler errors only mention equality operators, relational comparison operators also appear to be affected by this issue. For simplicity and consistency, rather than narrowing down the exact types of comparison that trigger these errors (e.g. by comparison type, argument type, etc.), the workaround is applied to all relational comparisons in optional test cases using pattern-matching find-and-replace.