build, depends: Fix `libmultiprocess` cross-compilation #29665

hebasto · 2024-03-16T15:19:50Z

On the master branch @ 3b12fc7, the following command fails:

$ make -C depends libmultiprocess HOST=arm64-apple-darwin MULTIPROCESS=1
...
[100%] Linking CXX executable mpgen
...
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
...

This PR prevents building all default targets that include mpgen, which expectedly fails to link when cross-compiling.

DrahtBot · 2024-03-16T15:19:53Z

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, fanquake

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

hebasto · 2024-03-16T15:20:03Z

cc @ryanofsky

hebasto · 2024-03-16T15:54:24Z

As an alternative, the mpgen target might be disabled when cross-compiling in the upstream build system.

4e70ad4 cmake, refactor: Rename target export files (Hennadii Stepanov) 694b6b1 cmake: Configure `LibmultiprocessGen` package (Hennadii Stepanov) 3b20e35 cmake: Configure `Libmultiprocess` package (Hennadii Stepanov) Pull request description: This PR adds configurations for the `Libmultiprocess` and `LibmultiprocessGen` packages. The Bitcoin Core branch, with uses this PR branch, is available [here](https://github.com/hebasto/bitcoin/tree/240329-cmake-CI-mpgen). As a suggestion for a follow-up, it seems worth disabling `mpgen` target for cross-compiling (see bitcoin/bitcoin#29665 (comment) and CMake [docs](https://cmake.org/cmake/help/book/mastering-cmake/chapter/Cross%20Compiling%20With%20CMake.html#running-executables-built-in-the-project)). Based on #95. ACKs for top commit: ryanofsky: 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. Tree-SHA512: 231d00fd3b03bff40d010d39636a4c6b26e071a1d2c220f204ef536d6f3f7b3c81d2d05a16e3bb23f405a45e2b763cd88344cabd9eb407f1967b766b328815a4

ryanofsky

Code review ACK 178065f

Thanks for the fix. I don't actually understand why the fix is needed, when this was broken, or why mpgen linking should fail when it is cross compiled. That seems like an upstream bug and it would be nice to have more information about it, though the workaround here seems acceptable and has some nice features like being more efficient and using separate source/build directories.

There are some things I don't like about the workaround, but I don't think they are too important:

I don't think it's great that is now hardcoding multiprocess and mpgen target names into the build script instead of relying on the fact that install-lib and install-bin targets will automatically build the targets that are needed. For example now if an another helper library is added upstream or one of the targets is renamed, the install commands will fail because the right targets will not have been built.
I think it would be better if the build and install logic remained upstream instead of being moved into the depends build. If it is important to be able to separate build and install steps, we could add new "bin" and "lib" targets upstream to complement existing "install-bin" and "install-lib" targets.
I think it would be nice if other cleanups in this PR were separate commits so all the changes would be easy to understand.
I don't understand the "Rebuilding during installation has been skipped." comment, because I would not think anything would have been actually been built during the install step if make was just run previously.

But again these are all minor concerns and this looks good to merge in its current form (probably with 1 other ACK)

hebasto · 2024-04-02T18:45:34Z

I don't understand the "Rebuilding during installation has been skipped." comment, because I would not think anything would have been actually been built during the install step if make was just run previously.

Consider the following commands in the libmultiprocess root directory:

$ cmake .
$ make
$ make DESTDIR=/home/hebasto/INSTALL install-lib
Consolidate compiler generated dependencies of target util
[ 20%] Built target util
Consolidate compiler generated dependencies of target multiprocess
[100%] Built target multiprocess
-- Install configuration: ""
-- Install component: "lib"
-- Installing: /home/hebasto/INSTALL/usr/local/lib/libmultiprocess.a
-- Installing: /home/hebasto/INSTALL/usr/local/include/mp/proxy.capnp.h
-- Installing: /home/hebasto/INSTALL/usr/local/include/mp/proxy-io.h
-- Installing: /home/hebasto/INSTALL/usr/local/include/mp/proxy-types.h
-- Installing: /home/hebasto/INSTALL/usr/local/include/mp/proxy.h
-- Installing: /home/hebasto/INSTALL/usr/local/include/mp/util.h
-- Installing: /home/hebasto/INSTALL/usr/local/lib/pkgconfig/libmultiprocess.pc
-- Installing: /home/hebasto/INSTALL/usr/local/lib/cmake/libmultiprocess-lib.cmake
-- Installing: /home/hebasto/INSTALL/usr/local/lib/cmake/libmultiprocess-lib-noconfig.cmake
[100%] Built target install-lib

From the build log it follows that the util and multiprocess targets were being rebuilt.

That's not the case when using cmake --install ... command.

ryanofsky · 2024-04-02T19:04:26Z

From the build log it follows that the util and multiprocess targets were being rebuilt.

I don't think that's true. I see lines like Building CXX object CMakeFiles/util.dir/src/mp/util.cpp.o and Linking CXX static library libmultiprocess.a when targets are actually being built and lines like Built target util and Built target multiprocess when targets were previously built. Running make install-bin or make install-lib after building shouldn't cause anything to be rebuilt unless sources have changed. So I think the comment "Rebuilding during installation has been skipped" might be inaccurate, and that you could revert the change switching "make install-lib" and "make install-bin" to cmake --install --component ..., although neither of these things are too important.

This change prevents building all default targets that include `mpgen`, which expectedly fails to link when cross-compiling.

hebasto · 2024-04-02T19:28:18Z

OK. I've dropped all unrelated / unneeded changes.

ryanofsky

Code review ACK 2de2ea2

Maybe @fanquake would be a good second reviewer

OK. I've dropped all unrelated / unneeded changes.

That's reasonable, and the change is very simple now. To be clear, I wasn't asking for anything to be reverted, and I'm fine with any version of this PR. I'm mostly curious to know why a link error even happens at all. But I guess can download the mac sdk to experiment. Thanks for the fix and information about this.

fanquake

ACK 2de2ea2 - I checked that this fixes the macOS cross-compilation issue. I'm assuming these packages are also likely to change further in the (near) future, given the changes going in upstream: https://github.com/chaincodelabs/libmultiprocess/pulls?q=is%3Apr+is%3Aclosed.

I don't actually understand why the fix is needed, when this was broken, or why mpgen linking should fail when it is cross compiled.

I agree with Russ that it'd be better to actually figure out what the root cause is, and fix it more directly, rather than working around it just to make things compile.

6c64fc2 fixup! ci: Test CMake edge cases (Hennadii Stepanov) c24647c cmake: Add `MULTIPROCESS` option (Hennadii Stepanov) aae82b3 depends: Bump libmultiprocess source (Hennadii Stepanov) Pull request description: Also a new "Linux 64-bit, multiprocess" CI task has been added. Also see: bitcoin#29665. Top commit has no ACKs. Tree-SHA512: c03255070fb843ff37e8997cf2813f10380925fb2a5b7178e5f0516cc4d829700ff334e18066265b246d87f7ba1a20472bb0638608983d11a74b6d68dd9c4306

…ilation 2de2ea2 build, depends: Fix `libmultiprocess` cross-compilation (Hennadii Stepanov) Pull request description: On the master branch @ 3b12fc7, the following command fails: ``` $ make -C depends libmultiprocess HOST=arm64-apple-darwin MULTIPROCESS=1 ... [100%] Linking CXX executable mpgen ... clang++: error: linker command failed with exit code 1 (use -v to see invocation) ... ``` This PR prevents building all default targets that include `mpgen`, which expectedly fails to link when cross-compiling. ACKs for top commit: ryanofsky: Code review ACK 2de2ea2 fanquake: ACK 2de2ea2 - I checked that this fixes the macOS cross-compilation issue. I'm assuming these packages are also likely to change further in the (near) future, given the changes going in upstream: https://github.com/chaincodelabs/libmultiprocess/pulls?q=is%3Apr+is%3Aclosed. Tree-SHA512: 563551afbe483c923b52c6171f9d73bcc30bc4febd821b5abfe8aadb2ac601b94c2d10a73746ace3710d9f0afa4798eb090e77ccb1ae66a819495912802d91c9

fbc0bce Merge bitcoin#29665: build, depends: Fix `libmultiprocess` cross-compilation (fanquake) 59a2145 Merge bitcoin#29747: depends: fix mingw-w64 Qt DEBUG=1 build (fanquake) bdb2fa6 Merge bitcoin#29671: index: avoid "failed to commit" errors on initialization (Ava Chow) 65377ea Merge bitcoin#29192: Weaken serfloat tests (fanquake) 4122404 Merge bitcoin#28891: test: fix `AddNode` unit test failure on OpenBSD (fanquake) 812ef53 Merge bitcoin#25677: refactor: make active_chain_tip a reference (MacroFake) 34e12fa Merge bitcoin#24564: doc: Clarify that CheckSequenceLocksAtTip is a validation function (glozow) d529751 Merge bitcoin#24788: doc: Add gpg key import instructions for Windows (fanquake) 61bae78 Merge bitcoin#24784: refactor: deduplicate integer serialization in RollingBloom benchmark (MarcoFalke) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK fbc0bce Tree-SHA512: 731f70b747712810d4f74fe64a90139b02ddb62e9ac260705fa2588595feb19bd6e5ffed521fd878bacaab0015683e582fed19ed1855c3e955f93cd223862a17

# Conflicts: # CONTRIBUTING.md # doc/dnsseed-policy.md

hebasto mentioned this pull request Mar 16, 2024

cmake: Introduce packages bitcoin-core/libmultiprocess#96

Merged

hebasto mentioned this pull request Mar 30, 2024

cmake: Add MULTIPROCESS option hebasto/bitcoin#118

Merged

ryanofsky approved these changes Apr 2, 2024

View reviewed changes

build, depends: Fix libmultiprocess cross-compilation

2de2ea2

This change prevents building all default targets that include `mpgen`, which expectedly fails to link when cross-compiling.

hebasto force-pushed the 240316-mp branch from 178065f to 2de2ea2 Compare April 2, 2024 19:26

ryanofsky approved these changes Apr 2, 2024

View reviewed changes

DrahtBot added the CI failed label Apr 2, 2024

fanquake approved these changes Apr 3, 2024

View reviewed changes

fanquake merged commit 5aff45a into bitcoin:master Apr 3, 2024

hebasto deleted the 240316-mp branch April 3, 2024 16:25

Pttn added a commit to Pttn/Bitcoin that referenced this pull request Jan 10, 2025

Merge bitcoin#29665

3ff81bd

# Conflicts: # CONTRIBUTING.md # doc/dnsseed-policy.md

bitcoin locked and limited conversation to collaborators Apr 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build, depends: Fix `libmultiprocess` cross-compilation #29665

build, depends: Fix `libmultiprocess` cross-compilation #29665

Uh oh!

hebasto commented Mar 16, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Mar 16, 2024 •

edited

Loading

Uh oh!

hebasto commented Mar 16, 2024

Uh oh!

hebasto commented Mar 16, 2024

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

hebasto commented Apr 2, 2024

Uh oh!

ryanofsky commented Apr 2, 2024

Uh oh!

hebasto commented Apr 2, 2024

Uh oh!

ryanofsky left a comment

Uh oh!

fanquake left a comment

Uh oh!

Uh oh!

build, depends: Fix libmultiprocess cross-compilation #29665

build, depends: Fix libmultiprocess cross-compilation #29665

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DrahtBot commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

hebasto commented Mar 16, 2024

Uh oh!

hebasto commented Mar 16, 2024

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!

hebasto commented Apr 2, 2024

Uh oh!

ryanofsky commented Apr 2, 2024

Uh oh!

hebasto commented Apr 2, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

build, depends: Fix `libmultiprocess` cross-compilation #29665

build, depends: Fix `libmultiprocess` cross-compilation #29665

hebasto commented Mar 16, 2024 •

edited

Loading

DrahtBot commented Mar 16, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading