cmake: Revamp handling of data files for `{test,bench}

hebasto commented at 11:44 am on September 14, 2024: member

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.

hebasto added the label Build system on Sep 14, 2024

hebasto added the label Tests on Sep 14, 2024

DrahtBot commented at 11:44 am on September 14, 2024: contributor

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

Reviewers, this pull request conflicts with the following ones:

#31504 (cmake: De-duplicate libraries on link lines opportunistically by hebasto)
#30911 (build: speed up by flattening the dependency graph by theuni)

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.

hebasto commented at 11:45 am on September 14, 2024: member

Friendly ping @ryanofsky :)

in src/test/CMakeLists.txt:140 in 0c35be69cf outdated

134+    data/sighash.json
135+    data/tx_invalid.json
136+    data/tx_valid.json
137+  RAW_FILES
138+    data/asmap.raw
139+  RAW_NAMESPACE test::data

maflcko commented at 8:25 am on September 16, 2024:

not sure this is going in the right direction. The namespace just happens to be a single value for all raw files. However, in theory, one could imagine several raw files having a different namespace each. This would then again increase the verbosity and complexity.

hebasto commented at 11:41 am on September 17, 2024:

Once such a scenario happens, we can use another call:

0target_data_sources(test_bitcoin
1  RAW_FILES
2    data/some_file_with_another_namespace.raw
3  RAW_NAMESPACE somewhere::another_namespace
4)

And I think it is better than (1) specifying data/some_file_with_another_namespace.raw to generate a header than (2) adding ${CMAKE_CURRENT_BINARY_DIR}/data/some_file_with_another_namespac.raw.h to the sources.

maflcko commented at 8:22 am on September 18, 2024:

Ok, fair enough. I just wanted to mention it. Seems fine either way.

fjahr commented at 9:42 am on September 20, 2024: contributor

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.

maflcko removed the label Tests on Sep 20, 2024

maflcko added the label DrahtBot Guix build requested on Sep 20, 2024

DrahtBot commented at 4:54 pm on September 20, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 197aa249551ea7b2280b6e187d2ad5378687beff(master)	commit 0bc2b979eacf67662631c50e67d952450903d8b3(master and this pull)
SHA256SUMS.part	`521f76d55c6d0681...`	`82df3d90acea09eb...`
*-aarch64-linux-gnu-debug.tar.gz	`d65768ddaa01b1e2...`	`b55abed2172b7cf8...`
*-aarch64-linux-gnu.tar.gz	`7979cd76ea72491b...`	`a1ea6ffd72a57c52...`
*-arm-linux-gnueabihf-debug.tar.gz	`f10a6f5c23d9591a...`	`18b49aeb3bb57391...`
*-arm-linux-gnueabihf.tar.gz	`243389753c7ad79a...`	`eff66b351f885633...`
*-arm64-apple-darwin-unsigned.tar.gz	`4de1c5efbfb6983a...`	`2fb0cc3e11743ea0...`
*-arm64-apple-darwin-unsigned.zip	`56768c5b2e77aa44...`	`5e9fdb3271af18d5...`
*-arm64-apple-darwin.tar.gz	`45f83ddd7d034ced...`	`6b9a11453016515c...`
*-powerpc64-linux-gnu-debug.tar.gz	`dbd3fd28edfe8346...`	`3dcb12423b002833...`
*-powerpc64-linux-gnu.tar.gz	`b30ca46023d94314...`	`6e6caa5f3db63e9a...`
*-riscv64-linux-gnu-debug.tar.gz	`3496ab1a2d69f873...`	`eb08343fd8aaff84...`
*-riscv64-linux-gnu.tar.gz	`e9fd9140d0c75276...`	`4185a04618d7f424...`
*-x86_64-apple-darwin-unsigned.tar.gz	`2d80ca5c684f9c3a...`	`0747b84d884272a7...`
*-x86_64-apple-darwin-unsigned.zip	`8d9c51153100b0a9...`	`f31f6e03d65beb2d...`
*-x86_64-apple-darwin.tar.gz	`2a6c00e8b0e9e53b...`	`780ded24e6d27356...`
*-x86_64-linux-gnu-debug.tar.gz	`0d6d2b136adbc951...`	`8cf9d88b75d6268d...`
*-x86_64-linux-gnu.tar.gz	`c8522c785d8bade4...`	`1b9e51de65d24364...`
*.tar.gz	`fdfcfc0072ad02f3...`	`f46aa53f989f4832...`
guix_build.log	`c3f3df05bf999a2d...`	`8f0c9f28397fb3eb...`
guix_build.log.diff		`f922aa1b8ac7af94...`

DrahtBot removed the label DrahtBot Guix build requested on Sep 20, 2024

DrahtBot added the label CI failed on Oct 13, 2024

DrahtBot removed the label CI failed on Oct 17, 2024

DrahtBot added the label Needs rebase on Dec 17, 2024

hebasto force-pushed on Dec 17, 2024

hebasto commented at 12:44 pm on December 17, 2024: member

Rebased due to the conflict with the merged bitcoin/bitcoin#31503.

fjahr commented at 1:22 pm on December 17, 2024: contributor

re-ACK 9f14069fde2a612b1f350ab0b49cdb5d96e8ac3e

Confirmed only rebase since last review per git range-diff master 0c35be69cf2a18a7a9173d0f9f88116b4417c892 9f14069fde2a612b1f350ab0b49cdb5d96e8ac3e

DrahtBot removed the label Needs rebase on Dec 17, 2024

DrahtBot added the label CI failed on Jan 4, 2025

DrahtBot commented at 11:05 am on January 4, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34533242400

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.

cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets

This change introduces a new `target_data_sources()` function.

4185ad1cb1

hebasto force-pushed on Jan 9, 2025

hebasto commented at 9:15 am on January 9, 2025: member

Rebased due to the conflict with the merged bitcoin/bitcoin#31542.

DrahtBot removed the label CI failed on Jan 9, 2025

fjahr commented at 12:38 pm on January 9, 2025: contributor

Re-ACK 4185ad1cb12060337c2d66a9a66e37be1d10f4ce

Confirmed only change was a rebase.

Sjors commented at 1:29 pm on January 17, 2025: member

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:

0[ 76%] Generating data/base58_encode_decode.json.h

cmake: Revamp handling of data files for {test,bench}_bitcoin targets #30901

Code Coverage & Benchmarks

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets #30901