cmake: Add optional sources to `minisketch` library directly #31880

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:250216-object-libs changing 1 files +22 −30
  1. hebasto commented at 12:52 PM on February 16, 2025: member

    This PR is a continuation of #31268 and applies similar changes to the minisketch library, which addresses this comment.

    Additionally, a workaround for a CMake bug has been removed.

  2. DrahtBot commented at 12:52 PM on February 16, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31880.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theuni
    Concept ACK purpleKarrot

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Build system on Feb 16, 2025
  4. purpleKarrot commented at 8:40 PM on February 16, 2025: contributor

    Concept NACK.

    I am not a big fan of simple utility functions that group a small set of builtin cmake commands. I think they increase the cognitive load and impede knowledge transfer from one cmake project to another. (I remember a time when builtin functions like add_library had to be wrapped and I authored many such wrappers, because there was no way of propagating interface requirements from one target to another. Those times are thankfully over since CMake version 3.0).

  5. DrahtBot added the label CI failed on Feb 17, 2025
  6. DrahtBot removed the label CI failed on Feb 17, 2025
  7. hebasto force-pushed on Feb 17, 2025
  8. DrahtBot added the label Needs rebase on Feb 18, 2025
  9. fanquake commented at 1:28 PM on February 18, 2025: member

    This PR provides an extended alternative to #31268,

    That was merged. Is this still relevant?

  10. cmake: Add optional sources to `minisketch` library directly
    This change eliminates the questionable use of an `OBJECT` library and
removes the corresponding workaround for a CMake bug.
    9919e92022
  11. hebasto force-pushed on Feb 18, 2025
  12. hebasto renamed this:
    cmake: Add optional source files to libraries directly
    cmake: Add optional sources to `minisketch` library directly
    on Feb 18, 2025
  13. hebasto commented at 2:58 PM on February 18, 2025: member

    @purpleKarrot

    Concept NACK.

    I am not a big fan of simple utility functions that group a small set of builtin cmake commands. I think they increase the cognitive load and impede knowledge transfer from one cmake project to another. (I remember a time when builtin functions like add_library had to be wrapped and I authored many such wrappers, because there was no way of propagating interface requirements from one target to another. Those times are thankfully over since CMake version 3.0).

    Based on the above comment, I’ve concluded that this was an “Approach NACK” rather than a “Concept NACK”. Please do let me know if I am mistaken.

    Reworked per feedback.

    This PR provides an extended alternative to #31268,

    That was merged. Is this still relevant?

    Yes. It remains relevant in its updated state.

  14. purpleKarrot commented at 3:02 PM on February 18, 2025: contributor

    ACK. lgtm.

  15. DrahtBot removed the label Needs rebase on Feb 18, 2025
  16. theuni commented at 5:40 PM on February 18, 2025: member

    What's the advantage here over simply making minisketch_clmul static?

  17. hebasto commented at 6:01 PM on February 18, 2025: member

    What's the advantage here over simply making minisketch_clmul static?

    To use the same approach across all similar cases in the project.

  18. theuni commented at 6:07 PM on February 18, 2025: member

    What's the advantage here over simply making minisketch_clmul static?

    To use the same approach across all similar cases in the project.

    Yeah, ok, agreed.

  19. theuni approved
  20. theuni commented at 6:08 PM on February 18, 2025: member

    utACK 9919e92022ba61d2dc1b13b1116b56035be459a6

    One less autotools-ism and CMake hack.

  21. fanquake merged this on Feb 25, 2025
  22. fanquake closed this on Feb 25, 2025
  23. hebasto deleted the branch on Feb 25, 2025
  24. yuvicc referenced this in commit 00b901cd81 on Feb 26, 2025
  25. sedited referenced this in commit 2cab7a09ea on Mar 16, 2025
  26. stickies-v referenced this in commit d760fd3dda on Mar 17, 2025
  27. stickies-v referenced this in commit cc83553352 on Mar 17, 2025
  28. stickies-v referenced this in commit 2614933f06 on Mar 17, 2025
  29. stickies-v referenced this in commit b70418c5fc on Mar 17, 2025
  30. stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025
  31. bug-castercv502 referenced this in commit a6aeab1eeb on Sep 28, 2025
  32. coreymheld commented at 5:24 PM on January 2, 2026: none

    Thank you for helping me figure this out

  33. ivanlele referenced this in commit abe38da954 on Apr 16, 2026
  34. Kino1994 referenced this in commit 5a63c01a0b on Jun 28, 2026
  35. BigcoinBGC referenced this in commit ac481e80b6 on Jun 30, 2026
  36. bitcoin locked this on Jul 30, 2026
Contributors
hebastoDrahtBotpurpleKarrotfanquaketheunicoreymheld
Labels
Build system
Linked (view graph)
#30911 build: simplify by flattening the dependency graph#31268 cmake: add optional source files to bitcoin_crypto and crc32c directly
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: 2026-08-04 00:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me