Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
theuni wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
579fa4e
build: split capnp includes into build/install interfaces and mark th…
theuni Jan 30, 2025
7ac1237
build: work around CMake quirk when building as a subdir of Bitcoin Core
theuni Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,24 @@ target_include_directories(multiprocess PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
${CAPNP_INCLUDE_DIRECTORY})
)

target_include_directories(multiprocess SYSTEM PUBLIC
$<BUILD_INTERFACE:${CAPNP_INCLUDE_DIRECTORY}>
)

# Hack to avoid exporting a path "which is prefixed in the source directory".
# If we're in another project's subdir and capnp is installed somewhere inside
# that project (as it is with depends in Bicoin Core) CMake refuses to allow it
# in the install interface. Use the "EXCLUDE_FROM_ALL" property as a proxy for
# detecting this condition, as in that case install won't happen anyway.
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()
Comment on lines +106 to +111
Copy link
Copy Markdown
Collaborator

@ryanofsky ryanofsky Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@theuni theuni Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@theuni theuni Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


target_link_libraries(multiprocess PRIVATE CapnProto::capnp)
target_link_libraries(multiprocess PRIVATE CapnProto::capnp-rpc)
target_link_libraries(multiprocess PRIVATE CapnProto::kj)
Expand Down