bitcoin
bitcoin copied to clipboard
bitcoin
cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets
This PR leverages the approach from the https://github.com/chaincodelabs/libmultiprocess project and introduces a new target_data_sources() function, which minimizes code needed to assign a *.json or *.raw data file to the test_bitcoin or bench_bitcoin target.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30901.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theuni, fjahr, Sjors |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
tACK 0c35be69cf2a18a7a9173d0f9f88116b4417c892
I have based #28792 on this now. It works and seems like a nicer way to do what I need there than the previous approach.
Take it with a grain of salt though because I don't think I am experienced enough with CMake to tell if there are any hidden issues with this approach or if there are even better ways to achieve this.
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]
re-ACK 9f14069fde2a612b1f350ab0b49cdb5d96e8ac3e
Confirmed only rebase since last review per git range-diff master 0c35be69cf2a18a7a9173d0f9f88116b4417c892 9f14069fde2a612b1f350ab0b49cdb5d96e8ac3e
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34533242400
Hints
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
-
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
-
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
-
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Re-ACK 4185ad1cb12060337c2d66a9a66e37be1d10f4ce
Confirmed only change was a rebase.
ACK 4185ad1cb12060337c2d66a9a66e37be1d10f4ce
My cmake knowledge is also limited, but I like that this removes duplication. E.g instead of base58_encode_decode.json.h and data/base58_encode_decode.json now test/CMakeLists.txt only contains the latter.
The build log looks the same, before and after:
[ 76%] Generating data/base58_encode_decode.json.h
Also, since you're messing with this, please consider the CODEGEN option for add_custom_command, which would need feature-gating same as DEPENDS_EXPLICIT_OPT.
- Rebased due to the conflict with the merged bitcoin/bitcoin#30911.
- Addressed the recent @theuni's feedback.
- Fixed minor bugs, such as: https://github.com/bitcoin/bitcoin/blob/18cc0a55595f9dc1f2e561743201a05754996b64/cmake/module/TargetDataSources.cmake#L6
Also, since you're messing with this, please consider the
CODEGENoption foradd_custom_command, which would need feature-gating same asDEPENDS_EXPLICIT_OPT.
Nice! Added.
Please note that cmake_policy(SET CMP0171 NEW) within TargetDataSources.cmake does not work for some reason. However, this behaviour is not documented by CMake.
tACK 18cc0a55595f9dc1f2e561743201a05754996b64
Again, I am not an expert on cmake but I don't see any downsides in the latest changes and I can confirm it works. I am using this latest version in #28792 now. And I find the change on function names and args is making things a bit nicer.
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
What CMake version?
@theuni
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
I've removed the MAIN_DEPENDENCY options (I don't recall the motivation behind their introduction). A comparison of the resulting build.ninja files reveals no changes in dependencies.
I've removed the
MAIN_DEPENDENCYoptions (I don't recall the motivation behind their introduction). A comparison of the resultingbuild.ninjafiles reveals no changes in dependencies.
I believe MAIN_DEPENDENCY is MSVC related, so it wouldn't show up in the build.ninja. Did it help with something there?
@theuni
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
Ok, Ignore this. I believe I was testing with make, where the stall is expected. Ninja is indeed unchanged.
re-ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7
Only removed MAIN_DEPENDENCY since last review.
re-tACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7
The range-diff shows that we persevere the DEPENDS_EXPLICIT_OPT check added in #30911.
Checked the same thing as last time: https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2598371523
My cmake knowledge is also limited, but I like that this removes duplication.
It still is, and this PR still does.
I believe
MAIN_DEPENDENCYis MSVC related, so it wouldn't show up in thebuild.ninja. Did it help with something there?
It:
is treated just like any value given to the
DEPENDSoption but also suggests to Visual Studio generators where to hang the custom command.
I'm not sure how useful it is.