build: Build minisketch test in `make check`, not in `make` #23670

hebasto commented at 11:01 AM on December 4, 2021: member

On master (d1e42659bbdd8da170542d8c638242cd94f71a7d):

$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
$ make 2>&1 | grep LD | grep -v .la
  CXXLD    bitcoind
  CXXLD    bitcoin-cli
  CXXLD    bitcoin-tx
  CXXLD    bitcoin-util
  CXXLD    test/test_bitcoin
  CXXLD    bench/bench_bitcoin
  CXXLD    minisketch/test
  CXXLD    test/fuzz/fuzz
  CXXLD    univalue/test/object
  CXXLD    univalue/test/unitester
$ make check 2>&1 | grep LD
  CCLD     exhaustive_tests
  CCLD     tests

With this PR:

$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
$ make 2>&1 | grep LD | grep -v .la
  CXXLD    bitcoind
  CXXLD    bitcoin-cli
  CXXLD    bitcoin-tx
  CXXLD    bitcoin-util
  CXXLD    test/test_bitcoin
  CXXLD    bench/bench_bitcoin
  CXXLD    test/fuzz/fuzz
  CXXLD    univalue/test/object
  CXXLD    univalue/test/unitester
$ make check 2>&1 | grep LD
  CXXLD    minisketch/test
  CCLD     exhaustive_tests
  CCLD     tests

In fact, this PR restores behavior that was before bitcoin/bitcoin#22646, and that behavior looks more optimal.

As an outcome, the contrib/guix/libexec/build.sh does not spend resources to build binaries which are not a part of the release package.

hebasto added the label Build system on Dec 4, 2021

hebasto marked this as a draft on Dec 4, 2021

jonatack commented at 6:38 PM on December 5, 2021: contributor

Concept ACK modulo Win64 issue. Same result as pull description using debian 5.15.5-1 (2021-11-26), clang 14.

master

(master)$ make 2>&1 | grep LD
  CXXLD    libunivalue.la
  CCLD     libsecp256k1.la
  CXXLD    bitcoind
  CXXLD    bitcoin-cli
  CXXLD    bitcoin-tx
  CXXLD    bitcoin-util
  CXXLD    test/test_bitcoin
  CXXLD    bench/bench_bitcoin
  CXXLD    minisketch/test
  CXXLD    test/fuzz/fuzz
  CXXLD    univalue/test/object
  CXXLD    univalue/test/unitester
  CXXLD    univalue/test/no_nul
  CXXLD    libbitcoinconsensus.la
(master)$ make check 2>&1 | grep LD
  CCLD     tests
  CCLD     valgrind_ctime_test
  CCLD     exhaustive_tests

this branch

((HEAD detached from origin/pr/23670))$ make 2>&1 | grep LD
  CXXLD    libunivalue.la
  CCLD     libsecp256k1.la
  CXXLD    bitcoind
  CXXLD    bitcoin-cli
  CXXLD    bitcoin-tx
  CXXLD    bitcoin-util
  CXXLD    test/test_bitcoin
  CXXLD    bench/bench_bitcoin
  CXXLD    test/fuzz/fuzz
  CXXLD    libbitcoinconsensus.la
((HEAD detached from origin/pr/23670))$ make check 2>&1 | grep LD
  CXXLD    minisketch/test
  CXXLD    univalue/test/object
  CXXLD    univalue/test/unitester
  CXXLD    univalue/test/no_nul
  CCLD     tests
  CCLD     valgrind_ctime_test
  CCLD     exhaustive_tests

hebasto force-pushed on Dec 5, 2021

laanwj commented at 4:03 PM on December 13, 2021: member

I think there's a slight value in always building all binaries (that are enabled by configure).

But as src/test/test_bitcoin bench/bench_bitcoin and test/fuzz/fuzz are still always built, and you're only referring to test binaries of included dependencies, I'm ok with this change.

jamesob commented at 9:54 PM on December 15, 2021: member

Concept ACK given, as @laanwj said, this is only omitting dependency-test compilation during make.

hebasto commented at 8:47 PM on December 27, 2021: member

~Closing. Seeing no straightforward way to integrate this PR into our CI setup.~

hebasto closed this on Dec 27, 2021

hebasto reopened this on Jan 2, 2022

hebasto force-pushed on Jan 2, 2022

hebasto marked this as ready for review on Jan 2, 2022

hebasto commented at 9:45 AM on January 2, 2022: member

Concept ACK modulo Win64 issue.

Fixed.

DrahtBot added the label Needs rebase on Feb 1, 2022

hebasto force-pushed on Feb 2, 2022

hebasto commented at 3:42 PM on February 2, 2022: member

Rebased c39c553eb6d308a420e49f68e469cb88c673e5d0 -> 1593837ecbadea1fa8395e9de14e8f3ad3c0bf91 (pr23670.03 -> pr23670.04) due to the conflict with #24212.

DrahtBot removed the label Needs rebase on Feb 2, 2022

DrahtBot commented at 5:13 AM on February 12, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan
Concept ACK	jonatack, jamesob, kristapsk
Stale 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:

#25797 (build: Add CMake-based build system 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 Needs rebase on Apr 28, 2022

hebasto force-pushed on Apr 28, 2022

hebasto commented at 7:11 PM on April 28, 2022: member

Rebased 1593837ecbadea1fa8395e9de14e8f3ad3c0bf91 -> e9fa9acfff6d28ea974e205111a23305c91c14bf (pr23670.04 -> pr23670.05) due to the conflict with #24322.

DrahtBot removed the label Needs rebase on Apr 28, 2022

DrahtBot added the label Needs rebase on Jul 18, 2022

hebasto force-pushed on Jul 20, 2022

hebasto commented at 9:12 AM on July 20, 2022: member

Rebased e9fa9acfff6d28ea974e205111a23305c91c14bf -> 04dae33cff761c9135454b4abb51749f3e2b5d83 (pr23670.05 -> pr23670.06).

DrahtBot removed the label Needs rebase on Jul 20, 2022

fanquake approved

fanquake commented at 10:05 AM on July 20, 2022: member

ACK 04dae33cff761c9135454b4abb51749f3e2b5d83 - you could update the PR description to remove the no-longer-existant binaries.

hebasto commented at 1:52 PM on July 20, 2022: member

@fanquake

you could update the PR description to remove the no-longer-existant binaries.

Thanks. The PR description has been updated.

in src/Makefile.test.include:377 in 04dae33cff outdated

 373 | @@ -374,7 +374,7 @@ endif
 374 |  
 375 |  if ENABLE_TESTS
 376 |  UNIVALUE_TESTS = univalue/test/object univalue/test/unitester
 377 | -noinst_PROGRAMS += $(UNIVALUE_TESTS)
 378 | +check_PROGRAMS += $(UNIVALUE_TESTS)

maflcko commented at 1:57 PM on July 20, 2022:

Isn't univalue no longer a dependency but now maintained in-tree?

hebasto commented at 2:05 PM on July 20, 2022:

@MarcoFalke Yes. it is. Why did you point at it?

maflcko commented at 2:09 PM on July 20, 2022:

Wouldn't it be better to compile it at the same time that test_bitcoin is compiled?

hebasto commented at 3:11 PM on July 20, 2022:

Why?

test_bitcoin is a part of the distributed package, but univalue tests are not. Why build them in contrib/guix/libexec/build.sh?

maflcko commented at 3:14 PM on July 20, 2022:

I wasn't aware this changes the guix build and the release tarball. Maybe mention it in the description?

hebasto commented at 3:30 PM on July 20, 2022:

The PR description has been updated.

hebasto commented at 3:32 PM on July 20, 2022:

This PR does not change the release tarball.

maflcko commented at 3:51 PM on July 20, 2022:

I find it convenient to be notified of univalue build failures on make when developing univalue.

hebasto commented at 4:18 PM on July 20, 2022:

"univalue build failures" or "univalue tests build failures"?

maflcko commented at 9:03 AM on July 21, 2022:

Both.

Did you measure how many milliseconds this shaves off of the guix build? I'd bet it is far less than 0.1% of overall build time.

Unrelated: It may overall be more useful for devs to introduce a make check_quick to only run the bitcoin core unit tests (without any secp exhaustive_tests)

hebasto commented at 9:51 AM on July 21, 2022:

Thanks! Updated.

kristapsk commented at 2:42 PM on July 20, 2022: contributor

Concept ACK

build: Build minisketch test in `make check`, not in `make` 6d58117a31

hebasto renamed this:
~~build: Build test binaries in `make check`, not in `make`~~
build: Build minisketch test in `make check`, not in `make`
on Jul 21, 2022

hebasto force-pushed on Jul 21, 2022

hebasto commented at 9:51 AM on July 21, 2022: member

Updated 04dae33cff761c9135454b4abb51749f3e2b5d83 -> 6d58117a31a88eec3f0a103f9d1fc26cf0b48348 (pr23670.06 -> pr23670.07):

addressed @MarcoFalke's comments

TheCharlatan commented at 5:17 PM on January 28, 2023: contributor

ACK 6d58117a31a88eec3f0a103f9d1fc26cf0b48348

fanquake merged this on Jan 31, 2023

fanquake closed this on Jan 31, 2023

hebasto deleted the branch on Jan 31, 2023

sidhujag referenced this in commit 8f36092b7a on Feb 1, 2023

bitcoin locked this on Jan 31, 2024