Install Exports for custom `install-{lib,bin}` targets #79

hebasto · 2022-09-17T08:36:17Z

This is a follow up of #74 and addresses #74 (review):

The only downside of the current implementation is that new install-lib and install-bin targets do not export the underlying targets.

Can confirm neither of the new install commands create $PREFIX/cmake/Multiprocess/Multiprocess.cmake and $PREFIX/lib/cmake/Multiprocess/Multiprocess-debug.cmake files.

Probably should update install() EXPORT Multiprocess arguments to EXPORT libmultiprocess-bin and EXPORT libmultiprocess-lib so the two installs can be independent. And maybe there is a way to get the new install-bin and install-lib commands to actually export these targets.

ryanofsky

Thanks for following up. I think PR is an improvement over status quo, but I suggested a few tweaks that might make it better.

Additionally, it could be good to add NAMESPACE Libmultiprocess:: to install commands to scope exported targets under the cmake project name.

Later, it might also be good to turn the install into a full cmake package (https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-a-package-configuration-file) so if a bitcoin cmake build is added, it could use find_package instead of include and take advantage of cmake package detection & versioning features.

ryanofsky · 2022-09-19T19:38:31Z

CMakeLists.txt

  ARCHIVE DESTINATION lib COMPONENT lib
  PUBLIC_HEADER DESTINATION include/mp COMPONENT lib)
 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/pkgconfig/libmultiprocess.pc"
  DESTINATION "lib/pkgconfig" COMPONENT lib)
+install(EXPORT libmultiprocess-lib DESTINATION lib/cmake/Multiprocess)


In commit "Install Exports for custom install-{lib,bin} targets" (78bfa2a)

Could you add COMPONENT lib to this command? It looks like that would allow dropping Unspecified install command below.

Thanks! Updated.

ryanofsky · 2022-09-19T19:47:18Z

CMakeLists.txt

  ARCHIVE DESTINATION lib COMPONENT lib
  PUBLIC_HEADER DESTINATION include/mp COMPONENT lib)
 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/pkgconfig/libmultiprocess.pc"
  DESTINATION "lib/pkgconfig" COMPONENT lib)
+install(EXPORT libmultiprocess-lib DESTINATION lib/cmake/Multiprocess)


In commit "Install Exports for custom install-{lib,bin} targets" (78bfa2a)

Could you change lib/cmake/Multiprocess to ${CMAKE_INSTALL_LIBDIR}/cmake?

I know Multiprocess subdirectory was used before this PR, but I think that directory name is a little misleading because it doesn't match the project name. Also, there is not really a point in creating a subdirectory since this is just exporting targets, not a full cmake package (https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-packages, https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure)

Could you change lib/cmake/Multiprocess to ${CMAKE_INSTALL_LIBDIR}/cmake?

Done in a new "Use GNUInstallDirs module" commit.

I know Multiprocess subdirectory was used before this PR, but I think that directory name is a little misleading because it doesn't match the project name. Also, there is not really a point in creating a subdirectory since this is just exporting targets, not a full cmake package (https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-packages, https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure)

Thanks! Updated.

ryanofsky · 2022-09-19T19:48:11Z

CMakeLists.txt

 add_custom_target(install-lib
  COMMAND ${CMAKE_COMMAND} -DCOMPONENT=lib -P ${CMAKE_CURRENT_BINARY_DIR}/cmake_install.cmake
+  COMMAND ${CMAKE_COMMAND} -DCOMPONENT=Unspecified -P ${CMAKE_CURRENT_BINARY_DIR}/cmake_install.cmake


In commit "Install Exports for custom install-{lib,bin} targets" (78bfa2a)

I think this could be dropped adding COMPONENT lib to install(EXPORT) command above

Thanks! Updated.

ryanofsky · 2022-09-19T19:49:09Z

CMakeLists.txt

  RUNTIME DESTINATION bin COMPONENT bin
  PUBLIC_HEADER DESTINATION include/mp COMPONENT bin)
 install(FILES "include/mpgen.mk"
  DESTINATION "include" COMPONENT bin)
+install(EXPORT libmultiprocess-bin DESTINATION lib/cmake/Multiprocess)


In commit "Install Exports for custom install-{lib,bin} targets" (78bfa2a)

Similar to above, could you add COMPONENT bin and change DESTINATION lib/cmake/Multiprocess to DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake?

Thanks! Updated.

hebasto · 2023-01-27T14:12:13Z

Updated 78bfa2a -> d795e96 (pr79.01 -> pr79.02):

rebased
addressed @ryanofsky's comments

hebasto · 2023-01-27T15:41:23Z

Additionally, it could be good to add NAMESPACE Libmultiprocess:: to install commands to scope exported targets under the cmake project name.

Done.

hebasto · 2023-01-28T16:14:10Z

Later, it might also be good to turn the install into a full cmake package (https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-a-package-configuration-file) so if a bitcoin cmake build is added, it could use find_package instead of include and take advantage of cmake package detection & versioning features.

FWIW, here is a branch with a proof of concept -- https://github.com/hebasto/libmultiprocess/commits/230128-export.DEMO.

And here is its use case -- https://github.com/hebasto/bitcoin/commits/230128-cmake.MP.DEMO.

The problem to be solved is the handling of all dependencies (capnp, kj etc) via config files...

ryanofsky · 2023-02-14T19:46:42Z

Reviewed dae3ff8 and confirmed that install-bin and install-lib now lib/cmake/libmultiprocess-bin.cmake and lib/cmake/libmultiprocess-bin.cmake files respectively, containing exported targets from the project. The only thing I think it would be good to do still is make the target namespace name libmultiprocess the same as the cmake project name Libmultiprocess.

ryanofsky · 2023-02-14T19:49:51Z

CMakeLists.txt

  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT lib
  PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mp COMPONENT lib)
 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/pkgconfig/libmultiprocess.pc"
  DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig COMPONENT lib)
+install(EXPORT libmultiprocess-lib
+  NAMESPACE libmultiprocess::


In commit "Install Exports for custom install-{lib,bin} targets" (dae3ff8)

Could you change NAMESPACE libmultiprocess:: to NAMESPACE Libmultiprocess:: here and below? Seems better not to use different namespace and project names unnecessarily, and from https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html, it seems like the convention for both is to use uppercase

Thanks! Updated.

hebasto · 2023-02-14T19:59:39Z

Updated dae3ff8 -> 2dda753 (pr79.03 -> pr79.04, diff):

addressed @ryanofsky's comment

ryanofsky

Code review ACK 2dda753. Only change since last review is changing namespace prefix to match project name as suggested (thanks!)

ryanofsky · 2023-11-20T11:18:59Z

CMakeLists.txt

 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/pkgconfig/libmultiprocess.pc"
-  DESTINATION "lib/pkgconfig" COMPONENT lib)
+  DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig COMPONENT lib)


In commit "Use GNUInstallDirs module" (54bd57f)

I think this change is right, but it did seem to cause a problem with a depends build that fanquake is fixing in bitcoin/bitcoin#28846 commit bitcoin/bitcoin@156366f

On fedora, replacing lib with ${CMAKE_INSTALL_LIBDIR} here causes the pkgconfig file to be installed in lib64/ instead of lib/, which seems right and makes sense according to explanations given https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html and https://github.com/Kitware/CMake/blob/master/Modules/GNUInstallDirs.cmake

But this is incompatible with the depends build that hardcodes the lib/ directory: https://github.com/bitcoin/bitcoin/blob/d752349029ec7a76f1fd440db2ec2e458d0f3c99/depends/config.site.in#L91, so hardcoding lib again in bitcoin/bitcoin#28846 is needed after this change.

Right. The current Bitcoin Core depends build system will handle any other package built and installed with CMake in this flawed way. A generic fix for it looks quite reasonable.

Right. The current Bitcoin Core depends build system will handle any other package built and installed with CMake in this flawed way.

The depends system definitely has a bug because it will install the library in "lib64" on some platforms, but look for it in "lib" regardless of the platform. But the approach isn't really flawed, is it? It seems reasonable for the depends system to just always install the library in "lib" and always look for it there.

I'm fine with either approach. But in the light of the upcoming migration from Autotools to CMake, the forced installation to the lib subdirectory looks useless.

On some systems, libmultiprocess would be installed into `lib64`, I assume due to the use of GNUInstallDirs, however all other libs we build in depends, go into lib/. Rather than adding lib64/ to the pkg-config and link flags, I opted for always installing into lib/. This was changed in bitcoin-core/libmultiprocess#79 upstream.

On some systems, libmultiprocess would be installed into `lib64`, I assume due to the use of GNUInstallDirs, however all other libs we build in depends, go into lib/. Rather than adding lib64/ to the pkg-config and link flags, I opted for always installing into lib/. This was changed in bitcoin-core/libmultiprocess#79 upstream. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

…rch64 bde8d63 depends: build libmultiprocess with position independant code (fanquake) 506634d depends: always install libmultiprocess to /lib (fanquake) beb3096 depends: always install capnp to /lib (fanquake) Pull request description: Change to always install libmultiprocess into `lib/`. On some systems (my Fedora aarch64 box), libmultiprocess was being installed into `lib64/`, and then configure would fail to pick it up, because we only add `lib/` to pkgconfig/ldflags out of depends. Rather than adding lib64 to those, I opted for installing libmultiprocess into lib, with every other dependency we build. This was broken in our build after bitcoin-core/libmultiprocess#79 upstream. ACKs for top commit: ryanofsky: Code review ACK bde8d63. Only changes since last review were reverting the native_capnp change as suggested, and changing the order of the first two commits. Tree-SHA512: ddd547e4ac224f2f199c569efd91104db7f2c243b124f9535aa0d9377315775ac566d699101580ce45ddd6676ad3e0c8cbe256334eeed9548205c2fa04d02102

Use @CMAKE_INSTALL_LIBDIR@ variable instead of hardcoding "lib" as the libdir. This is needed after 54bd57f (bitcoin-core#79) which changed the installation to sometimes install to "lib64". This fixes error "ld: cannot find -lmultiprocess: No such file or directory" in Bitcoin Core. Also replace hardcoded "include" with @CMAKE_INSTALL_INCLUDEDIR@ for consistency.

ryanofsky · 2024-01-11T19:06:30Z

Just to follow up, it turns out this change caused two regressions:

1 - A problem in the bitcoin core "depends" build when the bitcoin core autoconf script could not find the libmultiprocess.pc file because it was installed to "lib64" not "lib" (#79 (comment)). This was fixed in depends in bitcoin/bitcoin#28846.

2 - A second problem fixed in #90 where the libmultiprocess.pc points to "lib" not 'lib64" resulting in link errors when the libmultiprocess libary is installed in "lib64" and can't be found. The fix for the previous problem masked this issue in the depends build by making the depends build always install to "lib." But outside of the depends build, link errors could still happen. #90 fixes the problem by updating libmultiprocess.pc to point to the correct location.

0f9605b pkgconfig: Use @CMAKE_INSTALL_LIBDIR@ variable (Ryan Ofsky) Pull request description: Use @CMAKE_INSTALL_LIBDIR@ variable instead of hardcoding "lib" as the libdir. This is needed after 54bd57f (#79) which changed the installation to sometimes install to "lib64". This fixes error "ld: cannot find -lmultiprocess: No such file or directory" in Bitcoin Core. Also replace hardcoded "include" with @CMAKE_INSTALL_INCLUDEDIR@ for consistency. Top commit has no ACKs. Tree-SHA512: f6bf645b9ad9575b4f1847a65839de01546dea574787ac1789cfffc9b144545dfc70b552d986e003ef845c40b011393b6be0d1e324e4bd6ba4e4e89d29c3a91a

On some systems, libmultiprocess would be installed into `lib64`, I assume due to the use of GNUInstallDirs, however all other libs we build in depends, go into lib/. Rather than adding lib64/ to the pkg-config and link flags, I opted for always installing into lib/. This was changed in bitcoin-core/libmultiprocess#79 upstream. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

bde8d63 depends: build libmultiprocess with position independant code (fanquake) 506634d depends: always install libmultiprocess to /lib (fanquake) beb3096 depends: always install capnp to /lib (fanquake) Pull request description: Change to always install libmultiprocess into `lib/`. On some systems (my Fedora aarch64 box), libmultiprocess was being installed into `lib64/`, and then configure would fail to pick it up, because we only add `lib/` to pkgconfig/ldflags out of depends. Rather than adding lib64 to those, I opted for installing libmultiprocess into lib, with every other dependency we build. This was broken in our build after bitcoin-core/libmultiprocess#79 upstream. ACKs for top commit: ryanofsky: Code review ACK bde8d63. Only changes since last review were reverting the native_capnp change as suggested, and changing the order of the first two commits. Tree-SHA512: ddd547e4ac224f2f199c569efd91104db7f2c243b124f9535aa0d9377315775ac566d699101580ce45ddd6676ad3e0c8cbe256334eeed9548205c2fa04d02102

ryanofsky approved these changes Sep 19, 2022

View reviewed changes

Use GNUInstallDirs module

54bd57f

hebasto force-pushed the 220917-export branch from 78bfa2a to d795e96 Compare January 27, 2023 14:09

hebasto force-pushed the 220917-export branch from d795e96 to dae3ff8 Compare January 27, 2023 15:41

ryanofsky reviewed Feb 14, 2023

View reviewed changes

Install Exports for custom install-{lib,bin} targets

2dda753

hebasto force-pushed the 220917-export branch from dae3ff8 to 2dda753 Compare February 14, 2023 19:56

ryanofsky approved these changes Feb 14, 2023

View reviewed changes

ryanofsky merged commit 917877a into bitcoin-core:master Feb 14, 2023

hebasto deleted the 220917-export branch February 15, 2023 08:19

ryanofsky mentioned this pull request Nov 17, 2023

depends: Bump to capnproto-c++-1.0.1 bitcoin/bitcoin#28735

Merged

ryanofsky reviewed Nov 20, 2023

View reviewed changes

ryanofsky mentioned this pull request Nov 20, 2023

depends: fix libmultiprocess build on aarch64 bitcoin/bitcoin#28846

Merged

ryanofsky mentioned this pull request Dec 1, 2023

depends: Build the native_capnp and capnp packages with CMake bitcoin/bitcoin#28856

Merged

ryanofsky mentioned this pull request Jan 11, 2024

pkgconfig: Use @CMAKE_INSTALL_LIBDIR@ variable #90

Merged

ryanofsky mentioned this pull request Mar 19, 2024

cmake: Introduce packages #96

Merged

bitcoin-core locked and limited conversation to collaborators Jun 25, 2025

Install Exports for custom install-{lib,bin} targets #79

Install Exports for custom install-{lib,bin} targets #79

Uh oh!

Conversation

hebasto commented Sep 17, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 27, 2023

Uh oh!

hebasto commented Jan 27, 2023

Uh oh!

hebasto commented Jan 28, 2023

Uh oh!

ryanofsky commented Feb 14, 2023

Uh oh!

ryanofsky Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 14, 2023

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jan 11, 2024

Uh oh!

Uh oh!

Install Exports for custom `install-{lib,bin}` targets #79

Install Exports for custom `install-{lib,bin}` targets #79

ryanofsky Feb 14, 2023 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Nov 20, 2023 •

edited

Loading