cmake: Revamp handling of data files #30901

hebasto · 2024-09-14T11:44:49Z

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:

$ cmake --version
cmake version 3.31.5

CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ cmake -G "Ninja" -B build
$ cmake --build build --target codegen

DrahtBot · 2024-09-14T11:44:52Z

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 · 2024-09-14T11:45:08Z

Friendly ping @ryanofsky :)

src/test/CMakeLists.txt

fjahr · 2024-09-20T09:42:13Z

tACK 0c35be6

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.

DrahtBot · 2024-09-20T16:54:48Z

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

File	commit `197aa24` (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...`

hebasto · 2024-12-17T12:44:30Z

Rebased due to the conflict with the merged #31503.

fjahr · 2024-12-17T13:22:08Z

re-ACK 9f14069

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

DrahtBot · 2025-01-04T11:05:02Z

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

hebasto · 2025-01-09T09:15:56Z

Rebased due to the conflict with the merged #31542.

theuni · 2025-02-12T19:11:37Z

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 · 2025-02-13T12:42:49Z

Rebased due to the conflict with the merged build: simplify by flattening the dependency graph #30911.
Addressed the recent @theuni's feedback.
Fixed minor bugs, such as:

bitcoin/cmake/module/TargetDataSources.cmake

Line 6 in 18cc0a5

set(DEPENDS_EXPLICIT_OPT "")

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.

fjahr · 2025-02-13T20:45:48Z

tACK 18cc0a5

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.

theuni · 2025-02-13T21:13:31Z

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

hebasto · 2025-02-13T21:17:30Z

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

What CMake version?

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

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

hebasto · 2025-02-21T11:16:50Z

@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 · 2025-02-21T18:28:12Z

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 · 2025-02-21T18:58:09Z

@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

ACK ecf54a3

fjahr · 2025-02-21T20:36:47Z

re-ACK ecf54a3

Only removed MAIN_DEPENDENCY since last review.

Sjors · 2025-03-03T13:31:46Z

re-tACK ecf54a3

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 · 2025-03-03T13:38:03Z

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.

hebasto added Build system Tests labels Sep 14, 2024

hebasto mentioned this pull request Sep 14, 2024

contrib: fix check-deps.sh to check for weak symbols #30415

Merged

DrahtBot mentioned this pull request Sep 15, 2024

build: Embedded ASMap [3/3]: Build binary dump header file #28792

Open

maflcko reviewed Sep 16, 2024

View reviewed changes

src/test/CMakeLists.txt Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 17, 2024

build: simplify by flattening the dependency graph #30911

Merged

maflcko added DrahtBot Guix build requested and removed Tests labels Sep 20, 2024

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

DrahtBot added CI failed and removed CI failed labels Oct 13, 2024

DrahtBot mentioned this pull request Nov 14, 2024

[IBD] multi-byte block obfuscation #31144

Merged

DrahtBot mentioned this pull request Nov 25, 2024

kernel, refactor: Replace goto with RAII in bitcoin-chainstate #31362

Closed

This was referenced Dec 16, 2024

cmake: De-duplicate libraries on link lines opportunistically #31504

Closed

cmake: Link bitcoin_consensus as a library #31503

Merged

build: Use clang-cl to build on Windows natively #31507

Open

DrahtBot added the Needs rebase label Dec 17, 2024

hebasto force-pushed the 240914-data-sources branch from 0c35be6 to 9f14069 Compare December 17, 2024 12:43

DrahtBot removed the Needs rebase label Dec 17, 2024

hebasto mentioned this pull request Dec 20, 2024

test: Embed univalue json tests in binary #31542

Merged

DrahtBot added the CI failed label Jan 4, 2025

hebasto force-pushed the 240914-data-sources branch from 9f14069 to 4185ad1 Compare January 9, 2025 09:14

hebasto force-pushed the 240914-data-sources branch from 4185ad1 to 18cc0a5 Compare February 13, 2025 12:36

hebasto changed the title ~~cmake: Revamp handling of data files for {test,bench}_bitcoin targets~~ cmake: Revamp handling of data files Feb 13, 2025

DrahtBot removed the Needs rebase label Feb 13, 2025

DrahtBot requested review from Sjors and theuni February 13, 2025 20:45

hebasto added 2 commits February 21, 2025 11:11

cmake: Revamp handling of data files

a8c78a0

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

cmake: Add support for builtin codegen target

ecf54a3

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

hebasto force-pushed the 240914-data-sources branch from 18cc0a5 to ecf54a3 Compare February 21, 2025 11:13

theuni approved these changes Feb 21, 2025

View reviewed changes

DrahtBot requested a review from fjahr February 21, 2025 19:00

fanquake merged commit 79bbb38 into bitcoin:master Mar 3, 2025
18 checks passed

hebasto deleted the 240914-data-sources branch March 3, 2025 21:00

cmake: Revamp handling of data files #30901

cmake: Revamp handling of data files #30901

Uh oh!

Conversation

hebasto commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hebasto commented Sep 14, 2024

Uh oh!

Uh oh!

fjahr commented Sep 20, 2024

Uh oh!

DrahtBot commented Sep 20, 2024

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

Uh oh!

hebasto commented Dec 17, 2024

Uh oh!

fjahr commented Dec 17, 2024

Uh oh!

DrahtBot commented Jan 4, 2025

Uh oh!

hebasto commented Jan 9, 2025

Uh oh!

theuni commented Feb 12, 2025

Uh oh!

hebasto commented Feb 13, 2025

Uh oh!

fjahr commented Feb 13, 2025

Uh oh!

theuni commented Feb 13, 2025

Uh oh!

hebasto commented Feb 13, 2025

Uh oh!

hebasto commented Feb 21, 2025

Uh oh!

theuni commented Feb 21, 2025

Uh oh!

theuni commented Feb 21, 2025

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Feb 21, 2025

Uh oh!

Sjors commented Mar 3, 2025

Uh oh!

hebasto commented Mar 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Sep 14, 2024 •

edited

Loading

DrahtBot commented Sep 14, 2024 •

edited

Loading