Switch `MP_INCLUDE_DIR` to global property #168

hebasto commented at 2:04 PM on March 31, 2025: member

This PR automates handling of MP_INCLUDE_DIR in the target_capnp_sources function, removing the need to manually propagate it across directories for both this project and downstream projects.

in CMakeLists.txt:33 in b553830720 outdated

  29 | @@ -30,16 +30,14 @@ include("cmake/compat_config.cmake")
  30 |  include("cmake/pthread_checks.cmake")
  31 |  include(GNUInstallDirs)
  32 |  
  33 | -# Set convenience variables for subdirectories.
  34 | -set(MP_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include")
  35 | +# Set a convenience global property.

ryanofsky commented at 4:36 PM on March 31, 2025:

In commit "Switch MP_INCLUDE_DIR to global property" (b55383072016479f462fb019f90896f35a642450)

New comment seems to be dropping information about why a global variable or property is useful. Would suggest keeping more of original comment and changing "Set a convenience global property" to something like "Set MP_INCLUDE_DIR as a global property so target_capnp_sources function can use it, and its callers don't need to specify the include directory manually to avoid "error: Import failed: /mp/proxy.capnp" failures from capnproto. IMO, it's important for build systems to have comments like this explaining why they do things otherwise it's too easy to guess incorrectly what code was trying to do and break things when making changes.

hebasto commented at 5:01 PM on March 31, 2025:

Thanks! Taken.

in CMakeLists.txt:34 in b553830720 outdated

  29 | @@ -30,16 +30,14 @@ include("cmake/compat_config.cmake")
  30 |  include("cmake/pthread_checks.cmake")
  31 |  include(GNUInstallDirs)
  32 |  
  33 | -# Set convenience variables for subdirectories.
  34 | -set(MP_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include")
  35 | +# Set a convenience global property.
  36 | +set_property(GLOBAL PROPERTY LIBMULTIPROCESS_MP_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/include")

ryanofsky commented at 4:39 PM on March 31, 2025:

In commit "Switch MP_INCLUDE_DIR to global property" (b55383072016479f462fb019f90896f35a642450)

LIBMULTIPROCESS_MP prefix is redundant, would suggest calling the property just MP_INCLUDE_DIR or LIBMULTIPROCESS_INCLUDE_DIR, and I'd slightly prefer shorter MP_INCLUDE_DIR since this project has used "mp" prefix for namespacing since beginning, calling its c++ namespace mp::, installing C++ and capnproto include files in include/mp/, and naming the code generator mpgen. But either prefix would be ok.

hebasto commented at 5:02 PM on March 31, 2025:

Thanks! Reworked.

ryanofsky approved

ryanofsky commented at 4:50 PM on March 31, 2025: collaborator

Code review ACK b55383072016479f462fb019f90896f35a642450. Nice change that simplifies https://github.com/bitcoin/bitcoin/pull/31741 and seems to make good use of cmake's global property feature. I suggested some tweaks below but they could be followups.

Switch `MP_INCLUDE_DIR` to global property

This change automates handling of `MP_INCLUDE_DIR` in the
`target_capnp_sources` function, removing the need to manually propagate
it across directories for both this project and downstream projects.

a77c8e18eb

hebasto force-pushed on Mar 31, 2025

hebasto commented at 5:02 PM on March 31, 2025: member

@ryanofsky

Thank you for your review! Your feedback has been addressed.

ryanofsky approved

ryanofsky commented at 5:06 PM on March 31, 2025: collaborator

Code review ACK a77c8e18eb61df015741dc5359319147ad1a48b4

Thanks for updates! Will test this with simplification to #31741 and merge it if everything seems like it works.

ryanofsky merged this on Mar 31, 2025

ryanofsky closed this on Mar 31, 2025

hebasto deleted the branch on Mar 31, 2025

ryanofsky referenced this in commit d871f98b43 on Mar 31, 2025

ryanofsky referenced this in commit d6244f85c5 on Apr 2, 2025

janus referenced this in commit 85bcced4e9 on Sep 7, 2025

bitcoin-core locked this on Mar 31, 2026