cmake: make target_capnp_sources use CURRENT dirs #289

pull ryanofsky wants to merge 2 commits into bitcoin-core:master from ryanofsky:pr/starget changing 1 files +13 −8

ryanofsky commented at 1:10 AM on June 4, 2026: collaborator

Tweak target_capnp_sources to use CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_BINARY_DIR instead of CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR to compute include directory paths.

This allows target_capnp_sources to work properly inside libmultiprocess when libmultiprocess is used as a subproject and CMAKE_SOURCE_DIR point somewhere outside of it, as happens in support branch used by #287.

This is also a good change to make more generally because CMAKE_CURRENT_SOURCE_DIR is a more predictable path than CMAKE_SOURCE_DIR and is passed in by all current target_capnp_sources callers.

cmake: make target_capnp_sources use CURRENT dirs

TargetCapnpSources.cmake computed the build-side include directory by
computing file(RELATIVE_PATH) from CMAKE_SOURCE_DIR to include_prefix
and appending the result to CMAKE_BINARY_DIR. This assumes include_prefix
is inside CMAKE_SOURCE_DIR, which holds when master is the top-level
cmake project but breaks when it is added as a subdirectory of an external
project (e.g. the support branch) whose source root is elsewhere.

In that case the relative path contains ".." and the resulting
build_include_prefix points to a nonexistent directory instead of
CMAKE_CURRENT_BINARY_DIR where the generated headers actually live.

Fix by anchoring on CMAKE_CURRENT_SOURCE_DIR / CMAKE_CURRENT_BINARY_DIR
instead of the top-level CMAKE_SOURCE_DIR/BINARY_DIR. Since cmake
mirrors source-tree structure into the binary tree, the mapping is
equivalent for any include_prefix inside CMAKE_SOURCE_DIR, and correct
for paths outside it.

Also update docstring and example to use CMAKE_CURRENT_SOURCE_DIR.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

620f297f31

DrahtBot commented at 1:10 AM on June 4, 2026: none


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK hebasto

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #288 (Create support branch for CI scripts, documentation, and examples by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
hebasto approved
hebasto commented at 12:40 PM on June 4, 2026: member
ACK 620f297f311718f3d186b716676779e046e3255c, tested with the Bitcoin Core master branch where relative_path is now empty for all call sites.

From Professional CMake: A Practical Guide, 22nd Edition, Section 8.5:

Where possible, avoid using the CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR variables, as these typically break the ability of the project to be incorporated into a larger project hierarchy.

While touching this file, could we add some more improvements?
1. Document the ONLY_CAPNP option.
2. Use absolute paths when specifying the OUTPUT files. From Professional CMake: A Practical Guide, 22nd Edition, Section 20.3:
  
  If the output files are specified with no path or with a relative path, they are normally treated as relative to the current binary directory. In specific circumstances, they can be treated as relative to the current source directory instead ... To avoid that ambiguity, projects should always use absolute paths when specifying the OUTPUT files.

Type	Reviewers
ACK	hebasto

cmake: document ONLY_CAPNP option in target_capnp_sources

The ONLY_CAPNP keyword was parsed but not mentioned in the function's
docstring. Add a description explaining what it does and when to use it.

Suggested by hebasto in
https://github.com/bitcoin-core/libmultiprocess/pull/289#pullrequestreview-4427856835

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

615a94fe3a

hebasto approved
hebasto commented at 2:22 PM on June 5, 2026: member

re-ACK 615a94fe3a213380eb50cde82e4b4d718bf63dd5.
ryanofsky commented at 2:41 PM on June 5, 2026: collaborator

re: #289#pullrequestreview-4427856835

Thanks for the review!

While touching this file, could we add some more improvements?

Thanks I had claude implement both suggestions and added the first here. I didn't add the second one (c1f35afeac28c4bda58378da2caf9ad32407f7ba) because the advice to prefer absolute paths in this case sounds faulty to me, without knowing further details. (It sounds like the reason could be to hide cmake bugs, but I don't have the book.) There are many cases where absolute paths are better to use, but also cases where they are worse. In general they can make commands harder to read and mistakes and inconsistencies harder to notice. Switching to them can discard semantic information and make code more complicated fragile and buggy it needs to strip and re-add them. The bug this PR fixes happens in code failing to stripping out and add path prefixes, so is actually an example of a bug that would not happen if relative paths were used.



Added 1 commits 620f297f311718f3d186b716676779e046e3255c -> 615a94fe3a213380eb50cde82e4b4d718bf63dd5 (pr/starget.1 -> pr/starget.2, compare) documenting ONLY_CAPNP option
ryanofsky merged this on Jun 9, 2026
ryanofsky closed this on Jun 9, 2026

Contributors

ryanofsky

DrahtBot

hebasto

Linked (view graph)

#287 Split repository into `master` (library source) and `support` (CI, docs, examples) branches.

cmake: make target_capnp_sources use CURRENT dirs #289

Reviews

Conflicts