cmake: add optional source files to bitcoin_crypto directly #31268

1. 
   purpleKarrot commented at 11:22 am on November 11, 2024: none

   Avoid having many static libraries by adding the optional sources to the target bitcoin_crypto directly. Set the necessary compile options at the source file level, rather than the target level.
2. 
   DrahtBot commented at 11:22 am on November 11, 2024: 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/31268.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	sipa
   Approach ACK	BrandonOdiwuor

   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:

   • #31161 (cmake: Set top-level target output locations 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.

3. DrahtBot added the label Build system on Nov 11, 2024
4. in src/crypto/CMakeLists.txt:57 in 7cdae6d7e0 outdated

   72 if(HAVE_ARM_SHANI)
   73-  add_library(bitcoin_crypto_arm_shani STATIC EXCLUDE_FROM_ALL
   74-    sha256_arm_shani.cpp
   75+  target_compile_definitions(bitcoin_crypto PRIVATE ENABLE_ARM_SHANI)
   76+  target_sources(bitcoin_crypto PRIVATE sha256_arm_shani.cpp)
   77+  set_property(SOURCE sha256_x86_shani.cpp PROPERTY
   

   ---

   ---

   

   willcl-ark commented at 11:26 am on November 11, 2024:

   I think you have a copy-paste error here. x86 should be arm?
5. purpleKarrot force-pushed on Nov 11, 2024
6. DrahtBot added the label CI failed on Nov 11, 2024
7. 
   DrahtBot commented at 11:29 am on November 11, 2024: contributor

   🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32802162894

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

8. cmake: add optional source files to bitcoin_crypto directly ed2cd60878
9. purpleKarrot force-pushed on Nov 11, 2024
10. DrahtBot removed the label CI failed on Nov 11, 2024
11. 
    fanquake commented at 2:17 pm on November 11, 2024: member

    Avoid having many static libraries

    Can you expand the motivation.

12. 
    purpleKarrot commented at 2:44 pm on November 11, 2024: none

    There are four alternative implementations for sha256, each in its own source file which has to be compiled with dedicated compiler options. The current approach is to build a static library per source file, set the necessary compiler options for that library, and eventually link bitcoin_crypto against those libraries. This is needless complexity.

    My proposed approach is to compile the source file with its dedicated options, but link the object files into bitcoin_crypto directly.

13. 
    sipa commented at 3:28 pm on November 11, 2024: member

    Concept ACK. If it’s possible to have differently-compiled compilation units without needing separate libraries for each, then this does seem like a cleaner solution. I suspect we perhaps ended up in this state because in autotools separate libraries were the only way to achieve this?
14. 
    BrandonOdiwuor commented at 10:59 am on November 14, 2024: contributor

    Approach ACK. Great catch!

    Nice simplification! Adding the optional source files directly to bitcoin_crypto eliminating the need for intermediate libraries like bitcoin_crypto_sse41, bitcoin_crypto_avx2, and bitcoin_crypto_x86_shani

15. 
    hebasto commented at 8:01 pm on November 16, 2024: member

    Avoid having many static libraries by adding the optional sources to the target bitcoin_crypto directly. Set the necessary compile options at the source file level, rather than the target level.

    While working on the CMake staging branch, this was done on purpose (see https://github.com/hebasto/bitcoin/pull/172 and similar https://github.com/hebasto/bitcoin/pull/171). However, the goal PR has since been replaced.

    Additionally, from the Professional CMake: A Practical Guide 19th Edition:

    Developers should be aware of an implementation detail which may present a strong deterrent to their use in some situations. For some CMake generators (notably the Unix Makefiles generator), the dependencies between sources and source properties are stronger than one might expect. If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file. This is a limitation of how the dependency details are handled in the Makefile, where testing whether each source file’s compiler flags have changed brings with it a prohibitively big performance hit. The relevant Makefile dependencies were implemented at the target level instead to avoid that problem.

Contributors
purpleKarrot DrahtBot willcl-ark fanquake sipa BrandonOdiwuor hebasto

Labels
Build system