-
Notifications
You must be signed in to change notification settings - Fork 543
CXX-2803 Add ABI tag to library filenames on Windows #1081
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
I am making this comment only after reading the summary, before reviewing the actual code changes.
In the case of libcrypt, it is a simple symlink pointing to a specific implementation: libxcrypt. For MPI, it makes use of Debian's alternatives system to allow the admin/user to configure which of the installed MPI implementations are set as the default. There are similar possibilities for RPM. All of that to say that whatever default makes the most sense for the most users consuming the C++ driver from source (which I think would be
|
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.
The changes look good to me. I also compared the files (library files, .pc
files, etc) between the last official package upload Kyle prepared and a build I made from this branch and everything looked good there.
cmake/BsoncxxUtil.cmake
Outdated
elseif(BSONCXX_POLY_USE_STD) | ||
set(polyfill "s") | ||
else() | ||
set(polyfill "u") |
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.
set(polyfill "u") | |
set(polyfill "i") |
I think that either "u"
doesn't belong (I didn't see "unknown" in the list above), or "u"
belongs and another branch is needed for the "i"
value.
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.
"u" was meant to represent "unknown", similar to the build type. We can consider removing the "u" values entirely (both for the polyfill parameter and the build type parameter) and treat the unknown case as a configuration error if preferable.
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.
The point I was making was the comment lists 'mbxis' as the options, but the logic has branches for 'mbxus'. It wasn't clear to me if your branch for 'u' was meant to be the same thing as "- 'i' for bsoncxx (re)implementations." It would seem that either the 'i' entry should be remove from the comment (and replaced with a comment for 'u') or that a branch should be added to the conditional so that the 'i' case is handled. Though, I'm not sure which polyfill option would correspond to 'i', off the top of my head.
Sorry for not being more clear in my first comment.
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.
Oh, right, sorry. This is because the specifics of how bsoncxx implementations are chosen as polyfill is not yet determined; that will be addressed by CXX-2796. Currently, pending 3.10 release changes unconditionally use the bsoncxx implementation without any regards to availability of C++17 features. I (perhaps prematurely) reserved i
for when we have only i
or s
(dropping m
, b
, and x
in CXX-2797).
I decided to drop u
for simplicity and added a "branch stub" for the i
case that we can enable/update in the future.
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 expect the polyfill letter can be dropped from the ABI tag when polyfills are dropped (FYI @vector-of-bool). I have updated the description of CXX-2797 to note.
Summary
Resolves CXX-2803. Verified by this patch.
This PR introduces an "ABI tag" that will be used to distinguish libraries built with incompatible build configurations. Specifically, this is the first half of full ABI tag support; the second half will be addressed by CXX-2804.
Visual Studio, MSVC, and Runtime Libraries
CXX-2803 is primarily motivated by the complexity of ensuring library compatibility when using MSVC. Specifically:
At the moment, the CXX Driver does not address these two points very well. On top of limited multi-config generator support, the filenames for a given shared or static library are currently very non-descript: simply
bsoncxx(-static).lib
andmongocxx(-static).lib
regardless of the build configuration. This means building with one configuration overwrites the library files for another configuration, with no indication of what configuration the current library file corresponds to.This can easily cause confusion when a downstream project attempts to consume CXX Driver libraries using MSVC as their compiler. In particular, a common troublesome scenario is when the CXX Driver shared libraries are built with a Release configuration, but the downstream project is attempting to build with a Debug configuration. This can easily lead to segmentation faults at runtime due to (unfortunately) successful linking of the shared library that was built to use a different, incompatible runtime library. (Note: static libraries can detect the incompatibility at link-time thanks to runtime library headers, but the situation is still not pleasant.)
ABI Tag
This PR introduces an "ABI tag" used to distinguish build configurations from one another.
The following "core" parameters identify CXX Driver configuration properties that affect ABI compatibility (these are applicable regardless of compiler or target platform):
v_noabi
, soon to includevN
. Static libraries do not include the ABI version, thus the-static
suffix already being added to the library basename is treated as the "ABI version" (that is, none).The following "MSVC-specific" parameters further identify important compatibility requirements:
CMAKE_VS_PLATFORM_NAME
.CMAKE_VS_PLATFORM_TOOLSET
.MSVC_RUNTIME_LIBRARY
.These core and MSVC-only parameters are (hopefully) sufficient to distinguish build configurations from one another. Parameters may be added or removed as needed, but the full ABI tag spec must be established prior to a stable ABI release.
The MSVC-only
ENABLE_ABI_TAG_IN_LIBRARY_FILENAMES
configuration variable controls whether an ABI tag is embedded in library filenames. This provides an opt-out mechanism for downstream projects that may not be prepared to handle the new ABI-tagged library filenames. Extension of ABI-tagged library filenames to other compilers and platforms may be considered in the future.Library Filenames
Given the above, the library filename spec looks as follows:
The trio of core build type, mongoc link type, and polyfill library parameters (
rhs
) are expected to be the string used for inline ABI tag namespaces in CXX-2804 (e.g.inline namespace rhs { ... }
).The trio of MSVC-specific platform, toolset, and runtime library parameters (
-x64-v142-md
) are internally referred to as the "vs-suffix".Examples of expected library filenames include the following (note: this table does NOT imply that all the listed configurations are supported; it is only for illustrative purposes):
The last two rows are an example of when an MSVC compiler is used with a non-Visual Studio generator (note the platform name and toolset are missing).
These changes should hopefully address conflicts and confusion at the filename level regarding what configuration a given library is built with. Furthermore, because debug and release builds now produce different library names, CMake can assign the appropriate library file per build configuration (via
IMPORTED_LIBNAME_<CONFIG>
and other related properties) such that when a downstream project builds their project, the correct library file is selected according to their build configuration.However, this is not a foolproof solution for enforcing compatibility in downstream projects. For example, if only a release configuration is built and installed (no debug configuration provided), CMake will direct a downstream project to link against the release library even when the downstream project is using a debug configuration. This CMake behavior is deliberate due to build configuration flexibility:
If downstream projects do not set
CMAKE_MAP_IMPORTED_CONFIG_*
prior tofind_package()
of CXX Driver libraries (which is likely the vast majority of cases), configuration compatibility is not enforced at the CMake level. For static libraries, the incompatibility can be prevented at link-time thanks to runtime library mismatch checks, but for shared libraries, this may lead to opaque segfaults during execution. Enforcing compatibility for shared libraries at link-time (regardless of CMake build configurations) will be better handled by inline ABI tag namespaces in CXX-2804.Generator Expressions
To support multi-config generators (notably Visual Studio), the ABI tag must be computed at generate-time, not compile-time. Specifically, the following parameters require the use of generator expressions:
The build type can be specified by the CMake flag
--config <CONFIG>
or via Visual Studio's "build configuration" dropdown menu during the build step. By default, this also determines the MSVC runtime library used by targets (note:MSVC_RUNTIME_LIBRARY
is a target property, not a variable). This means the library filename must be computed during or after the generate step.This is accomplished by using generator expressions when setting the
OUTPUT_NAME
target property inBsoncxxUtil.cmake
andMongocxxUtil.cmake
(see definitions of thebuild_type
andruntime
variables). The rest of the ABI tag parameters that do not depend on generate-time properties are set normally according to configure-time variables.Target Properties and Compatible Interface Requirements
When defining ABI tag core parameters for bsoncxx, their values are also used to set target properties for the bsoncxx library targets. This allows for convenient reuse by mongocxx, whose configuration must be consistent with that of the bsoncxx library it is linked with. To better enforce this consistency at the CMake level, the core parameters are also defined as compatible interface properties for bsoncxx library targets. This ensures the bsoncxx and mongocxx libraries are consistent in their configuration when used by downstream projects.
For example, suppose the bsoncxx libraries are configured, built, and installed separately from the mongocxx libraries. If the polyfill library configurations do not match, the following error is expected:
Although this could also be detected and enforced by inline ABI tag namespaces in CXX-2804, this should (hopefully) provide a more informative message than a linker or runtime error for both shared and static libraries when CMake is able to detect the incompatibility. If the compatible interface requirements does not detect it, hopefully the library filenames will; if library filenames does not detect it, hopefully the inline ABI tag namespace in CXX-2804 will.
Note, although MSVC-only parameters are included in the library filenames, they are deliberately not included in compatible interface requirements due to their complexity (i.e. v142 is binary-compatible with v141).
pkg-config
The contents of the generated pkg-config metadata files also depend on the build configuration and generate-time properties. This means their generation must be deferred to generate-time or later. This is accomplished by using
add_custom_command()
to generate the configuration-specific.pc
file using agenerate-pc.cmake
script file. This command is added to the dependencies of the library target file via thegenerate-lib${TARGET}-pc
custom target.To properly ensure the metadata file is dependent on the build configuration, the name of the generated (but not necessarily installed, see below) pkg-config metadata file is defined in terms of the
OUTPUT_NAME
of the library target obtained viaget_target_property()
. The generator expressions in the output name are evaluated during the${CMAKE_COMMAND}
when passed as arguments to thegenerate-pc.cmake
script. This ensures correct configuration-specific dependencies, including during the install step (e.g. an existinglibbsoncxx.pc
in the install prefix should not be treated as up-to-date when installing a different configuration).To keep things relatively simple, the embedding of the ABI tag string in the metadata filename is disabled by default. The
install()
command renames the metadata file to be consistent with its current naming scheme (although it is now*_OUTPUT_BASENAME
-aware). This is to continue allowingpkg-config libbsoncxx --cflags --libs
rather than requiring the much more verbosepkg-config libbsoncxx-v_noabi-rhs-x64-v142-md --cflags --libs
. This behavior can be enabled viaENABLE_ABI_TAG_IN_PKGCONFIG_FILENAMES=ON
if desirable. I am conflicted about setting either ON or OFF as the default for this option; I would prefer directing users to use CMake instead.