cmake: Revamp handling of data files

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 functions target_json_data_sources() and target_raw_data_sources(), which minimize the amount of code required to assign to assign a *.json or *.raw data file to the test_bitcoin, bench_bitcoin or unitester targets.

As requested in #30901 (comment), the codegen build target is now supported, if available:

0$ cmake --version
1cmake version 3.31.5
2
3CMake suite maintained and supported by Kitware (kitware.com/cmake).
4$ cmake -G "Ninja" -B build
5$ cmake --build build --target codegen

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

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.

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

hebasto commented at 9:48 am on February 10, 2025: member

Friendly ping @theuni :)

DrahtBot added the label Needs rebase on Feb 12, 2025

in cmake/module/TargetDataSources.cmake:5 in 4185ad1cb1 outdated

0@@ -0,0 +1,36 @@
1+# Copyright (c) 2023-present The Bitcoin Core developers
2+# Distributed under the MIT software license, see the accompanying
3+# file COPYING or https://opensource.org/license/mit/.
4+
5+function(target_data_sources target)

theuni commented at 7:05 pm on February 12, 2025:

I think this would make sense as 2 separate functions. Seems a weird mix of args otherwise. target_raw_data_source and target_json_data_source?

I think it’d be fine to call both funcs for targets that need both types of files generated.

hebasto commented at 12:43 pm on February 13, 2025:

Thanks! Reworked.

in cmake/module/TargetDataSources.cmake:30 in 4185ad1cb1 outdated

16+    add_custom_command(
17+      OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${json_file}.h
18+      COMMAND ${CMAKE_COMMAND} -DJSON_SOURCE_PATH=${CMAKE_CURRENT_SOURCE_DIR}/${json_file} -DHEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/${json_file}.h -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromJson.cmake
19+      DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${json_file} ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromJson.cmake
20+      MAIN_DEPENDENCY ${CMAKE_CURRENT_SOURCE_DIR}/${json_file}
21+      VERBATIM

theuni commented at 7:06 pm on February 12, 2025:

These are now missing DEPENDS_EXPLICIT_OPT from #30911.

hebasto commented at 12:43 pm on February 13, 2025:

Thanks! Rebased.

theuni commented at 7:07 pm on February 12, 2025: member

Concept ACK.

theuni commented at 7:11 pm on February 12, 2025: member

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.

hebasto force-pushed on Feb 13, 2025

hebasto commented at 12:42 pm on February 13, 2025: member

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 CODEGEN option for add_custom_command, which would need feature-gating same as DEPENDS_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.

hebasto renamed this:
~~cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets~~
cmake: Revamp handling of data files
on Feb 13, 2025

DrahtBot removed the label Needs rebase on Feb 13, 2025

fjahr commented at 8:45 pm on February 13, 2025: contributor

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.

DrahtBot requested review from Sjors on Feb 13, 2025

DrahtBot requested review from theuni on Feb 13, 2025

theuni commented at 9:13 pm on February 13, 2025: member

Hmm, something here has broken the dependency flattening. I’m seeing a stall again due to the file generation.

hebasto commented at 9:17 pm on February 13, 2025: member

Hmm, something here has broken the dependency flattening. I’m seeing a stall again due to the file generation.

What CMake version?

cmake: Revamp handling of data files

This change introduces new functions `target_json_data_sources()` and
`target_raw_data_sources()`.

a8c78a0574

cmake: Add support for builtin `codegen` target

Additionally, this change removes unnecessary braces in the `if()`
command for improved robustness, readability and consistency with CMake
guidelines.

ecf54a32ed

hebasto force-pushed on Feb 21, 2025

hebasto commented at 11:16 am on February 21, 2025: member

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

theuni commented at 6:28 pm on February 21, 2025: member

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 believe MAIN_DEPENDENCY is MSVC related, so it wouldn’t show up in the build.ninja. Did it help with something there?

theuni commented at 6:58 pm on February 21, 2025: member

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

theuni approved

theuni commented at 7:00 pm on February 21, 2025: member

ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7

DrahtBot requested review from fjahr on Feb 21, 2025

fjahr commented at 8:36 pm on February 21, 2025: contributor

re-ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7

Only removed MAIN_DEPENDENCY since last review.

Sjors commented at 1:31 pm on March 3, 2025: member

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: #30901 (comment)

My cmake knowledge is also limited, but I like that this removes duplication.

It still is, and this PR still does.

hebasto commented at 1:38 pm on March 3, 2025: member

I believe MAIN_DEPENDENCY is MSVC related, so it wouldn’t show up in the build.ninja. Did it help with something there?

It:

is treated just like any value given to the DEPENDS option but also suggests to Visual Studio generators where to hang the custom command.

I’m not sure how useful it is.

fanquake merged this on Mar 3, 2025

fanquake closed this on Mar 3, 2025

hebasto deleted the branch on Mar 3, 2025

cmake: Revamp handling of data files #30901

Code Coverage & Benchmarks

Reviews

Conflicts

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