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 https://github.com/chaincodelabs/libmultiprocess/pull/79 upstream.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK.
8 | @@ -9,6 +9,7 @@ $(package)_dependencies += native_capnp 9 | endif 10 | 11 | define $(package)_set_vars := 12 | +$(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/
In commit "depends: always install libmultiprocess to /lib" (156366f10aa38c612e26a1f93d8503786f3d3a34)
Thanks for this fix. It seems like a regression after https://github.com/chaincodelabs/libmultiprocess/pull/79#discussion_r1399049836
Would suggest a comment here like "# Hardcode library install path to "lib" to match the PKG_CONFIG_PATH setting in depends/config.site.in which also hardcodes "lib". Without this setting, cmake by default would use the OS library directory, which might be "lib64" or something else, not "lib", on multiarch systems."
Would also suggest dropping trailing slash in "lib/" since it doesn't seem like directories are normally represented with a trailing slash in https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
Rather than adding lib64/ to the pkg-config and link flags, I opted for always installing into lib/.
Mind sharing your reasoning behind this decision? Asking because the alternative looks more generic and will work if any other dependency package will switch to CMake.
A side note, not directly related to this PR: the CMake-based build system has no such an issue at all.
Rather than adding lib64/ to the pkg-config and link flags, I opted for always installing into lib/.
Mind sharing your reasoning behind this decision?
The quote you're responding is from fanquake, but I think it makes sense for the depends build to hardcode "lib" here because it is already hardcoding "lib" in config.site.in:
So this change just keeps the build consistent and ensure the bitcoin configure script can find the library built by depends on all platforms.
Asking because the alternative looks more generic and will work if any other dependency package will switch to CMake.
This change isn't very clean so I agree there are probably better alternatives, but I'm actually not sure which alternative you're referring to here. I do agree it should become simpler when more of the build switches to cmake, though.
In commit "depends: always install libmultiprocess to /lib" (68823aa3c071b49754fe3396be49f5531aa33fd4)
I still think it would be nice to add some code comment here, maybe like the one I suggested earlier:
# Hardcode library install path to "lib" to match the PKG_CONFIG_PATH setting in depends/config.site.in which also hardcodes "lib". Without this setting, cmake by default would use the OS library directory, which might be "lib64" or something else, not "lib", on multiarch systems.
If you'd prefer not to add a comment like this so the makefile is shorter and minimal, that's fine. But personally I think the depends recipes would be easier to work with if they explained what they were trying to do without requiring git history to understand them.
Sure, pushed up your comment and added co-author.
9 | @@ -10,6 +10,7 @@ endif 10 | 11 | define $(package)_set_vars := 12 | $(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/ 13 | +$(package)_config_opts += -DCMAKE_POSITION_INDEPENDENT_CODE=ON
In commit "depends: build libmultiprocess with position independant code" (c3a962be10a98192a93518b0222c21fc7f115404)
I don't think I really understand what's going on here, but this seems like a reasonable change given that --with-pic seems to be used on so many other depends builds [^1]. I guess it is a little unclear why in the other depends builds, the --with-pic option seems to be selectively applied for individual platforms like linux, freebsd, netbsd, openbsd (and never darwin), while this PIC option apples to all platforms. But maybe always setting the option for cmake is fine.
[^1]: I asked chatgpt to explain the link error https://chat.openai.com/share/6d5445a7-14ac-493b-a41a-d2b23899caa6 but didn't have enough details about the build to be able to narrow down specifically what is happening.
I guess I have 3 questions:
--disable-shared to the capnp build for this platform like we are already doing the for the android platform? Maybe we should just be building capnp with --disable-shared unconditionally?Opposite question: is the android --disable-shared workaround needed anymore if -DCMAKE_POSITION_INDEPENDENT_CODE=ON is used here? Maybe we could partially revert #25322 after this?
Is this workaround still needed with #28856? That PR seems to drop the android workaround, so maybe switching both of these libraries to cmake would just fix the problem automatically.
re: #28846 (review)
In commit "depends: build libmultiprocess with position independant code" (965d38d0ab2cfb9b40827e422f354a061fa31209)
Yes, the switch to CMake in #28856 didn't fix this issue.
I'm still not exactly clear why this change is needed here and also why --with-pic options seem to be used for many other packages as well. But at least after #28856 the PIC option is used more consistently, and it should cause no harm in any case.
Maybe in the future the upstream build could do a better job of determining whether PIC code is needed itself, and this setting could be dropped.
Maybe in the future the upstream build could do a better job of determining whether PIC code is needed itself, and this setting could be dropped.
I agree with your comments, and will probably followup with some improvements. I think historically, the usage of --with-pic, has been a bit whack-a-mole esqu, where it's been added as issues have arrison / as people have tested things of various platforms, leading to the inconsistent state we have today.
Code review ACK c3a962be10a98192a93518b0222c21fc7f115404. These changes seems safe, and the first commit seems like a clear bugfix. But I did have some minor suggestions to improve the first commit, and some questions about the second commit below.
Thanks for reopening, code review ACK 68823aa3c071b49754fe3396be49f5531aa33fd4 for just the third commit. This PR could be a draft since it is based on another PR.
Code review ACK 1a90ac42e2591b5105ac846a5f6d07ec4b95fa1a, just adding comment since last review (thanks!)
~ACK 6293a3f5a2b61e4429c6016cc941e1ee3dc22eb3, tested on Fedora 37, aarch64.~
ACK https://github.com/bitcoin/bitcoin/commit/6293a3f5a2b61e4429c6016cc941e1ee3dc22eb3, tested on Fedora 37, aarch64.
The changes here don't link after #28856:
/usr/bin/ld: cannot find -lcapnp-rpc: No such file or directory
/usr/bin/ld: cannot find -lcapnp: No such file or directory
/usr/bin/ld: cannot find -lkj-async: No such file or directory
/usr/bin/ld: cannot find -lkj: No such file or directory
collect2: error: ld returned 1 exit status
because by switching to CMake, that PR introduced more of the same issue (installation into native/lib64) being fixed here. If we are going to keep changing things, I might just switch to changing pkg-config.
Fixed up capnp as well. Discussed this a bit with @theuni yesterday, and it seems to most straightforward to just keep using /lib (also discussed above) given thats how depends was built to work, and it's not clear why changing things to exist in various (depending on the system and it's configuration) GNU dirs is any better (for now). If we actually switched all packages in depends to using CMake, maybe we could revisit this (also assuming they all themselves use GNUInstallDirs).
11 | +# directory, which might be "lib64" or something else, not "lib", on multiarch systems. 12 | define $(package)_set_vars 13 | $(package)_config_opts := -DBUILD_TESTING=OFF 14 | $(package)_config_opts += -DWITH_OPENSSL=OFF 15 | $(package)_config_opts += -DWITH_ZLIB=OFF 16 | + $(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/
In commit "depends: always install capnp to /lib" (6d7e71ed47f9f540d981d5433b8f4efcd0767641)
I think probably it would be good revert this line and only make this change in capnp.mk not native_capnp.mk since the PKG_CONFIG_PATH mentioned in the comment above is used to find cross-compiled dependencies, not native dependencies, so the reasoning in the comment doesn't really apply here. Reverting this change would also make the native_capnp package definition simpler and more consistent with the native_libmultiprocess package definition.
On the other hand, If this change is actually needed on some platforms, that wouldn't be shocking because ways packages detect dependencies can be fragile. But I wouldn't expect this to be necessary and think it would be better not to override the build setting without a clear reason.
Have dropped this change.
Code review ACK 965d38d0ab2cfb9b40827e422f354a061fa31209. Left a suggestion to simplify the second commit, but it is not important and this change looks good as.
On some systems, capnp 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/.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
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
https://github.com/chaincodelabs/libmultiprocess/pull/79 upstream.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This matches what we do with all other dependencies, see `--with-pic`,
and fixes build failures, like #26943.
Guix build (aarch64):
1415d765a10d1686e0a37f550aaf83fadad17d2d94c17688ae298ececcca17e2 guix-build-bde8d63b1763/output/aarch64-linux-gnu/SHA256SUMS.part
7e6f42f033469d23300b93c3541babae3d60a7e21615ea3ba08f243131339e5c guix-build-bde8d63b1763/output/aarch64-linux-gnu/bitcoin-bde8d63b1763-aarch64-linux-gnu-debug.tar.gz
3eba206402f4a6f7bcbe73c53f1b95f01944e35b7c3a94cf0cb1daf2db08afc8 guix-build-bde8d63b1763/output/aarch64-linux-gnu/bitcoin-bde8d63b1763-aarch64-linux-gnu.tar.gz
571ba070158c084fd47947f78275cbcfd94c346ebc01d843b4b62f360a6418ce guix-build-bde8d63b1763/output/arm-linux-gnueabihf/SHA256SUMS.part
56325636f6808c08a7a03f396fdf2225ff2f2d6600fa0208aae1dd8fb6012aa4 guix-build-bde8d63b1763/output/arm-linux-gnueabihf/bitcoin-bde8d63b1763-arm-linux-gnueabihf-debug.tar.gz
04ad3daf53c04763471233797c3c083e3dffb0c7e954f7bc13c48bf9dd688696 guix-build-bde8d63b1763/output/arm-linux-gnueabihf/bitcoin-bde8d63b1763-arm-linux-gnueabihf.tar.gz
d6d75f9e4d5b5e6f339cc16b15c74f8e9a70361e96892d7e225178970a4bd706 guix-build-bde8d63b1763/output/arm64-apple-darwin/SHA256SUMS.part
347a53b175b853deaa04c710a4e0b4d9d3ae4fe9ec7a687694a702835d39a0d8 guix-build-bde8d63b1763/output/arm64-apple-darwin/bitcoin-bde8d63b1763-arm64-apple-darwin-unsigned.tar.gz
cb2c32a31be45ce36067e31e29290aba1a71014320b54eed669e9e5d9a72fd6f guix-build-bde8d63b1763/output/arm64-apple-darwin/bitcoin-bde8d63b1763-arm64-apple-darwin-unsigned.zip
52838e8b6df3fc7b2cc377fd6279e8713e215dd256670b8be01c1d4ae12731c6 guix-build-bde8d63b1763/output/arm64-apple-darwin/bitcoin-bde8d63b1763-arm64-apple-darwin.tar.gz
28917d61e7101d04d5c7263f6127db817abf9895d89f7fcc89766f34eadb199e guix-build-bde8d63b1763/output/dist-archive/bitcoin-bde8d63b1763.tar.gz
638512416661c7a40bbe2c9dd5239004bbcb0e4a33ce3602252d1e962384fad5 guix-build-bde8d63b1763/output/powerpc64-linux-gnu/SHA256SUMS.part
cbe937cb603b53bb8c762d9466942bcf718e931952a8abbd4cea6ffad1c9b465 guix-build-bde8d63b1763/output/powerpc64-linux-gnu/bitcoin-bde8d63b1763-powerpc64-linux-gnu-debug.tar.gz
7b9122d4d0be2e5fc2cd6ce4a4c9b113c9cde4f3414ecd2702ffd9d13b8156fb guix-build-bde8d63b1763/output/powerpc64-linux-gnu/bitcoin-bde8d63b1763-powerpc64-linux-gnu.tar.gz
6a6a63f4dff48bae35b89b799c97166d1f62c669c023c7e38da7109ec7f1223a guix-build-bde8d63b1763/output/powerpc64le-linux-gnu/SHA256SUMS.part
cbe2f744280720bcf4a470386aeacba7fccb68e4d95dfd8920635c4be43b4b10 guix-build-bde8d63b1763/output/powerpc64le-linux-gnu/bitcoin-bde8d63b1763-powerpc64le-linux-gnu-debug.tar.gz
5afb328262a13ec03cb2980e19eccb439518602e00a7ba2b86d719036dec2658 guix-build-bde8d63b1763/output/powerpc64le-linux-gnu/bitcoin-bde8d63b1763-powerpc64le-linux-gnu.tar.gz
facc6df736e6b6c2b7d38e13f39c89dd579541cfa49548ac94b6793d5dc3110f guix-build-bde8d63b1763/output/riscv64-linux-gnu/SHA256SUMS.part
5b5e2423891191531498de4047f0d55946350b40f1792be52c7e68b0a954e3d4 guix-build-bde8d63b1763/output/riscv64-linux-gnu/bitcoin-bde8d63b1763-riscv64-linux-gnu-debug.tar.gz
44bb66d007fccbd4400a4fe275e381ad04adf6b85ff5a98ec385ce135312a1b1 guix-build-bde8d63b1763/output/riscv64-linux-gnu/bitcoin-bde8d63b1763-riscv64-linux-gnu.tar.gz
fd4efa0cf1a66e28e890d2d3c5149936f7f0d9d7a2a945ef4f90814b695da024 guix-build-bde8d63b1763/output/x86_64-apple-darwin/SHA256SUMS.part
b1b8048236cbe2190e3a11466649635e7e264564e9efbc669636834c18a69e1b guix-build-bde8d63b1763/output/x86_64-apple-darwin/bitcoin-bde8d63b1763-x86_64-apple-darwin-unsigned.tar.gz
4fa00d6a8a9b86718bcc5d18dfc9ef6641f42a4a6127f49bbc209f26622276df guix-build-bde8d63b1763/output/x86_64-apple-darwin/bitcoin-bde8d63b1763-x86_64-apple-darwin-unsigned.zip
de5446b26cf0c5b6b6a6d1415699a2a08fcd8805a4db2dbf02a5f8618e96fc76 guix-build-bde8d63b1763/output/x86_64-apple-darwin/bitcoin-bde8d63b1763-x86_64-apple-darwin.tar.gz
2821e45438e71d7a9c57813697997007ef8cc9a0e61d0692b92710221250b9cf guix-build-bde8d63b1763/output/x86_64-linux-gnu/SHA256SUMS.part
86d97c023d9c4ca9c0922fe95ea900b06072118fd0d8e08078d773e2bdda0e32 guix-build-bde8d63b1763/output/x86_64-linux-gnu/bitcoin-bde8d63b1763-x86_64-linux-gnu-debug.tar.gz
162c3b42fd842b58b07ba06f2eeac2ffa2b33a59a9cec2e3cb0a73419aeca960 guix-build-bde8d63b1763/output/x86_64-linux-gnu/bitcoin-bde8d63b1763-x86_64-linux-gnu.tar.gz
ea477b6f88f81d598feb8f167edc838d90af7b1b5ea286e2ba9c41f6c247a753 guix-build-bde8d63b1763/output/x86_64-w64-mingw32/SHA256SUMS.part
7a766bfd6d6946c2a1b13442f735e330b0c334cbb0808595695229d4e1536830 guix-build-bde8d63b1763/output/x86_64-w64-mingw32/bitcoin-bde8d63b1763-win64-debug.zip
f317dfd6763882331067be9b27de9186abfb581436c74d580651d9fd847dc2df guix-build-bde8d63b1763/output/x86_64-w64-mingw32/bitcoin-bde8d63b1763-win64-setup-unsigned.exe
0c5bf7ab53dfa88780ce92dd5e801bf9ca7653849c01085c865fdaad9f6ad999 guix-build-bde8d63b1763/output/x86_64-w64-mingw32/bitcoin-bde8d63b1763-win64-unsigned.tar.gz
b114c4d87a7a405c9f66e634e95348596906e2f3098c93c6f4b90eb450eafeed guix-build-bde8d63b1763/output/x86_64-w64-mingw32/bitcoin-bde8d63b1763-win64.zip
Code review ACK bde8d63b17637c507a543cebe90f2998b5847373. Only changes since last review were reverting the native_capnp change as suggested, and changing the order of the first two commits.
<!--9cd9c72976c961c55c7acef8f6ba82cd-->