cmake: Add optional sources to minisketch library directly #31880

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

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

   Code Coverage & Benchmarks

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

   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.

   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.

Contributors
hebasto DrahtBot purpleKarrot fanquake theuni

Labels
Build system