build: exclude mptest target from compile commands #35418

Fixes #35361

The mptest target includes generated .capnp.h files that don't exist at CMake configure time. When IWYU reads compile_commands.json, it sees these non-existent files and fails.

This PR excludes the mptest target from the compilation database by setting EXPORT_COMPILE_COMMANDS OFF, following the same pattern already applied to the mpcalculator, mpprinter, and mpexample targets in the same file.

This change belongs in cmake/libmultiprocess.cmake (the integration layer).

<!--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/35418.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

Reviewers, this pull request conflicts with the following ones:

• #35454 (cmake: skip missing example targets in libmultiprocess.cmake by ryanofsky)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

why not add this to line 37?

@willcl-ark Thank You! I've updated the PR. mptest is now grouped with the existing EXPORT_COMPILE_COMMANDS OFF targets.

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

@maflcko Thanks. I've squashed the commits and rebased the branch onto the latest upstream master.

Should we add mputil and mpgen here too? I assume we want to keep multiprocess? or should that also be excluded?

@willcl-ark I investigated mputil, mpgen, and multiprocess.

mptest appears to be the only target in libmultiprocess that uses target_capnp_sources():

target_capnp_sources(mptest ${CMAKE_CURRENT_SOURCE_DIR} mp/test/foo.capnp)

I couldn't find similar generated Cap'n Proto sources for mputil or mpgen, and the issue report specifically mentions missing generated .capnp.* , so the IWYU issue seems specific to mptest.

cc @ryanofsky @hebasto

Should we add mputil and mpgen here too? I assume we want to keep multiprocess? or should that also be excluded?

We consistently avoid linting code in all subtrees except for src/ipc/libmultiprocess. I vaguely recall the related discussion, but I can't track down the link right now.

#35361#issue-4503305159:

... we shouldn't be trying to lint a subtree in the first place...

I agree. This can be achieved in the same way it is done for src/secp256k1:

--- a/cmake/libmultiprocess.cmake
+++ b/cmake/libmultiprocess.cmake
@@ -3,6 +3,7 @@
 # file COPYING or https://opensource.org/license/mit/.
 
 function(add_libmultiprocess subdir)
+  set(CMAKE_EXPORT_COMPILE_COMMANDS OFF)
   # Set BUILD_TESTING to match BUILD_TESTS. BUILD_TESTING is a standard cmake
   # option that controls whether enable_testing() is called, but in the bitcoin
   # build a BUILD_TESTS option is used instead.
@@ -29,10 +30,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()

Fixes #35361

Not the entire issue, but only its first part:

/home/admin/actions-runner/_work/_temp/src/ipc/libmultiprocess/test/mp/test/test.cpp:5:10: fatal error: 'mp/test/foo.capnp.h' file not found 5 | #include <mp/test/foo.capnp.h> | ^~~~~~~~~~~~~~~~~~~~~

Code review ACK 4731049ba4f8a820cc4aa13a745e41bdfbee284a. Seems ok to exclude mptest from tidy & iwyu checks as this PR does. It also seems ok to go further and exclude all of libmultiprocess from tidy & iwyu checks as hebasto suggested #35418#pullrequestreview-4428862711

re: #35418#pullrequestreview-4428862711

#35361 (comment):

... we shouldn't be trying to lint a subtree in the first place...

I agree. This can be achieved in the same way it is done for src/secp256k1:

I don't agree, but it seems ok to disable some checks if they are too much trouble to maintain.

(Specifically, I don't understand why IWYU and cmake are having so much trouble here and why the IWYU job shouldn't do make bitcoin_ipc_headers bitcoin_ipc_test_headers to generate missing files.)

Generally, it seems to me like a good thing if subtrees are checked, to prevent things like remove_all from sneaking in, without other projects needing to duplicate all the same checks as bitcoin core.

Generally, it seems to me like a good thing if subtrees are checked...

It creates friction when upgrading a tool that requires accompanying code changes in a subtree.

It creates friction when upgrading a tool that requires accompanying code changes in a subtree.

Yes all tests and checks create friction. The question is whether the friction has more costs than benefits. I am objecting to the blanket statement that all linting should be disabled, not saying that every check needs to be enabled.

I think we all agree that we should not lint vendored thirdy-party subtrees, but perhaps there is still a somewhat-open question here around: "should we lint subtrees we actively co-maintain" (libmultiprocess) that we should clarify.

I think this PR is a fine minimal fix, and I'd be fine with a simple "never lint any subtrees" rule too, but there are potentially some benefits to some linting as Russ alludes to. As he points out, the lint in this case has arguably highlighed a cmake "bug" where it's not building the required files int he corretc order (or perhaps highlighting a dependency/order issue), and instead of fixing that we are disabling the check.

The flip side is that we can surely run IWYU and friends on libmultiprocess in its own repo :)

... we can surely run IWYU and friends on libmultiprocess in its own repo :)

+1

I think this is fine to merge now, and we can continue with followups (#35454, #35468), of which either will be possible after the next subtree pull (need either https://github.com/bitcoin-core/libmultiprocess/pull/287 or https://github.com/bitcoin-core/libmultiprocess/pull/291).

I was wrong about this being fine, it has just broken the build in some configurations (reported on IRC). Reverted for now in #35523. I guess we can fix this properly with the changes from the subtree.