build: Fix for vendored subdir in Bitcoin Core when using depends

theuni commented at 7:01 pm on January 30, 2025: contributor

This is a really wonky issue (it took me quite a while to figure out why it was complaining). It works around a quirky path issue that presents itself when using libmultiprocess as a subdir of Core and using a depends-built capnproto within the same source tree:

0CMake Error in src/ipc/libmultiprocess/CMakeLists.txt:
1  Target "multiprocess" INTERFACE_INCLUDE_DIRECTORIES property contains path:
2
3    "/home/cory/dev/bitcoin/depends/x86_64-pc-linux-gnu/include"
4
5  which is prefixed in the source directory.

It doesn’t present in https://github.com/bitcoin/bitcoin/pull/31741 because in that case, the lib is built in depends rather than as part of the source tree.

This is part of my work to always build from the source tree rather than from depends. I can push up the other changes (the EXTERNAL_MPGEN option )needed for that, but I wanted to break this one out in case it needed further discussion/explanation. It’s safe to go in as-is though, it should just be a no-op.

While I was at it, because the capnp headers spew annoying and harmless warnings when built with Core’s flags turned on, I marked them as SYSTEM.

See the commit description for more details.

build: split capnp includes into build/install interfaces and mark them as SYSTEM

We're not interested in warnings from Capnp's headers.

The interfaces will be treated differently in the next commit.

579fa4e46f

build: work around CMake quirk when building as a subdir of Bitcoin Core

When building as a subdir of Core and using depends, capnp includes may end up
being installed in a path like:
/dev/bitcoin/depends/HOST/include

It then detects that the install interface has been given an absolute path
inside of the project dir:
/dev/bitcoin/

In that case it throws an error because it wants paths within the project to
always be relative.

This shouldn't matter *at all* because we're never actually going to install
the lib anyway, but CMake populates the install interface despite the
EXCLUDE_FROM_ALL property being set.

Since other projects may wish to include libmultiprocess as a subdir and also
install it, don't use the subdir condition to check for this case. Instead,
test for the EXCLUDE_FROM_ALL on the directory, as if that's set, the parent
project won't be installing.

7ac1237ea3

hebasto commented at 7:17 pm on January 30, 2025: member

While I was at it, because the capnp headers spew annoying and harmless warnings when built with Core’s flags turned on, I marked them as SYSTEM.

A fix for https://github.com/chaincodelabs/libmultiprocess/issues/138?

in CMakeLists.txt:102 in 579fa4e46f outdated

 98+target_include_directories(multiprocess SYSTEM PUBLIC
 99+  $<BUILD_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}>
100+)
101+target_include_directories(multiprocess SYSTEM PUBLIC
102+  $<INSTALL_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}>
103+)

hebasto commented at 7:18 pm on January 30, 2025:

~~style nit: Why not a single call?~~

nm, did not see the following commit

hebasto commented at 7:22 pm on January 30, 2025: member

Hmm, indeed a hacky fix… I’ll try to look for alternatives.

theuni commented at 7:41 pm on January 30, 2025: contributor

Hmm, indeed a hacky fix… I’ll try to look for alternatives.

If you’d like to reproduce the error, you can try my branch here where I’ve been hacking stuff together: https://github.com/theuni/bitcoin/commits/pr31741-split-native/

It’s pretty much the same as what’s here. You can remove the if() to hit the bug.

Edit: That’s obviously a WIP branch, I’m sure I’ve broken some stuff there.

in CMakeLists.txt:111 in 7ac1237ea3

109+get_directory_property(mp_skip_install EXCLUDE_FROM_ALL)
110+if(NOT "${mp_skip_install}")
111+  target_include_directories(multiprocess SYSTEM PUBLIC
112+    $<INSTALL_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}>
113+  )
114+endif()

ryanofsky commented at 11:14 am on January 31, 2025:

In commit “build: work around CMake quirk when building as a subdir of Bitcoin Core” (7ac1237ea32aad4a174aaf1958fe2fe0ac129662)

This fix should work as you explained it, since when this build is a subdirectory of a bitcoin core build the libmultiprocess.a will be internal library that should not be installed and doesn’t need to propagate any include paths after installation.

But I think this fix could be cleaner if it included the first half of this diff https://github.com/chaincodelabs/libmultiprocess/issues/138#issuecomment-2624919124, that way both uses of CAPNP_INCLUDE_DIRECTORY here could be deleted, because the necessary include paths would already be brought in by the target_link_libraries call, so specifying it again in target_include_directories should be redundant.

It seemed like even that diff alone was enough to fix the problem with header warnings according to what we saw in Sjors CI job: https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2625235833. The header warnings are reported in #138 but they don’t happen locally for me locally (possibly because I use a nix shell which adds a bit of include/link magic for specified buildInputs)

theuni commented at 2:06 pm on January 31, 2025:

Thanks @ryanofsky, I’ll play with that. If it wasn’t clear from my description, I acknowledge that this is a weird/ugly fix and I don’t like it, so I’m up for any nicer alternative that fixes the problem.

As to the warnings, that’s another good example imo of why we want a unified in-tree build, so that everyone’s seeing the same thing. I’ll get the rest of the pre-requisites for that pushed up today. This is the ugly one, the other should be more straightforward (though I expect you’ll have some suggestions to tidy up the impl).

theuni commented at 3:30 pm on January 31, 2025:

But I think this fix could be cleaner if it included the first half of this diff https://github.com/chaincodelabs/libmultiprocess/issues/138#issuecomment-2624919124, that way both uses of CAPNP_INCLUDE_DIRECTORY here could be deleted, because the necessary include paths would already be brought in by the target_link_libraries call, so specifying it again in target_include_directories should be redundant.

I don’t really understand why, because I thought target_include_directories was kinda an implied subset of target_link_libraries, so I would’ve expect this to cause the same problem… but yes, that does seem to work :)

hebasto commented at 11:37 pm on February 1, 2025: member

From the CMake docs:

Generally, a dependency should be specified in a use of target_link_libraries() with the PRIVATE keyword if it is used by only the implementation of a library, and not in the header files. If a dependency is additionally used in the header files of a library (e.g. for class inheritance), then it should be specified as a PUBLIC dependency.

I believe, that all we need is target_link_libraries(multiprocess PUBLIC CapnProto::capnp) without low-level manipulations with CAPNP_INCLUDE_DIRECTORY.

purpleKarrot commented at 11:43 pm on February 1, 2025: contributor

I believe, that all we need is target_link_libraries(multiprocess PUBLIC CapnProto::capnp) without low-level manipulation with CAPNP_INCLUDE_DIRECTORY.

Yes, this is the correct approach. The only valid usecase for target_include_directories is to set the target’s own include directories. It should never be used for adding include directories originating from another target.

ryanofsky referenced this in commit 976f62fa02 on Feb 3, 2025

ryanofsky referenced this in commit 72326b5d1e on Feb 3, 2025

ryanofsky referenced this in commit 3d83c7aef1 on Feb 3, 2025

ryanofsky commented at 6:10 pm on February 3, 2025: collaborator

https://github.com/chaincodelabs/libmultiprocess/pull/146 should solve the original issue here so will close this

ryanofsky closed this on Feb 3, 2025

theuni commented at 6:18 pm on February 3, 2025: contributor

Good, thanks for the proper cleanup. I hated this :)

build: Fix for vendored subdir in Bitcoin Core when using depends #141