cmake: Improve safety and robustness during building crc32c subtree #31779

1. 
   hebasto commented at 1:32 pm on February 2, 2025: member

   It was agreed not to adopt the upstream buildsystem the crc32c subtree.

   So this PR ensures that our custom build script properly handles potentially harmful situations, including:

   1. The upstream buildsystem changes remaining unnoticed during the subtree update, even when some of those changes need to be reflected in crc32c.cmake.

   2. The order of include(cmake/introspection.cmake) and include(cmake/crc32c.cmake) being inadvertently altered during refactoring or other changes in the main buildsystem.

2. cmake, refactor: Add `crc32c_subtree_dir` helper variable

   This change minimizes repetitions and improves readability.

   cf7e0e0080
3. 
   DrahtBot commented at 1:32 pm on February 2, 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/31779.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31662 (cmake: Do not modify CMAKE_TRY_COMPILE_TARGET_TYPE globally by hebasto)
   • #31268 (cmake: add optional source files to bitcoin_crypto and crc32c directly by purpleKarrot)

   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.

4. DrahtBot added the label Build system on Feb 2, 2025
5. hebasto marked this as a draft on Feb 2, 2025
6. 
   purpleKarrot commented at 5:27 pm on February 2, 2025: none

   Whether the upstream CMakeLists.txt file is used or not, the crc32c subtree should be added with add_subdirectory and not include. The latter prevents variables from leaking into the parent directory.
7. cmake, crc32c: Check for upstream buildsystem changes

   This change ensures that no upstream modifications go unnoticed during
   the crc32c subtree update. Certain changes, such as conditionally
   including a new source file, must be reflected in the `crc32c.cmake`
   script.

   bbc947e776
8. cmake, crc32c: Improve check robustness

   This change prevents accidental breakage of the compiled code when
   moving code around `include(cmake/crc32c.cmake)`.

   5e98b5122f
9. in cmake/crc32c.cmake:12 in 46a21be30c outdated

    9+
   10+# We do not use the upstream CMake-based buildsystem.
   11+# See the discussion in https://github.com/bitcoin/bitcoin/pull/30905.
   12+# However, we want to ensure that no changes upstream remain unnoticed.
   13+file(SHA256 ${crc32c_subtree_dir}/CMakeLists.txt upstream_cmakelists_hash)
   14+if(NOT upstream_cmakelists_hash STREQUAL "f5c90773b3afd4ee95e792356a802758a29a3751878d0388c555c575c19ed4ce")
   

   ---

   ---

   

   fanquake commented at 10:15 am on February 3, 2025:

   Why does this file sha256sum to something different in the Windows builds: https://github.com/bitcoin/bitcoin/actions/runs/13099284830/job/36545477809?pr=31779#step:8:925:

   0 CMake Error at cmake/crc32c.cmake:12 (message):
   1  D:/a/bitcoin/bitcoin/src/crc32c/CMakeLists.txt has been modified.
   2Call Stack (most recent call first):
   3  CMakeLists.txt:594 (include)
   4
   5-- Configuring incomplete, errors occurred!
   

   ---

   

   hebasto commented at 10:22 am on February 3, 2025:

   I think this happens due to the Windows-style line endings.

   ---

   

   hebasto commented at 11:43 am on February 3, 2025:

   Fixed in the recent push.
10. hebasto force-pushed on Feb 3, 2025
11. 
    hebasto commented at 11:50 am on February 3, 2025: member

    @purpleKarrot

    Whether the upstream CMakeLists.txt file is used or not, the crc32c subtree should be added with add_subdirectory and not include. The latter prevents variables from leaking into the parent directory.

    Certainly, I agree with keeping things as scoped as possible.

    The initial plan was to replace include(cmake/crc32c.cmake) with add_subdirectory(src/crc32c). However, the perspective has since changed.

    Your suggestion is not specific to crc32c alone but applies to other subtrees as well. It seems beyond the scope of this PR and might be better suited for a separate one, no?

12. 
    fanquake commented at 12:03 pm on February 3, 2025: member

    ~0.

    However, the perspective has since #30905 (comment).

    #30905 wasn’t merged because it just took upstreams compilation flags, which didn’t make sense for us, and because switching came with pretty much no benefit (and still couldn’t be done without other modifications to the subtree).

    I think time would be better spent modifying our subtree, so we could use the CMake file (if that’s what we really want to do), rather than trying to implement changes here to detect situations that are unlikely to occur (especially given the state of upstream development) and would just be prevented by switching.

13. hebasto marked this as ready for review on Feb 3, 2025
14. 
    fanquake commented at 6:06 pm on February 11, 2025: member

    Another thought, isn’t this going to break git bisect, or, result in having to do unclean subtree pulls? As the commit where you update the subtree will no-longer compile unless it also simultaneously changes the hash hardcoded into our build system.
15. 
    hebasto commented at 9:25 am on February 14, 2025: member

    Another thought, isn’t this going to break git bisect, or, result in having to do unclean subtree pulls? As the commit where you update the subtree will no-longer compile unless it also simultaneously changes the hash hardcoded into our build system.

    Fair enough.

16. hebasto closed this on Feb 14, 2025

    ---

Contributors
hebasto DrahtBot purpleKarrot fanquake

Labels
Build system