cmake: Introduce packages #96
Merged
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
This was referenced Mar 16, 2024
|
Looks nice! Will review this. I remembered you also posted a proof of concept branch defining packages last year in #79 (comment) that I guess this supercedes? |
Yes, it does. |
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The `<Package>Targets` pattern is widely used and does not cause confusion.
ryanofsky
added a commit
that referenced
this pull request
Mar 29, 2024
66643d8 cmake, refactor: Use `target_capnp_sources` for examples (Hennadii Stepanov) bd2dfe2 cmake, refactor: Use `target_capnp_sources` for `mptest` target (Hennadii Stepanov) d9ec22f cmake: Add `LibmultiprocessMacros` module (Hennadii Stepanov) Pull request description: This PR introduces the `LibmultiprocessMacros` module that is used internally and might be exported as a part of the (future) `LibmultiprocessGen` package (it is a subject of the follow-up [PR](#96)). Also see a discussion in hebasto/bitcoin#118. ACKs for top commit: ryanofsky: Code review ACK 66643d8 Tree-SHA512: b97fcb2ce7b2085b3a041c422e3f51f06b8bf7a0abaa5baa7f39db2bf03491447bf7af073c95fb8bb6bc602d30e0c8856ae523954980cdb85afd87442c0b909a
ryanofsky
approved these changes
Mar 30, 2024
There was a problem hiding this comment.
Code review ACK 4e70ad4. This should make it easier to depend on libmultprocess in cmake projects, using a find_package call and not having to include files directly.
I think I would like to rename a few things in the PR, to make new package names match existing component names (bin and lib). I also don't like the redundant "Libmultiprocess/Libmultiprocess" in some installed filenames, and the way one package name is a prefix of the other, since they are side-by-side dependencies. I can post a followup PR with some renames though.
| DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake COMPONENT lib) | ||
| DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/Libmultiprocess COMPONENT lib) | ||
| include(CMakePackageConfigHelpers) | ||
| configure_package_config_file( |
There was a problem hiding this comment.
I think there may be need to be some additional changes to this file to make the package "relocatable" according to https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages.
Specifically the PUBLIC include directory $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> on line 84 above and ${CAPNP_INCLUDE_DIRECTORY}` on line 85 look like they will add include paths from the build system to the exported targets, and potentially cause problems in some build configurations. This might not be a problem for the bitcoin depends build, though, because it is building everything on one system.
Possible fixes to replace the $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> line might be:
- To replace it with
$<INSTALL_INTERFACE:include>or$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/includeas suggested in https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages - To use
INCLUDES DESTINATIONdirective described https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#include-directories-and-usage-requirements which is apparently "equivalent to appending ${CMAKE_INSTALL_PREFIX}/include"
Possible fix to replace the ${CAPNP_INCLUDE_DIRECTORY} line might be to change target_link_libraries(multiprocess PRIVATE CapnProto::capnp) from PRIVATE to PUBLIC, assuming that making capnproto a public dependency will cause cmake to propagate the necessary includes.
ryanofsky
added a commit
to ryanofsky/libmultiprocess
that referenced
this pull request
Mar 30, 2024
bitcoin-core#96 Rename "LibmultiprocessMacros.cmake" module to "TargetCapnpSources.cmake" to match the name of the target_capnp_sources function it contains. Also install it <prefix>/lib/cmake/Libmultiprocess/TargetCapnpSources.cmake instead of: <prefix>/lib/cmake/LibmultiprocessGen/LibmultiprocessMacros.cmake Rename the "LibmultiprocessGen" package to "LibmultiprocessBin" and rename the "Libmultiprocess" package to "LibmultiprocessLib", so package names are consistent with component names "bin" and "lib", and one package name is not a prefix of the other. Also rename intermediate files to match component names. |----------------+----------------------------------+----------------------------------------------| | | New | Current | |----------------+----------------------------------+----------------------------------------------| | Component name | lib | lib | | Package name | LibmultiprocessLib | Libmultiprocess | | Config file | LibmultiprocessLibConfig.cmake | Libmultiprocess/LibmultiprocessConfig.cmake | | Targets file | Libmultiprocess/LibTargets.cmake | Libmultiprocess/LibmultiprocessTargets.cmake | |----------------+----------------------------------+----------------------------------------------| |----------------+----------------------------------+----------------------------------------------------| | | New | Current | |----------------+----------------------------------+----------------------------------------------------| | Component name | bin | bin | | Package name | LibmultiprocessBin | LibmultiprocessGen | | Config file | LibmultiprocessBinConfig.cmake | LibmultiprocessGen/LibmultiprocessGenConfig.cmake | | Targets file | Libmultiprocess/BinTargets.cmake | LibmultiprocessGen/LibmultiprocessGenTargets.cmake | |----------------+----------------------------------+----------------------------------------------------|
ryanofsky
added a commit
that referenced
this pull request
Mar 30, 2024
…#96 c6a1d7f cmake: rename new packages and module introduced in #95 and #96 (Ryan Ofsky) Pull request description: Rename "LibmultiprocessMacros.cmake" module introduced in #95 to "TargetCapnpSources.cmake" to match the name of the `target_capnp_sources` function it contains. Also install it to: ```sh <prefix>/lib/cmake/Libmultiprocess/TargetCapnpSources.cmake ``` instead of: ```sh <prefix>/lib/cmake/LibmultiprocessGen/LibmultiprocessMacros.cmake ``` Rename the "Libmultiprocess" and "LibmultiprocessGen" packages introduced in #96 to "LibmultiprocessLib" and "LibmultiprocessBin", so package names are consistent with component names "lib" and "bin", and one package name is not a prefix of the other. Also rename intermediate files to match component names. | | New | Current | |----------------|----------------------------------|----------------------------------------------| | Component name | lib | lib | | Package name | LibmultiprocessLib | Libmultiprocess | | Config file | LibmultiprocessLibConfig.cmake | Libmultiprocess/LibmultiprocessConfig.cmake | | Targets file | Libmultiprocess/LibTargets.cmake | Libmultiprocess/LibmultiprocessTargets.cmake | | | New | Current | |----------------|----------------------------------|----------------------------------------------------| | Component name | bin | bin | | Package name | LibmultiprocessBin | LibmultiprocessGen | | Config file | LibmultiprocessBinConfig.cmake | LibmultiprocessGen/LibmultiprocessGenConfig.cmake | | Targets file | Libmultiprocess/BinTargets.cmake | LibmultiprocessGen/LibmultiprocessGenTargets.cmake | Top commit has no ACKs. Tree-SHA512: db099f34e14f5433c644190359bef0f3b836401b31c8eea3c4f7d732ec06402eb01063360d8b7ebbed9522be9c6758e157e46e15cafd80ac2bebc5c915ad2160
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jun 13, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 11, 2024
depends: Update libmultiprocess library Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 12, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 16, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 16, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Jul 17, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in #95 and #96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Jul 17, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in #95 and #96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Jul 18, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in #95 and #96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Jul 18, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in #95 and #96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 18, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 18, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Jul 19, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in #95 and #96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 24, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 26, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Sep 26, 2024
Fix "connection: run async cleanups in LIFO not FIFO order" bitcoin-core/libmultiprocess#101 is needed to prevent CI failure https://cirrus-ci.com/task/4549686449668096 caused by wallet processes deadlocking during shutdown when node process is killed. This also includes other recent changes: bitcoin-core/libmultiprocess#95: cmake: Introduce `LibmultiprocessMacros` module bitcoin-core/libmultiprocess#96: cmake: Introduce packages bitcoin-core/libmultiprocess#97: cmake: rename new packages and module introduced in bitcoin#95 and bitcoin#96 bitcoin-core/libmultiprocess#98: cmake: Combine installed packages bitcoin-core/libmultiprocess#99: proxy-types: Fix missing space in server destroy log print bitcoin-core/libmultiprocess#100: doc: Add various code comments and documentation bitcoin-core/libmultiprocess#102: doc: Document shutdown sequences better
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR adds configurations for the
LibmultiprocessandLibmultiprocessGenpackages.The Bitcoin Core branch, with uses this PR branch, is available here.
As a suggestion for a follow-up, it seems worth disabling
mpgentarget for cross-compiling (see bitcoin/bitcoin#29665 (comment) and CMake docs).Based on #95.