ipc/test
#33774
This PR follows up on #33445 and:
src/test/.clang-tidy.in.See the previous discussion:
Additionally, the locality of the bitcoin_ipc_test build target description has been improved.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33774.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
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.
src/test/kernel also be moved to src/kernel/test to keep the project structure Consistent. ?
should
src/test/kernelalso be moved tosrc/kernel/testto 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.
329- # bitcoin_ipc_test library target is defined here in src/CMakeLists.txt
330- # instead of src/test/CMakeLists.txt so capnp files in src/test/ are able to
331- # reference capnp files in src/ipc/capnp/ by relative path. The Cap'n Proto
332- # compiler only allows importing by relative path when the importing and
333- # imported files are underneath the same compilation source prefix, so the
334- # source prefix must be src/, not src/test/
In commit “cmake, test: Improve locality of bitcoin_ipc_test library description” (f26c8b03f968843e6e1381d1e1721f4379f7e7ab)
I think this comment is still useful to keep because otherwise there is no explanation of why bitcoin_ipc_test in defined src/ipc/ instead of src/ipc/test where it would make more sense.
Comment would just need to be tweaked slightly to be kept: src/CMakeLists.txt -> src/ipc/CMakeLists.txt, src/test/CMakeLists.txt -> src/ipc/test/CMakeLists.txt, and src/test/ to src/ipc/test/.
One of the things I dislike most about working with our build code is that I see a lot of unexpected things done with no explanation and it’s not clear what those things were done intentionally or are accidental sources of complexity that could be cleaned up. I think it is better to call out inconsistencies like this and make intent clear than try to make code that is doing something unusual appear normal.
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)
Would you like to take another look at this PR?
8@@ -9,7 +9,7 @@ add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
9 process.cpp
10 )
11
12-target_capnp_sources(bitcoin_ipc ${PROJECT_SOURCE_DIR}
13+target_capnp_sources(bitcoin_ipc ${CMAKE_CURRENT_SOURCE_DIR}
In commit “cmake, test: Improve locality of bitcoin_ipc_test library description” (866bbb98fd365962840ee99df0d9f7ad557cc025)
This change should be correct, but I’m confused about how it was working before. This change causes includes in generated files such build/src/ipc/capnp/common.capnp.proxy.h to be generated correctly relative to src/:
0-#include <src/ipc/capnp/common.capnp.h> // IWYU pragma: keep
1+#include <capnp/common.capnp.h> // IWYU pragma: keep
So that seems good. But I don’t understand why the #include <src/...> lines were working previously? If i run make with VERBOSE=1 I see the top level build directory (not build/src) in the list of include directories passed to the compiler. But I don’t see a target_include_directories call adding that directory anywhere, and I don’t know why anything should be included relative to the top level project build directory, so maybe there is still another problem here?
But I don’t understand why the
#include <src/...>lines were working previously?
This code is responsible for adding the required include path:https://github.com/bitcoin/bitcoin/blob/75baff98fcf987735437196a4db1919e390c4bd2/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake#L96-L103
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_sourcescalls 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:
0$ git checkout 866bbb98fd365962840ee99df0d9f7ad557cc025
1$ cmake --build ../build-rel -j 40 -t bitcoin_ipc_test
2[ 0%] Built target mputil
3[ 50%] Built target mpgen
4[ 50%] Built target multiprocess
5[ 50%] Built target bitcoin_ipc_headers
6[ 50%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.cpp.o
7[ 50%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-client.c++.o
8[ 50%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-types.c++.o
9[100%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.capnp.proxy-server.c++.o
10In file included from build-rel/src/ipc/test/ipc_test.capnp.proxy-types.c++:5:
11build-rel/src/test/ipc_test.capnp.proxy.h:6:10: fatal error: src/test/ipc_test.capnp.h: No such file or directory
12 6 | #include <src/test/ipc_test.capnp.h> // IWYU pragma: keep
13 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
14compilation terminated.
15gmake[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
16gmake[3]: *** Waiting for unfinished jobs....
17In file included from build-rel/src/ipc/test/ipc_test.capnp.proxy-server.c++:6:
18build-rel/src/test/ipc_test.capnp.proxy.h:6:10: fatal error: src/test/ipc_test.capnp.h: No such file or directory
19 6 | #include <src/test/ipc_test.capnp.h> // IWYU pragma: keep
20 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
21compilation 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:
0$ git checkout 866bbb98fd365962840ee99df0d9f7ad557cc025 1$ cmake --build ../build-rel -j 40 -t bitcoin_ipc_test 2...
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”.
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.
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.
This broke compilation for me…
The change responsible for this behaviour is the switch from
PROJECT_SOURCE_DIRtoCMAKE_CURRENT_SOURCE_DIRin theinclude_prefixargument 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:
0--- a/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake
1+++ b/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake
2@@ -78,7 +78,7 @@ function(target_capnp_sources target include_prefix)
3 add_custom_command(
4 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
5 COMMAND ${MPGEN_BINARY} ${CMAKE_CURRENT_SOURCE_DIR} ${include_prefix} ${CMAKE_CURRENT_SOURCE_DIR}/${capnp_file} ${TCS_IMPORT_PATHS} ${mp_include_dir}
6- DEPENDS ${capnp_file}
7+ DEPENDS ${capnp_file} ${CMAKE_CURRENT_FUNCTION_LIST_FILE}
8 VERBATIM
9 )
10 # Skip linting for capnp-generated files but keep it for mpgen-generated ones
Obviously, it’s not an optimal solution.
This broke compilation for me…
The change responsible for this behaviour is the switch from
PROJECT_SOURCE_DIRtoCMAKE_CURRENT_SOURCE_DIRin theinclude_prefixargument 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?
0--- a/src/ipc/CMakeLists.txt
1+++ b/src/ipc/CMakeLists.txt
2@@ -9,7 +9,7 @@ add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
3 process.cpp
4 )
5
6-target_capnp_sources(bitcoin_ipc ${CMAKE_CURRENT_SOURCE_DIR}
7+target_capnp_sources(bitcoin_ipc ${PROJECT_SOURCE_DIR}/src
8 capnp/common.capnp
9 capnp/echo.capnp
10 capnp/init.capnp
11@@ -32,7 +32,7 @@ if(BUILD_TESTS)
12 add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL
13 test/ipc_test.cpp
14 )
15- target_capnp_sources(bitcoin_ipc_test ${CMAKE_CURRENT_SOURCE_DIR}
16+ target_capnp_sources(bitcoin_ipc_test ${PROJECT_SOURCE_DIR}/src
17 test/ipc_test.capnp
18 )
19 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.