Multiprocess build support

ryanofsky commented at 6:01 pm on July 10, 2019: member

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.

in test/functional/test_framework/test_framework.py:147 in 79d6064efc outdated

142@@ -143,8 +143,8 @@ def main(self):
143         config = configparser.ConfigParser()
144         config.read_file(open(self.options.configfile))
145         self.config = config
146-        self.options.bitcoind = os.getenv("BITCOIND", default=config["environment"]["BUILDDIR"] + '/src/bitcoind' + config["environment"]["EXEEXT"])
147-        self.options.bitcoincli = os.getenv("BITCOINCLI", default=config["environment"]["BUILDDIR"] + '/src/bitcoin-cli' + config["environment"]["EXEEXT"])
148+        self.options.bitcoind = os.path.join(config["environment"]["BUILDDIR"], os.getenv("BITCOIND", default='src/bitcoind' + config["environment"]["EXEEXT"]))
149+        self.options.bitcoincli = os.path.join(config["environment"]["BUILDDIR"], os.getenv("BITCOINCLI", default='src/bitcoin-cli' + config["environment"]["EXEEXT"]))

MarcoFalke commented at 6:09 pm on July 10, 2019:

Why is this changed?

We already export the PATH two lines below, so setting BITCOIND=bitcoin-node should be sufficient

ryanofsky commented at 7:09 pm on July 10, 2019:

Oh, I didn’t know about the PATH feature, so I’ll revert this.

I changed this to get BITCOIND=src/bitcoin-node test/functional/test_runner.py not to fail, but I should probably take advantage of the PATH setting and change BITCOIND=src/bitcoin-node to BITCOIND=bitcoin-node instead.

DrahtBot commented at 7:47 pm on July 10, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17941 (depends: update to Boost 1.72 by Sjors)
#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.

DrahtBot added the label Build system on Jul 10, 2019

DrahtBot added the label Docs on Jul 10, 2019

DrahtBot added the label Tests on Jul 10, 2019

fanquake requested review from theuni on Jul 11, 2019

fanquake requested review from dongcarl on Jul 11, 2019

fanquake added this to the "In progress" column in a project

Sjors commented at 1:32 pm on July 11, 2019: member

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.

in depends/packages/boost.mk:4 in 4cf100f210 outdated

0@@ -1,7 +1,7 @@
1 package=boost
2 $(package)_version=1_70_0
3 $(package)_download_path=https://dl.bintray.com/boostorg/release/1.70.0/source/
4-$(package)_file_name=$(package)_$($(package)_version).tar.bz2
5+$(package)_file_name=boost_$($(package)_version).tar.bz2

Sjors commented at 1:45 pm on July 11, 2019:

Is this an unrelated improvement?

ryanofsky commented at 2:05 pm on July 11, 2019:

It is related. I do think it’s an improvement to not use our internal package name in a tarball URL that’s generated and hosted by a third party and we don’t control. But this change is actually needed to make the build work.

Specifically, this is needed to make native_boost package work, since it derives from the boost package but has a different package name. The native_boost package is a new package needed to build the native_libmultiprocess package which currently uses boost::optional. The native_libmultiprocess package is needed to run a code generation tool in cross-compiled builds (since a tool that runs at build time should be native, not cross-compiled). I’m probably going to try to drop the boost::optional dependency at some point, but for now just adding a native_boost package seemed like the path of least resistance, since boost::optional is so convenient.

dongcarl commented at 9:15 pm on October 29, 2019:

Perhaps it’s cleaner to inherit from the native package like you did for capnp and libmultiprocess? Maybe I’m missing something here though.

ryanofsky commented at 10:12 pm on October 29, 2019:

Perhaps it’s cleaner to inherit from the native package like you did for capnp and libmultiprocess? Maybe I’m missing something here though.

That’d be another approach. I wouldn’t mind adding that, but it would make the change to the boost package here a little bigger and less minimal than actually needed for the PR.

Also, I do think the change here is an improvement by itself. It seems janky to me tie our local package name to the convention the upstream project is using for download urls.

theuni commented at 6:29 pm on March 5, 2020:

+1. It’s fine when it’s convenient shorthand or more clear, but no need to keep them tied if it makes anything harder.

Sjors commented at 9:53 am on March 8, 2020:

Now that native_libmultiprocess doesn’t need boost::optional, do you still need this? If you do need it, maybe make that a separate commit. See also #16367#pullrequestreview-369969746

ryanofsky commented at 2:30 pm on July 11, 2019: member

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:

0#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 commented at 3:16 pm on July 11, 2019: member

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.

ryanofsky force-pushed on Jul 12, 2019

ryanofsky force-pushed on Jul 15, 2019

in src/Makefile.am:555 in 5233494f3f outdated

519@@ -515,25 +520,25 @@ libbitcoin_cli_a_SOURCES = \
520 nodist_libbitcoin_util_a_SOURCES = $(srcdir)/obj/build.h
521 #
522 
523-# bitcoind binary #
524-bitcoind_SOURCES = bitcoind.cpp
525-bitcoind_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
526-bitcoind_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
527-bitcoind_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
528+# bitcoind, bitcoin-wallet, bitcoin-node binaries #

ariard commented at 6:21 pm on July 25, 2019:

nit: bitcoin-wallet has already its own comment below

hebasto commented at 4:53 pm on March 5, 2020:

I think the comment in its current state is correct as bitcoind_common_{cppflags|cxxflags|ldflags} are used to define bitcoin_wallet_{CPPFLAGS|CXXFLAGS|LDFLAGS} as well.

ryanofsky commented at 5:08 pm on March 5, 2020:

re: #16367 (review)

I think the comment in its current state is correct as bitcoind_common_{cppflags|cxxflags|ldflags} are used to define bitcoin_wallet_{CPPFLAGS|CXXFLAGS|LDFLAGS} as well.

It’s true that the flags are shared, but it’s also true that this section of the file is specifically adding the bitcoind and bitcoin-node targets, while a different section now is adding the bitcoin-wallet target, so I updated the comment. The comment precedes #13926, from a time when this section was actually adding the bitcoin-wallet target

in depends/README.md:94 in db07ff9d47 outdated

72@@ -73,6 +73,7 @@ The following can be set when running make: make FOO=bar
73     NO_UPNP: Don't download/build/cache packages needed for enabling upnp
74     DEBUG: disable some optimizations and enable more runtime checking
75     RAPIDCHECK: build rapidcheck (experimental, requires cmake)
76+    MULTIPROCESS: build libmultiprocess (experimental, requires cmake)

ariard commented at 6:51 pm on July 25, 2019:

Could libmultiprocess be a subtreee instead of a dependency ? What’s the rational (not everyone want multi-process I guess but boost and libevent are also there and seems mandatory to me) ?

ryanofsky commented at 7:58 pm on July 25, 2019:

Could libmultiprocess be a subtree instead of a dependency ? What’s the rational (not everyone want multi-process I guess but boost and libevent are also there and seems mandatory to me) ?

Few things here:

Libmultiprocess is a code generation tool (mpgen) in addition to a library (libmultiprocess.a), so to support cross compilation, mpgen has to be built for the build machine architecture and libmultiprocess.a has to be built for host machine architecture. This means even if we add libmultiprocess as a subtree, the make depends native package would still need to exist to support cross compiled builds, and having that tied to a subtree instead of a standalone repository would be an additional complication. Alternately if autotools has support for separate host & build dependencies, the depends code could go away, but this would require writing new autotools code. I mainly just chose the current setup because depends seemed like the path of least resistance to get cross compilation working (which was something that was previously broken before I separated libmultiprocess).
I’m not actually sure what problem adding libmultiprocess as a subtree would be solving. It makes sense to me to subtree consensus-critical code that could cause forks, or code that we patch heavily for more convenient maintenance, but I’m not sure about other cases.
This more of a preference than a rational reason, but I’m slowly whittling down #10102 to be a small as possible, and adding a subtree would seem to increase the total changes required. In general I’d like multiprocess support to be small, optional, and not get in the way of things.
Finally, even though most packaging concerns aren’t relevant here, it’s sometimes considered not good practice to bundle libraries. Luke is frequently linking to https://wiki.gentoo.org/wiki/Why_not_bundle_dependencies and https://fedoraproject.org/wiki/Bundled_Libraries.

ariard commented at 10:46 pm on July 26, 2019:

Thanks for the great explication, bookmarked!

ariard commented at 7:02 pm on July 25, 2019: member

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 commented at 9:51 pm on July 25, 2019: member

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 commented at 10:46 pm on July 26, 2019: member

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 referenced this in commit f89e4b32dc on Aug 6, 2019

ryanofsky referenced this in commit 9cd1a5a9c1 on Aug 6, 2019

ryanofsky force-pushed on Aug 6, 2019

ryanofsky force-pushed on Aug 20, 2019

ryanofsky commented at 3:24 pm on August 20, 2019: member

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 commented at 3:35 am on August 23, 2019: member

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 https://github.com/bitcoin/bitcoin/pull/16367/commits/7149b1ae67f0e3508407f1b668b11ee8e7456ab3 and https://github.com/bitcoin/bitcoin/pull/16367/commits/a295b9decb14534a98f6c993df9a9379ce752aeb into a separate PR. They are clear improvements, and will likely clear up some confusion here and in other depends related PRs.

fanquake referenced this in commit 442a9c6477 on Aug 23, 2019

ryanofsky force-pushed on Aug 28, 2019

ryanofsky force-pushed on Aug 29, 2019

ryanofsky force-pushed on Aug 30, 2019

ryanofsky force-pushed on Sep 18, 2019

in ci/test/06_script_b.sh:39 in 6832a3a343 outdated

22 fi
23 
24 if [ "$RUN_FUZZ_TESTS" = "true" ]; then
25   BEGIN_FOLD fuzz-tests
26-  DOCKER_EXEC test/fuzz/test_runner.py -l DEBUG ${DIR_FUZZ_IN}
27+  DOCKER_EXEC LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib test/fuzz/test_runner.py -l DEBUG ${DIR_FUZZ_IN}

dongcarl commented at 9:18 pm on October 29, 2019:

Curious: where is this used?

ryanofsky commented at 9:53 pm on October 29, 2019:

Curious: where is this used?

The capnp package installs dynamic libraries, so this is needed to make them load on travis. I guess variable probably wasn’t necessary before because our other depends packages link everything statically. I’m actually not sure if static linking is a requirement of our packaging process, or just something we do for convenience. I can look into configure flags for making capnp packages produce static libraries, though, too.

in depends/funcs.mk:133 in fd9e4089f1 outdated

129@@ -130,6 +130,7 @@ $(1)_config_env+=$($(1)_config_env_$(host_arch)_$(host_os)) $($(1)_config_env_$(
130 
131 $(1)_config_env+=PKG_CONFIG_LIBDIR=$($($(1)_type)_prefix)/lib/pkgconfig
132 $(1)_config_env+=PKG_CONFIG_PATH=$($($(1)_type)_prefix)/share/pkgconfig
133+$(1)_config_env+=CMAKE_MODULE_PATH=$($($(1)_type)_prefix)/lib/cmake

dongcarl commented at 9:22 pm on October 29, 2019:

Curious: which package recipe will write CMAKE modules to the prefix? Are they used by other depends packages or by the non-depends Bitcoin build?

ryanofsky commented at 9:55 pm on October 29, 2019:

Curious: which package recipe will write CMAKE modules to the prefix? Are they used by other depends packages or by the non-depends Bitcoin build?

capnp packages write cmake modules and libmultiprocess packages use them to locate capnp

in src/Makefile.qt.include:315 in 858a3f90fc outdated

426 endif
427-qt_bitcoin_qt_LDADD = qt/libbitcoinqt.a $(LIBBITCOIN_SERVER)
428+bitcoin_qt_common_ldadd = qt/libbitcoinqt.a $(LIBBITCOIN_SERVER)
429 if ENABLE_WALLET
430-qt_bitcoin_qt_LDADD += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET)
431+bitcoin_qt_common_ldadd += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET) $(LIBBITCOIN_SERVER)

dongcarl commented at 9:28 pm on October 29, 2019:

The last $(LIBBITCOIN_SERVER) is probably unneeded…

hebasto commented at 6:16 pm on March 5, 2020:

Yeah, it seems unconditionally included in: https://github.com/bitcoin/bitcoin/blob/d6921b176186fd8304b6d6a13772fd0fc32fe726/src/Makefile.qt.include#L313

ryanofsky commented at 7:55 pm on March 5, 2020:

re: #16367 (review)

The last $(LIBBITCOIN_SERVER) is probably unneeded…

Removed with Hennadii’s diff #16367 (comment)

in src/Makefile.qt.include:336 in 858a3f90fc outdated

463+qt_bitcoin_qt_LIBTOOLFLAGS = $(bitcoin_qt_common_libtoolflags)
464+
465+bitcoin_gui_CPPFLAGS = $(bitcoin_qt_common_cppflags)
466+bitcoin_gui_CXXFLAGS = $(bitcoin_qt_common_cxxflags)
467+bitcoin_gui_SOURCES = $(bitcoin_qt_common_sources)
468+bitcoin_gui_LDADD = $(bitcoin_qt_common_ldadd) $(LIBBITCOIN_COMMON)

dongcarl commented at 9:35 pm on October 29, 2019:

The last $(LIBBITCOIN_COMMON) is probably unneeded…

ryanofsky commented at 10:16 pm on October 29, 2019:

The last $(LIBBITCOIN_COMMON) is probably unneeded…

Will try to remove, but not so sure about this one. The util and wallet libraries do (no longer) depend on the server library in the previous line you commented on, but a bunch of the earlier libraries do depend on the common library, so it’s not surprising that might need to be repeated.

hebasto commented at 6:17 pm on March 5, 2020:

The last $(LIBBITCOIN_COMMON) is probably unneeded…

It seems unconditionally included in: https://github.com/bitcoin/bitcoin/blob/d6921b176186fd8304b6d6a13772fd0fc32fe726/src/Makefile.qt.include#L320-L322

ryanofsky commented at 7:54 pm on March 5, 2020:

re: #16367 (review)

The last $(LIBBITCOIN_COMMON) is probably unneeded…

Removed with Hennadii’s diff #16367 (comment)

ryanofsky commented at 10:19 pm on October 29, 2019: member

Thanks for the review!

ryanofsky force-pushed on Jan 8, 2020

ryanofsky force-pushed on Jan 24, 2020

ryanofsky force-pushed on Feb 25, 2020

ryanofsky commented at 7:50 pm on February 25, 2020: member

Rebased 6a84d0553f712a3e228fd697d3a831d21f210fa5 -> 71aa1a2c9be6ded8daccf6e8af318b82f3a0f0ca (pr/ipc-build.13 -> pr/ipc-build.14, compare) due to conflict with #18166

ryanofsky marked this as ready for review on Feb 26, 2020

Sjors commented at 5:55 pm on February 27, 2020: member

I’m unable to build depends on macOS 10.15.3: make MULTIPROCESS=1 results in A C++ library with support for C++11 features is required, see log (Apple clang version 11.0.0 (clang-1100.0.33.17))

Sjors commented at 7:28 pm on February 27, 2020: member

Concept ACK. I was able to build with Ubuntu and depends. I’ll try Ubuntu and libmultiprocess install later, as well as macOS.

https://github.com/chaincodelabs/libmultiprocess/pull/25 lets us drop boost_native from this PR which is nice (we should gracefully detect missing c++17 support in that case).

ryanofsky commented at 1:28 am on March 4, 2020: member

re: #16367 (comment)

I’m unable to build depends on macOS 10.15.3: make MULTIPROCESS=1 results in A C++ library with support for C++11 features is required, see log (Apple clang version 11.0.0 (clang-1100.0.33.17))

The same “configure: error: *** A C++ library with support for C++11 features is required.” was also reported https://github.com/chaincodelabs/libmultiprocess/issues/5, and the fix seemed to be to be fixed by some combination of upgrading and cleaning the build.

FWIW, I’m also able to reproduce the problem. Looking in depends/work/build/x86_64-apple-darwin18.0.0/native_capnp/0.7.0-7b1af401b59/config.log the detailed error is:

 0configure:5116: /Library/Developer/CommandLineTools/usr/bin/clang++ -std=gnu++14 -stdlib=libc++ -c -O2 -DNDEBUG -I/Users/user/src/bitcoin/depends/x86_64-apple-darwin18.0.0/native/include     conftest.cpp >&5
 1In file included from conftest.cpp:13:
 2In file included from /Library/Developer/CommandLineTools/usr/include/c++/v1/unordered_map:369:
 3In file included from /Library/Developer/CommandLineTools/usr/include/c++/v1/__hash_table:16:
 4In file included from /Library/Developer/CommandLineTools/usr/include/c++/v1/memory:653:
 5In file included from /Library/Developer/CommandLineTools/usr/include/c++/v1/typeinfo:61:
 6In file included from /Library/Developer/CommandLineTools/usr/include/c++/v1/exception:82:
 7In file included from /Library/Developer/CommandLineTools/usr/include/c++/v1/cstdlib:86:
 8/Library/Developer/CommandLineTools/usr/include/c++/v1/stdlib.h:94:15: fatal error: 'stdlib.h' file not found
 9#include_next <stdlib.h>
10              ^~~~~~~~~~
111 error generated.

which according to https://discourse.brew.sh/t/clang-can-no-longer-find-usr-include-header-files-fatal-error-stdlib-h-file-not-found/4523/6 has something to do with mac os removing /usr/include, but it’s unclear how this should be fixed.

Just running checking out this PR and running “make -C depends NO_QT=1 MULTIPROCESS=1” on a mac was sufficient to reproduce this

ryanofsky force-pushed on Mar 4, 2020

ryanofsky commented at 3:28 pm on March 4, 2020: member

I can reproduce the mac depends problem independently without bitcoin or depends:

0$ echo '#include <stdlib.h>' > test.cpp
1$ /Library/Developer/CommandLineTools/usr/bin/clang++ -c test.cpp
2In file included from test.cpp:1:
3/Library/Developer/CommandLineTools/usr/include/c++/v1/stdlib.h:94:15: fatal error: 'stdlib.h' file not found
4#include_next <stdlib.h>
5              ^~~~~~~~~~
61 error generated.

Adding -isysroot seems to fix it, so I guess it needs to be added to the depends build:

0$ /Library/Developer/CommandLineTools/usr/bin/clang++ -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -c test.cpp

ryanofsky commented at 3:43 pm on March 4, 2020: member

@Sjors, the following change gets past the reported mac os problem #16367 (comment):

 0diff --git a/depends/builders/darwin.mk b/depends/builders/darwin.mk
 1index eb587fca890..69c394ec1db 100644
 2--- a/depends/builders/darwin.mk
 3+++ b/depends/builders/darwin.mk
 4@@ -1,5 +1,5 @@
 5-build_darwin_CC:=$(shell xcrun -f clang)
 6-build_darwin_CXX:=$(shell xcrun -f clang++)
 7+build_darwin_CC:=$(shell xcrun -f clang) --sysroot $(shell xcrun --show-sdk-path)
 8+build_darwin_CXX:=$(shell xcrun -f clang++) --sysroot $(shell xcrun --show-sdk-path)
 9 build_darwin_AR:=$(shell xcrun -f ar)
10 build_darwin_RANLIB:=$(shell xcrun -f ranlib)
11 build_darwin_STRIP:=$(shell xcrun -f strip)

I’ll add it to the PR assuming the rest of the build succeeds. Thanks for testing and reporting!

hebasto commented at 11:01 am on March 5, 2020: member

Concept ACK.

in configure.ac:247 in 87e85d9cba outdated

242+  [with_libmultiprocess=$withval],
243+  [with_libmultiprocess=auto])
244+
245+AC_ARG_WITH([mpgen],
246+  [AS_HELP_STRING([--with-mpgen=yes|no|auto|PREFIX],
247+  [Build with libmultiprocess codegen tool. Useful to specify different libmultiprocess host system library and build system codegen tool prefixes when cross-compiling (default is host system libmultiprocess prefix])],

hebasto commented at 2:29 pm on March 5, 2020:

nit: a missed paren in the end of help string:

0  [Build with libmultiprocess codegen tool. Useful to specify different libmultiprocess host system library and build system codegen tool prefixes when cross-compiling (default is host system libmultiprocess prefix)])],

ryanofsky commented at 3:21 pm on March 5, 2020:

re: #16367 (review)

nit: a missed paren in the end of help string:

Thanks, added

in configure.ac:241 in 87e85d9cba outdated

235@@ -236,6 +236,24 @@ if test x$enable_bip70 != xno; then
236   AC_MSG_ERROR([BIP70 is no longer supported!])
237 fi
238 
239+AC_ARG_WITH([libmultiprocess],
240+  [AS_HELP_STRING([--with-libmultiprocess=yes|no|auto],
241+  [Build with libmultiprocess library. (default: detect with pkg-config)])],

hebasto commented at 2:34 pm on March 5, 2020:

nit: The help string could clear state that default=auto:

“(default is auto, i.e., detect with pkg-config)”

or smth similar.

ryanofsky commented at 3:21 pm on March 5, 2020:

re: #16367 (review)

nit: The help string could clear state that default=auto:

“(default is auto, i.e., detect with pkg-config)”

or smth similar.

Thanks, changed

in depends/config.site.in:34 in 7cdd178685 outdated

27@@ -25,6 +28,10 @@ if test -z $enable_wallet && test -n "@no_wallet@"; then
28   enable_wallet=no
29 fi
30 
31+if test -z $enable_multiprocess && test -n "@multiprocess@"; then
32+  enable_multiprocess=yes
33+fi
34+

hebasto commented at 3:08 pm on March 5, 2020:

This is a very convenient feature. But, OTOH, it is a bit confusing as --enable-multiprocess help string states that “default is no”.

May I suggest to drop this addition?

As another approach, the default value of --enable-multiprocess could be auto (but not now, when it is an experimental feature).

ryanofsky commented at 3:21 pm on March 5, 2020:

re: #16367 (review)

May I suggest to drop this addition?

I could much more easily imagine someone surprised that the MULTIPROCESS=1 option didn’t enable multiprocess support, than by someone being surprised that MULTIPROCESS=1 enables multiprocess support. So I think dropping this would be a worse default. I also don’t think there is anything new about this. For example when we build bitcoin without wallet support when NO_WALLET is passed, even though the default configure behavior is to enable wallet support

theuni commented at 6:37 pm on March 5, 2020:

Good points. I think as long as this is experimental, probably best to play it safe and require it to be –enabled by hand. Arguably it already has been because of the way depends was built, but it’s easy to forget when bouncing around branches.

Not a strong preference, though.

ryanofsky referenced this in commit d6921b1761 on Mar 5, 2020

ryanofsky force-pushed on Mar 5, 2020

hebasto commented at 5:25 pm on March 5, 2020: member

From Cap’n Proto docs:

The minimum versions are:

GCC 4.9

Clang 3.5

Our requirements are:

GCC 4.8+
Clang 3.3+

Could this be an issue?

hebasto approved

hebasto commented at 5:46 pm on March 5, 2020: member

ACK d6921b176186fd8304b6d6a13772fd0fc32fe726, tested on Linux Mint 19.3 (gcc 7.4.0) and Fedora 31 (gcc 9.2.1); built depends with MULTIPROCESS=1, and with/without NO_QT=1.

Verified new downloads (capnp and libmultiprocess) paths and hashes.

In commit “Add multiprocess travis configuration” (f077d9cb46c6b825ff560f7c8a1b655a8d9fc2be) could an environment file be used as it has done in #16623?

in src/Makefile.am:556 in a56950c54a outdated

556-bitcoind_SOURCES = bitcoind.cpp
557-bitcoind_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
558-bitcoind_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
559-bitcoind_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
560+# bitcoind, bitcoin-wallet, bitcoin-node binaries #
561+bitcoind_common_sources = bitcoind.cpp

theuni commented at 6:06 pm on March 5, 2020:

Please make these uppercase so they stand out from automake variables. As-is, this looks like a new helper lib.

ryanofsky commented at 8:12 pm on March 5, 2020:

Please make these uppercase so they stand out from automake variables. As-is, this looks like a new helper lib.

I don’t think this works. If I make everything uppercase I see errors:

0src/Makefile.am:568: warning: variable 'BITCOIND_COMMON_LDFLAGS' is defined but no program or
1src/Makefile.am:568: library has 'BITCOIND_COMMON' as canonical name (possible typo)

These are lowercase specifically because automake seems to pick up uppercase suffixes as target properties.

Tested with:

0for f in bitcoind_common_sources bitcoind_common_cppflags bitcoind_common_cxxflags bitcoind_common_ldflags bitcoin_qt_common_cppflags bitcoin_qt_common_cxxflags bitcoin_qt_common_sources bitcoin_qt_common_ldadd bitcoin_qt_common_ldflags bitcoin_qt_common_libtoolflags; do
1git grep -l $f | xargs sed -i "s/$f/$(echo $f | tr '[:lower:]' '[:upper:]')/g"
2done

hebasto commented at 9:47 pm on March 5, 2020:

~~Being agreed with @theuni, may I suggest a tradeoff:~~ ~~the underscore after _COMMON_ could be dropped, i.e.,~~

~~s/BITCOIND_COMMON_LDFLAGS/BITCOIND_COMMONLDFLAGS/~~

See: #16367 (review)

ryanofsky commented at 10:48 pm on March 5, 2020:

re: #16367 (review)

the underscore after _COMMON_ could be dropped

I’m confused about what problem this suggestion would be solving. Shouldn’t the goal to be to avoid these variables getting confused with the special variables processed by automake?

If these variables are lowercase the are clearly normal variables, not special automake variables. I can understand not caring about the distinction between special automake variables and normal automake variables and just wanting everything to be uppercase for consistency. But if you have to give up consistency anyway to silence an automake warning about this exact problem, I don’t know why you’d do that and misleadingly make these appear to be special variables to get the worst of both worlds

hebasto commented at 11:04 am on March 8, 2020:

Agree that naming of convenient variables with all_lower_case makes them distinguishable from the Automake special variables.

in src/Makefile.qt.include:305 in a56950c54a outdated

303+$(qt_libbitcoinqt_a_OBJECTS) $(qt_bitcoin_qt_OBJECTS) $(bitcoin_gui_OBJECTS) : | $(QT_MOC)
304 
305-# bitcoin-qt binary #
306-qt_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
307+# bitcoin-qt and bitcoin-gui binaries #
308+bitcoin_qt_common_cppflags = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \

theuni commented at 6:09 pm on March 5, 2020:

Same with these, please use uppercase for helper vars.

ryanofsky commented at 8:12 pm on March 5, 2020:

re: #16367 (review)

Same with these, please use uppercase for helper vars.

Seeing

0src/Makefile.qt.include:323: warning: variable 'BITCOIN_QT_COMMON_LDFLAGS' is defined but no program or
1src/Makefile.qt.include:323: library has 'BITCOIN_QT_COMMON' as canonical name (possible typo)

with this

hebasto commented at 9:52 pm on March 5, 2020:

~~https://github.com/bitcoin/bitcoin/pull/16367#discussion_r388585280~~ ~~s/BITCOIN_QT_COMMON_LDFLAGS/BITCOIN_QT_COMMONLDFLAGS/ ?~~

See: #16367 (review)

hebasto commented at 6:14 pm on March 5, 2020: member

FWIW, with applied diff:

 0diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
 1index c5da67b0e..7924917d1 100644
 2--- a/src/Makefile.qt.include
 3+++ b/src/Makefile.qt.include
 4@@ -312,7 +312,7 @@ if TARGET_WINDOWS
 5 endif
 6 bitcoin_qt_common_ldadd = qt/libbitcoinqt.a $(LIBBITCOIN_SERVER)
 7 if ENABLE_WALLET
 8-bitcoin_qt_common_ldadd += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET) $(LIBBITCOIN_SERVER)
 9+bitcoin_qt_common_ldadd += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET)
10 endif
11 if ENABLE_ZMQ
12 bitcoin_qt_common_ldadd += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
13@@ -333,7 +333,7 @@ qt_bitcoin_qt_LIBTOOLFLAGS = $(bitcoin_qt_common_libtoolflags)
14 bitcoin_gui_CPPFLAGS = $(bitcoin_qt_common_cppflags)
15 bitcoin_gui_CXXFLAGS = $(bitcoin_qt_common_cxxflags)
16 bitcoin_gui_SOURCES = $(bitcoin_qt_common_sources)
17-bitcoin_gui_LDADD = $(bitcoin_qt_common_ldadd) $(LIBBITCOIN_COMMON)
18+bitcoin_gui_LDADD = $(bitcoin_qt_common_ldadd)
19 bitcoin_gui_LDFLAGS = $(bitcoin_qt_common_ldflags)
20 bitcoin_gui_LIBTOOLFLAGS = $(bitcoin_qt_common_libtoolflags)

I have built:

0$ ./autogen.sh
1$ make -C depends MULTIPROCESS=1
2$ CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
3$ make

and then successfully run all Qt binaries:

0$ ./src/qt/test/test_bitcoin-qt

0$ ./src/qt/bitcoin-qt

0$ ./src/bitcoin-gui

in depends/packages/native_capnp.mk:4 in 6757f4c1cd outdated

0@@ -0,0 +1,17 @@
1+package=native_capnp
2+$(package)_version=0.7.0
3+$(package)_download_path=https://capnproto.org/
4+$(package)_file_name=capnproto-c++-$($(package)_version).tar.gz

theuni commented at 6:41 pm on March 5, 2020:

IIRC the “+” in the filename gives some tools/filesystems grief, which is why we hack it out in native_cctools. You can use (untested):

0$(package)_download_file=capnproto-c++-$($(package)_version).tar.gz
1$(package)_file_name=capnproto-$($(package)_version).tar.gz

I’ll try to track down a good explanation for why we do that first, though. Maybe it’s no longer necessary.

Edit: fixed

ryanofsky commented at 8:11 pm on March 5, 2020:

re: #16367 (review)

IIRC the “+” in the filename gives some tools/filesystems grief, which is why we hack it out in native_cctools.

Thanks. It’d be nice to not have to do this if you find out it’s unnecessary, but replaced + with x for now

theuni commented at 6:49 pm on March 5, 2020: member

Concept ACK to the build/depends changes. I’ve been absent for the libmultiprocess conversation thus far, so I’m just reviewing with the assumption that it’s sane.

Very nice work on the depends changes. I left a few nits, but for the most part that all looks good from a quick glance.

theuni commented at 7:03 pm on March 5, 2020: member

@Sjors, the following change gets past the reported mac os problem #16367 (comment):

 0diff --git a/depends/builders/darwin.mk b/depends/builders/darwin.mk
 1index eb587fca890..69c394ec1db 100644
 2--- a/depends/builders/darwin.mk
 3+++ b/depends/builders/darwin.mk
 4@@ -1,5 +1,5 @@
 5-build_darwin_CC:=$(shell xcrun -f clang)
 6-build_darwin_CXX:=$(shell xcrun -f clang++)
 7+build_darwin_CC:=$(shell xcrun -f clang) --sysroot $(shell xcrun --show-sdk-path)
 8+build_darwin_CXX:=$(shell xcrun -f clang++) --sysroot $(shell xcrun --show-sdk-path)
 9 build_darwin_AR:=$(shell xcrun -f ar)
10 build_darwin_RANLIB:=$(shell xcrun -f ranlib)
11 build_darwin_STRIP:=$(shell xcrun -f strip)

I’ll add it to the PR assuming the rest of the build succeeds. Thanks for testing and reporting!

These tools have changed significantly since this was created. Might be easier (and more correct these days?) to just switch to using “clang” and “clang++” here. I’m pretty sure that’s essentially what we’re recreating anyway.

ryanofsky referenced this in commit 4968ddfd9e on Mar 5, 2020

ryanofsky force-pushed on Mar 5, 2020

ryanofsky commented at 7:23 pm on March 5, 2020: member

Thanks for the testing and review!

Updated d6921b176186fd8304b6d6a13772fd0fc32fe726 -> 4968ddfd9e9b570503edef7707ca24ad4bee87ab (pr/ipc-build.16 -> pr/ipc-build.17, compare) with suggestions

re: #16367 (comment)

The minimum versions are […] Could this be an issue?

I don’t see an issue here, but let me know if you have a specific suggestion or want to describe a problem in more detail. I think each project is describing its build requirements accurately right now. It’s also possible to install capnproto without installing any compiler, for example by using an os package (https://packages.ubuntu.com/bionic/capnproto). We can write more documentation about this, but I think we should not add documentation about building or installing other projects if there isn’t a specific reason to think it will be helpful to someone.

re: #16367#pullrequestreview-369782143

In commit “Add multiprocess travis configuration” (f077d9c) could an environment file be used as it has done in #16623?

Thanks, done. It was just like this because this PR preceded #16623.

re: #16367 (comment)

FWIW, with applied diff:

Thanks, applied diff

in configure.ac:241 in 4968ddfd9e outdated

235@@ -236,6 +236,24 @@ if test x$enable_bip70 != xno; then
236   AC_MSG_ERROR([BIP70 is no longer supported!])
237 fi
238 
239+AC_ARG_WITH([libmultiprocess],
240+  [AS_HELP_STRING([--with-libmultiprocess=yes|no|auto],
241+  [Build with libmultiprocess library. (default: detect is auto, i.e. detect with pkg-config)])],

hebasto commented at 7:58 pm on March 5, 2020:

It seems now the first “detect” is unneeded, no?

ryanofsky commented at 8:02 pm on March 5, 2020:

re: #16367 (review)

It seems now the first “detect” is unneeded, no?

Thanks, fixed

ryanofsky referenced this in commit 04bec8d284 on Mar 5, 2020

ryanofsky force-pushed on Mar 5, 2020

ryanofsky commented at 8:51 pm on March 5, 2020: member

Updated 4968ddfd9e9b570503edef7707ca24ad4bee87ab -> 04bec8d2844e0cd2e09a62b21b006292f33f8093 (pr/ipc-build.17 -> pr/ipc-build.18, compare) with new suggestions from Hennadii and cory. Replaced mac os workaround 4968ddfd9e9b570503edef7707ca24ad4bee87ab with 04bec8d2844e0cd2e09a62b21b006292f33f8093

Sjors commented at 5:39 pm on March 6, 2020: member

I can build 04bec8d2844e0cd2e09a62b21b006292f33f8093 on macOS now, with capnp 0.7.0 via Homebrew and libmultiprocess installed with its latest macOS fix. I’ll try again once the boost obliteration from https://github.com/chaincodelabs/libmultiprocess/pull/25 is used here.

ryanofsky commented at 5:56 pm on March 6, 2020: member

re: #16367 (comment)

I can build 04bec8d on macOS now, with capnp 0.7.0 via Homebrew and libmultiprocess installed with its latest macOS fix. I’ll try again once the boost obliteration from chaincodelabs/libmultiprocess#25 is used here.

Note: boost PR is already incorporated here:

ryanofsky referenced this in commit 562c5e22ef on Mar 6, 2020

ryanofsky referenced this in commit d1412addf9 on Mar 6, 2020

ryanofsky referenced this in commit c416f158a3 on Mar 6, 2020

hebasto approved

hebasto commented at 11:24 pm on March 7, 2020: member

re-ACK 04bec8d2844e0cd2e09a62b21b006292f33f8093, only suggested changes applied since d6921b176186fd8304b6d6a13772fd0fc32fe726 (https://github.com/bitcoin/bitcoin/pull/16367#pullrequestreview-369782143).

Sjors commented at 11:58 am on March 9, 2020: member

Copying my earlier inline comment, since I can hardly find it:

Now that native_libmultiprocess doesn’t need boost::optional, do you still need to add native_boost? If you do need it, maybe make that a separate commit.

ryanofsky commented at 3:03 pm on March 9, 2020: member

Copying my earlier inline comment, since I can hardly find it:

Now that native_libmultiprocess doesn’t need boost::optional, do you still need to add native_boost? If you do need it, maybe make that a separate commit.

The three main commits in the is PR are adding (1) autotools (2) travis and (3) depends support for libmultiprocess, and there are also two smaller commits with travis and depends fixes that are needed multiprocess support but make sense independently. I’m pretty sure breaking up the depends commit fab9fe51b999cca7e9d2122f9baa1786b77f4cb0 would just make it harder to understand because it’s not that big right now and split up it’d be harder to see that packages there are following consistent conventions and see where new definitions are referenced.

As for removing native_boost, I think it would work temporarily, but I don’t want to remove it because https://github.com/chaincodelabs/libmultiprocess/pull/25 isn’t the end of the story on boost support. I merged that PR because I like the way it removed extraneous boost usages and dropped the boost requirement. But I’m pretty sure I want to add back some boost support. At least an optional libmultiprocess library dependency on boost to drop the duplicative boost::optional code I had to add in #10102 and an mpgen dependency on boost to get rid of verbose and nonportable fork exec stuff there right now.

ryanofsky referenced this in commit 1f9a2699a3 on Mar 9, 2020

ryanofsky force-pushed on Mar 9, 2020

ryanofsky commented at 4:32 pm on March 9, 2020: member

Updated 04bec8d2844e0cd2e09a62b21b006292f33f8093 -> 1f9a2699a3e4b6ccdf4912600ccfdbf0c8eb20a4 (pr/ipc-build.18 -> pr/ipc-build.19, compare) just switching to $DEPENDS_DIR instead of using $TRAVIS_BUILD_DIR/depends Updated 1f9a2699a3e4b6ccdf4912600ccfdbf0c8eb20a4 -> ac0f2fb1095d40b3a851150f38199fc2966b546f (pr/ipc-build.19 -> pr/ipc-build.20, compare) bumping dependency version

ryanofsky referenced this in commit 4660949f56 on Mar 9, 2020

ryanofsky referenced this in commit f388ccba11 on Mar 20, 2020

ryanofsky referenced this in commit ac0f2fb109 on Mar 20, 2020

ryanofsky force-pushed on Mar 20, 2020

hebasto commented at 6:07 pm on March 23, 2020: member

re-ACK ac0f2fb1095d40b3a851150f38199fc2966b546f, since previous review:

$TRAVIS_BUILD_DIR/depends –> $DEPENDS_DIR
libmultiprocess updated to https://github.com/chaincodelabs/libmultiprocess/tree/78f2f75d4722c42988988efd2b0074d3ec64099e

in src/Makefile.am:579 in ac0f2fb109 outdated

575@@ -572,7 +576,19 @@ bitcoind_LDADD = \
576   $(LIBMEMENV) \
577   $(LIBSECP256K1)
578 
579-bitcoind_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(MINIUPNPC_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) $(ZMQ_LIBS)
580+bitcoind_common_ldadd += $(BOOST_LIBS) $(BDB_LIBS) $(MINIUPNPC_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) $(ZMQ_LIBS) $(LIBMULTIPROCESS_LIBS)

MarcoFalke commented at 8:08 pm on March 23, 2020:

Why is LIBMULTIPROCESS_LIBS neeeded for the single-binary bitcoind?

ryanofsky commented at 2:28 am on March 25, 2020:

re: #16367 (review)

Why is LIBMULTIPROCESS_LIBS neeeded for the single-binary bitcoind?

It’s not needed, removed entirely from this PR and removed from bitcoind link line in #10102

in src/Makefile.am:637 in ac0f2fb109 outdated

631@@ -616,29 +632,14 @@ bitcoin_tx_LDADD += $(BOOST_LIBS)
632 
633 # bitcoin-wallet binary #
634 bitcoin_wallet_SOURCES = bitcoin-wallet.cpp
635-bitcoin_wallet_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
636-bitcoin_wallet_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
637-bitcoin_wallet_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
638+bitcoin_wallet_CPPFLAGS = $(bitcoind_common_cppflags)
639+bitcoin_wallet_CXXFLAGS = $(bitcoind_common_cxxflags)
640+bitcoin_wallet_LDFLAGS = $(bitcoind_common_ldflags)

MarcoFalke commented at 8:14 pm on March 23, 2020:

Why are the bitcoind common flags used for the bitcoin wallet tool? Could remove the d?

ryanofsky commented at 2:28 am on March 25, 2020:

re: #16367 (review)

Why are the bitcoind common flags used for the bitcoin wallet tool? Could remove the d?

Makes sense, changed the names as suggested

MarcoFalke commented at 8:15 pm on March 23, 2020: member

Some dumb style questions

MarcoFalke added the label Needs gitian build on Mar 23, 2020

DrahtBot commented at 11:17 pm on March 24, 2020: member

Gitian builds

File	commit 97b0687501cee77a9170f9e288755a5d268e9bd4(master)	commit 6027e5db1301c34630ab80753fbbf122287f046f(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz	`7260c91322d82300...`	`324f144a420a43c0...`
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz	`be65012c3a53b578...`	`083a94788d5bef03...`
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz	`9bac4388162dc0a8...`	`87939e23a3fb3a39...`
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz	`9e410f69d407a61d...`	`251fcda1191c9e97...`
bitcoin-0.19.99-osx-unsigned.dmg	`27f65bd66efda624...`	`7efaf2b59a375ba9...`
bitcoin-0.19.99-osx64.tar.gz	`6a195fe8a00dbde9...`	`839cc027b30ec786...`
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz	`ee932eb82f08241d...`	`72a85c60e59dca93...`
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz	`a0d5446f6f754b99...`	`f05635e2f8a1d098...`
bitcoin-0.19.99-win64-debug.zip	`dd039834c140ec2e...`	`1bc73a8e2d1fd9f3...`
bitcoin-0.19.99-win64-setup-unsigned.exe	`84de0daceca0cceb...`	`e3eec400a2ddf29b...`
bitcoin-0.19.99-win64.zip	`f49e4aeba287379b...`	`7b8ab2a1ad63b8fd...`
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz	`b0516e1c6d427605...`	`cc8bcc13fac25732...`
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz	`971606878e2a293e...`	`89976af337720a14...`
bitcoin-0.19.99.tar.gz	`c7400029d295c415...`	`5e4fcd30eb87ba9c...`
bitcoin-core-linux-0.20-res.yml	`5ea8b7b5ad29e51c...`	`e0ec98aa48179c40...`
bitcoin-core-osx-0.20-res.yml	`8f4eb71802425782...`	`689f8156b882edd4...`
bitcoin-core-win-0.20-res.yml	`2ddb9e211fda4101...`	`8fcaa0d7630cd4e2...`
linux-build.log	`8b32031c6542efaa...`	`17f6b3ecc32a91a4...`
osx-build.log	`759793b56c7053bb...`	`6ae5bde494095935...`
win-build.log	`026fbb8f65312bf4...`	`30aa785738fcb02b...`
bitcoin-core-linux-0.20-res.yml.diff		`c02f6a36171f305f...`
bitcoin-core-osx-0.20-res.yml.diff		`9c0d18b70c42e324...`
bitcoin-core-win-0.20-res.yml.diff		`2b292c3660dc288b...`
linux-build.log.diff		`c9938b4d46c38ff5...`
osx-build.log.diff		`56fe88d9196779f2...`
win-build.log.diff		`0877039d1f40e2c8...`

DrahtBot removed the label Needs gitian build on Mar 24, 2020

ryanofsky referenced this in commit b53cf5a3f7 on Mar 25, 2020

ryanofsky force-pushed on Mar 25, 2020

ryanofsky commented at 2:55 am on March 25, 2020: member

Thanks for reviews!

Updated ac0f2fb1095d40b3a851150f38199fc2966b546f -> b53cf5a3f73ecdd57e4b421f5cb26bfe175868a6 (pr/ipc-build.20 -> pr/ipc-build.21, compare) with suggested makefile style cleanups, also bumped libmultiprocess depends version again

MarcoFalke added the label Needs gitian build on Mar 25, 2020

MarcoFalke removed the label Needs gitian build on Mar 25, 2020

ryanofsky referenced this in commit 06d2f9128a on Mar 25, 2020

ryanofsky force-pushed on Mar 25, 2020

ryanofsky commented at 2:07 pm on March 25, 2020: member

Updated b53cf5a3f73ecdd57e4b421f5cb26bfe175868a6 -> 06d2f9128a4f5144cc459e04b7b6d2ab3316b105 (pr/ipc-build.21 -> pr/ipc-build.22, compare) to fix link errors building bitcoin-wallet:

0libbitcoin_util.a(libbitcoin_util_a-request.o): In function `JSONRPCReply[abi:cxx11](UniValue const&, UniValue const&, UniValue const&)':
1src/rpc/request.cpp:48: undefined reference to `UniValue::write[abi:cxx11](unsigned int, unsigned int) const'
2libbitcoin_util.a(libbitcoin_util_a-system.o): In function `ArgsManager::logArgsPrefix(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<UniValue, std::allocator<UniValue> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::vector<UniValue, std::allocator<UniValue> > > > > const&) const':
3src/util/system.cpp:875: undefined reference to `UniValue::write[abi:cxx11](unsigned int, unsigned int) const'

MarcoFalke added the label Needs gitian build on Mar 26, 2020

DrahtBot commented at 9:20 am on March 28, 2020: member

Gitian builds

File	commit 7f9dedb22dcd9550ca525c0e35fec38b2d59e029(master)	commit c002ad809b59aa8ce567800873070885f0ccdb97(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz	`8aa34845a916014d...`	`bd6942793eb07fb2...`
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz	`0fb3dd16db30ea00...`	`a8f27e1ec7d87279...`
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz	`797f185a8e62f6fe...`	`b78eb5d112c723ab...`
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz	`871094b510da0ac7...`	`31e4ee415a51cd7b...`
bitcoin-0.19.99-osx-unsigned.dmg	`a159b1b99b607907...`	`24b10dc0f2bc34c9...`
bitcoin-0.19.99-osx64.tar.gz	`1fb6370553afcb22...`	`e13c3c52d07ef04c...`
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz	`0b86916151aea5a3...`	`55b06b9f7423e743...`
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz	`4efb63771485711b...`	`cf6d05b98b38cbe3...`
bitcoin-0.19.99-win64-debug.zip	`dd825e6168d39c90...`	`f75cd9a0cf3df596...`
bitcoin-0.19.99-win64-setup-unsigned.exe	`d1f790ad8c257da0...`	`24413508dd6b0d2f...`
bitcoin-0.19.99-win64.zip	`49dc4119ae2a01c4...`	`22d14253e7d5122a...`
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz	`2a144f502390b6e3...`	`227750e520a68cc3...`
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz	`af6a5d2d555569a4...`	`8293519bae297403...`
bitcoin-0.19.99.tar.gz	`900859565b6ee1fe...`	`ad282d262bad2336...`
bitcoin-core-linux-0.20-res.yml	`70c720b679a19c93...`	`9dd1a1339e17c443...`
bitcoin-core-osx-0.20-res.yml	`5766a2f7c8d8547a...`	`2d15da69ef32afbd...`
bitcoin-core-win-0.20-res.yml	`a95a890f0d0cb882...`	`f2d0559ec218e6df...`
linux-build.log	`8510c43606df6b17...`	`4a96a552457d5dbb...`
osx-build.log	`b2fce58471126628...`	`21505fbaf972c7b8...`
win-build.log	`630e3d23ee0657f4...`	`96f20af64c26eec7...`
bitcoin-core-linux-0.20-res.yml.diff		`35321863651cd55c...`
bitcoin-core-osx-0.20-res.yml.diff		`576ffb08664acb15...`
bitcoin-core-win-0.20-res.yml.diff		`471bb35a8a28689b...`
linux-build.log.diff		`870b03fb1f7ef671...`
osx-build.log.diff		`431c004ca52a3a46...`
win-build.log.diff		`4ed0a21927ceb855...`

DrahtBot removed the label Needs gitian build on Mar 28, 2020

hebasto commented at 8:22 pm on March 28, 2020: member

Tested 06d2f9128a4f5144cc459e04b7b6d2ab3316b105 on Linux Mint 19.3 with depends.

Yes, it builds successfully, but…

Why is LIBMULTIPROCESS_LIBS neeeded for the single-binary bitcoind?

It’s not needed, removed entirely from this PR

It seems questionable for me not using LIBMULTIPROCESS_LIBS and LIBMULTIPROCESS_CFLAGS in src/Makefile.am entirely, as Libs and Cflags in lib/pkgconfig/libmultiprocess.pc are not empty.

Multiprocess build changes

autotools and automake changes to support multiprocess execution.

This adds a new --enable-multiprocess flag, and build configuration code to
detect libraries needed for multiprocess support. The --enable-multiprocess
flag builds new bitcoin-node and bitcoin-gui executables, which are updated in
https://github.com/bitcoin/bitcoin/pull/10102 to communicate across processes.
But for now they are functionally equivalent to existing bitcoind and
bitcoin-qt executables.

e6e44eedd5

libmultiprocess depends build d630646662

Set LD_LIBRARY_PATH consistently in travis tests

Remove inconsistency between functional and unit test environments and make it
possible to substitute bitcoin-qt and bitcoin-node in place of bitcoind in
python tests, or to link bitcoind against shared libraries.

787f40668d

Add multiprocess travis configuration d54f64c6c7

depends: Use default macos clang compiler

Suggested by Cory Fields <cory-nospam-@coryfields.com>
https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-595393546
as alternate workaround for problem described
https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-594600985

b919efadff

DrahtBot added the label Needs rebase on Apr 6, 2020

ryanofsky force-pushed on Apr 6, 2020

ryanofsky referenced this in commit 47158b22d5 on Apr 6, 2020

ryanofsky commented at 3:45 pm on April 6, 2020: member

Rebased 06d2f9128a4f5144cc459e04b7b6d2ab3316b105 -> b919efadff3d0393f4a8c3c1dc735f7ac5c665bb (pr/ipc-build.22 -> pr/ipc-build.23, compare) due to conflict with #18514

ryanofsky commented at 3:56 pm on April 6, 2020: member

re: #16367 (comment)

It seems questionable for me not using LIBMULTIPROCESS_LIBS

The point of this PR is to reduce the size and review burden of #10102. In order for that to be possible and to not do everything in a single giant PR, I need to be able to make smaller PRs (listed here) that add changes and bits of functionality which are initially unused. This PR pulls out all the more complicated build changes from #10102 here and leaves the simple build changes there. I consider usage of LIBMULTIPROCESS_LIBS to be a simple build change. I could add some usages of LIBMULTIPROCESS_LIBS here, but then the complaint could be that I’m adding unneeded dependencies. You have to draw the line somewhere when you are splitting things up, and this seemed like a reasonable place.

DrahtBot removed the label Needs rebase on Apr 6, 2020

hebasto commented at 7:20 pm on April 6, 2020: member

re-ACK b919efadff3d0393f4a8c3c1dc735f7ac5c665bb, rebased only since my previous review.

MarcoFalke merged this on Apr 10, 2020

MarcoFalke closed this on Apr 10, 2020

ryanofsky moved this from the "In progress" to the "Done" column in a project

theuni commented at 10:28 pm on April 10, 2020: member

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 commented at 10:35 pm on April 10, 2020: member

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.

MarcoFalke commented at 10:37 pm on April 10, 2020: member

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 commented at 10:46 pm on April 10, 2020: member

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

MarcoFalke commented at 11:13 pm on April 10, 2020: member

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:

 0[18:40] <MarcoFalke> cfields: Are you assuming that multiprocess was merged for 0.20.0rc1?
 1[18:40] <cfields> MarcoFalke: no. It's an enormous change even for master.
 2[18:40] <fanquake> No. We are just confused why it was merged at all
 3[18:40] <cfields> afaik we've never merged a half-decided feature?
 4[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.
 5[18:41] <MarcoFalke> I have reviewed the changes as well (not libmultiprocess)
 6[18:42] <MarcoFalke> If it is controversial, it should be reverted 
 7[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.
 8[18:42] <cfields> Rather, I don't know if I have a problem. I assume anything ryanofsky comes up with is reasonable :)
 9[18:43] <MarcoFalke> It was not clear from the discussion in the pull request that this was not ready
10[18:44] <cfields> MarcoFalke: I don't see how it can be ready unless the overall approach is agreed upon.
11[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.
12[18:47] <MarcoFalke> Or controversial
13[18:47] <fanquake> As Cory has just mentioned, we can't do it in 0.21.0 anyways, if it requires c++17 ?
14[18:48] <MarcoFalke> Ok, so the goal is to revert and then schedule for 0.22, together with C++17?
15[18:48] <fanquake> Given the discussion in [#16684](/bitcoin-bitcoin/16684/)
16[18:48] <gribble> [#16684](/bitcoin-bitcoin/16684/) | Discussion: upgrading to C++17 · Issue [#16684](/bitcoin-bitcoin/16684/) · bitcoin/bitcoin · GitHub
17[18:48] <cfields> MarcoFalke: again, I'm not arguing against the idea. I just don't think the the specifics have been discussed.
18[18:49] <MarcoFalke> Though, it doesn't require C++17, if the user doesn't ask for it
19[18:50] <fanquake> MarcoFalke: can you clarify https://github.com/chaincodelabs/libmultiprocess/blob/master/CMakeLists.txt#L84 ?
20[18:50] <MarcoFalke> fanquake: If you opt out of multiprocess, which is the default, you can go with C++11
21[18:51] <cfields> So we've merged something we can't build?
22[18:51] <fanquake> Sure, but that means we can't actually use multiprocess until after c++17 is a requirement for the project.
23[18:51] <MarcoFalke> cfields: Gitian could use it
24[18:52] <MarcoFalke> Aren't we building releases for 0.21.0 with C++17?
25[18:52] <MarcoFalke> The project stays at C++11
26[18:53] <MarcoFalke> Ok, to sum up: People like multiprocess as a concept, but the current implementation was not agree upon?
27[18:53] <MarcoFalke> *agreed
28[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?
29[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
30[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
31[18:58] <ryanofsky> right but this doesn't seem that different than the build change that added support for rapidcheck
32[18:58] <ryanofsky> just an optional thing turned off by default that we had build/depends support for
33[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.
34[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.
35[19:00] <ryanofsky> sure, i'm just making sure there isn't some misunderstanding about the change
36[19:02] <ryanofsky> whatever people think the next steps should be is fine with me
37[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.
38[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).
39[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
40[19:11] <cfields> ^^ +1
41[19:11] <MarcoFalke> All of this must happen before it is taken out of experimental

ryanofsky commented at 11:14 pm on April 10, 2020: member

@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 https://github.com/chaincodelabs/libmultiprocess/pull/25 to be able to use std::optional. It would be possible to have c++11 support if neccessary, too

MarcoFalke commented at 11:20 pm on April 10, 2020: member

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.

MarcoFalke commented at 11:26 pm on April 10, 2020: member

Further discussion about reverting the changes now:

0[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.
1[19:19] <cfields> MarcoFalke: up to you. I feel heard either way :)
2[19:20] <ryanofsky> either way's fine with me too
3[19:20] <MarcoFalke> fanquake: Mind flipping a coin?
4[19:21] <fanquake> I don't mind leaving it in, looking forward to thrashing all this out in some discussion

MarcoFalke referenced this in commit f29bd546ec on Apr 10, 2020

MarcoFalke referenced this in commit a5623ba89f on Apr 11, 2020

jnewbery commented at 3:47 pm on April 12, 2020: member

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 commented at 11:56 pm on April 12, 2020: member

Where is all this discussion taking place? @jnewbery It’s happening in the #bitcoin-builds channel. Logs available here: http://gnusha.org/bitcoin-builds/.

sidhujag referenced this in commit 292df1690f on Apr 13, 2020

sidhujag referenced this in commit 5b6b4cb7c5 on Apr 13, 2020

ryanofsky referenced this in commit 47477975ff on Apr 15, 2020

ryanofsky referenced this in commit f00f3ab15c on Apr 15, 2020

ryanofsky moved this from the "Done" to the "In progress" column in a project

ryanofsky commented at 6:01 pm on April 16, 2020: member

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:

What should steps and requirements be for moving chaincodelabs/libmultiprocess to bitcoin-core/, assuming this would be a better home for the library.
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)?

ryanofsky referenced this in commit c8672dfeb7 on Apr 16, 2020

ryanofsky referenced this in commit f80a689013 on Apr 20, 2020

ryanofsky referenced this in commit 1e94a2bcbc on Apr 22, 2020

ryanofsky referenced this in commit fba09de775 on Apr 22, 2020

ryanofsky referenced this in commit 114b2103ad on May 1, 2020

fanquake referenced this in commit 56611b0e24 on May 7, 2020

sidhujag referenced this in commit d51641ba68 on May 12, 2020

fanquake referenced this in commit 97b21b302a on May 21, 2020

sidhujag referenced this in commit 6a4320e7a9 on May 21, 2020

ryanofsky moved this from the "In progress" to the "Done" column in a project

jasonbcox referenced this in commit ce5bf0a880 on Sep 23, 2020

janus referenced this in commit 968e072979 on Nov 5, 2020

janus referenced this in commit d9bbecfcf9 on Nov 8, 2020

maaku referenced this in commit 59aef72c88 on Jan 23, 2021

maaku referenced this in commit dd6d81a504 on Jan 23, 2021

furszy referenced this in commit 424ea0949e on Jun 30, 2021

UdjinM6 referenced this in commit 66d80b6a30 on Jul 19, 2021

PhotoshiNakamoto referenced this in commit 3085f314c0 on Dec 11, 2021

DrahtBot locked this on Feb 15, 2022

Multiprocess build support #16367

Conflicts

Gitian builds

Gitian builds