ci: build multiprocess on most jobs #30975
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30975. ReviewsSee 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. 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. |
|
I'll look into the linter once it's clear if the |
|
If multiprocess is (currently) unsupported on Windows, then that should be fixed in depends, i.e (for now) don't even try building the packages/functionality for that platform, not leaked into Guix with a workaround. More generally, if the project decides to start shipping multiprocess as part of it's releases, then there's a number of other things that need to happen first. We should be switching multiprocess from an opt-in in depends, to being built/used by default, as well as testing all of this, on all relevant platforms in our CIs, not just enabling it for some platforms as part of the release process. There are also documentation/website/release process updates that will be needed, as this change is going to produce a release tarball, which will contain: bitcoin-cli
bitcoin-gui
bitcoin-node
bitcoin-qt
bitcoin-tx
bitcoin-util
bitcoin-wallet
bitcoind
test_bitcoinwith no explanation of why there is
ls bitcoin-28.0rc2/bin
bitcoin-cli
bitcoin-qt
bitcoin-tx
bitcoin-util
bitcoin-wallet
bitcoind
test_bitcoin |
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
I would also prefer it if Another approach, if touching the
I'll look into that.
I'll add some commits here to do these things. Those can be split into separate PRs later for a more incremental approach.
Ah, I either misremembered or things changed. But in #19460 it's not / no longer the plan to have |
|
It's good to have this PR open as a draft to see what it looks like but it sounds like there are a number of followups that need to happen for it to be ready. For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term. It would also be good to review and merge #30940 which this is currently based on. |
8b2546c to
df97390
Compare
|
I moved the Windows build skip workaround from Guix to depends. This made me excited about the prospect of moving that bit of Autotools stuff to CMake as well. :-) Made some minimal documentation changes. I didn't change the non-depends build (yet). All CI instances that use depends now use multiprocess, the others don't (yet). Let's see if that breaks anything... |
|
From CI: |
Update: currently working on getting mingw32 build working, and I created issue #30983 to lay out different multiprocess release options. |
ci_container_base/src/ipc/capnp/mining.cpp -DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES
In file included from /ci_container_base/src/ipc/capnp/mining.cpp:5:
In file included from /ci_container_base/src/ipc/capnp/mining-types.h:9:
In file included from /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/common.capnp.proxy-types.h:6:
In file included from /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/common.capnp.proxy.h:7:
In file included from /ci_container_base/depends/x86_64-pc-linux-gnu/include/mp/proxy.h:8:
/ci_container_base/depends/x86_64-pc-linux-gnu/include/mp/util.h:207:20: error: no template named 'function' in namespace 'std'; did you mean 'kj::Function'?
207 | using FdToArgsFn = std::function<std::vector<std::string>(int fd)>;
| ^~~~~
/ci_container_base/depends/x86_64-pc-linux-gnu/include/kj/exception.h:34:29: note: 'kj::Function' declared here
34 | template <typename T> class Function;
| ^
1 error generated.TSAN compile failure fixed in bitcoin-core/libmultiprocess#113. |
df97390 to
9cd4666
Compare
|
Rebased after #30940 |
9cd4666 to
3ef2a7c
Compare
3ef2a7c to
43c8866
Compare
|
Rebased after #30043.
Added a commit to bump libmultiprocess to include this (can be its own PR later). |
|
MSAN says https://cirrus-ci.com/task/6248232848719872: |
|
Thanks! I created bitcoin-core/libmultiprocess#115 with details about this, and I think I have a potential fix. |
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, the "macOS 14 native no depends job" fails due to warnings in boost headers treated as errors, reported in bitcoin#30975 (comment) and bitcoin-core/libmultiprocess#138
This change is technically not needed to add 8000 libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, clang-tidy job fails due to missing generated example files as reported bitcoin#30975 (comment) Different fixes were suggested bitcoin#30975 (comment) and bitcoin#30975 (comment) Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
0f61a2b to
7c96e38
Compare
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, several jobs fail due to the mptest executable not being built by default, as reported bitcoin#30975 (comment) Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, the "macOS 14 native no depends job" fails due to warnings in boost headers treated as errors, reported in bitcoin#30975 (comment) and bitcoin-core/libmultiprocess#138
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, clang-tidy job fails due to missing generated example files as reported bitcoin#30975 (comment) Different fixes were suggested bitcoin#30975 (comment) and bitcoin#30975 (comment) Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitco 6D40 in#30975 which enables multiprocess build option in more CI jobs. In that PR, several jobs fail due to the mptest executable not being built by default, as reported bitcoin#30975 (comment) Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, the "macOS 14 native no depends job" fails due to warnings in boost headers treated as errors, reported in bitcoin#30975 (comment) and bitcoin-core/libmultiprocess#138
This change is technically not needed to add libmultiprocess as a subtree, but it avoids a CI failure in followup PR bitcoin#30975 which enables multiprocess build option in more CI jobs. In that PR, clang-tidy job fails due to missing generated example files as reported bitcoin#30975 (comment) Different fixes were suggested bitcoin#30975 (comment) and bitcoin#30975 (comment) Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
7c96e38 to
369da6a
Compare
|
(not rebasing yet, CI already ran on the latest base PR push) |
Except: 1. i686, DEBUG (changed to "no multiprocess") 2. Windows due to lack of support
Install capnp where needed. The bitcoin-node binary is built on all platforms which have multiprocess enabled, but for functional tests it's only used in the macOS native (no depends) and CentOS native (depends) jobs.
369da6a to
747af17
Compare
There was a problem hiding this comment.
No, objection, but I wonder what the goal of the changes here is.
In the OP you mention that it is too early to enable it by default in depends, so to me it seems better to just have CI follow when this is turned on by default in depends.
However, in #31802 (comment) you claim that this will be turned on by default for devs as well.
It would seem like useless churn if most of the changes here would be reverted soon after anyway, which is why I am asking.
|
@maflcko #31802 turns IPC on in the guix build and by default for devs. But even (especially?) if it's not turned on by default, it's good to have CI coverage.
This is because turning it on in depends turns it on in guix and ships it in the next release. That seems better to decide in the next PR.
I could do that here as well, but prefer to keep this PR limited to CI changes. Also it's a bit weird to turn it on for developers only if they don't use depends. (in other words, the coupling between depends defaults and guix builds is a bit annoying) |
|
Code review ACK 747af17 I think I agree with marco, and it might be simpler to just close this PR and turn on ENABLE_IPC by default in ci, in depends, and in cmake all in a single PR instead of turning it on in CI first, which makes CI builds and default builds less consistent with each other and creates churn in the CI configuration files. I can also see the other point of view that that a change to enable IPC in releases is a bigger decision than enabling IPC in CI and developer builds, so maybe that should have a dedicated PR. I think my preference would depend on how soon we think #31802 should be merged. If pretty soon, the simplest thing would be to close this PR and just update cmake default, depends default, and CI all together in #31802. But if we want to wait more time before merging #31802, it would be helpful to have ENABLE_IPC turned on in CI earlier so there's better CI testing for other IPC/multiprocess PR's being worked on. |
|
I've marked #31802 as ready for review. It includes all the commits here. But I'll leave this open for now. |
|
Since I reordered the commits in #31802 it's now a non-trivial rebase, so I'm closing this. |
This builds most of the CI jobs with multiprocess enabled.
Two jobs run the functional test suite, one with depends and one with the regular build.
The Windows job does not use multiprocess since that's not supported yet.
The previous version of this PR changed the
dependssystem to build libmultiprocess by default as one of the final steps for #31098 - but based discussion here and on IRC it was decided it's too early to ship it. This now happens in the followup #31802.Based on #31741