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

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:211204-check changing 3 files +4 −1
  1. hebasto commented at 11:01 am on December 4, 2021: member

    On master (d1e42659bbdd8da170542d8c638242cd94f71a7d):

     0$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
 1$ make 2>&1 | grep LD | grep -v .la
 2  CXXLD    bitcoind
 3  CXXLD    bitcoin-cli
 4  CXXLD    bitcoin-tx
 5  CXXLD    bitcoin-util
 6  CXXLD    test/test_bitcoin
 7  CXXLD    bench/bench_bitcoin
 8  CXXLD    minisketch/test
 9  CXXLD    test/fuzz/fuzz
10  CXXLD    univalue/test/object
11  CXXLD    univalue/test/unitester
12$ make check 2>&1 | grep LD
13  CCLD     exhaustive_tests
14  CCLD     tests

    With this PR:

     0$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
 1$ make 2>&1 | grep LD | grep -v .la
 2  CXXLD    bitcoind
 3  CXXLD    bitcoin-cli
 4  CXXLD    bitcoin-tx
 5  CXXLD    bitcoin-util
 6  CXXLD    test/test_bitcoin
 7  CXXLD    bench/bench_bitcoin
 8  CXXLD    test/fuzz/fuzz
 9  CXXLD    univalue/test/object
10  CXXLD    univalue/test/unitester
11$ make check 2>&1 | grep LD
12  CXXLD    minisketch/test
13  CCLD     exhaustive_tests
14  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.

  2. hebasto added the label Build system on Dec 4, 2021
  3. hebasto marked this as a draft on Dec 4, 2021
  4. 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

     0(master)$ make 2>&1 | grep LD
 1  CXXLD    libunivalue.la
 2  CCLD     libsecp256k1.la
 3  CXXLD    bitcoind
 4  CXXLD    bitcoin-cli
 5  CXXLD    bitcoin-tx
 6  CXXLD    bitcoin-util
 7  CXXLD    test/test_bitcoin
 8  CXXLD    bench/bench_bitcoin
 9  CXXLD    minisketch/test
10  CXXLD    test/fuzz/fuzz
11  CXXLD    univalue/test/object
12  CXXLD    univalue/test/unitester
13  CXXLD    univalue/test/no_nul
14  CXXLD    libbitcoinconsensus.la
15(master)$ make check 2>&1 | grep LD
16  CCLD     tests
17  CCLD     valgrind_ctime_test
18  CCLD     exhaustive_tests

    this branch

     0((HEAD detached from origin/pr/23670))$ make 2>&1 | grep LD
 1  CXXLD    libunivalue.la
 2  CCLD     libsecp256k1.la
 3  CXXLD    bitcoind
 4  CXXLD    bitcoin-cli
 5  CXXLD    bitcoin-tx
 6  CXXLD    bitcoin-util
 7  CXXLD    test/test_bitcoin
 8  CXXLD    bench/bench_bitcoin
 9  CXXLD    test/fuzz/fuzz
10  CXXLD    libbitcoinconsensus.la
11((HEAD detached from origin/pr/23670))$ make check 2>&1 | grep LD
12  CXXLD    minisketch/test
13  CXXLD    univalue/test/object
14  CXXLD    univalue/test/unitester
15  CXXLD    univalue/test/no_nul
16  CCLD     tests
17  CCLD     valgrind_ctime_test
18  CCLD     exhaustive_tests
  5. hebasto force-pushed on Dec 5, 2021
  6. 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.

  7. 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.
  8. hebasto commented at 8:47 pm on December 27, 2021: member
    ~Closing. Seeing no straightforward way to integrate this PR into our CI setup.~
  9. hebasto closed this on Dec 27, 2021
  10. hebasto reopened this on Jan 2, 2022
  11. hebasto force-pushed on Jan 2, 2022
  12. hebasto force-pushed on Jan 2, 2022
  13. hebasto marked this as ready for review on Jan 2, 2022
  14. hebasto commented at 9:45 am on January 2, 2022: member

    Concept ACK modulo Win64 issue.

    Fixed.

  15. DrahtBot added the label Needs rebase on Feb 1, 2022
  16. hebasto force-pushed on Feb 2, 2022
  17. hebasto commented at 3:42 pm on February 2, 2022: member
    Rebased c39c553eb6d308a420e49f68e469cb88c673e5d0 -> 1593837ecbadea1fa8395e9de14e8f3ad3c0bf91 (pr23670.03 -> pr23670.04) due to the conflict with #24212.
  18. DrahtBot removed the label Needs rebase on Feb 2, 2022
  19. 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.

  20. DrahtBot added the label Needs rebase on Apr 28, 2022
  21. hebasto force-pushed on Apr 28, 2022
  22. hebasto commented at 7:11 pm on April 28, 2022: member
    Rebased 1593837ecbadea1fa8395e9de14e8f3ad3c0bf91 -> e9fa9acfff6d28ea974e205111a23305c91c14bf (pr23670.04 -> pr23670.05) due to the conflict with #24322.
  23. DrahtBot removed the label Needs rebase on Apr 28, 2022
  24. DrahtBot added the label Needs rebase on Jul 18, 2022
  25. hebasto force-pushed on Jul 20, 2022
  26. hebasto commented at 9:12 am on July 20, 2022: member
    Rebased e9fa9acfff6d28ea974e205111a23305c91c14bf -> 04dae33cff761c9135454b4abb51749f3e2b5d83 (pr23670.05 -> pr23670.06).
  27. DrahtBot removed the label Needs rebase on Jul 20, 2022
  28. fanquake approved
  29. 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.
  30. 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.

  31. 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.
  32. kristapsk commented at 2:42 pm on July 20, 2022: contributor
    Concept ACK
  33. build: Build minisketch test in `make check`, not in `make` 6d58117a31
  34. 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
  35. hebasto force-pushed on Jul 21, 2022
  36. hebasto commented at 9:51 am on July 21, 2022: member

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

  37. TheCharlatan commented at 5:17 pm on January 28, 2023: contributor
    ACK 6d58117a31a88eec3f0a103f9d1fc26cf0b48348
  38. fanquake merged this on Jan 31, 2023
  39. fanquake closed this on Jan 31, 2023
  40. hebasto deleted the branch on Jan 31, 2023
  41. sidhujag referenced this in commit 8f36092b7a on Feb 1, 2023
  42. bitcoin locked this on Jan 31, 2024


hebasto jonatack laanwj jamesob DrahtBot fanquake maflcko kristapsk TheCharlatan

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me