depends: Build the `native_capnp` and `capnp` packages with CMake

hebasto commented at 11:48 am on November 12, 2023: member

The first commit fixes two bugs when cross-compiling the capnp package on the master branch @ 160d23677ad799cf9b493eaa923b2ac080c3fb8e:

for x86_64-w64-mingw32 (see #28735 (comment)):

0libtool: link: x86_64-w64-mingw32-g++-posix -shared -nostdlib /usr/lib/gcc/x86_64-w64-mingw32/12-posix/../../../../x86_64-w64-mingw32/lib/dllcrt2.o /usr/lib/gcc/x86_64-w64-mingw32/12-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/12-posix -L/usr/lib/gcc/x86_64-w64-mingw32/12-posix/../../../../x86_64-w64-mingw32/lib -lstdc++ -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lkernel32 -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lkernel32 /usr/lib/gcc/x86_64-w64-mingw32/12-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
1/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x1dc): undefined reference to `__imp_inet_ntop'
2/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x44b): undefined reference to `__imp_inet_pton'
3collect2: error: ld returned 1 exit status

for arm64-apple-darwin:

0checking build system type... x86_64-pc-linux-gnu
1checking host system type... Invalid configuration `arm64-apple-darwin': machine `arm64-apple' not recognized
2configure: error: /bin/bash build-aux/config.sub arm64-apple-darwin failed

The second commit applies the same changes for the native_capnp package for consistency.

DrahtBot commented at 11:48 am on November 12, 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	ryanofsky
Concept ACK	fanquake

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 Nov 12, 2023

hebasto commented at 11:49 am on November 12, 2023: member

cc @ryanofsky

fanquake commented at 10:59 am on November 13, 2023: member

Any reason to not change native_capnp at the same time?

hebasto commented at 1:52 pm on November 13, 2023: member

Any reason to not change native_capnp at the same time?

~The libmultiprocess package build fails when linking the mpgen executable. Going to look into this issue, but that shouldn’t be a blocker here, right?~

fanquake commented at 1:59 pm on November 13, 2023: member

Not really, but it’s a bit odd to be building the same code, in two different ways, in depends. Would be good to know why one build fails and the other doesn’t.

hebasto force-pushed on Nov 13, 2023

hebasto renamed this:
~~depends: Build `capnp` with CMake~~
depends: Build the `native_capnp` and `capnp` packages with CMake
on Nov 13, 2023

hebasto commented at 2:37 pm on November 13, 2023: member

Any reason to not change native_capnp at the same time?

Done.

fanquake commented at 3:14 pm on November 30, 2023: member

Concept ACK - I think this will now also fix #26943, and the build failure coming from the lib vs lib64 discrepency.

in depends/packages/capnp.mk:27 in 2cdb2f8bd6 outdated

21@@ -25,3 +22,7 @@ endef
22 define $(package)_stage_cmds
23   $(MAKE) DESTDIR=$($(package)_staging_dir) install
24 endef
25+
26+define $(package)_postprocess_cmds
27+  rm -rf lib/pkgconfig

ryanofsky commented at 3:33 pm on December 1, 2023:

In commit “depends: Build capnp package with CMake” (2cdb2f8bd6aaff64803482e4128da34db34effa8)

Is this needed? It seems like a potential bug in libmultiprocess if its build system is getting confused by the pkgconfig install files.

hebasto commented at 4:06 pm on December 1, 2023:

Is this needed?

No, it isn’t. However, we usually try to minimize a prefix size by dropping unneeded stuff.

in depends/packages/libmultiprocess.mk:23 in d1604d4b1d outdated

19@@ -20,7 +20,7 @@ define $(package)_config_cmds
20 endef
21 
22 define $(package)_build_cmds
23-  $(MAKE)
24+  $(MAKE) multiprocess

ryanofsky commented at 3:36 pm on December 1, 2023:

In commit “depends: Build capnp package with CMake” (2cdb2f8bd6aaff64803482e4128da34db34effa8)

This change seems like a good way to speed things up by only building the libmultiprocess runtime library, not the code generation tool in the non-native cross build (assuming that is the intention here). But it would be good to write this in a comment so it is clear what is going on.

hebasto commented at 2:54 pm on December 4, 2023:

Although this change is an improvement, I dropped it in the recent push as it is not related to the changes in this PR.

ryanofsky approved

ryanofsky commented at 4:01 pm on December 1, 2023: contributor

Code review ACK d1604d4b1d1ee8df279a1776303e167cc3d06193. I left a question and suggestion, but they are not critical. Overall seems good to switch from autotools to cmake here. I’m actually not sure why I didn’t write this using cmake originally, since the cmake build has been around since capnproto 0.5. It also seems good to get rid of the special –disable-shared flag for android, since that was inconsistent with other platforms.

I think I understand how this change works around the mingw32 build issue #28735 (comment) reported upstream https://github.com/capnproto/capnproto/issues/1833, but I don’t understand how this would fix either of the two problems the closed PR #28846 was supposed to fix. Overall though using cmake consistently and dropping the android special case should make debugging simpler and at least help out that way.

DrahtBot requested review from fanquake on Dec 1, 2023

fanquake commented at 4:17 pm on December 1, 2023: member

but I don’t understand how this would fix either of the two problems the closed PR

You are right, this PR doesn’t fix at least the first problem, which is configure failing to find libmultiprocess:

0checking for libmultiprocess... no
1configure: error: --enable-multiprocess=yes option specified but libmultiprocess library was not found. May need to install libmultiprocess library, or specify install path with PKG_CONFIG_PATH environment variable. Running 'pkg-config --debug libmultiprocess' may be helpful for debugging.

ryanofsky commented at 4:30 pm on December 1, 2023: contributor

You are right, this PR doesn’t fix at least the first problem, which is configure failing to find libmultiprocess:

Make sense, I think the first commit from #28846 (156366f10aa38c612e26a1f93d8503786f3d3a34) is a good fix for that problem.

fanquake commented at 5:19 pm on December 1, 2023: member

Yea, happy for https://github.com/bitcoin/bitcoin/pull/28846/commits/156366f10aa38c612e26a1f93d8503786f3d3a34 to be cherry-picked in here.

ryanofsky commented at 6:07 pm on December 1, 2023: contributor

Yea, happy for 156366f to be cherry-picked in here.

In case you do cherry pick it, I suggested a comment to go along with the code here: #28846 (review). I think it would also be good link to https://github.com/chaincodelabs/libmultiprocess/pull/79 in the commit message or pull request description since that’s what triggered the problem.

Also seems fine if you don’t want to cherrypick it here, since it’s not directly related to this PR and might make it more confusing. I’m happy to open a new PR or review one if it doesn’t get cherrypicked.

hebasto commented at 11:19 am on December 4, 2023: member

Also seems fine if you don’t want to cherrypick it here, since it’s not directly related to this PR and might make it more confusing. I’m happy to open a new PR or review one if it doesn’t get cherrypicked. @ryanofsky

Please do. Thank you.

fanquake commented at 11:21 am on December 4, 2023: member

I’ll just reopen my PR then. Not sure we need a third PR, for what is a really straightforward bugfix.

hebasto marked this as a draft on Dec 4, 2023

depends: Build `capnp` package with CMake

This change fixes the `capnp` package cross-compiling for the
`x86_64-w64-mingw32` and `arm64-apple-darwin` platforms.

90389c95e9

depends: Build `native_capnp` package with CMake 11d797e3a0

hebasto force-pushed on Dec 4, 2023

hebasto marked this as ready for review on Dec 4, 2023

hebasto commented at 2:55 pm on December 4, 2023: member

Rebased to include the recent changes in the depends build system.

The PR description has been updated to mention one more fixed bug.

ryanofsky approved

ryanofsky commented at 3:45 pm on December 4, 2023: contributor

Code review ACK 11d797e3a078b8f5f0039a1073047d3f0a8c6cdc. Since last review arm64-apple-darwin platform is now mentioned in the commit message, and the change to depends/packages/libmultiprocess.mk in d1604d4b1d1ee8df279a1776303e167cc3d06193 which was unrelated (but probably still a good optimization) was reverted.

fanquake commented at 12:19 pm on December 5, 2023: member

The PR description has been updated to mention one more fixed bug.

Is this #28993, which turned out to not be a bug, or is this a different bug? Because that conclusion from #28993 was that cross-compilation for Darwin, using master, does work? If so, what is this PR fixing in regards to cross-compiling for arm-64-apple-darwin?

hebasto commented at 12:26 pm on December 5, 2023: member

The PR description has been updated to mention one more fixed bug.

Is this #28993, which turned out to not be a bug, or is this a different bug?

A different one, which is outdated config.guess and config.sub files in the capnp package.

Because that conclusion from #28993 was that cross-compilation for Darwin, using master, does work?

Right. Cross-compiling for x86_64-apple-darwin works indeed.

If so, what is this PR fixing in regards to cross-compiling for arm-64-apple-darwin?

CMake does not rely on config.guess and config.sub files at all.

fanquake merged this on Dec 5, 2023

fanquake closed this on Dec 5, 2023

hebasto deleted the branch on Dec 5, 2023

depends: Build the native_capnp and capnp packages with CMake #28856

Code Coverage

Reviews

depends: Build the `native_capnp` and `capnp` packages with CMake #28856