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 ( |
8412fcdc65 Merge bitcoin-core/libmultiprocess#295: Mark Waiter m_cv as guarded by m_mutex 1593ee2d18 Merge bitcoin-core/libmultiprocess#294: test: Add passDouble smoke test 9885d7dd33 Merge bitcoin-core/libmultiprocess#286: proxy-client: fix TSan data race in clientDestroy fa35501c4f Mark Waiter m_cv as guarded by m_mutex faaedb11f8 test: Add passDouble smoke test 733c64318d Merge bitcoin-core/libmultiprocess#292: type-number: fix clang-tidy modernize-use-nullptr 9cc3479ab3 Merge bitcoin-core/libmultiprocess#291: cmake: Add `mp_headers` custom target 201abd9e3a Merge bitcoin-core/libmultiprocess#289: cmake: make target_capnp_sources use CURRENT dirs 99820c8aec Merge bitcoin-core/libmultiprocess#279: doc: Add comments to FIELD_* constants in proxy.h 73b985540c Merge bitcoin-core/libmultiprocess#278: doc: Fix and expand design.md e7e91b2e23 Merge bitcoin-core/libmultiprocess#277: Add std::unordered_set support and a helper BuildList to dedup list build handlers 91a951f59a tidy fix: modernize-use-nullptr 16362f42d0 cmake: Add `mp_headers` custom target 615a94fe3a cmake: document ONLY_CAPNP option in target_capnp_sources 90982f75c6 mpgen: iwyu changes required by previous commit 25bb3e67f3 proxy-client: fix TSan data race in clientDestroy 620f297f31 cmake: make target_capnp_sources use CURRENT dirs 9de4b885aa test: use camelCase + $Proxy.name for FooStruct fields 011b91793d type: add std::unordered_set support 20d19b9644 proxy: add BuildList helper and dedup map/set/vector build handlers e863c6cdf6 doc: Add comments to FIELD_* constants in proxy.h 18db0ab957 doc: Fix and expand design.md 61de697536 Merge bitcoin-core/libmultiprocess#273: proxy-client: tolerate exceptions from remote destroy during cleanup 9cec9d6ca5 Merge bitcoin-core/libmultiprocess#243: mpgen: support primitive std::optional struct fields 4aaff11374 Merge bitcoin-core/libmultiprocess#238: cmake, ci: updates for recent nixpkgs 2ac55a56b5 Merge bitcoin-core/libmultiprocess#218: Better error and log messages 6de92e1c73 proxy-client: tolerate exceptions from remote destroy during cleanup 90be8354d4 test: regression for ~ProxyClient destroy after peer disconnect 3c69d125a1 Merge bitcoin-core/libmultiprocess#260: event loop: tolerate unexpected exceptions in `post()` callbacks b8a48c65e6 event loop: tolerate unexpected exceptions in `post()` callbacks f787863d2c Merge bitcoin-core/libmultiprocess#270: doc: Bump version 10 > 11 a22f602910 doc: Bump version 10 > 11 4eae445d6d debug: Add TypeName() function and log statements for Proxy objects being created and destroyed f326c5b1b7 logging: Add better logging on IPC server-side failures 6dbfa56a04 mpgen: support primitive std::optional struct fields 8d1277deb5 mpgen refactor: add AccessorType function db716bbcba mpgen refactor: Move field handling code to FieldList class db7acb3ce2 ci: Fix shell.nix compatibility with CMake 4.0 91a7759a9a cmake: Fix IWYU in nix by adding CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 8412fcdc659e1379f9b4dea896c26bc1c5f3afa8
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.