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 react with 👎 to this comment and the bot will ignore it on the next update.
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?
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.