cmake: skip missing example targets in libmultiprocess.cmake #35454

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/supportrm changing 1 files +5 −1
  1. ryanofsky commented at 11:39 PM on June 3, 2026: contributor

    bitcoin-core/libmultiprocess#287 is removing the mpcalculator, mpprinter, and mpexample targets, which causes a configuration error when Bitcoin Core is compiled against the updated upstream libmultiprocess:

    CMake Error at cmake/libmultiprocess.cmake:37 ... Can not find target to add properties to: mpcalculator
    

    Fix this by iterating over the targets and only calling set_target_properties if the target exists. This is not needed for subtree builds (where upstream changes do not apply directly), but is useful for custom builds with an external libmultiprocess, and is needed to keep Bitcoin Core CI jobs working in the libmultiprocess repository.

  2. cmake: skip missing example targets in libmultiprocess.cmake
    bitcoin-core/libmultiprocess#287 is removing the mpcalculator,
    mpprinter, and mpexample targets, which causes a configuration error
    when Bitcoin Core is compiled against the updated upstream libmultiprocess:
    
      CMake Error at cmake/libmultiprocess.cmake:37 ...
      Can not find target to add properties to: mpcalculator
    
    Fix this by iterating over the targets and only calling
    set_target_properties if the target exists. This is not needed for
    subtree builds (where upstream changes do not apply directly), but is
    useful for custom builds with an external libmultiprocess, and is needed
    to keep Bitcoin Core CI jobs working in the libmultiprocess repository.
    
    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
    b29f764890
  3. DrahtBot added the label Build system on Jun 3, 2026
  4. DrahtBot commented at 11:39 PM on June 3, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35454.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK hebasto

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. hebasto commented at 2:09 PM on June 4, 2026: member

    Concept ACK on being defensive when integrating a subproject, regardless of https://github.com/bitcoin-core/libmultiprocess/pull/287.

    However, I believe that such issues should be handled by the subprojects themselves, as they shouldn't force parent projects to process CMake code they will never use. For example, for Libmultiprocess, this could be achieved with the following patch:

    --- a/cmake/libmultiprocess.cmake
    +++ b/cmake/libmultiprocess.cmake
    @@ -29,10 +29,4 @@ function(add_libmultiprocess subdir)
         # Add tests to "all" target so ctest can run them
         set_target_properties(mptest PROPERTIES EXCLUDE_FROM_ALL OFF)
       endif()
    -  # Exclude examples from compilation database, because the examples are not
    -  # built by default, and they contain generated c++ code. Without this
    -  # exclusion, tools like clang-tidy and IWYU that make use of compilation
    -  # database would complain that the generated c++ source files do not exist. An
    -  # alternate fix could build "mpexamples" by default like "mptests" above.
    -  set_target_properties(mpcalculator mpprinter mpexample PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
     endfunction()
    --- a/src/ipc/libmultiprocess/CMakeLists.txt
    +++ b/src/ipc/libmultiprocess/CMakeLists.txt
    @@ -260,5 +260,7 @@ add_custom_target(install-lib
     add_dependencies(install-lib multiprocess)
     
     # Example and test subdirectories
    -add_subdirectory(example EXCLUDE_FROM_ALL)
    +if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
    +  add_subdirectory(example EXCLUDE_FROM_ALL)
    +endif()
     add_subdirectory(test EXCLUDE_FROM_ALL)
    
  6. ryanofsky commented at 3:04 PM on June 5, 2026: contributor
     # Example and test subdirectories
    -add_subdirectory(example EXCLUDE_FROM_ALL)
    +if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
    +  add_subdirectory(example EXCLUDE_FROM_ALL)
    +endif()
     add_subdirectory(test EXCLUDE_FROM_ALL)
    

    This doesn't seem like a great code change to me, especially without any comment explaining why example and test directories are treated differently. In general, it seems better to me to have fewer assumptions about parent projects in subprojects, not more. So I think https://github.com/bitcoin-core/libmultiprocess/pull/287 is a cleaner way to avoid the fragility we are seeing here.

  7. hebasto commented at 3:16 PM on June 5, 2026: member

    In general, it seems better to me to have fewer assumptions about parent projects in subprojects...

    Sure. As long as it does not "force parent projects to process CMake code they will never use."

    The add_subdirectory() commands may be gated by dedicated build options.

  8. hebasto commented at 3:17 PM on June 5, 2026: member
  9. ryanofsky commented at 4:10 PM on June 5, 2026: contributor

    In general, it seems better to me to have fewer assumptions about parent projects in subprojects...

    Sure. As long as it does not "force parent projects to process CMake code they will never use."

    The add_subdirectory() commands may be gated by dedicated build options.

    Agree a build option would be slightly better than hardcoding a "will never use" assumption, which would be a false assumption. There are plenty of reasons parent projects might want to include examples, especially parent projects created for CI or deployment which may want to test & package them.

    Would probably oppose the build option, as well, though. Especially in this case when modularization seems like a better choice. Also, in other cases where defining all targets and having callers explicitly reference the targets they want to build is better than adding a level of indirection where options need to exist for every target and reconfiguring is necessary to build new targets. Whatever awkwardness currently exists around the compilation database seems like it should be fixed by code using the compilation database, because hardcoding targets to be excluded from database or adding options to exclude individual targets from compilation database add complexity that seems unnecessary if users of the database are can just use the parts they need and ignore the parts they don't need.

  10. ryanofsky referenced this in commit e1608e87c0 on Jun 11, 2026
  11. DrahtBot added the label Needs rebase on Jun 12, 2026
  12. DrahtBot removed the label Needs rebase on Jun 13, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-06-24 12:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me