build: Fix for vendored subdir in Bitcoin Core when using depends #141
Conversation
…em as SYSTEM We're not interested in warnings from Capnp's headers. The interfaces will be treated differently in the next commit.
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.
A fix for #138? |
|
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 Edit: That's obviously a WIP branch, I'm sure I've broken some stuff there. |
| get_directory_property(mp_skip_install EXCLUDE_FROM_ALL) | ||
| if(NOT "${mp_skip_install}") | ||
| target_include_directories(multiprocess SYSTEM PUBLIC | ||
| $<INSTALL_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}> | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
In commit "build: work around CMake quirk when building as a subdir of Bitcoin Core" (7ac1237)
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 #138 (comment), 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: bitcoin/bitcoin#30975 (comment). 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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
But I think this fix could be cleaner if it included the first half of this diff #138 (comment), 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 :)
|
From the CMake docs:
I believe, that all we need is |
Yes, this is the correct approach. The only valid usecase for |
cmake: Simplify capnp include handling Stop using lower level target_include_directories to add CAPNP_INCLUDE_DIRECTORY includes. This is no longer needed now that newer capnproto releases provide targets that set their own includes. This change is based on some similar cleanups originally implemented by Cory Fields <cory-nospam-@coryfields.com> in bitcoin-core#141
Stop using lower level target_include_directories to add CAPNP_INCLUDE_DIRECTORY includes. This is no longer needed now that newer capnproto releases provide targets that set their own includes. This change is based on some similar cleanups originally implemented by Cory Fields <cory-nospam-@coryfields.com> in bitcoin-core#141
72326b5 cmake: Simplify capnp include handling (Ryan Ofsky) 65c7048 cmake: Suppress compiler warnings from capnproto headers (Ryan Ofsky) Pull request description: Tweak `target_link_libraries` calls to correctly declare dependencies on capnproto libraries as public, so capnproto include directories get correctly added to downstream targets. Without this, compiler warnings can be triggered from capnproto headers because they are not treated like external headers. Fixes #138 This should be an alternative to #141 Top commit has no ACKs. Tree-SHA512: fc8349335db98bd578e59bd8206da6f440aa6be3468cbb08099e94a336dca4c6534a653b1cf25e79c6aef5481f873c8272df895eeeb57988bb621e5b705c562a
|
#146 should solve the original issue here so will close this |
|
Good, thanks for the proper cleanup. I hated this :) |
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:
It doesn't present in bitcoin/bitcoin#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_MPGENoption )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.