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 :)
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.