cmake: Move IPC tests to `ipc/test` #33774

This PR follows up on #33445 and:

1. Organizes the IPC tests in the same way as the wallet tests.
2. Removes no longer needed src/test/.clang-tidy.in.

See the previous discussion:

• #33445 (review)
• #33445#pullrequestreview-3411868329

Additionally, the locality of the bitcoin_ipc_test build target description has been improved.

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

<!--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:

• #29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
• #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
• #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
• #10102 (Multiprocess bitcoin 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-->

cc @ryanofsky

I added a commit with further improvements.

should src/test/kernel also be moved to src/kernel/test to keep the project structure Consistent. ?

should src/test/kernel also be moved to src/kernel/test to keep the project structure Consistent. ?

Maybe.

Moving IPC tests provides additional benefits. I'm not sure the same justification applies to the kernel tests, so let's leave them for a follow-up.

Code review ACK f26c8b03f968843e6e1381d1e1721f4379f7e7ab. Nice change avoiding the need to duplicate clang-tidy suppressions. Thanks for the followup!

The feedback from @ryanofsky has been addressed.

Additionally, PROJECT_SOURCE_DIR were replaced with CMAKE_CURRENT_SOURCE_DIR.

ACK 866bbb98fd365962840ee99df0d9f7ad557cc025

On macOS 26.0.1 I ran the full test suite with -DENABLE_IPC set to OFF and ON.

ACK 866bbb98fd365962840ee99df0d9f7ad557cc025

This PR moves the test files to the IPC directory, equal to the wallet folder structure. This removes the tidy exception file from the test folder which was only applicable (and there) for the ipc_tests. And some CMakeLists changes to accommodate the design choices made in mpgen (regarding prefix paths capnp)

Have invalidated a ipc_test to see if the ipc_tests would indicate a failure (it did)

@ryanofsky

Would you like to take another look at this PR?

Code review ACK 866bbb98fd365962840ee99df0d9f7ad557cc025, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time.

As mentioned below i am confused about include paths used to build the bitcoin_ipc librariy which somehow look like -I/.../bitcoin/build/src -I/.../bitcoin/src -I/.../bitcoin/build for me. The last path in that list pointing at the build root should not be there I think, and the bad target_capnp_sources calls fixed here were relying on it to work. It would be nice to remove if we can find the source.

Going to merge this. Leaving the following for follow-ups:

The last path in that list pointing at the build root should not be there I think, and the bad target_capnp_sources calls fixed here were relying on it to work. It would be nice to remove if we can find the source.

This broke compilation for me:

$ git checkout 866bbb98fd365962840ee99df0d9f7ad557cc025
$ cmake --build ../build-rel -j 40 -t bitcoin_ipc_test
[  0%] Built target mputil
[ 50%] Built target mpgen
[ 50%] Built target multiprocess
[ 50%] Built target bitcoin_ipc_headers
[ 50%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.cpp.o
[ 50%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-client.c++.o
[ 50%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-types.c++.o
[100%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-server.c++.o
In file included from build-rel/src/ipc/test/ipc_test.capnp.proxy-types.c++:5:
build-rel/src/test/ipc_test.capnp.proxy.h:6:10: fatal error: src/test/ipc_test.capnp.h: No such file or directory
    6 | #include <src/test/ipc_test.capnp.h> // IWYU pragma: keep
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
gmake[3]: *** [src/ipc/CMakeFiles/bitcoin_ipc_test.dir/build.make:157: src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-types.c++.o] Error 1
gmake[3]: *** Waiting for unfinished jobs....
In file included from build-rel/src/ipc/test/ipc_test.capnp.proxy-server.c++:6:
build-rel/src/test/ipc_test.capnp.proxy.h:6:10: fatal error: src/test/ipc_test.capnp.h: No such file or directory
    6 | #include <src/test/ipc_test.capnp.h> // IWYU pragma: keep
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

The parent commit still compiles. Turning ENABLE_IPC off also compiles. Turning ENABLE_IPC back on fails again. Manually removing all the generated *ipc_test.capnp* seems to make it work at least.

This broke compilation for me:

$ git checkout 866bbb98fd365962840ee99df0d9f7ad557cc025
$ cmake --build ../build-rel -j 40 -t bitcoin_ipc_test
...

Building in a clean build directory should help.

Building in a clean build directory should help.

Huh? The point of dependency based build systems is that you shouldn't need to do that; and when it breaks, it's an indication that some dependency is missing...

Building in a clean build directory should help.

Huh? The point of dependency based build systems is that you shouldn't need to do that; and when it breaks, it's an indication that some dependency is missing...

~The build system creates all artifacts within the build subdirectory (aka the build tree), including files generated by the libmultiprocess mpgen tool from *.capnp files.~

~This change modifies both the content and the relative paths of those generated files. The compilation error you encountered clearly shows that it is caused by stale versions of ipc_test.capnp.proxy-types.c++ and ipc_test.capnp.proxy-server.c++ remaining in the build tree. Removing these files is therefore required for a successful build of this PR. I don't see how this constitutes "an indication that some dependency is missing".~

@ajtowns

Please disregard my previous comment.

Are you using the "Ninja" CMake generator?

Nope, just using defaults, and cmake is just running gmake -f Makefile.

Trying it with ninja gives the same result -- fresh build dir, build ae2e438b257f2b5322080087ed7dc8843d4f9cca works fine; checkout 866bbb98fd365962840ee99df0d9f7ad557cc025 (without cleaning the build dir), build fails, run rm $(find ../build-rel/ | grep ipc_test.capn), build succeeds.

@ajtowns

This broke compilation for me...

The change responsible for this behaviour is the switch from PROJECT_SOURCE_DIR to CMAKE_CURRENT_SOURCE_DIR in the include_prefix argument discussed by @ryanofsky in #33774 (review).

I'm not sure how to handle such a change as a "dependency" that triggers re-build.

@ajtowns

This broke compilation for me...

The change responsible for this behaviour is the switch from PROJECT_SOURCE_DIR to CMAKE_CURRENT_SOURCE_DIR in the include_prefix argument discussed by @ryanofsky in #33774 (comment).

I'm not sure how to handle such a change as a "dependency" that triggers re-build.

The following diff should handle such behaviour:

--- a/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake
+++ b/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake
@@ -78,7 +78,7 @@ function(target_capnp_sources target include_prefix)
     add_custom_command(
       OUTPUT ${capnp_file}.c++ ${capnp_file}.h ${capnp_file}.proxy-client.c++ ${capnp_file}.proxy-types.h ${capnp_file}.proxy-server.c++ ${capnp_file}.proxy-types.c++ ${capnp_file}.proxy.h
       COMMAND ${MPGEN_BINARY} ${CMAKE_CURRENT_SOURCE_DIR} ${include_prefix} ${CMAKE_CURRENT_SOURCE_DIR}/${capnp_file} ${TCS_IMPORT_PATHS} ${mp_include_dir}
-      DEPENDS ${capnp_file}
+      DEPENDS ${capnp_file} ${CMAKE_CURRENT_FUNCTION_LIST_FILE}
       VERBATIM
     )
     # Skip linting for capnp-generated files but keep it for mpgen-generated ones

Obviously, it's not an optimal solution.

@ajtowns

This broke compilation for me...

The change responsible for this behaviour is the switch from PROJECT_SOURCE_DIR to CMAKE_CURRENT_SOURCE_DIR in the include_prefix argument discussed by @ryanofsky in #33774 (comment).

Does this branch work for you?

Yeah, I think so; took it as this diff on top of this PR's tip commit. CURRENT_SOURCE_DIR would be src/ipc in this context, wouldn't it?

--- a/src/ipc/CMakeLists.txt
+++ b/src/ipc/CMakeLists.txt
@@ -9,7 +9,7 @@ add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
   process.cpp
 )

-target_capnp_sources(bitcoin_ipc ${CMAKE_CURRENT_SOURCE_DIR}
+target_capnp_sources(bitcoin_ipc ${PROJECT_SOURCE_DIR}/src
   capnp/common.capnp
   capnp/echo.capnp
   capnp/init.capnp
@@ -32,7 +32,7 @@ if(BUILD_TESTS)
   add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL
     test/ipc_test.cpp
   )
-  target_capnp_sources(bitcoin_ipc_test ${CMAKE_CURRENT_SOURCE_DIR}
+  target_capnp_sources(bitcoin_ipc_test ${PROJECT_SOURCE_DIR}/src
     test/ipc_test.capnp
   )
   add_dependencies(bitcoin_ipc_test bitcoin_ipc_headers)

CURRENT_SOURCE_DIR would be src/ipc in this context, wouldn't it?

Yes, being presented as an absolute path.