Skip to content

CXX-2748 Disable generation of built tree targets file #1024

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.

Sign up for GitHub

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 14 commits into from
Sep 18, 2023
Merged

CXX-2748 Disable generation of built tree targets file #1024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4c79951
CXX-2748 Disable generation of build tree targets file
eramongodb Sep 14, 2023
b8f6dc1
Fix target of BSONCXX_STATIC compile definition for bsoncxx_add_libra…
eramongodb Sep 14, 2023
d340670
Fix find_dependency() specification in CMake package config files
eramongodb Sep 14, 2023
0d4834c
Fix include directive for libbson and libmongoc headers
eramongodb Sep 14, 2023
9c45434
CXX-2749 Unconditionally use <core/...> for mnmlstc/core headers
eramongodb Sep 14, 2023
a5bb151
Address ENABLE_AUTOMATIC_INIT_OR_CLEANUP warnings
eramongodb Sep 15, 2023
e456708
Fix path to build/hello_mongocxx
eramongodb Sep 15, 2023
e54ecb8
Merge remote-tracking branch 'upstream/master' into HEAD
eramongodb Sep 18, 2023
8f15fce
Remove stray MONGOCXX project variable blocks
eramongodb Sep 15, 2023
22a7aeb
Remove build tree targets file code entirely
eramongodb Sep 18, 2023
248afa7
Update CHANGELOG with removal of build tree targets file
eramongodb Sep 18, 2023
13ace6d
Tweak wording in CHANGELOG
eramongodb Sep 18, 2023
5f01ab9
Document addition of find_dependency() in CHANGELOG
eramongodb Sep 18, 2023
2c3bacc
Merge remote-tracking branch 'upstream/master' into cxx-2748
eramongodb Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .mci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,10 +1119,9 @@ tasks:
export CMAKE
CMAKE="$(find_cmake_latest)"
command -v "$CMAKE"
cd build
$CMAKE ..
$CMAKE --build . -- -j $(nproc)
./hello_mongocxx
$CMAKE -S . -B build -DENABLE_AUTOMATIC_INIT_AND_CLEANUP=OFF
$CMAKE --build build
./build/hello_mongocxx

- name: debian-package-build
commands:
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
- Explicitly document that throwing an exception from an APM callback is undefined behavior.
- Do not prematurely install mnmlstc/core headers during the CMake build step.
- Require a C Driver CMake package is found via `find_dependency()` for all installed CXX Driver package configurations.

### Removed
- Remove support for exported targets from the CMake project build tree.
10 changes: 0 additions & 10 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,6 @@ if(TARGET mongoc_shared OR TARGET mongoc_static)
# If these targets exist, then libmongoc has already been included as a project
# sub-directory
message (STATUS "Found libmongoc targets declared in current build scope; version not checked")

if(NOT MONGOCXX_LINK_WITH_STATIC_MONGOC)
set(libmongoc_target mongoc_shared)
else()
set(libmongoc_target mongoc_static)
endif()

if(MONGOCXX_BUILD_STATIC)
set(mongocxx_pkg_dep "find_dependency(mongoc-1.0 REQUIRED)")
endif()
else()
find_package(mongoc-${LIBMONGOC_REQUIRED_ABI_VERSION} ${LIBMONGOC_REQUIRED_VERSION} QUIET)
if(mongoc-${LIBMONGOC_REQUIRED_ABI_VERSION}_FOUND)
Expand Down
7 changes: 1 addition & 6 deletions cmake/BsoncxxUtil.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function(bsoncxx_add_library TARGET OUTPUT_NAME LINK_TYPE)
endif()

if(LINK_TYPE STREQUAL "STATIC")
target_compile_definitions(bsoncxx_static PUBLIC BSONCXX_STATIC)
target_compile_definitions(${TARGET} PUBLIC BSONCXX_STATIC)
endif()

if(BSONCXX_POLY_USE_MNMLSTC AND NOT BSONCXX_POLY_USE_SYSTEM_MNMLSTC)
Expand Down Expand Up @@ -82,11 +82,6 @@ function(bsoncxx_install BSONCXX_TARGET_LIST BSONCXX_PKG_DEP BSONCXX_BOOST_PKG_D
@ONLY
)

export(EXPORT bsoncxx_targets
NAMESPACE mongo::
FILE "${CMAKE_CURRENT_BINARY_DIR}/bsoncxx_targets.cmake"
)

install(EXPORT bsoncxx_targets
NAMESPACE mongo::
FILE bsoncxx_targets.cmake
Expand Down
5 changes: 0 additions & 5 deletions cmake/MongocxxUtil.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,6 @@ function(mongocxx_install MONGOCXX_TARGET_LIST MONGOCXX_PKG_DEP)
@ONLY
)

export(EXPORT mongocxx_targets
NAMESPACE mongo::
FILE "${CMAKE_CURRENT_BINARY_DIR}/mongocxx_targets.cmake"
)

install(EXPORT mongocxx_targets
NAMESPACE mongo::
FILE mongocxx_targets.cmake
Expand Down
18 changes: 4 additions & 14 deletions src/bsoncxx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ if(TARGET bson_shared OR TARGET bson_static)
set(libbson_target bson_static)
endif()

if(BSONCXX_BUILD_STATIC)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous if(BSONCXX_BUILD_STATIC) suggests the libbson package dependency is only needed when consuming the static bsoncxx target.

With this changeset, this scenario appears to result in an error:

The C++ driver is configured with -DBUILD_SHARED_AND_STATIC_LIBS=OFF and installed.

A test application links to the mongocxx::mongocxx_shared target:

cmake_minimum_required(VERSION 3.2 FATAL_ERROR)
project(CXX-2748 LANGUAGES C CXX)
find_package(mongocxx REQUIRED)
add_executable (main.out main.cpp)
target_link_libraries(main.out PRIVATE mongo::mongocxx_shared)

The test application is configured, setting CMAKE_PREFIX_PATH to the location of the C++ driver install (but not C driver install). This results in an error:

CMake Error at /opt/homebrew/Cellar/cmake/3.27.4/share/cmake/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
  By not providing "Findmongoc-1.0.cmake" in CMAKE_MODULE_PATH this project
  has asked CMake to find a package configuration file provided by
  "mongoc-1.0", but CMake did not find one.

This does not result in an error before the changeset.

Should the if(BSONCXX_BUILD_STATIC) be added back?

Copy link
Contributor Author

@eramongodb eramongodb Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This PR's proposed changes deliberately produce that error.

Currently, the shared library dependency is being resolved via the dynamic linker by encoding the dependency in the shared library file's list of required shared libraries for symbol table resolution. However, this does not express/satisfy the library dependencies at the CMake project level, which can have an impact on certain platforms and CMake configurations.

This pre-PR behavior is discussed in this CMake issue:

[...] having the library should be enough, we shouldn't need a full-blown config package for it. We don't need headers or build targets to link to it, as long as we either know where it is or we know it will be available in a directory the linker will be searching already.

However, the pre-PR behavior is arguably not ideal. Both pre-PR and post-PR, the CMake package targets file that is generated and installed for mongocxx includes the following command (for a release configuration; bsoncxx has symmetric content):

set_target_properties(mongo::mongocxx_shared PROPERTIES
  IMPORTED_LINK_DEPENDENT_LIBRARIES_RELEASE "mongo::mongoc_shared"
  IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libmongocxx.so.3.99.0-pre"
  IMPORTED_SONAME_RELEASE "libmongocxx.so._noabi"
  )

Without the addition of find_dependency(mongoc-1.0 REQUIRED), the referenced mongo::mongoc_shared target for IMPORTED_LINK_DEPENDENT_LIBRARIES_RELEASE may never be resolved. Per CMake documentation:

On some platforms the linker searches for the dependent libraries of shared libraries they are including in the link. [...] On platforms requiring dependent shared libraries to be found at link time CMake uses this list to add appropriate files or paths to the link command line.

This is also acknowledged in the CMake issue linked above:

I can confirm that if I add find_package(A) to src/app/CMakeLists.txt in the reproducer, so that an imported A target is created, then a proper rpath-link is generated and the project succeeds to link. [...] The question then is, how would CMake know what value to give to -rpath-link. There's a lack of info for CMake to know where the potential library would be. With an imported target, it knows where the lib is.

You're right though, without an imported target, we probably won't be able to satisfy those constraints. But equally, without a target, we don't know if that's a problem or not without reproducing the linker's entire search logic, which isn't really feasible.

Note, at the moment, there is deliberately no warning on missing find_dependency() for a target in IMPORTED_LINK_DEPENDENT_LIBRARIES as documented here:

CMake 3.24 introduced a change that makes libraries in IMPORTED_LINK_DEPENDENT_LIBRARIES turn up in the missing files list, which breaks libraries which do not explicitly import these libraries before including the target script. [...] Thus the "new" behaviour seems more correct at first glance, but also leads to sudden breakage on GitHub Actions as GitHub has been rolling out CMake 3.24 on its Windows systems. [...] This behavior change was unintentional.

Ideally this call to find_dependency() would automatically be generated by CMake as part of install(EXPORT) for packages whose imported targets are depended on by a target exported by our CMake project (such as mongo::mongocxx_shared -> mongo::mongoc_shared), but this feature is not yet implemented due to complexities concerning the "we don't know if that's a problem" problem. Therefore, it's currently our responsibility to ensure find_dependency() is present in the package config file for packages providing targets we depend on in our own exported targets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the explanation.

set(bsoncxx_pkg_dep "find_dependency(bson-1.0 REQUIRED)")
endif()
set(bsoncxx_pkg_dep "find_dependency(bson-${LIBBSON_REQUIRED_ABI_VERSION} REQUIRED)")
else()
# Attempt to find libbson by new package name (without lib).
find_package(bson-${LIBBSON_REQUIRED_ABI_VERSION} ${LIBBSON_REQUIRED_VERSION} QUIET)
Expand All @@ -122,10 +120,7 @@ else()
else()
set(libbson_target mongo::bson_static)
endif()

if(BSONCXX_BUILD_STATIC)
set(bsoncxx_pkg_dep "find_dependency(bson-1.0 REQUIRED)")
endif()
set(bsoncxx_pkg_dep "find_dependency(bson-${LIBBSON_REQUIRED_ABI_VERSION} REQUIRED)")
else()
# Require package of old libbson name (with lib).
if(NOT BSONCXX_LINK_WITH_STATIC_MONGOC)
Expand All @@ -134,18 +129,14 @@ else()
set(libbson_target ${BSON_LIBRARIES})
set(libbson_include_directories ${BSON_INCLUDE_DIRS})
set(libbson_definitions ${BSON_DEFINITIONS})
if(BSONCXX_BUILD_STATIC)
set(bsoncxx_pkg_dep "find_dependency(libbson-1.0 REQUIRED)")
endif()
set(bsoncxx_pkg_dep "find_dependency(libbson-${LIBBSON_REQUIRED_ABI_VERSION} REQUIRED)")
else()
find_package(libbson-static-${LIBBSON_REQUIRED_ABI_VERSION} ${LIBBSON_REQUIRED_VERSION} REQUIRED)
message ("found libbson version ${BSON_STATIC_VERSION}")
set(libbson_target ${BSON_STATIC_LIBRARIES})
set(libbson_include_directories ${BSON_STATIC_INCLUDE_DIRS})
set(libbson_definitions ${BSON_STATIC_DEFINITIONS})
if(BSONCXX_BUILD_STATIC)
set(bsoncxx_pkg_dep "find_dependency(libbson-static-1.0 REQUIRED)")
endif()
set(bsoncxx_pkg_dep "find_dependency(libbson-static-${LIBBSON_REQUIRED_ABI_VERSION} REQUIRED)")
endif()
endif()
endif()
Expand Down Expand Up @@ -232,7 +223,6 @@ endif()
if(BSONCXX_BUILD_STATIC)
bsoncxx_install_deprecated_cmake(bsoncxx-static)
list(APPEND bsoncxx_target_list bsoncxx_static)
set(bsoncxx_pkg_dep "find_dependency(bson-1.0 REQUIRED)")
endif()
if(BSONCXX_POLY_USE_BOOST)
set(bsoncxx_boost_pkg_dep "find_dependency(Boost 1.56.0 REQUIRED)")
Expand Down
2 changes: 1 addition & 1 deletion src/bsoncxx/private/libbson.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
// TODO: CXX-1366 Disable MSVC warnings for libbson
#endif

#include <bson.h>
#include <bson/bson.h>

#if defined(__clang__)
#pragma clang diagnostic pop
Expand Down
4 changes: 0 additions & 4 deletions src/bsoncxx/stdx/make_unique.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@

#if defined(BSONCXX_POLY_USE_MNMLSTC)

#if defined(MONGO_CXX_DRIVER_COMPILING) || defined(BSONCXX_POLY_USE_SYSTEM_MNMLSTC)
#include <core/memory.hpp>
#else
#include <bsoncxx/third_party/mnmlstc/core/memory.hpp>
#endif

namespace bsoncxx {
BSONCXX_INLINE_NAMESPACE_BEGIN
Expand Down
4 changes: 0 additions & 4 deletions src/bsoncxx/stdx/optional.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@

#if defined(BSONCXX_POLY_USE_MNMLSTC)

#if defined(MONGO_CXX_DRIVER_COMPILING) || defined(BSONCXX_POLY_USE_SYSTEM_MNMLSTC)
#include <core/optional.hpp>
#else
#include <bsoncxx/third_party/mnmlstc/core/optional.hpp>
#endif

namespace bsoncxx {
BSONCXX_INLINE_NAMESPACE_BEGIN
Expand Down
4 changes: 0 additions & 4 deletions src/bsoncxx/stdx/string_view.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@

#if defined(BSONCXX_POLY_USE_MNMLSTC)

#if defined(MONGO_CXX_DRIVER_COMPILING) || defined(BSONCXX_POLY_USE_SYSTEM_MNMLSTC)
#include <core/string.hpp>
#else
#include <bsoncxx/third_party/mnmlstc/core/string.hpp>
#endif

namespace bsoncxx {
BSONCXX_INLINE_NAMESPACE_BEGIN
Expand Down
18 changes: 4 additions & 14 deletions src/mongocxx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@ if(TARGET mongoc_shared OR TARGET mongoc_static)
set(libmongoc_target mongoc_static)
endif()

if(MONGOCXX_BUILD_STATIC)
set(mongocxx_pkg_dep "find_dependency(mongoc-1.0 REQUIRED)")
endif()
set(mongocxx_pkg_dep "find_dependency(mongoc-${LIBMONGOC_REQUIRED_ABI_VERSION} REQUIRED)")
else()
# Attempt to find libmongoc by new package name (without lib).
find_package(mongoc-${LIBMONGOC_REQUIRED_ABI_VERSION} ${LIBMONGOC_REQUIRED_VERSION} QUIET)
Expand All @@ -56,28 +54,21 @@ else()
else()
set(libmongoc_target mongo::mongoc_static)
endif()

if(MONGOCXX_BUILD_STATIC)
set(mongocxx_pkg_dep "find_dependency(mongoc-1.0 REQUIRED)")
endif()
set(mongocxx_pkg_dep "find_dependency(mongoc-${LIBMONGOC_REQUIRED_ABI_VERSION} REQUIRED)")
else()
# Require package of old libmongoc name (with lib).
if(NOT MONGOCXX_LINK_WITH_STATIC_MONGOC)
find_package(libmongoc-${LIBMONGOC_REQUIRED_ABI_VERSION} ${LIBMONGOC_REQUIRED_VERSION} REQUIRED)
message ("found libmongoc version ${MONGOC_VERSION}")
set(libmongoc_target ${MONGOC_LIBRARIES})
set(libmongoc_definitions ${MONGOC_DEFINITIONS})
if(MONGOCXX_BUILD_STATIC)
set(mongocxx_pkg_dep "find_dependency(libmongoc-1.0 REQUIRED)")
endif()
set(mongocxx_pkg_dep "find_dependency(libmongoc-${LIBMONGOC_REQUIRED_ABI_VERSION} REQUIRED)")
else()
find_package(libmongoc-static-${LIBMONGOC_REQUIRED_ABI_VERSION} ${LIBMONGOC_REQUIRED_VERSION} REQUIRED)
message ("found libmongoc version ${MONGOC_STATIC_VERSION}")
set(libmongoc_target ${MONGOC_STATIC_LIBRARIES})
set(libmongoc_definitions ${MONGOC_STATIC_DEFINITIONS})
if(MONGOCXX_BUILD_STATIC)
set(mongocxx_pkg_dep "find_dependency(libmongoc-static-1.0 REQUIRED)")
endif()
set(mongocxx_pkg_dep "find_dependency(libmongoc-static-${LIBMONGOC_REQUIRED_ABI_VERSION} REQUIRED)")
endif()
endif()
endif()
Expand Down Expand Up @@ -245,7 +236,6 @@ endif()
if(MONGOCXX_BUILD_STATIC)
mongocxx_install_deprecated_cmake(mongocxx-static)
list(APPEND mongocxx_target_list mongocxx_static)
set(mongocxx_pkg_dep "find_dependency(mongoc-1.0 REQUIRED)")
endif()
mongocxx_install("${mongocxx_target_list}" "${mongocxx_pkg_dep}")

Expand Down
2 changes: 1 addition & 1 deletion src/mongocxx/private/libmongoc.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
// TODO: CXX-1366 Disable MSVC warnings for libmongoc
#endif

#include <mongoc.h>
#include <mongoc/mongoc.h>

#if defined(__clang__)
#pragma clang diagnostic pop
Expand Down