build: Build minisketch test in `make check`, not in `make` by hebasto · Pull Request #23670 · bitcoin/bitcoin

hebasto · 2021-12-04T11:01:43Z

On master (d1e4265):

$ ./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 #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.

jonatack

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

laanwj · 2021-12-13T16:03:12Z

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 · 2021-12-15T21:54:08Z

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

hebasto · 2021-12-27T20:47:09Z

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

hebasto · 2022-01-02T09:45:06Z

Concept ACK modulo Win64 issue.

Fixed.

hebasto · 2022-02-02T15:42:10Z

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

DrahtBot · 2022-02-12T05:13:39Z

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.

hebasto · 2022-04-28T19:11:20Z

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

hebasto · 2022-07-20T09:12:47Z

Rebased e9fa9ac -> 04dae33 (pr23670.05 -> pr23670.06).

fanquake

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

hebasto · 2022-07-20T13:52:06Z

@fanquake

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

Thanks. The PR description has been updated.

maflcko · 2022-07-20T13:57:21Z

 if ENABLE_TESTS
 UNIVALUE_TESTS = univalue/test/object univalue/test/unitester
-noinst_PROGRAMS += $(UNIVALUE_TESTS)
+check_PROGRAMS += $(UNIVALUE_TESTS)


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

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

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

Why?

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

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

This PR does not change the release tarball.

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

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

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)

Thanks! Updated.

kristapsk · 2022-07-20T14:42:54Z

Concept ACK

hebasto · 2022-07-21T09:51:14Z

Updated 04dae33 -> 6d58117 (pr23670.06 -> pr23670.07):

addressed @MarcoFalke's comments

sedited · 2023-01-28T17:17:32Z

ACK 6d58117

…t in `make` 6d58117 build: Build minisketch test in `make check`, not in `make` (Hennadii Stepanov) Pull request description: On master (d1e4265): ``` $ ./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#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. ACKs for top commit: TheCharlatan: ACK 6d58117 Tree-SHA512: 4957c8f88a01aca005813bf4c1c26f433756bf68ea0c958481c638ead229fa8e23ecae3a8ac31ea555876ba6f2cc10ecd91caf2e2f664de5cb529ec05fb38fa7

hebasto added the Build system label Dec 4, 2021

hebasto marked this pull request as draft December 4, 2021 15:36

jonatack reviewed Dec 5, 2021

View reviewed changes

hebasto force-pushed the 211204-check branch from 193face to 355329f Compare December 5, 2021 22:24

hebasto closed this Dec 27, 2021

hebasto reopened this Jan 2, 2022

hebasto force-pushed the 211204-check branch 2 times, most recently from ff3f93a to c39c553 Compare January 2, 2022 09:10

hebasto marked this pull request as ready for review January 2, 2022 09:12

DrahtBot added the Needs rebase label Feb 1, 2022

hebasto force-pushed the 211204-check branch from c39c553 to 1593837 Compare February 2, 2022 15:38

DrahtBot removed the Needs rebase label Feb 2, 2022

DrahtBot mentioned this pull request Feb 11, 2022

[kernel 0/n] Introduce bitcoin-chainstate #24304

Merged

DrahtBot mentioned this pull request Feb 13, 2022

[kernel 1/n] Introduce initial libbitcoinkernel #24322

Merged

DrahtBot added the Needs rebase label Apr 28, 2022

hebasto force-pushed the 211204-check branch from 1593837 to e9fa9ac Compare April 28, 2022 19:09

DrahtBot removed the Needs rebase label Apr 28, 2022

DrahtBot mentioned this pull request Jul 16, 2022

refactor: univalue test cleanups #25617

Merged

DrahtBot added the Needs rebase label Jul 18, 2022

hebasto force-pushed the 211204-check branch from e9fa9ac to 04dae33 Compare July 20, 2022 09:11

DrahtBot removed the Needs rebase label Jul 20, 2022

fanquake approved these changes Jul 20, 2022

View reviewed changes

maflcko reviewed Jul 20, 2022

View reviewed changes

build: Build minisketch test in make check, not in make

6d58117

hebasto changed the title ~~build: Build test binaries in make check, not in make~~ build: Build minisketch test in make check, not in make Jul 21, 2022

hebasto force-pushed the 211204-check branch from 04dae33 to 6d58117 Compare July 21, 2022 09:43

DrahtBot mentioned this pull request Nov 12, 2022

build: Add CMake-based build system #25797

Closed

16 tasks

fanquake merged commit b5868f4 into bitcoin:master Jan 31, 2023

hebasto deleted the 211204-check branch January 31, 2023 18:57

naumenkogs mentioned this pull request Oct 13, 2023

Erlay Project Tracking #28646

Closed

17 tasks

bitcoin locked and limited conversation to collaborators Jan 31, 2024

Uh oh!

Conversation

hebasto commented Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 13, 2021

Uh oh!

jamesob commented Dec 15, 2021

Uh oh!

hebasto commented Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 2, 2022

Uh oh!

hebasto commented Feb 2, 2022

Uh oh!

DrahtBot commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto commented Apr 28, 2022

Uh oh!

hebasto commented Jul 20, 2022

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Jul 20, 2022

Uh oh!

hebasto commented Jul 21, 2022

Uh oh!

sedited commented Jan 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

hebasto commented Dec 4, 2021 •

edited

Loading

hebasto commented Dec 27, 2021 •

edited

Loading

DrahtBot commented Feb 12, 2022 •

edited

Loading

hebasto Jul 20, 2022 •

edited

Loading