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

hebasto · 2023-11-12T11:48:36Z

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

for x86_64-w64-mingw32 (see depends: Bump to capnproto-c++-1.0.1 #28735 (comment)):

libtool: 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
/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x1dc): undefined reference to `__imp_inet_ntop'
/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x44b): undefined reference to `__imp_inet_pton'
collect2: error: ld returned 1 exit status

for arm64-apple-darwin:

checking build system type... x86_64-pc-linux-gnu
checking host system type... Invalid configuration `arm64-apple-darwin': machine `arm64-apple' not recognized
configure: 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 · 2023-11-12T11:48:39Z

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.

hebasto · 2023-11-12T11:49:31Z

cc @ryanofsky

fanquake · 2023-11-13T10:59:08Z

Any reason to not change native_capnp at the same time?

hebasto · 2023-11-13T13:52:45Z

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 · 2023-11-13T13:59:23Z

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 · 2023-11-13T14:37:19Z

Any reason to not change native_capnp at the same time?

Done.

fanquake · 2023-11-30T15:14:41Z

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

ryanofsky

Code review ACK d1604d4. 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 capnproto/capnproto#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.

depends/packages/capnp.mk

ryanofsky · 2023-12-01T15:36:40Z

depends/packages/libmultiprocess.mk

@@ -20,7 +20,7 @@ define $(package)_config_cmds
 endef

 define $(package)_build_cmds
-  $(MAKE)
+  $(MAKE) multiprocess


In commit "depends: Build capnp package with CMake" (2cdb2f8)

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.

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

fanquake · 2023-12-01T16:17:43Z

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:

checking for libmultiprocess... no
configure: 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 · 2023-12-01T16:30:30Z

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 (156366f) is a good fix for that problem.

fanquake · 2023-12-01T17:19:31Z

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

ryanofsky · 2023-12-01T18:07:31Z

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 (comment). I think it would also be good link to bitcoin-core/libmultiprocess#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 · 2023-12-04T11:19:58Z

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 · 2023-12-04T11:21:52Z

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

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

hebasto · 2023-12-04T14:55:17Z

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

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

ryanofsky

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

fanquake · 2023-12-05T12:19:00Z

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 · 2023-12-05T12:26:04Z

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.

…ckages with CMake 11d797e depends: Build `native_capnp` package with CMake (Hennadii Stepanov) 90389c9 depends: Build `capnp` package with CMake (Hennadii Stepanov) Pull request description: The first commit fixes two bugs when cross-compiling the `capnp` package on the master branch @ 160d236: - for `x86_64-w64-mingw32` (see bitcoin#28735 (comment)): ``` libtool: 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 /usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x1dc): undefined reference to `__imp_inet_ntop' /usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x44b): undefined reference to `__imp_inet_pton' collect2: error: ld returned 1 exit status ``` - for `arm64-apple-darwin`: ``` checking build system type... x86_64-pc-linux-gnu checking host system type... Invalid configuration `arm64-apple-darwin': machine `arm64-apple' not recognized configure: 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](bitcoin#28856 (comment)). ACKs for top commit: ryanofsky: Code review ACK 11d797e. Since last review arm64-apple-darwin platform is now mentioned in the commit message, and the change to `depends/packages/libmultiprocess.mk` in d1604d4 which was unrelated (but probably still a good optimization) was reverted. Tree-SHA512: c636e53073ce6fcda9724723bc59f3990fa9629a3b2f73d93dbc102a5a1badfbe8f4c5fef841f03588ebcad5cd4883f3ce32b128afcd75f6bc21eb801796a586

2f751ed fixup! Merge bitcoin#30567: qt, build: Drop `QT_STATICPLUGIN` macro (pasta) 142245d Merge bitcoin#29733: build, macos: Drop unused `osx_volname` target (fanquake) 02f81e5 Merge bitcoin#23511: require glibc 2.18+ (pasta) 9f0e4ae Merge bitcoin#29706: depends: set two CMake options globally (fanquake) be07bbe Merge bitcoin#28846: depends: fix libmultiprocess build on aarch64 (fanquake) 0dea194 Merge bitcoin#28856: depends: Build the `native_capnp` and `capnp` packages with CMake (fanquake) a23eee1 partial Merge bitcoin#23619: build: Propagate user-defined flags to host packages (fanquake) 7cdacdc Merge bitcoin#30513: depends: Bump `libmultiprocess` for CMake fixes (merge-script) 4f44750 Merge bitcoin#30491: Fix MSVC warning C4273 "inconsistent dll linkage" (merge-script) 5ba1309 Merge bitcoin#30567: qt, build: Drop `QT_STATICPLUGIN` macro (merge-script) Pull request description: ## Issue being fixed or feature implemented Batch of more PRs that I found during make work ## What was done? ## How Has This Been Tested? hasn't yet ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 2f751ed UdjinM6: utACK 2f751ed Tree-SHA512: 1d8433daaf8dc8c8f04beca1cf0281f0dc29a623e5e8ed941bcb556568d72d8ce0ac5b5c001b10645fdffaa4e7083b76d61075049b2418bb8dd9b5ba0f53a8a9

DrahtBot added the Build system label Nov 12, 2023

hebasto force-pushed the 231112-capnp branch from fe0796e to d1604d4 Compare November 13, 2023 14:36

hebasto changed the title ~~depends: Build capnp with CMake~~ depends: Build the native_capnp and capnp packages with CMake Nov 13, 2023

ryanofsky mentioned this pull request Nov 20, 2023

depends: fix libmultiprocess build on aarch64 #28846

Merged

ryanofsky approved these changes Dec 1, 2023

View reviewed changes

DrahtBot requested a review from fanquake December 1, 2023 16:01

hebasto marked this pull request as draft December 4, 2023 13:26

hebasto added 2 commits December 4, 2023 14:39

depends: Build capnp package with CMake

90389c9

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

depends: Build native_capnp package with CMake

11d797e

hebasto force-pushed the 231112-capnp branch from d1604d4 to 11d797e Compare December 4, 2023 14:52

hebasto marked this pull request as ready for review December 4, 2023 14:53

ryanofsky approved these changes Dec 4, 2023

View reviewed changes

fanquake merged commit 6d57909 into bitcoin:master Dec 5, 2023

hebasto deleted the 231112-capnp branch December 5, 2023 13:29

bitcoin locked and limited conversation to collaborators Dec 5, 2024

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

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

Uh oh!

Conversation

hebasto commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

hebasto commented Nov 12, 2023

Uh oh!

fanquake commented Nov 13, 2023

Uh oh!

hebasto commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Nov 13, 2023

Uh oh!

hebasto commented Nov 13, 2023

Uh oh!

fanquake commented Nov 30, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Dec 1, 2023

Uh oh!

ryanofsky commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Dec 1, 2023

Uh oh!

ryanofsky commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 4, 2023

Uh oh!

fanquake commented Dec 4, 2023

Uh oh!

hebasto commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 5, 2023

Uh oh!

Uh oh!

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

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

hebasto commented Nov 12, 2023 •

edited

Loading

DrahtBot commented Nov 12, 2023 •

edited

Loading

hebasto commented Nov 13, 2023 •

edited

Loading

hebasto Dec 4, 2023 •

edited

Loading

ryanofsky commented Dec 1, 2023 •

edited

Loading

ryanofsky commented Dec 1, 2023 •

edited

Loading

hebasto commented Dec 4, 2023 •

edited

Loading

fanquake commented Dec 5, 2023 •

edited

Loading