-
Notifications
You must be signed in to change notification settings - Fork 38.7k
build: Fix libmultiprocess cross-compiling to Linux hosts
#25046
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
Conversation
To successfully call the `capnp_generate_cpp()` function, the `libmultiprocess` build system must be provided with paths to the native `capnp` and `capnpc-c++` tools.
make[2]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/src'
Makefile:20771: /tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/native/include/mpgen.mk: No such file or directory
make[2]: *** No rule to make target '/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/native/include/mpgen.mk'. Stop.
make[2]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/src'
make[1]: *** [Makefile:922: distdir-am] Error 1 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Thanks for picking this up @hebasto. I spend a day looking into this issue #24387 (comment) but got too confused by what I was seeing. Taking a quick look at this, first commit passing executable paths to libmultiprocess makes sense. Second commit dropping native_libmultiprocess at first thought does not seem to make sense. Third commit optimizing native builds seems ok, but perhaps not worth added complexity. For the second commit, I wouldn't think you could drop the native_libmultiprocess package. The libmultiprocess & capnp packages are both fat packages producing multiple outputs which are sometimes unnecessary. A less wasteful capnp packaging would be divided in two parts, with one package to build command-line tools So I don't understand how the second commit dropping Questions / Recommendations for this PR
|
|
Thank you for your review.
Dropped second and third commits. |
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK c0f5cc1
Thanks for fixing this
| $(package)_cmake_opts := -DCAPNP_EXECUTABLE="$$(native_capnp_prefixbin)/capnp" | ||
| $(package)_cmake_opts += -DCAPNPC_CXX_EXECUTABLE="$$(native_capnp_prefixbin)/capnpc-c++" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "build: Fix libmultiprocess cross-compiling to Linux hosts" (c0f5cc1)
Note (just for understanding): This overrides the capnp tool paths used by the cross-compiled libmultiprocess build, so it will run native capnp tools, instead of trying to run cross compiled ones found by find_package(CapnProto). The find_package line in the libmultiprocess build is only intended to find the cross-compiled capnp library paths, but it also finds the cross-compiled tool paths and will use them if they are not overridden.
There's some documentation about overriding these two variables: https://github.com/capnproto/capnproto/blob/55a368beb987abf9eeb9b3843e9c5423ad37ab29/c%2B%2B/cmake/CapnProtoConfig.cmake.in#L18-L22
| $(package)_download_path=$(native_$(package)_download_path) | ||
| $(package)_file_name=$(native_$(package)_file_name) | ||
| $(package)_sha256_hash=$(native_$(package)_sha256_hash) | ||
| $(package)_dependencies=native_$(package) capnp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "build: Fix libmultiprocess cross-compiling to Linux hosts" (c0f5cc1)
In a different PR, could probably drop native_$(package) as a dependency here. The cross compiled libmultiprocess packages doesn't actually depend on native libmultiprocess package for anything. (This is different from the cross compiled capnp which does depend on the native capnp for codegen tools used during cross-compilation).
The depends system exists as a middle ground between requiring Guix to build Bitcoin core, and being completely reliant on system packages, neither of which are ideal / usable for everyone scenarios.
This is fine unless your package manger has versions of our dependencies that are too old, broken in some way, or just not packaged. Package managers also have a tendency to patch their sources, and those patches may undermine some assumption we make, or subtly break things. Having the ability to compile using unpatched (or only our patches) source is nice. Package manger vendored libraries are also no-doubt configured / compiled (for better or worse) differently to how we build libraries in depends. I think for self-compiling end users, who may want to customize their builds slightly (i.e no port forwarding libs, no wallet, additional compiler flags etc), but otherwise want to compile a Bitcoin Core as close as possible to how we build releases, depends is very useful. Depends is also very convenient for cross-compilation. If we delete depends tomorrow, and I'm using a Debian machine, and want to cross-compile a deployable Bitcoin Core for macOS (with all optional features + gui). What would be your recommended way of doing so? It's probably going to end up being something like what depends would have been doing anyways, but I'm just going to have to put everything together manually, rather than running Having a single source of truth, that defines the dependencies required to build Bitcoin Core, and how they are patched / compiled, which isn't reliant on other systems (i.e guix / nix) or tooling (other than, fairly basic compiler + utls + make etc), I think, is important, and, if for some reason we had to move away from Guix tomorrow, we could take depends, plug it into a different setup/environment that provides reproducibility, and just carry on. |
|
Thanks @fanquake! I didn't realize is that GUIX isn't actually available on macos. I assumed guix was like nix could easily be installed everywhere. I also didn't realize our GUIX build doesn't really define packages. It just calls
Presumably guix has an option to cross compile packages. In nix it is something like
The depends system just seems like a bootleg version of nix or guix to me with nonstandard package definitions that are fragile painful to debug[*]. So I don't know why anybody would use depends if there were guix package definitions easily available somewhere. This seems like it is not the case, and guix itself is not very portable, and guix may be generally less useful as a package manager for development than nix (https://nix.dev/). [*] My main problem debugging depends packages is It seems to love immediately deleting stuff after building it. So if anything isn't built correctly I have to wait for a slow rebuild and figure out some incantation that will do a partial build keeping the output I want to look at. I was waiting for GUIX to solve this but I guess the scope of our GUIX build is just to provide a build environment, not be a development package manager. |
Yes, that is possible. Although just like NixOS, it would produce libraries and executables that expects a loader and libs under /gnu/store just like /nix/store.
Agreed! Although to be clear there's nothing about Guix that makes it inherently that makes it less portable, it's just that no one has done the work yet. Our usecase for Guix is specifically has a bootstrappable and reproducible release builder. And it works very well for that. NixOS doesn't have a good bootstrapping story so far (although nothing about NixOS makes it inherently unable to be bootstrapped), and we will see how that evolves.
Have you tried: |
This is up to the package definitions, no? The autoconf/cmake wrappers nix and guix provide would build everything with /nix/store or /gnu/store prefixes by default, but we wouldn't call these. The guix packages should be able to perform the same build steps as depends packages, calling ./configure manually with all same arguments and outputs. The package outputs would physically be stored in /gnu/store, but that is no different than depends system physically storing outputs in
Fair enough. Nix definitely has its warts as well. But I was surprised to see guix wasn't trying to compete as as a portable package manager and development tool the same way https://nix.dev/ wants to (not to mention other package managers like vcpkg, conan, spack, etc) But in general it does seem not feasible to get rid of depends packages and replace them with guix packages if guix doesn't run on a major platform like macos.
Yes, but I forgot to mention my other pain point with the depends system, which is that every time I made a change to |
|
While I am listing my grievances against the depends system, another thing I remain confused by is the overall way it handles cross compilation and distinguishes cross options tools and options from native tools and options, and why it makes you define different native and cross-compiled versions of the same package, when other package managers let you build any package with any set of tools. |
|
I guess on the plus side, if we want to minimize dependencies, maybe having the depends system make it painful to add dependencies is a good thing! |
I'm all for minimizing dependencies to the point where needing a package manager becomes unnecessary. |
…Linux hosts c0f5cc1 build: Fix `libmultiprocess` cross-compiling to Linux hosts (Hennadii Stepanov) Pull request description: To successfully call the [`capnp_generate_cpp()`](https://github.com/chaincodelabs/libmultiprocess/blob/d576d975debdc9090bd2582f83f49c76c0061698/CMakeLists.txt#L45) function, the `libmultiprocess` build system must be provided with paths to the native `capnp` and `capnpc-c++` tools. This [comment](bitcoin#24387 (comment)) points the same: > I think `packages/libmultiprocess.mk` probably needs to be passing a `-DCAPNP_EXECUTABLE=.../depends/arm-linux-gnueabihf/native/bin/capnp` argument to cmake. Also the package should have dependencies on both `capnp` and `native_capnp`. Fixes bitcoin#24387. ACKs for top commit: ryanofsky: Code review ACK c0f5cc1 Tree-SHA512: 2986d8bf98d2761eceba21b1897145c5185a0922d4c2084e8812d4d07dc94237e5c2809036641c4f7c491a3414727fff328cba91ce138b89e37ec5cba61d8f61
Summary: This is a backport of [[bitcoin/bitcoin#19685 | core#19685]], [[bitcoin/bitcoin#20046 | core#20046]] and [[bitcoin/bitcoin#25046 | core#25046]] (with a contribution from bitcoin/bitcoin@603fd6a#diff-04ed828869a0888df31a01e355417d994dcf2f3a0b3a35b82c7510ebcd294943) Note the use of `ninja` (introduced in some mk files in D865 and D13222) Test Plan: CI builds (gitian) Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13506
To successfully call the
capnp_generate_cpp()function, thelibmultiprocessbuild system must be provided with paths to the nativecapnpandcapnpc-c++tools.This comment points the same:
Fixes #24387.