cmake: make target_capnp_sources use CURRENT dirs#289
Conversation
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>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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
left a comment
hebasto
left a comment
There was a problem hiding this comment.
ACK 620f297, 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_DIRandCMAKE_BINARY_DIRvariables, 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?
- Document the
ONLY_CAPNPoption. - Use absolute paths when specifying the
OUTPUTfiles.
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
OUTPUTfiles.
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 bitcoin-core#289 (review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
re: #289 (review) Thanks for the review!
Thanks I had claude implement both suggestions and added the first here. I didn't add the second one (c1f35af) 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 620f297 -> 615a94f ( |
3 participants
Tweak
target_capnp_sourcesto useCMAKE_CURRENT_SOURCE_DIRandCMAKE_CURRENT_BINARY_DIRinstead ofCMAKE_SOURCE_DIRandCMAKE_BINARY_DIRto compute include directory paths.This allows
target_capnp_sourcesto work properly inside libmultiprocess when libmultiprocess is used as a subproject andCMAKE_SOURCE_DIRpoint 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_DIRis a more predictable path thanCMAKE_SOURCE_DIRand is passed in by all currenttarget_capnp_sourcescallers.