Skip to content

cmake: Support being included with add_subdirectory #136

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

Jump to bottom
Merged
merged 2 commits into from
Jan 27, 2025
Merged

cmake: Support being included with add_subdirectory #136

merged 2 commits into from
Jan 27, 2025

Conversation

ryanofsky
Copy link
Collaborator

This implements changes needed to make the cmake project work well when it is not the top level project and has been included with add_subdirectory.

  • Avoids defining "tests" and "check" targets when project is not at the top level to avoid clashes. Add new "mptests" and "mpcheck" alternatives instead.
  • Renames "example" target to "mpexamples"
  • Renames "util" target to "mputil"
  • Makes various internal changes needed to resolve other issues. See commit messages for details.

@ryanofsky
cmake: Define and use MP_INCLUDE_DIR variable
1103f86 
This is needed to refer to the include directory when the cmake project is not
a top-level project (when it is included with add_subdirectory) because
${CMAKE_SOURCE_DIR}/include will no longer refer to the right location.

This is also nice because the variable can be used in the target_capnp_sources
function and simplify calls to that function.
@ryanofsky
cmake: Support being included with add_subdirectory
6adbb1d 
Make changes needed to make the cmake project work well when it is not the top
level project and has been included with add_subdirectory:

- Avoid defining "tests" and "check" targets when project is not at the top
  level to avoid clashes. Add new "mptests" and "mpcheck" alternatives instead.
- Rename "example" target to "mpexamples"
- Rename "util" target to "mputil"
- Export MP_INCLUDE_DIR variable to parent scope so target_capnp_sources()
  function can work well there.
- Replace uses of CMAKE_SOURCE_DIR with CMAKE_CURRENT_SOURCE_DIR
- Reset INCLUDE_DIRECTORIES property so include_directories calls in the parent
  project do not affect this project. This was causing errors because the
  bitcoin core build was adding global include directories and causing its init.h
  file to take precedence over local one.
@ryanofsky ryanofsky merged commit e89b2c6 into bitcoin-core:master Jan 27, 2025
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 27, 2025
@ryanofsky
depends: Update libmultiprocess library before converting to subtree
90b116b 
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
@ryanofsky ryanofsky mentioned this pull request Jan 27, 2025
Merged
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 27, 2025
@ryanofsky
depends: Update libmultiprocess library before converting to subtree
2221c88 
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Jan 28, 2025
@ryanofsky @Sjors
depends: Update libmultiprocess library before converting to subtree
b66fe2f 
This should be the final update to the libmultiprocess package via the depends
system. It brings in the libmultiprocess cmake changes from
bitcoin-core/libmultiprocess#136 needed to support
building as subtree. After this, a followup PR will add libmultiprocess as a
git subtree and depends will just use the git subtree instead of hardcoding its
own version hash.

Since there have been libmultiprocess API changes since the last update, this
commit also updates bitcoin code to be compatible with them.

This update brings in the following changes:

bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
bitcoin-core/libmultiprocess#94 c++ 20 cleanups
bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Jan 29, 2025
@fanquake
Merge #31740: depends: Update libmultiprocess library before converti…
ad2f932 
…ng to subtree

4e0aa18 test: Add test for IPC serialization bug (Ryan Ofsky)
2221c88 depends: Update libmultiprocess library before converting to subtree (Ryan Ofsky)

Pull request description:

  This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, followup PR #31741 will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash.

  Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them.

  This update has the following new changes since previous update #31105:

  bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object
  bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch
  bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
  bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races.
  bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code
  bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment
  bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds
  bitcoin-core/libmultiprocess#94 c++ 20 cleanups
  bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup
  bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
  bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links

ACKs for top commit:
  Sjors:
    ACK 4e0aa18
  vasild:
    ACK 4e0aa18
  TheCharlatan:
    ACK 4e0aa18

Tree-SHA512: 6d81cdf7f44762c7f476212295f6224054fd0a61315bb54786bc7758a2b33e5a2fce925c71e36f7bda320049aa14e7218a458ceb03dacbb869632c466c4789b0
hebasto
hebasto reviewed Jan 29, 2025
View reviewed changes
CMakeLists.txt
# Set convenience variables for subdirectories.
set(MP_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include")
if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
set(MP_STANDALONE TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

Once the minimum required CMake version is bumped up to 3.21+, the MP_STANDALONE variable might be replaced with PROJECT_IS_TOP_LEVEL.

hebasto
hebasto reviewed Jan 29, 2025
View reviewed changes
CMakeLists.txt
Comment on lines +34 to +36
# Set MP_INCLUDE_DIR for parent directories too, so target_capnp_sources calls
# in parent directories can use it and not need to specify include directories
# manually or see capnproto error "error: Import failed: /mp/proxy.capnp"
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not clarify why the propagating a variable to the parent namespace is better than specifying "include directories manually".

I think that the latter is better. I do think that add_subdirectory calls should not introduce any side effects in the call sites.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do think specifying include directories is better in general. But I don't believe libmultiprocess exporting its own include path to callers and then requiring callers to pass that include path back to it is necessarily better. For example when you use a toolchain's cc command, the cc command will allow you to override the toolchain include paths and require you to specify your own include paths, but will not force you to specify the toolchain include paths.

So I think the change in 1103f86 is reasonable and good simplification, but I'm also fine with reverting it or just updating the comment here. You can let me know what you prefer and of course a PR would be welcome.

This was referenced Jan 29, 2025
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hebasto hebasto hebasto left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryanofsky @hebasto