Skip to content

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

Merged
merged 10 commits into from
Jan 19, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jan 12, 2024

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:

  • Visual Studio (the most common way MSVC is used as the compiler) is a "multi-config" generator. This means files and artifacts may depend on properties that can only be determined after configure-time, and must be handled in a manner that does not introduce conflicts across configurations.
  • Libraries and executables built with MSVC must link with a specific runtime library depending on the build configuration.

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 and mongocxx(-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):

  • ABI Version: currently only v_noabi, soon to include vN. 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).
  • Build Type: Debug or Release. (RelWithDebInfo and MinSizeRel are considered equivalent to Release for CXX Driver configuration purposes.)
  • mongoc Link Type: shared or static. This is referring to how C Driver libraries are linked, not the CXX Driver libraries.
  • C++17 Polyfill Library: mnmlstc/core, Boost, experimental stdlib, or stdlib.

The following "MSVC-specific" parameters further identify important compatibility requirements:

  • Platform Name (optional): x86, x64, etc. as indicated by CMAKE_VS_PLATFORM_NAME.
  • Platform Toolset (optional): v140, v141, v142, etc. as indicated by CMAKE_VS_PLATFORM_TOOLSET.
  • Runtime Library: MultiThreadedDLL, MultiThreadedDebugDLL, etc. as indicated by 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:

bsoncxx-v_noabi-rhs-x64-v142-md
^~~~~~~ ^~~~~~~ ^^^ ^~~ ^~~~ ^~
|       |       ||| |   |    |
|       |       ||| |   |    Runtime Library
|       |       ||| |   Toolset
|       |       ||| Platform
|       |       ||Polyfill Library
|       |       |mongoc Link Type
|       |       Build Type
|       ABI version (or static)
Basename

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):

Filename ABI Build Type mongoc Polyfill Generator
bsoncxx-v_noabi-dtm-x64-v142-mdd.lib _noabi Debug static mnmlstc MSVC
bsoncxx-v_noabi-rhs-x64-v142-md.lib _noabi Release shared stdlib MSVC
bsoncxx-static-dtb-win32-v144-mtd.lib N/A Debug static Boost MSVC
bsoncxx-static-rhb-arm64-v144-mt.lib N/A Release shared Boost MSVC
bsoncxx-v1-rhi-x64-v140-md.lib 1 Release shared bsoncxx MSVC
bsoncxx-v1-rhi-x64-v141-md.lib 1 Release shared bsoncxx MSVC
bsoncxx-v_noabi-dhs-mdd.lib _noabi Debug shared stdlib Ninja
bsoncxx-v_noabi-rhs-md.lib _noabi Release shared stdlib Ninja

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:

Targets imported from another project may not provide the same set of configuration names available in the current project. [...] The first configuration in the list found to be provided by the imported target (i.e. via IMPORTED_LOCATION_<CONFIG> for the mapped-to <CONFIG>) is selected. As a special case, an empty list element refers to the configuration-less imported target location (i.e. IMPORTED_LOCATION). If this property is set and no matching configurations are available, then the imported target is considered to be not found. This property is ignored for non-imported targets.

If downstream projects do not set CMAKE_MAP_IMPORTED_CONFIG_* prior to find_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:

  • Build Type (Debug, Release, etc.)
  • MSVC Runtime library (MultiThreadedDLL, MultiThreadedDebugDLL, etc.)

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 in BsoncxxUtil.cmake and MongocxxUtil.cmake (see definitions of the build_type and runtime 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:

# Assume successful.
find_package(bsoncxx CONFIG REQUIRED PATHS <Prefix A>)

# Assume successful, but with a different prefix.
# `find_dependency(bsoncxx)` satisfied by the above.
find_package(mongocxx CONFIG REQUIRED PATHS <Prefix B>)

add_executable(downstream)

# CMake Error: The INTERFACE_BSONCXX_ABI_TAG_POLYFILL_LIBRARY property of
# "mongo::bsoncxx_shared" does not agree with the value of
# BSONCXX_ABI_TAG_POLYFILL_LIBRARY already determined for "downstream".
target_link_libraries(downstream PRIVATE mongo::mongocxx_shared)

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 a generate-pc.cmake script file. This command is added to the dependencies of the library target file via the generate-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 via get_target_property(). The generator expressions in the output name are evaluated during the ${CMAKE_COMMAND} when passed as arguments to the generate-pc.cmake script. This ensures correct configuration-specific dependencies, including during the install step (e.g. an existing libbsoncxx.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 allowing pkg-config libbsoncxx --cflags --libs rather than requiring the much more verbose pkg-config libbsoncxx-v_noabi-rhs-x64-v142-md --cflags --libs. This behavior can be enabled via ENABLE_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.

@eramongodb eramongodb requested a review from kevinAlbs January 12, 2024 20:08
@eramongodb eramongodb self-assigned this Jan 12, 2024
@kevinAlbs kevinAlbs requested a review from rcsanchez97 January 16, 2024 16:31
@rcsanchez97
Copy link
Contributor

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 allowing pkg-config libbsoncxx --cflags --libs rather than requiring the much more verbose pkg-config libbsoncxx-v_noabi-rhs-x64-v142-md --cflags --libs. This behavior can be enabled via ENABLE_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.

I am making this comment only after reading the summary, before reviewing the actual code changes.

<div style=distro-packager-hat>
So, I can definitely see the utility in either one as the default. When it comes to the packaging side of things, I would actually want the longer name for the .pc file and then I would configure a symbolic link. For instance, in my Debian system, here are some examples:

roberto@manzanillo:~/src/mongodb$  ls -l /usr/lib/x86_64-linux-gnu/pkgconfig/libcrypt.pc
lrwxrwxrwx 1 root root 12 Jan  6  2023 /usr/lib/x86_64-linux-gnu/pkgconfig/libcrypt.pc -> libxcrypt.pc
roberto@manzanillo:~/src/mongodb$  ls -l /usr/lib/x86_64-linux-gnu/pkgconfig/mpi.pc
lrwxrwxrwx 1 root root 41 Mar  1  2021 /usr/lib/x86_64-linux-gnu/pkgconfig/mpi.pc -> /etc/alternatives/mpi.pc-x86_64-linux-gnu
roberto@manzanillo:~/src/mongodb$  ls -l /etc/alternatives/mpi.pc-x86_64-linux-gnu
lrwxrwxrwx 1 root root 43 Mar  1  2021 /etc/alternatives/mpi.pc-x86_64-linux-gnu -> /usr/lib/x86_64-linux-gnu/pkgconfig/ompi.pc

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 OFF) is what we should do. It is easy enough to adjust the options for packaging builds.

</div>

Copy link
Contributor

@rcsanchez97 rcsanchez97 left a 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.

elseif(BSONCXX_POLY_USE_STD)
set(polyfill "s")
else()
set(polyfill "u")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a 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.

@eramongodb eramongodb requested a review from kevinAlbs January 19, 2024 16:30
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.

3 participants