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

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:

0cmake -S . -B build
1make -C build -j24
2touch src/primitives/transaction.cpp
3make -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.

mooncityorg commented at 8:41 pm on October 1, 2024: none

CMAKE_SKIP_TEST_ALL_DEPENDENCY is false? @bitcoin-core-merge-script

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(master)	commit 0b64d6ea527236ce6d30af0aaa30ad2dd1f04423(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

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

make -C build test ARGS=-j24

 0[  0%] Built target secp256k1_precomputed
 1[  1%] Built target secp256k1
 2[  1%] Generating bitcoin-build-info.h
 3[  1%] Built target generate_build_info
 4[  1%] Built target bitcoin_clientversion
 5[  2%] Built target bitcoin_crypto_arm_shani
 6[  5%] Built target bitcoin_crypto
 7--> [  6%] Building CXX object src/CMakeFiles/bitcoin_consensus.dir/primitives/transaction.cpp.o
 8[  6%] Linking CXX static library libbitcoin_consensus.a
 9[  8%] Built target bitcoin_consensus
10[ 10%] Built target univalue
11[ 16%] Built target bitcoin_util
12[ 26%] Built target bitcoin_common
13[ 33%] Built target bitcoin_wallet
14[ 34%] Linking CXX executable bitcoin-wallet
15ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
16[ 34%] Built target bitcoin-wallet
17[ 34%] Built target crc32c_arm64
18[ 35%] Built target crc32c
19[ 44%] Built target leveldb
20[ 46%] Built target minisketch
21[ 65%] Built target bitcoin_node
22[ 65%] Linking CXX executable bitcoind
23ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
24[ 65%] Built target bitcoind
25[ 65%] Built target bitcoin_cli
26[ 65%] Linking CXX executable bitcoin-cli
27ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
28[ 65%] Built target bitcoin-cli
29[ 65%] Linking CXX executable bitcoin-tx
30ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
31[ 65%] Built target bitcoin-tx
32[ 65%] Linking CXX executable bitcoin-util
33ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
34[ 66%] Built target bitcoin-util
35[ 66%] Built target unitester
36[ 66%] Built target object
37[ 66%] Built target noverify_tests
38[ 66%] Built target tests
39[ 66%] Built target exhaustive_tests
40[ 70%] Built target test_util
41[ 70%] Linking CXX executable test_bitcoin
42ld: warning: ignoring duplicate libraries: '../secp256k1/src/libsecp256k1.a'
43ld: warning: search path '/usr/local/opt/opencv@4/lib' not found
44[100%] Built target test_bitcoin
45--> Running tests...
46Test project /Users/ioannis/Documents/projects/bitdev/bitcoin/build
47        Start   1: util_test_runner
48        Start   2: util_rpcauth_test
49        Start   3: univalue_test
50        Start   4: univalue_object_test

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

build: have “make test” depend on “make all” #31015

Code Coverage

Reviews

Conflicts

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