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.

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

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Sep 15, 2023

This PR does not properly address the issues that cause CXX-2748. It just prevents it from affecting the build_examples_with_add_subdirectory tasks from failing on waterfall. Although if we want to drop support for the build tree targets file entirely, I would not be opposed to it. Verified by this patch this patch.

Besides the outright disabling of the build tree targets file (the reason for target export failures), there are other changes addressed by this PR that were identified by the re-enabling of the build_examples_with_add_subdirectory task:

  • Incorrect target specification when setting the BSONCXX_STATIC compile definition.
  • Missing calls to find_dependency() for libbson/libmongoc packages: the modern CXX Driver CMake package config files always require a libbson/libmongoc package to satisfy the corresponding target dependencies.
  • Update internal libbson.hh/libmongoc.hh headers to include bson.h and mongoc.h using proper include directives as a consuming library (need the bson/ and mongoc/ prefix).
  • Followup to CXX-2749 Use FetchContent and find_package() to obtain mnmlstc/core #1019 that fixes include directives for mnmlstc/core when MONGO_CXX_DRIVER_COMPILING is not set.
  • Set ENABLE_AUTOMATIC_INIT_AND_CLEANUP=OFF when configuring in build_examples_with_add_subdirectory tasks to address the corresponding deprecation warning.

@eramongodb eramongodb self-assigned this Sep 15, 2023
@eramongodb
Copy link
Contributor Author

After further discussion, determined removal of build tree targets file support as a solution for CXX-2748. PR and CHANGELOG have been updated accordingly.

@@ -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

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.

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.

LGTM. The fix is much appreciated.

@eramongodb eramongodb merged commit 25f3318 into mongodb:master Sep 18, 2023
@eramongodb eramongodb deleted the cxx-2748 branch September 18, 2023 20:12
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