Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

depends: fix libmultiprocess build on aarch64 #28846

Merged
merged 3 commits into from Dec 13, 2023

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Nov 10, 2023

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 chaincodelabs/libmultiprocess#79 upstream.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 10, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ryanofsky
Stale ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member

hebasto commented Nov 10, 2023

Concept ACK.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK c3a962b. 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.

depends/packages/libmultiprocess.mk Show resolved Hide resolved
@@ -10,6 +10,7 @@ endif

define $(package)_set_vars :=
$(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/
$(package)_config_opts += -DCMAKE_POSITION_INDEPENDENT_CODE=ON
Copy link
Contributor

@ryanofsky ryanofsky Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "depends: build libmultiprocess with position independant code" (c3a962b)

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.

I guess I have 3 questions:

  1. Is this workaround still needed if you add --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?

$(package)_config_opts_android := --disable-shared

  1. Opposite question: is the android --disable-shared workaround needed anymore if -DCMAKE_POSITION_INDEPENDENT_CODE=ON is used here? Maybe we could partially revert build: Fix capnp package build for Android #25322 after this?

  2. Is this workaround still needed with depends: Build the native_capnp and capnp packages with CMake #28856? That PR seems to drop the android workaround, so maybe switching both of these libraries to cmake would just fix the problem automatically.

Footnotes

  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.

Copy link
Member Author

@fanquake fanquake Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this workaround still needed with

I think this can be deffered for #28856. I haven't tested everything, but with that change, we seem to avoid the issues here. I'll close this for now in favour of that change, and properly review shortly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Yes, the switch to CMake in #28856 didn't fix this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #28846 (comment)

In commit "depends: build libmultiprocess with position independant code" (965d38d)

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fanquake fanquake closed this Nov 30, 2023
@fanquake fanquake deleted the fixup_multiprocess_arm64 branch November 30, 2023 15:14
@fanquake fanquake restored the fixup_multiprocess_arm64 branch December 4, 2023 11:53
@fanquake fanquake reopened this Dec 4, 2023
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reopening, code review ACK 68823aa for just the third commit. This PR could be a draft since it is based on another PR.

depends/packages/libmultiprocess.mk Show resolved Hide resolved
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 1a90ac4, just adding comment since last review (thanks!)

@fanquake
Copy link
Member Author

fanquake commented Dec 5, 2023

Rebased after #28856.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6293a3f, tested on Fedora 37, aarch64.

@fanquake
Copy link
Member Author

fanquake commented Dec 5, 2023

ACK 6293a3f, 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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 6293a3f. No changes since last review other than rebasing after #28856 merge

@fanquake
Copy link
Member Author

fanquake commented Dec 6, 2023

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).

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 965d38d. Left a suggestion to simplify the second commit, but it is not important and this change looks good as.

define $(package)_set_vars
$(package)_config_opts := -DBUILD_TESTING=OFF
$(package)_config_opts += -DWITH_OPENSSL=OFF
$(package)_config_opts += -DWITH_ZLIB=OFF
$(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "depends: always install capnp to /lib" (6d7e71e)

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have dropped this change.

@@ -10,6 +10,7 @@ endif

define $(package)_set_vars :=
$(package)_config_opts += -DCMAKE_INSTALL_LIBDIR=lib/
$(package)_config_opts += -DCMAKE_POSITION_INDEPENDENT_CODE=ON
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #28846 (comment)

In commit "depends: build libmultiprocess with position independant code" (965d38d)

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.

fanquake and others added 3 commits December 12, 2023 13:58
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
chaincodelabs/libmultiprocess#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 bitcoin#26943.
@fanquake
Copy link
Member Author

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DrahtBot
Copy link
Contributor

Guix builds (on x86_64)

File commit a7484be
(master)
commit 900a6fd
(master and this pull)
SHA256SUMS.part dc61ebdbca4d4c20... 4758cc98450bc6b5...
*-aarch64-linux-gnu-debug.tar.gz 23c119c0f6b1b49d... e26a62c280b795e9...
*-aarch64-linux-gnu.tar.gz 3b4c3137af8c2140... 5cab076e93a97e46...
*-arm-linux-gnueabihf-debug.tar.gz 4a04c5d1e5bdee34... b84e4d6294c90cbe...
*-arm-linux-gnueabihf.tar.gz 218e0d6e7a930111... c7e43085a68b0101...
*-arm64-apple-darwin-unsigned.tar.gz 96396cdd0858682a... 8d16e2da9021178f...
*-arm64-apple-darwin-unsigned.zip e9b5ad6be95b4189... ce67efb59127a4df...
*-arm64-apple-darwin.tar.gz b2a93626e607157c... 2b4365c4159cb2ba...
*-powerpc64-linux-gnu-debug.tar.gz dc8c9116c47ac1dd... eaf5b668d23c4bc0...
*-powerpc64-linux-gnu.tar.gz 3051947e764e94f2... 7aa18ca756a51dc3...
*-powerpc64le-linux-gnu-debug.tar.gz 31aba8992ab979fd... c03c65694b15868b...
*-powerpc64le-linux-gnu.tar.gz 9d302d71bf5f6b7b... c0e8456a6c2f9be2...
*-riscv64-linux-gnu-debug.tar.gz 5318802eac26a4b0... da030d659f1ae67f...
*-riscv64-linux-gnu.tar.gz 34743dd8039dcf8b... d6d0f303dc4f4289...
*-x86_64-apple-darwin-unsigned.tar.gz 15b74a6b0d11129d... 5b1859fd3f049ae4...
*-x86_64-apple-darwin-unsigned.zip 76001bb57b18424b... e3a7335e121888e4...
*-x86_64-apple-darwin.tar.gz 474cd048415e1ded... e713acde33ee6826...
*-x86_64-linux-gnu-debug.tar.gz f7fb07a847c2d2b7... 0aba3f51712aa3e2...
*-x86_64-linux-gnu.tar.gz 4075f314dc725cce... eea4e36e833c428f...
*.tar.gz 986a781a7719709d... 7c83c718c1c11872...
guix_build.log b12955749623ee00... f4971958a626c95c...
guix_build.log.diff ecafa1b5645c100a...

@fanquake fanquake merged commit 54f6756 into bitcoin:master Dec 13, 2023
16 checks passed
@fanquake fanquake deleted the fixup_multiprocess_arm64 branch December 13, 2023 10:44
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2024
We currently do this sporadically. Not only amongst packages, but across
OS's, i.e something it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.

See related discussion in
bitcoin#28846 (comment).
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2024
We currently do this sporadically. Not only amongst packages, but across
OS's, i.e something it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.

See related discussion in
bitcoin#28846 (comment).
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2024
We currently do this sporadically. Not only amongst packages, but across
OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.

See related discussion in
bitcoin#28846 (comment).
fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 14, 2024
We currently do this sporadically. Not only amongst packages, but across
OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.

See related discussion in
bitcoin#28846 (comment).
fanquake added a commit that referenced this pull request Mar 25, 2024
e037c4f depends: always configure with --with-pic (fanquake)

Pull request description:

  We currently do this sporadically. Not only amongst packages, but across OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

  Configure with `--with-pic` globally instead. I think this generally makes more sense, and should not have any downsides.

  See related discussion in #28846 (comment).

ACKs for top commit:
  hebasto:
    ACK e037c4f.

Tree-SHA512: efc743ff92f9f99f3ac16514e98363ad395c6f956cd4be7e785b5c573685baf7fcd68c51d6a705ee8761fc676eb045b7e61676595be0eb0f70f34e99174cddc0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants