depends: Bump to capnproto-c++-1.0.1

maflcko commented at 11:45 am on October 26, 2023: member

Reasons:

Debian is starting to ship this version in Trixie (https://packages.debian.org/trixie/capnproto), which will likely become the version shipped with Ubuntu 24.04 LTS. So testing with this version will help to find any issues before real users start to use those distro packages.
The feature is currently experimental, so bumping the version shouldn’t cause any production issues.
With multiprocess begin a priority project for 27.0, it seems better to do build system changes/bumps early, rather than later, to allow for more time testing them.

DrahtBot commented at 11:45 am on October 26, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, fanquake, ryanofsky

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

DrahtBot added the label Build system on Oct 26, 2023

maflcko commented at 12:08 pm on October 26, 2023: member

https://cirrus-ci.com/task/6337577544843264?logs=ci#L1948

 0  CXX      bitcoin_node-bitcoind.o
 1  CXX      init/bitcoin_node-bitcoin-node.o
 2  GEN      ipc/capnp/echo.capnp.h
 3  GEN      ipc/capnp/init.capnp.h
 4  CXX      ipc/capnp/libbitcoin_ipc_a-echo.capnp.o
 5  CXX      ipc/capnp/libbitcoin_ipc_a-init.capnp.o
 6  CXX      ipc/capnp/libbitcoin_ipc_a-echo.capnp.proxy-client.o
 7  CXX      ipc/capnp/libbitcoin_ipc_a-init.capnp.proxy-client.o
 8  CXX      ipc/capnp/libbitcoin_ipc_a-echo.capnp.proxy-server.o
 9  CXX      ipc/capnp/libbitcoin_ipc_a-init.capnp.proxy-server.o
10  CXX      ipc/capnp/libbitcoin_ipc_a-echo.capnp.proxy-types.o
11  CC       src/libsecp256k1_precomputed_la-precomputed_ecmult.lo
12  CXX      ipc/capnp/libbitcoin_ipc_a-init.capnp.proxy-types.o
13In file included from ipc/capnp/echo.capnp.proxy-server.c++:4:
14/ci_container_base/depends/i686-pc-linux-gnu/include/mp/proxy-types.h:122:13: error: 'mvCapture<(lambda at /ci_container_base/depends/i686-pc-linux-gnu/include/mp/proxy-types.h:123:13), capnp::CallContext<ipc::capnp::messages::Echo::DestroyParams, ipc::capnp::messages::Echo::DestroyResults> &>' is deprecated: Use C++14 generalized captures instead. [-Werror,-Wdeprecated-declarations]
15        kj::mvCapture(server_context.call_context,
16            ^

maflcko force-pushed on Oct 26, 2023

DrahtBot added the label CI failed on Oct 26, 2023

maflcko marked this as a draft on Oct 26, 2023

maflcko commented at 1:09 pm on October 26, 2023: member

I presume the same warning will be printed when compiling with system packages on Debian Trixie? If yes, I guess it could make sense to somehow fix the warnings? cc @ryanofsky

DrahtBot removed the label CI failed on Oct 26, 2023

ryanofsky commented at 8:40 pm on October 26, 2023: contributor

I presume the same warning will be printed when compiling with system packages on Debian Trixie? If yes, I guess it could make sense to somehow fix the warnings? cc @ryanofsky

Thanks for the PR. I created an issue https://github.com/chaincodelabs/libmultiprocess/issues/87 to track the deprecation warnings, and can probably implement a quick fix. Fixing them in libmultiprocess will require bumping that package as well, so it might be preferable to just pass -Wno-error=deprecated-declarations here. It seems a little extreme to treat calls to deprecated functions as hard errors anyway.

maflcko commented at 8:19 am on October 27, 2023: member

It seems a little extreme to treat calls to deprecated functions as hard errors anyway.

I think this is set in the CI, so that warnings are caught before they hit the users, who would otherwise then create issues that a local build on their system spits out warnings. Also, this is set to be aware of warnings in CI builds that wouldn’t occur locally. (Turning warnings into errors is the only way to surface them, because no one reads the logs of a green CI)

I’ll leave this pull in draft for a while. If someone fixes https://github.com/chaincodelabs/libmultiprocess/issues/87, I can pull it in here. If not, I’ll undraft at some point and open this pull for review.

maflcko added the label Waiting for other on Oct 27, 2023

maflcko force-pushed on Nov 1, 2023

maflcko removed the label Waiting for other on Nov 1, 2023

maflcko marked this as ready for review on Nov 1, 2023

maflcko commented at 7:59 am on November 1, 2023: member

Taken out of draft. Future improvements to chaincodelabs/libmultiprocess can be done in the future.

in ci/test/00_setup_env_i686_multiprocess.sh:18 in fa7d8377f7 outdated

14@@ -15,3 +15,4 @@ export GOAL="install"
15 export BITCOIN_CONFIG="--enable-debug CC='clang -m32' CXX='clang++ -m32' \
16 LDFLAGS='--rtlib=compiler-rt -lgcc_s' CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE'"
17 export TEST_RUNNER_ENV="BITCOIND=bitcoin-node"
18+export NO_WERROR=1  # Temporary workaround to avoid -Wdeprecated-declarations from KJ

ryanofsky commented at 10:11 pm on November 1, 2023:

In commit “depends: Bump to capnproto-c++-1.0.1” (fa7d8377f7541b0785049c4659dc61bf727bd3f3)

I think setting -Wno-error=deprecated-declarations would be a little better.

Treating warnings as errors can be helpful since warnings often signal potential bugs. But treating deprecated declarations as errors is not the same, since deprecated declarations in capnproto suggest that the code works fine, but there’s a just newer API available.

Not too important, though, since is easy to change libmultiprocess to avoid this warning while being backward compatible.

maflcko commented at 8:41 am on November 2, 2023:

I agree, but I don’t think there is an easy and recommended way to disable a single warning to not error. I could embed it into the compiler (CXX='clang++ -Wno-error=deprecated-declarations), but I presume this will be overridden by the configure logic to enable errors. I could set it in CXXFLAGS, but I presume this will either be overridden in the configure logic, or it will cause other horrible side-effects, such as disabling O2 (https://github.com/bitcoin/bitcoin/pull/28071/files). Finally, I could patch the configure logic, but that seems overkill for a temporary workaround.

ryanofsky approved

ryanofsky commented at 10:13 pm on November 1, 2023: contributor

Code review ACK fa7d8377f7541b0785049c4659dc61bf727bd3f3

TheCharlatan approved

TheCharlatan commented at 8:00 am on November 2, 2023: contributor

ACK fa7d8377f7541b0785049c4659dc61bf727bd3f3

maflcko assigned ryanofsky on Nov 2, 2023

hebasto commented at 9:53 am on November 2, 2023: member

 0$ make -C depends capnp MULTIPROCESS=1 HOST=x86_64-w64-mingw32
 1make: Entering directory '/home/hebasto/git/bitcoin/depends'
 2Building capnp...
 3make[1]: Entering directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7'
 4make  all-am
 5make[2]: Entering directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7'
 6/bin/bash ./libtool  --tag=CXX   --mode=link x86_64-w64-mingw32-g++-posix -I./src -I./src -DKJ_HEADER_WARNINGS -DCAPNP_HEADER_WARNINGS -DCAPNP_INCLUDE_DIR='"/home/hebasto/git/bitcoin/depends/x86_64-w64-mingw32/include"' -mthreads -pipe -std=c++17 -O2 -mthreads -DKJ_USE_FIBERS -release 1.0.1 -no-undefined -L/home/hebasto/git/bitcoin/depends/x86_64-w64-mingw32/lib -o libkj.la -rpath /home/hebasto/git/bitcoin/depends/x86_64-w64-mingw32/lib src/kj/cidr.lo src/kj/common.lo src/kj/units.lo src/kj/memory.lo src/kj/refcount.lo src/kj/array.lo src/kj/list.lo src/kj/string.lo src/kj/string-tree.lo src/kj/source-location.lo src/kj/hash.lo src/kj/table.lo src/kj/encoding.lo src/kj/exception.lo src/kj/debug.lo src/kj/arena.lo src/kj/io.lo src/kj/mutex.lo src/kj/thread.lo src/kj/time.lo src/kj/filesystem.lo src/kj/filesystem-disk-unix.lo src/kj/filesystem-disk-win32.lo src/kj/test-helpers.lo src/kj/main.lo src/kj/parse/char.lo  
 7libtool: link: rm -fr  .libs/libkj.dll.a
 8libtool: link: x86_64-w64-mingw32-g++-posix -shared -nostdlib /usr/lib/gcc/x86_64-w64-mingw32/10-posix/../../../../x86_64-w64-mingw32/lib/dllcrt2.o /usr/lib/gcc/x86_64-w64-mingw32/10-posix/crtbegin.o  src/kj/.libs/cidr.o src/kj/.libs/common.o src/kj/.libs/units.o src/kj/.libs/memory.o src/kj/.libs/refcount.o src/kj/.libs/array.o src/kj/.libs/list.o src/kj/.libs/string.o src/kj/.libs/string-tree.o src/kj/.libs/source-location.o src/kj/.libs/hash.o src/kj/.libs/table.o src/kj/.libs/encoding.o src/kj/.libs/exception.o src/kj/.libs/debug.o src/kj/.libs/arena.o src/kj/.libs/io.o src/kj/.libs/mutex.o src/kj/.libs/thread.o src/kj/.libs/time.o src/kj/.libs/filesystem.o src/kj/.libs/filesystem-disk-unix.o src/kj/.libs/filesystem-disk-win32.o src/kj/.libs/test-helpers.o src/kj/.libs/main.o src/kj/parse/.libs/char.o   -L/home/hebasto/git/bitcoin/depends/x86_64-w64-mingw32/lib -L/usr/lib/gcc/x86_64-w64-mingw32/10-posix -L/usr/lib/gcc/x86_64-w64-mingw32/10-posix/../../../../x86_64-w64-mingw32/lib -lstdc++ -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt /usr/lib/gcc/x86_64-w64-mingw32/10-posix/crtend.o  -mthreads -O2 -mthreads   -mthreads -o .libs/libkj-1-0-1.dll -Wl,--enable-auto-image-base -Xlinker --out-implib -Xlinker .libs/libkj.dll.a
 9/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x1e5): undefined reference to `__imp_inet_ntop'
10/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x467): undefined reference to `__imp_inet_pton'
11collect2: error: ld returned 1 exit status
12make[2]: *** [Makefile:1941: libkj.la] Error 1
13make[2]: Leaving directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7'
14make[1]: *** [Makefile:1525: all] Error 2
15make[1]: Leaving directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7'
16make: *** [funcs.mk:291: /home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7/./.stamp_built] Error 2
17make: Leaving directory '/home/hebasto/git/bitcoin/depends'

fanquake commented at 10:20 am on November 2, 2023: member

While we are changing this, could also add the following. I assume we don’t use the TLS lib for anything, so we can skip the configure checks, and building the library if OpenSSL happens to be found:

 0--- a/depends/packages/native_capnp.mk
 1+++ b/depends/packages/native_capnp.mk
 2@@ -5,6 +5,10 @@ $(package)_download_file=capnproto-c++-$($(package)_version).tar.gz
 3 $(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz
 4 $(package)_sha256_hash=0f7f4b8a76a2cdb284fddef20de8306450df6dd031a47a15ac95bc43c3358e09
 5 
 6+define $(package)_set_vars
 7+  $(package)_config_opts=--with-openssl=no
 8+endef
 9+
10 define $(package)_config_cmds
11   $($(package)_autoconf)
12 endef

maflcko commented at 11:20 am on November 2, 2023: member

make: *** [funcs.mk:291: /home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/capnp/1.0.1-f19a52e23f7/./.stamp_built] Error 2

I also tried 0.9.2, with another error:

 0# make capnp MULTIPROCESS=1 HOST=x86_64-w64-mingw32 
 1Building capnp...
 2make[1]: Entering directory '/bitcoin-core/depends/work/build/x86_64-w64-mingw32/capnp/0.9.2-e1454ca26e8'
 3make  all-am
 4make[2]: Entering directory '/bitcoin-core/depends/work/build/x86_64-w64-mingw32/capnp/0.9.2-e1454ca26e8'
 5depbase=`echo src/capnp/compiler/capnp.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
 6x86_64-w64-mingw32-g++-posix -DHAVE_CONFIG_H -I.   -I/bitcoin-core/depends/x86_64-w64-mingw32/include -I./src -I./src -DKJ_HEADER_WARNINGS -DCAPNP_HEADER_WARNINGS -DCAPNP_INCLUDE_DIR='"/bitcoin-core/depends/x86_64-w64-mingw32/include"' -mthreads -pipe -std=c++17 -O2 -mthreads -MT src/capnp/compiler/capnp.o -MD -MP -MF $depbase.Tpo -c -o src/capnp/compiler/capnp.o src/capnp/compiler/capnp.c++ &&\
 7mv -f $depbase.Tpo $depbase.Po
 8src/capnp/compiler/capnp.c++:27:10: fatal error: kj/win32-api-version.h: No such file or directory
 9   27 | #include <kj/win32-api-version.h>
10      |          ^~~~~~~~~~~~~~~~~~~~~~~~
11compilation terminated.
12make[2]: *** [Makefile:2307: src/capnp/compiler/capnp.o] Error 1
13make[2]: Leaving directory '/bitcoin-core/depends/work/build/x86_64-w64-mingw32/capnp/0.9.2-e1454ca26e8'
14make[1]: *** [Makefile:1500: all] Error 2
15make[1]: Leaving directory '/bitcoin-core/depends/work/build/x86_64-w64-mingw32/capnp/0.9.2-e1454ca26e8'
16make: *** [funcs.mk:291: /bitcoin-core/depends/work/build/x86_64-w64-mingw32/capnp/0.9.2-e1454ca26e8/./.stamp_built] Error 2

maflcko commented at 11:51 am on November 2, 2023: member

The compile error also happens outside of depends, so I guess someone can report it upstream?

maflcko force-pushed on Nov 2, 2023

maflcko commented at 12:55 pm on November 2, 2023: member

Disabled openssl, should be easy to re-ACK

maflcko added the label DrahtBot Guix build requested on Nov 3, 2023

fanquake commented at 11:24 am on November 3, 2023: member

so I guess someone can report it upstream?

https://github.com/capnproto/capnproto/issues/1833

in depends/packages/native_capnp.mk:9 in fa24f00fb6 outdated

 6 $(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz
 7-$(package)_sha256_hash=c9a4c0bd88123064d483ab46ecee777f14d933359e23bff6fb4f4dbd28b4cd41
 8+$(package)_sha256_hash=0f7f4b8a76a2cdb284fddef20de8306450df6dd031a47a15ac95bc43c3358e09
 9+
10+define $(package)_set_vars
11+  $(package)_config_opts=--with-openssl=no

TheCharlatan commented at 11:42 am on November 3, 2023:

Should this be added to the non-native package too?

Nit: --without-openssl?

depends: Bump to capnproto-c++-1.0.1 3333f14efa

maflcko force-pushed on Nov 3, 2023

TheCharlatan approved

TheCharlatan commented at 12:34 pm on November 3, 2023: contributor

Re-ACK 3333f14efac815ba3c885398a6795c7e8ce68d08

DrahtBot requested review from ryanofsky on Nov 3, 2023

maflcko added this to the milestone 27.0 on Nov 3, 2023

fanquake approved

fanquake commented at 2:56 pm on November 3, 2023: member

ACK 3333f14efac815ba3c885398a6795c7e8ce68d08 - the response from upstream is that if we submit a PR, they can take a look, so if anyone would like this to work for Windows, I’d suggest sending a patch.

maflcko commented at 3:33 pm on November 3, 2023: member

Given that their CI uses cmake (and passes), maybe an alternative could be to try that?

fanquake commented at 3:42 pm on November 3, 2023: member

maybe an alternative could be to try that?

I don’t think so? What they are doing (generating MinGW Makefiles) only works if you’re running on Windows, as far as I can tell. That CI job is running on Microsoft Windows Server 2022, from what I can see, i.e https://github.com/capnproto/capnproto/actions/runs/6711273836/job/18238293576#step:1:3.

If I try and perform the same CMake build running on Linux, I get:

0/capnproto# cmake -Hc++ -Bbuild-output -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=debug
1CMake Error: Could not create named generator MinGW Makefiles

DrahtBot commented at 2:30 am on November 4, 2023: contributor

Guix builds (on x86_64)

File	commit f23ac10ca5a322e87664b58233dccc4cb74c0570(master)	commit 0e574f761392f452eb568683ad5146c26f27e24f(master and this pull)
SHA256SUMS.part	`92659357fc97fc8e...`	`43b7703e770f4e69...`
*-aarch64-linux-gnu-debug.tar.gz	`8e85af4277097e63...`	`29235d3f9effb21b...`
*-aarch64-linux-gnu.tar.gz	`d29c219e45146ed7...`	`0a89c13e8df65473...`
*-arm-linux-gnueabihf-debug.tar.gz	`e9564bc7151e514d...`	`6c2b9eecda144471...`
*-arm-linux-gnueabihf.tar.gz	`0dea490de2900729...`	`33478e33bab0c843...`
*-arm64-apple-darwin-unsigned.tar.gz	`ed905a38a6985922...`	`879657ce107d71c7...`
*-arm64-apple-darwin-unsigned.zip	`5ff300abccc86e16...`	`02e6453a10220fe4...`
*-arm64-apple-darwin.tar.gz	`a46b5b97c3daaad9...`	`b051fabec9a76a43...`
*-powerpc64-linux-gnu-debug.tar.gz	`ed88e835f99cd5cc...`	`35f177f845188f9c...`
*-powerpc64-linux-gnu.tar.gz	`0be7a7cb50a4dd4a...`	`66440f0797da891b...`
*-powerpc64le-linux-gnu-debug.tar.gz	`9565b3c366b349e0...`	`012f3b387426fa5e...`
*-powerpc64le-linux-gnu.tar.gz	`01c7d06acaa2bda9...`	`97b0b45388b31404...`
*-riscv64-linux-gnu-debug.tar.gz	`bcc048b2f5be12e1...`	`ceabaec402ce9c51...`
*-riscv64-linux-gnu.tar.gz	`e9b064d98292bf76...`	`6b9b0a5686693345...`
*-x86_64-apple-darwin-unsigned.tar.gz	`a0715a07c1dcf9d3...`	`38c2503d1425a706...`
*-x86_64-apple-darwin-unsigned.zip	`31b6d7106c930a54...`	`b3d7bcd1aa427bab...`
*-x86_64-apple-darwin.tar.gz	`5ab5018cc8162e08...`	`99524f8b9917f631...`
*-x86_64-linux-gnu-debug.tar.gz	`250b3ad75c897648...`	`8e549faead23ec86...`
*-x86_64-linux-gnu.tar.gz	`2011c32866473392...`	`76f76710d25ca458...`
*.tar.gz	`2ea368703376018d...`	`20b85b23137c689b...`
guix_build.log	`cb9055f69265de9e...`	`2b27afb1388a3808...`
guix_build.log.diff		`37639fc82069bc71...`

DrahtBot removed the label DrahtBot Guix build requested on Nov 4, 2023

ryanofsky approved

ryanofsky commented at 8:57 pm on November 4, 2023: contributor

Code review ACK 3333f14efac815ba3c885398a6795c7e8ce68d08

Am wondering if we know why the guix build succeeds and CI passes if the ubuntu mingw build fails manually. Is that expected? Would it make any sense to change what platforms CI covers, or is the failing build platform pretty close to the ones that succeed and the failure is just a quirk?

maflcko commented at 5:36 pm on November 5, 2023: member

I don’t think the CI win64-cross build has multiprocess enabled. There is only one non-windows one (multiprocess, i686, DEBUG). Also, I don’t think guix has multiprocess enabled, and probably won’t have it enabled until at least mid-next year. Anyone is welcome to fix the win64-cross multiprocess build, and even enable it in CI or guix, if they want to.

fanquake merged this on Nov 5, 2023

fanquake closed this on Nov 5, 2023

ryanofsky commented at 7:59 pm on November 5, 2023: contributor

I don’t think the CI win64-cross build has multiprocess enabled

Thanks, I forgot it would only be built if that flag is enabled

maflcko deleted the branch on Nov 6, 2023

in depends/packages/native_libmultiprocess.mk:2 in 3333f14efa

0@@ -1,8 +1,8 @@
1 package=native_libmultiprocess
2-$(package)_version=1af83d15239ccfa7e47b8764029320953dd7fdf1
3+$(package)_version=61d5a0e661f20a4928fbf868ec9c3c6f17883cc7

ryanofsky commented at 8:23 pm on November 17, 2023:

I think it probably wasn’t required to bump libmultiprocess at the same time as bumping capnproto, but good to do it anyway since to provide test coverage for newer code. Changes in the bump were:

chaincodelabs/libmultiprocess#79
chaincodelabs/libmultiprocess#83
chaincodelabs/libmultiprocess#84
chaincodelabs/libmultiprocess#85
chaincodelabs/libmultiprocess#86

https://github.com/chaincodelabs/libmultiprocess/compare/1af83d15239ccfa7e47b8764029320953dd7fdf1...61d5a0e661f20a4928fbf868ec9c3c6f17883cc7

ryanofsky referenced this in commit 21bfee0720 on Nov 17, 2023

ryanofsky commented at 11:09 pm on November 17, 2023: contributor

PR description lists a few reasons for bumping the capnproto version, but it doesn’t actually mention the original motivation, which was to fix a C++20 compiler issue: #28349 (comment), https://github.com/capnproto/capnproto/pull/1618. Just wanted to note this explicitly

depends: Bump to capnproto-c++-1.0.1 #28735

Code Coverage

Reviews

Guix builds (on x86_64)