Multiprocess build support

[ryanofsky]

This PR is part of the process separation project.

This splits autotools, depends build, and travis changes out of #10102, so code changes and build system changes can be reviewed separately.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• depends: update to Boost 1.72 #17941 (depends: update to Boost 1.72 by Sjors)
• util: add RunCommandParseJSON #15382 (util: add runCommandParseJSON by Sjors)

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.

[Sjors]

Adding capnp probably deserves it's own PR, but at least a separate commit.

What is native_boost?

On macOS, I looks like I need to do more than just brew install cmake. See flood of errors.

[ryanofsky]

On macOS, I looks like I need to do more than just brew install cmake. See flood of errors.

Thanks for the log! It looks like the error is:

#error "This code requires C++14. Either your compiler does not support it or it is not enabled."

So probably what's happening is that for whatever reason my compiler defaults to c++14, while your compiler defaults to an older c++ version. It looks like I could fix the libmultiprocess build to specify this:

https://cmake.org/cmake/help/latest/command/target_compile_features.html
https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES

An alternate approach might be to downgrade capnp from 0.7.0, since 0.7.0 is the first version that requires c++14, and I'm actually not relying on any 0.7.0 features.

I do need to try building and running this code again on mac soon, since I haven't tried it in a while.

[Sjors]

See also #13356 for C++14 discussion. However, as long as multiprocess is optional, I think it's more future proof to use the C++14, with the latest capnp version, and fix the macOS build.

[ariard]

OK, I succeed to build bitcoin-gui and bitcoin-node and bitcoin-wallet on Debian 9.

Main problem I had to build first libmultiprocess as a dependency and then pass its library directory with --with-libmultiprocess to ./configure which isn't documented in the multiprocess.md. Then to run bitcoin-node and bitcoin-wallet I had to add libcapnp-rpc-0.7.0.so in runtime search path. But it succeed and both are working successfully. Don't try to run bitcoin-gui as I was lacking more dependencies.

IMO you should update doc/build-unix.md with a multiprocess section which tell you exactly what to do step by step and requirements. And we should blindly follow these sections on Debian/Fedora/Gentoo and see if it's smooth enough for a user.

But overall, super excited to go forward with multiprocess !

[ryanofsky]

Thanks for testing, ariard! It sounds like you encountered bugs that you worked around, but I'd like to know more specifics about what wasn't working. I'm happy to help debug anytime, or if you want to work online, you can post issues to https://github.com/chaincodelabs/libmultiprocess/issues/new (general issues, bitcoin-specific issues, and support requests all welcome there). Regarding the specific workarounds:

• Passing a prefix directory to --with-libmultiprocess (or specifying it at all) shouldn't be necessary, and I actually removed the option to hardcode a prefix in some new changes I haven't pushed yet. The configure script should be able to locate dependencies through pkgconf. If that isn't happening please let me know the specifics or open an issue!

• Regarding documentation, configure --help text should serve as a reference for config options, and the --enable-multiprocess error messages should provide clear guidance if anything goes wrong. If the help text isn't helpful, I'd want to open an issue to track that. multiprocess.md and libmultiprocess README.md could definitely be improved, but for install steps I would mostly want to add to the existing install steps rather than add separate instructions. So far I've only really looked at the depends instructions, not the unix/mac/windows instructions, and I need to get to those at some point.

• The problem with library paths sounds like a bug. Rpaths should be set correctly so you shouldn't need to anything special at runtime.

[ariard]

For others reviewers, after talking with Russ, my mistake was not to use depends prefix as described in depends/README.md. Will give it another try after updates.

[ryanofsky]

re: http://gnusha.org/bitcoin-builds/2019-08-16.log

<dongcarl> cfields: I think the boostrap part of boost is actually "native", as in, should be moved to a boost_native package like the native part of protobuf was... Does that ring somewhat true to you?

@dongcarl this PR adds a native_boost package, but there should be no need for a native package just to use the bootstrap tool, unless you need to use the bootstrap tool from other packages, and not just use it internally in the boost package.

[fanquake]

I depends built, compiled and played around with this (macOS). Going to re-read all the discussion in #10102.

I'm also going to extract 7149b1a and a295b9d into a separate PR. They are clear improvements, and will likely clear up some confusion here and in other depends related PRs.

[theuni]

Uh, was this actually ready to go in? Have we determined for sure that this is the approach we want? Has libmultiprocess been well-reviewed?

Maybe I missed the discussion, but I thought this was still at the RFC stage.

[ryanofsky]

Uh, was this actually ready to go in? Have we determined for sure that this is the approach we want? Has libmultiprocess been well-reviewed?

Definitely not reviewed enough to be turned on. This PR is just adding options to build with it which are off by default and marked experimental.

[maflcko]

Have we determined for sure that this is the approach we want?

@theuni No, we haven't. Multiprocess is experimental and turned off by default. We have 6 months to come up with a better solution or revert it before the 0.21.0 release.

[theuni]

@MarcoFalke Well it requires c++17, so it won't fit into 0.21.0 with the schedule we've come up with iirc.

[maflcko]

0.21.0 releases are planned to be built with C++17, and the multiprocess build is disabled by default. Not to mention that this doesn't include a user-visible feature, so there is nothing to ship in either case.

The changes here add a build skeleton for the eventual multiprocess goal and some ci files to have it run on travis.

Some more context from IRC for archival purposes:

[18:40] <MarcoFalke> cfields: Are you assuming that multiprocess was merged for 0.20.0rc1?
[18:40] <cfields> MarcoFalke: no. It's an enormous change even for master.
[18:40] <fanquake> No. We are just confused why it was merged at all
[18:40] <cfields> afaik we've never merged a half-decided feature?
[18:41] <fanquake> Regardless of the questions Cory posted. Having a single ACK for what is the basis of a major architectural change for a project seems a bit weak.
[18:41] <MarcoFalke> I have reviewed the changes as well (not libmultiprocess)
[18:42] <MarcoFalke> If it is controversial, it should be reverted 
[18:42] <cfields> to be clear: I don't have a problem with this way forward afaik. I just think it deserves way more discussion.
[18:42] <cfields> Rather, I don't know if I have a problem. I assume anything ryanofsky comes up with is reasonable :)
[18:43] <MarcoFalke> It was not clear from the discussion in the pull request that this was not ready
[18:44] <cfields> MarcoFalke: I don't see how it can be ready unless the overall approach is agreed upon.
[18:45] <MarcoFalke> multiprocess has been discussed for three years as an idea to split the gui from the node (or even longer). I must have missed the discussions that multiprocess is unwanted.
[18:47] <MarcoFalke> Or controversial
[18:47] <fanquake> As Cory has just mentioned, we can't do it in 0.21.0 anyways, if it requires c++17 ?
[18:48] <MarcoFalke> Ok, so the goal is to revert and then schedule for 0.22, together with C++17?
[18:48] <fanquake> Given the discussion in #16684
[18:48] <gribble> https://github.com/bitcoin/bitcoin/issues/16684 | Discussion: upgrading to C++17 · Issue #16684 · bitcoin/bitcoin · GitHub
[18:48] <cfields> MarcoFalke: again, I'm not arguing against the idea. I just don't think the the specifics have been discussed.
[18:49] <MarcoFalke> Though, it doesn't require C++17, if the user doesn't ask for it
[18:50] <fanquake> MarcoFalke: can you clarify https://github.com/chaincodelabs/libmultiprocess/blob/master/CMakeLists.txt#L84 ?
[18:50] <MarcoFalke> fanquake: If you opt out of multiprocess, which is the default, you can go with C++11
[18:51] <cfields> So we've merged something we can't build?
[18:51] <fanquake> Sure, but that means we can't actually use multiprocess until after c++17 is a requirement for the project.
[18:51] <MarcoFalke> cfields: Gitian could use it
[18:52] <MarcoFalke> Aren't we building releases for 0.21.0 with C++17?
[18:52] <MarcoFalke> The project stays at C++11
[18:53] <MarcoFalke> Ok, to sum up: People like multiprocess as a concept, but the current implementation was not agree upon?
[18:53] <MarcoFalke> *agreed
[18:53] <ryanofsky> cfields, just to make sure there's no confusion, you know the whole multiprocess feature is implemented to be an optional add-on?
[18:54] <ryanofsky> it's not like there's a switch from single process to multiprocess, it's an option to build a new set of binaries alongside existing ones
[18:57] <MarcoFalke> ryanofsky: I think the disagreement is mostly about the additions to the depends/bitcoind Makefile and general concerns about libmultiprocess being ready
[18:58] <ryanofsky> right but this doesn't seem that different than the build change that added support for rapidcheck
[18:58] <ryanofsky> just an optional thing turned off by default that we had build/depends support for
[18:59] <cfields> ryanofsky: yes, but again, I thought that approach was at the RFC stage. When I reviewed it, I reviewed it on-merit, without arguing potential alternatives yet.
[19:00] <cfields> ryanofsky: I really really don't want to discourage this, that's not my aim. But I doubt I'm alone in being caught off-guard by the merge.
[19:00] <ryanofsky> sure, i'm just making sure there isn't some misunderstanding about the change
[19:02] <ryanofsky> whatever people think the next steps should be is fine with me
[19:07] <MarcoFalke> It is more invasive than rapidcheck, but to me it felt like the same concept: Merge it early when the code has received review, but mark it experimental so that it can be pulled out and reverted quickly if needed.
[19:09] <cfields> MarcoFalke: yeah, I see that now. To me, the merge implied a commitment to eventual default-ness because afaik that's the path we've always taken for things like this (thinking of univalue/secp256k1, at least).
[19:10] <fanquake> I think it would still be good for someone to outline how they expect this integration to play out. When does it get turned on by default? When do Cores build requirements change? When is review of the libmultiprocess lib/repo happening etc
[19:11] <cfields> ^^ +1
[19:11] <MarcoFalke> All of this must happen before it is taken out of experimental

[ryanofsky]

@MarcoFalke Well it requires c++17, so it won't fit into 0.21.0 with the schedule we've come up with iirc.

I'm little confused about this comment. What's the issue with c++17? Requirements to build bitcoin aren't changed by this PR. This PR adds a new --enable-multiprocess configure option and new MULTIPROCESS=1 depends option. Only these new options which didn't exist before require c++17, so this is fully backwards compatible. Also the switch to c++17 from c++14 in libmultiprocess was done very recently in bitcoin-core/libmultiprocess#25 to be able to use std::optional. It would be possible to have c++11 support if neccessary, too

[maflcko]

It would be possible to have c++11 support if neccessary, too

C++17 will most likely be in Bitcoin Core before multiprocess is enabled by default, so I don't think effort should be put into C++11 support.

[maflcko]

Further discussion about reverting the changes now:

[19:18] <MarcoFalke> Is there anything to do before next weeks meeting? Right now it would be a clean git revert and I think everyone would be ok with reverting it until at least next week's meeting.
[19:19] <cfields> MarcoFalke: up to you. I feel heard either way :)
[19:20] <ryanofsky> either way's fine with me too
[19:20] <MarcoFalke> fanquake: Mind flipping a coin?
[19:21] <fanquake> I don't mind leaving it in, looking forward to thrashing all this out in some discussion

[jnewbery]

Some more context from IRC for archival purposes:

Further discussion about reverting the changes now:

Where is all this discussion taking place? I can't find it in my local #bitcoin-core-dev log, and I don't see it in http://www.erisian.com.au/bitcoin-core-dev/ either

[fanquake]

Where is all this discussion taking place?

@jnewbery It's happening in the #bitcoin-builds channel. Logs available here: http://gnusha.org/bitcoin-builds/.

[ryanofsky]

This PR, which was reverted in #18588, is a proposed meeting topic. Background in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Process-Separation

Topic could be broad, but two questions I'd like to get input on are:

1. What should steps and requirements be for moving chaincodelabs/libmultiprocess to bitcoin-core/, assuming this would be a better home for the library.

2. What should review requirements be for this PR, which adds --enable-multiprocess autoconf option (disabled by default and labeled experimental), and a travis build, and a depends definition to build libmultiprocess (also disabled by default and labeled experimental)?