-
Notifications
You must be signed in to change notification settings - Fork 543
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
Conversation
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) |
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 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?
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.
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)
tosrc/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.
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.
Thank you for the explanation.
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.
LGTM. The fix is much appreciated.
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 bythis patchthis 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:BSONCXX_STATIC
compile definition.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.libbson.hh
/libmongoc.hh
headers to includebson.h
andmongoc.h
using proper include directives as a consuming library (need thebson/
andmongoc/
prefix).MONGO_CXX_DRIVER_COMPILING
is not set.ENABLE_AUTOMATIC_INIT_AND_CLEANUP=OFF
when configuring inbuild_examples_with_add_subdirectory
tasks to address the corresponding deprecation warning.