build: have "make test" depend on "make all" #31015

theuni commented at 7:04 PM on October 1, 2024: member

Unfortunately, this seems to have no effect when directly executing ctest :(

This brings the test -> hack -> test cycle more inline with how it worked with autotools.

With CMAKE_SKIP_TEST_ALL_DEPENDENCY set to FALSE, make test will trigger a rebuild, ensuring that test binaries are current before running them.

To test:

cmake -S . -B build
make -C build -j24
touch src/primitives/transaction.cpp
make -C build test ARGS=-j24

Without this commit, the above will not rebuild before running tests.

build: have "make test" depend on "make all" 2957ca9611

DrahtBot commented at 7:04 PM on October 1, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, itornaza, laanwj
Concept ACK	fanquake

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:

#31161 (cmake: Set top-level target output locations by hebasto)

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.

DrahtBot added the label Build system on Oct 1, 2024

theuni commented at 7:05 PM on October 1, 2024: member

Ping @hebasto . Also ping @fanquake as I think you've complained about this.

hebasto commented at 7:06 PM on October 1, 2024: member

Does this solution support parallelism in the test phase?

theuni commented at 7:10 PM on October 1, 2024: member

With ARGS=-jX, yes.

laanwj commented at 9:11 PM on October 1, 2024: member

Concept ACK

fanquake commented at 10:59 AM on October 2, 2024: member

Concept ACK. Nice that after 15 years there is now a solution (https://cmake.org/Bug/view.php?id=8774, https://gitlab.kitware.com/cmake/cmake/-/issues/8774).

tdb3 approved

tdb3 commented at 12:33 PM on October 3, 2024: contributor

ACK 2957ca9611916efb570d157b9c7a0b188161660d

Not sure how many people this would affect in the near term (e.g. I've experienced Debian 12 (with apt) and Fedora 40 (with dnf) default to cmake versions under 3.29), but seems like a useful improvement nonetheless.

Tested with Fedora 40 manually install a more recent cmake version (3.30.4). Saw that make -C build test forces recompile of transaction.cpp.

DrahtBot requested review from fanquake on Oct 3, 2024

DrahtBot requested review from laanwj on Oct 3, 2024

maflcko added the label DrahtBot Guix build requested on Oct 7, 2024

DrahtBot commented at 11:56 PM on October 7, 2024: contributor

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

File	commit 62e4516722115c2d5aeb6c197abc73ca7c078b23<br>(master)	commit 0b64d6ea527236ce6d30af0aaa30ad2dd1f04423<br>(master and this pull)
SHA256SUMS.part	`f2d49fb72517ef0c...`	`f8aef59c77977919...`
*-aarch64-linux-gnu-debug.tar.gz	`d37cbe26a25f7540...`	`1030d128b1d0ff3e...`
*-aarch64-linux-gnu.tar.gz	`42b4d4db0c531caf...`	`f9be7acff7206d6f...`
*-arm-linux-gnueabihf-debug.tar.gz	`63eddcf3cfdb6c32...`	`e00af74f685f3279...`
*-arm-linux-gnueabihf.tar.gz	`fd2eb3904357b5f4...`	`027c71c2a1e721c1...`
*-arm64-apple-darwin-unsigned.tar.gz	`71a08c8f938313cf...`	`55871afff8ab5c1d...`
*-arm64-apple-darwin-unsigned.zip	`ed11994d8f055418...`	`960ec981f470a691...`
*-arm64-apple-darwin.tar.gz	`0c11a002f4f3030c...`	`af3f011f192418d4...`
*-powerpc64-linux-gnu-debug.tar.gz	`a76483c3abcddcc1...`	`57b89dec92458570...`
*-powerpc64-linux-gnu.tar.gz	`335ec17b370a19d5...`	`12029e9233fd1d90...`
*-riscv64-linux-gnu-debug.tar.gz	`8494e354e088382e...`	`07756f4fe9c24813...`
*-riscv64-linux-gnu.tar.gz	`457df2421685bdda...`	`3f8378468ace8de7...`
*-x86_64-apple-darwin-unsigned.tar.gz	`3cedc5861a78c8a5...`	`5864f3d1d0fbf3cc...`
*-x86_64-apple-darwin-unsigned.zip	`19b332dccbc792b4...`	`34632e850716fa51...`
*-x86_64-apple-darwin.tar.gz	`54c6788eb266f818...`	`ada4be2e79750364...`
*-x86_64-linux-gnu-debug.tar.gz	`a761c3712ed00392...`	`7c58144150d10237...`
*-x86_64-linux-gnu.tar.gz	`fceda20d8f50b5ab...`	`0652537296c02734...`
*.tar.gz	`38978a4be1a7c7eb...`	`6a08ba043ab6f3d7...`
guix_build.log	`b4092f80775416ae...`	`bad5bed29f65f39e...`
guix_build.log.diff		`4159e55b5b132f7c...`

DrahtBot removed the label DrahtBot Guix build requested on Oct 7, 2024

itornaza approved

itornaza commented at 7:07 PM on October 9, 2024: contributor

ACK 2957ca9611916efb570d157b9c7a0b188161660d

Followed the testing steps from the description on macOS 15.0.1 with cmake version 3.30.5 from brew.

cmake -S . -B build
cmake -C build -j24
touch src/primitives/transaction.cpp

<details> <summary>Confirmed the timestamp change on the file compared to the other ones</summary>

transaction.cpp -rw-r--r--  1 ioannis  staff   4170  9 Oct 21:42 transaction.cpp
transaction.h -rw-r--r--  1 ioannis  staff  13730  5 Aug 16:49 transaction.h

</details>

make -C build test ARGS=-j24

<details> <summary>Indeed the transaction.cpp is rebuild and the tests run afterwards</summary>

[  0%] Built target secp256k1_precomputed
[  1%] Built target secp256k1
[  1%] Generating bitcoin-build-info.h
[  1%] Built target generate_build_info
[  1%] Built target bitcoin_clientversion
[  2%] Built target bitcoin_crypto_arm_shani
[  5%] Built target bitcoin_crypto
--> [  6%] Building CXX object src/CMakeFiles/bitcoin_consensus.dir/primitives/transaction.cpp.o
[  6%] Linking CXX static library libbitcoin_consensus.a
[  8%] Built target bitcoin_consensus
[ 10%] Built target univalue
[ 16%] Built target bitcoin_util
[ 26%] Built target bitcoin_common
[ 33%] Built target bitcoin_wallet
[ 34%] Linking CXX executable bitcoin-wallet
ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
[ 34%] Built target bitcoin-wallet
[ 34%] Built target crc32c_arm64
[ 35%] Built target crc32c
[ 44%] Built target leveldb
[ 46%] Built target minisketch
[ 65%] Built target bitcoin_node
[ 65%] Linking CXX executable bitcoind
ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
[ 65%] Built target bitcoind
[ 65%] Built target bitcoin_cli
[ 65%] Linking CXX executable bitcoin-cli
ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
[ 65%] Built target bitcoin-cli
[ 65%] Linking CXX executable bitcoin-tx
ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
[ 65%] Built target bitcoin-tx
[ 65%] Linking CXX executable bitcoin-util
ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
[ 66%] Built target bitcoin-util
[ 66%] Built target unitester
[ 66%] Built target object
[ 66%] Built target noverify_tests
[ 66%] Built target tests
[ 66%] Built target exhaustive_tests
[ 70%] Built target test_util
[ 70%] Linking CXX executable test_bitcoin
ld: warning: ignoring duplicate libraries: '../secp256k1/src/libsecp256k1.a'
ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
[100%] Built target test_bitcoin
--> Running tests...
Test project /Users/ioannis/Documents/projects/bitdev/bitcoin/build
        Start   1: util_test_runner
        Start   2: util_rpcauth_test
        Start   3: univalue_test
        Start   4: univalue_object_test

</details>

laanwj approved

laanwj commented at 11:56 AM on October 30, 2024: member

ACK 2957ca9611916efb570d157b9c7a0b188161660d

achow101 merged this on Oct 30, 2024

achow101 closed this on Oct 30, 2024

TheCharlatan referenced this in commit a73b2bd0f0 on Nov 14, 2024

bug-castercv502 referenced this in commit fdcc066ca0 on Sep 28, 2025

bitcoin locked this on Oct 30, 2025