Simplify Deprecated CMake Packages as Wrappers Around Current Packages #1384
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The computation of BUILD_VERSION is now lifted above project(), which allows us to use that value in the project() call itself, giving us correct PROJECT_VERSION values throughout. - Define BUILD_VERSION using mongo_setting(), and use the calc_release_version.py to compute its default value.
- Remove a lot of variable capture that was relied upon when generating - The _LIBRARY/_LIBRARIES variables in the legacy packages now point to the imported target of their respective packages, which will handle all other "magic variable" definitions.
vector-of-bool
commented
Aug 17, 2023
Comment on lines
-35
to
-37
| set (CPACK_PACKAGE_VERSION_MAJOR ${BSON_MAJOR_VERSION}) | ||
| set (CPACK_PACKAGE_VERSION_MINOR ${BSON_MINOR_VERSION}) | ||
|
|
There was a problem hiding this comment.
These values are inferred from project(), which now contains the correct version information.
(Not sure if we have many CPack users anyway, though)
vector-of-bool
commented
Aug 17, 2023
Comment on lines
-370
to
-374
| set_target_properties(${TARGETS_TO_INSTALL} PROPERTIES | ||
| pkg_config_NAME libbson | ||
| pkg_config_DESCRIPTION "The libbson BSON serialization library." | ||
| pkg_config_VERSION "${BSON_VERSION}" | ||
| ) |
There was a problem hiding this comment.
Inferred from project()
vector-of-bool
commented
Aug 17, 2023
Comment on lines
-393
to
-408
| set (LIBBSON_LIBRARIES "") | ||
| foreach (_lib ${BSON_LIBRARIES}) | ||
| if (_lib MATCHES ".*/.*" OR _lib MATCHES "^-") | ||
| set (LIBBSON_LIBRARIES "${LIBBSON_LIBRARIES} ${_lib}") | ||
| else () | ||
| set (LIBBSON_LIBRARIES "${LIBBSON_LIBRARIES} -l${_lib}") | ||
| endif () | ||
| endforeach () | ||
| # System dependencies don't match the above regexs, but also don't want the -l | ||
| foreach (_lib ${BSON_SYSTEM_LIBRARIES}) | ||
| set (LIBBSON_LIBRARIES "${LIBBSON_LIBRARIES} ${_lib}") | ||
| endforeach () | ||
|
|
||
| include (CMakePackageConfigHelpers) | ||
| set (INCLUDE_INSTALL_DIRS "${BSON_HEADER_INSTALL_DIR}") | ||
| set (LIBRARY_INSTALL_DIRS ${CMAKE_INSTALL_LIBDIR}) |
There was a problem hiding this comment.
Only used by the old script generation during configure_file, so these aren't needed otherwise.
rcsanchez97
approved these changes
Aug 17, 2023
Co-authored-by: Roberto C. Sánchez <[email protected]>
kevinAlbs
approved these changes
Aug 18, 2023
| set (MONGOC_VERSION_FULL @libmongoc_VERSION_FULL@) | ||
|
|
||
| include(CMakeFindDependencyMacro) | ||
| find_dependency (mongoc-1.0 "@libmongoc_VERSION_MAJOR@.@libmongoc_VERSION_MINOR@") |
There was a problem hiding this comment.
Is @libmongoc_VERSION_MAJOR@.@libmongoc_VERSION_MINOR@ needed? libbson-1.0-config.cmake.in does does not include the version in the call to find_dependency.
There was a problem hiding this comment.
Good question. Presumably, if this file is installed then the equivalent new package will be installed, so this version may be redundant.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old CMake config-file-packages were generated by mostly-identical scripts and processes, but are deprecated in favor of our newer config-file-packages. I think it is safe at this point to replace the content of the deprecated package scripts with scripts that import the new packages and expose them in a mostly-compatible* manner.
The legacy package generation significantly cluttered and complicated the CMakeLists for the respective projects, since a lot of work was done to populate variables that were inserted into the script templates, whereas now those usage requirements are propagated via targets alone.
The new deprecated scripts no longer define/use the
INCLUDE_DIRS/DEFINES/etc variables, and onlyXYZ_LIBRARIES/XYZ_LIBRARYremain, which just contain the name of the equivalent imported target from the new CMake config-file-packages.* "mostly compatible" because the primary case of linking to e.g.
${BSON_LIBRARIES}will "just work" withtarget_link_libraries, but users that depended on the specificfind_librarycall and/or inspected other variables e.g.BSON_INCLUDE_DIRSwill find that those are now not available. I think it is reasonable to assume that very few users, if any, were relying on this exact case.(This PR contains commits that are already merged by #1382.)