cmake: Combine installed packages #98

ryanofsky · 2024-04-01T18:38:03Z

This change combines previous installed:

cmake/LibmultiprocessLibConfig.cmake
cmake/LibmultiprocessBinConfig.cmake

files into a single:

cmake/Libmultiprocess/LibmultiprocessConfig.cmake

file, so it can be imported with find_package(Libmultiprocess).

The previous locations which were set in #97 were not compatible with find_package search behavior by default.

The change also adds some documentation about using the new package to doc/install.md.

Move code and add comments, no substantive changes.

This change combines previous installed "cmake/LibmultiprocessLibConfig.cmake" and "cmake/LibmultiprocessBinConfig.cmake" files into a single "cmake/Libmultiprocess/LibmultiprocessConfig.cmake" file, so it can be imported with "find_package(Libmultiprocess)". The previous locations which were set in bitcoin-core#97 were not actually compatible with find_package search behavior by default. The change also adds some documentation about using the new package to doc/install.md.

ryanofsky · 2024-04-01T19:02:29Z

Other comments about this

#97 (comment)
#95 (comment)
#97 (comment)
#95 (comment)
#97 (comment)

hebasto · 2024-04-01T19:43:48Z

I did not test this PR yet, but have a question regarding the combined package configuration.

In the Bitcoin Core depends, how does it suppose to work considering that the mpgen tool is installed in the native subdirectory of the depends prefix?

ryanofsky · 2024-04-01T20:33:52Z

I'm not sure exactly, but I'd imagine you'd just find the package twice with different components instead of finding two packages separately. Even before this PR you would need to point cmake at the native directory to find the code generation package, so I don't think that aspect is changing.

ryanofsky · 2024-04-01T20:55:53Z

Or if the issue is that cmake doesn't allow importing two packages with the same name, maybe a good solution would be to add an option here controlling the installed package name.

If multiple libmultiprocess builds are installed (native and cross-compiled) it seems good if they are all allowed to be full installs, and downstream cmake projects can choose which components of each install to use. Even though depends system only does half-builds for efficiency (executable only or library only), this doesn't seem like something that should be required for other builds.

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

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

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

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

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

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

d318c4e depends: bump libmultiprocess for CMake fixes (Cory Fields) Pull request description: Broken out of #30454 . Bumped [even further](4883197#r1684802528) after bitcoin-core/libmultiprocess#98 was merged upstream. hebasto Presumably this approach works now with the CMake branch? ACKs for top commit: ryanofsky: Code review ACK d318c4e. Tree-SHA512: 4b5491f73c0063d09e8339829cd831b1f4c441dd7b55a22037c9337c80cce19bb00a3e5cf925efa77d6d4e89ab45482f40f5799bc14948f8cabcbad3c3549430

hebasto · 2024-07-20T17:27:11Z

I'm not sure exactly, but I'd imagine you'd just find the package twice with different components instead of finding two packages separately.

Once a package found, the ${PACKAGE}_DIR variable is cached.

Therefore, to work with Bitcoin Core's depends, the code as follows should be used:

find_package(Libmultiprocess COMPONENTS Lib)
unset(Libmultiprocess_DIR CACHE)
find_package(Libmultiprocess COMPONENTS Bin
  NO_CMAKE_FIND_ROOT_PATH
  PATHS ${CMAKE_FIND_ROOT_PATH}/native
)

But this approach undermines the user's ability to specify non-standard package component locations because the Libmultiprocess_DIR variable is overloaded.

Or if the issue is that cmake doesn't allow importing two packages with the same name, maybe a good solution would be to add an option here controlling the installed package name.

If such an option will be leveraged by depends, Bitcoin Core probably will fail to find the Libmultiprocess package provided by other means.

ryanofsky · 2024-07-22T20:23:43Z

re: #98 (comment)

@hebasto can you use the find_package NAMES option to give the packages different names? From the find_package documentation:

A cache entry called <PackageName>_DIR is created to hold the directory containing the file. By default, the command searches for a package with the name <PackageName>. If the NAMES option is given, the names following it are used instead of <PackageName>.

So modifying your example above, you could write:

find_package(Libmultiprocess COMPONENTS Lib)
find_package(LibmultiprocessNative COMPONENTS Bin
  NAMES Libmultiprocess
  NO_CMAKE_FIND_ROOT_PATH
  PATHS ${CMAKE_FIND_ROOT_PATH}/native
)

This should result in separate Libmultiprocess_DIR and LibmultiprocessNative_DIR variables

hebasto · 2024-07-23T11:53:58Z

@ryanofsky

can you use the find_package NAMES option to give the packages different names? From the find_package documentation:

Thank you! The suggested approach looks nice to me.

It only requires a bugfix suggested in #103.

c373a94 cmake: Fix package configuration file (Hennadii Stepanov) Pull request description: When loading a package configuration file, `find_package` defines variables to provide information about the call arguments. In particular, `CMAKE_FIND_PACKAGE_NAME` represents the package name. This PR uses this variable instead of a hardcoded name, making the use case suggested in #98 (comment) workable: ```cmake find_package(Libmultiprocess COMPONENTS Lib) find_package(LibmultiprocessNative COMPONENTS Bin NAMES Libmultiprocess ) ``` ACKs for top commit: ryanofsky: Code review ACK c373a94 Tree-SHA512: 493cfd62c4992c9753772844c0a96004d45eb1c807986b7ce7871fdf0debede79d9f9fbe22ecce0a92d4d0b5a2d24f0067c2b5816f5210a8f5bcbb41d04fb917

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

d318c4e depends: bump libmultiprocess for CMake fixes (Cory Fields) Pull request description: Broken out of bitcoin#30454 . Bumped [even further](bitcoin@4883197#r1684802528) after bitcoin-core/libmultiprocess#98 was merged upstream. hebasto Presumably this approach works now with the CMake branch? ACKs for top commit: ryanofsky: Code review ACK d318c4e. Tree-SHA512: 4b5491f73c0063d09e8339829cd831b1f4c441dd7b55a22037c9337c80cce19bb00a3e5cf925efa77d6d4e89ab45482f40f5799bc14948f8cabcbad3c3549430

ryanofsky added 2 commits April 1, 2024 14:19

cmake: CMakeLists.txt cleanup

2ed1e9a

Move code and add comments, no substantive changes.

ryanofsky merged commit e29f74e into bitcoin-core:master Apr 1, 2024

ryanofsky mentioned this pull request Apr 1, 2024

cmake: rename new packages and module introduced in #95 and #96 #97

Merged

This was referenced Jul 19, 2024

build: Introduce CMake-based build system bitcoin/bitcoin#30454

Merged

depends: bump libmultiprocess for CMake fixes bitcoin/bitcoin#30490

Merged

hebasto mentioned this pull request Jul 23, 2024

cmake: Fix package configuration file #103

Merged

bitcoin-core locked and limited conversation to collaborators Jul 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmake: Combine installed packages #98

cmake: Combine installed packages #98

Uh oh!

ryanofsky commented Apr 1, 2024 •

edited

Loading

Uh oh!

ryanofsky commented Apr 1, 2024

Uh oh!

hebasto commented Apr 1, 2024

Uh oh!

ryanofsky commented Apr 1, 2024

Uh oh!

ryanofsky commented Apr 1, 2024 •

edited

Loading

Uh oh!

hebasto commented Jul 20, 2024 •

edited

Loading

Uh oh!

ryanofsky commented Jul 22, 2024

Uh oh!

hebasto commented Jul 23, 2024

Uh oh!

Uh oh!

cmake: Combine installed packages #98

cmake: Combine installed packages #98

Uh oh!

Conversation

ryanofsky commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 1, 2024

Uh oh!

hebasto commented Apr 1, 2024

Uh oh!

ryanofsky commented Apr 1, 2024

Uh oh!

ryanofsky commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jul 22, 2024

Uh oh!

hebasto commented Jul 23, 2024

Uh oh!

Uh oh!

ryanofsky commented Apr 1, 2024 •

edited

Loading

ryanofsky commented Apr 1, 2024 •

edited

Loading

hebasto commented Jul 20, 2024 •

edited

Loading