build: create Depends build type for depends and use it by default for depends builds #31920

1. 
   theuni commented at 9:08 pm on February 20, 2025: member

   As discussed a good bit with fanquake and hebasto. Would be nice to have for v29, but it’s very late, so no worries if it doesn’t make it.

   Fundamentally, this creates a “Depends” build type which represents the flags that were used to build depends as opposed to colliding with the other types.

   This allows the forwarding of optional flags into the CMake build for the “Depends” build type, but the user can now optionally use an existing (Debug/RelWitDebInfo/etc.) type to ignore the optimization flags set by depends and use the ones from that type instead.

   As an example, a user may do: make -C depends cmake –toolchain depends/arm64-apple-darwin/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug

   This would compile depends with the default optimization flags for depends (-O2) but build Core with the default debug optimization flags from CMake

   Depends note: For hosts, $host_*FLAGS variables now represent the mandatory flags for compiling and will be forwarded to all Core builds via the toolchain file, regardless of the build type. For most platforms they should be empty, but are useful at least for darwin.

   When setting (for example) CFLAGS from the command line when building depends, these flags will be stored in $host_release_CFLAGS (or debug), and will only be forwarded to the Depends build type.

2. depends: hard-code necessary c(xx)flags rather than setting them per-host

   The per-host flag values now represent the mandatory flags that cannot be
   overridden by the environment. Additionally, these flags (-pipe and -std=xx)
   will no longer be passed into the CMake build.

   40a01e9d30
3. depends: set env-provided flags to per-release

   Users can now safely set flags and have them override the optional
   per-release-type flags.
   
   This also means that user-provided flags will now be forwarded to CMake's
   build-type flags variables so that they'll correctly override the existing
   values.

   f7906de7e8
4. build: create Depends build type for depends and use it by default

   Rather than modifying the existing build types and adding flags for each,
   create a new type specifically for depends.
   
   This allows the forwarding of optional flags into the CMake build for the
   "Depends" build type, but the user can now optionally use an existing
   (Debug/RelWitDebInfo/etc.) type to ignore the optimization flags set by depends
   and use the ones from that type instead.
   
   As an example, a user may do:
   make -C depends
   cmake --toolchain depends/arm64-apple-darwin/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug
   
   This would compile depends with the default optimization flags for depends
   (-O2) but build Core with the default optimization flags from CMake.
   
   Depends note:
   For hosts, $host_*FLAGS variables now represent the mandatory flags for
   compiling and will be forwarded to all Core builds in the toolchain file,
   regardless of the build type. For most platforms they should be empty, but
   are useful at least for darwin.
   
   When setting (for example) CFLAGS from the command line when building depends,
   these flags will be stored in $host_release_CFLAGS (or _debug_), and will only
   be forwarded to the Depends build type.

   dfc074cd5c
5. 
   DrahtBot commented at 9:08 pm on February 20, 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/31920.

   Reviews

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

6. DrahtBot added the label Build system on Feb 20, 2025
7. 
   maflcko commented at 9:16 pm on February 20, 2025: member

   How does this interact with the depends DEBUG build? It looks like this is a breaking change for people (and the ci) picking the Debug build type afterwards. It would be good to clarify this and also adjust the CI scripts, if needed.
8. 
   ryanofsky commented at 9:39 pm on February 20, 2025: contributor

   Current integration between depends and cmake builds definitely does not seem ideal, but it’s hard for me to figure out if this PR makes the situation better. Maybe the PR description can be updated to state some advantages of this new approach? Or some problems with the current approach that this new approach solves?

   I listed a few aspects of the cmake/depends integration that I thought seemed broken and counterintuitive in #30813 (comment) (after “Specifically in the current system…”), and it seems like none of these things are changing after this PR, so I can imagine this PR solving a different set of problems, but I’m not sure which problems.

9. DrahtBot added the label CI failed on Feb 21, 2025
10. 
    DrahtBot commented at 1:54 am on February 21, 2025: contributor

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

    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.

11. 
    theuni commented at 8:18 pm on February 21, 2025: member

    Current integration between depends and cmake builds definitely does not seem ideal, but it’s hard for me to figure out if this PR makes the situation better. Maybe the PR description can be updated to state some advantages of this new approach? Or some problems with the current approach that this new approach solves?

    I listed a few aspects of the cmake/depends integration that I thought seemed broken and counterintuitive in #30813 (comment) (after “Specifically in the current system…”), and it seems like none of these things are changing after this PR, so I can imagine this PR solving a different set of problems, but I’m not sure which problems.

    Yeah, I don’t think this is intended to address any of those concerns. This is intended to define a sane way of forwarding compile flags from depends. I do agree feature options aren’t ideal at the moment though.

    How does this interact with the depends DEBUG build? It looks like this is a breaking change for people (and the ci) picking the Debug build type afterwards. It would be good to clarify this and also adjust the CI scripts, if needed.

    With this, building depends with DEBUG simply means that the “Depends” type now holds the debug flags (currently -O1 -g from depends).

    Here’s a quick before/after reference:

     0make -C depends
     1cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake
     2resulting cxxflags before: -pipe -std=c++20 -O2 -O2 -g
     3resulting cxxflags after:  -O2
     4
     5make -C depends
     6cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug
     7resulting cxxflags before: -pipe -std=c++20 -O0 -ftrapv -g3
     8resulting cxxflags after:  -O0 -ftrapv -g3
     9
    10make -C depends DEBUG=1
    11cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake
    12resulting cxxflags before: -pipe -std=c++20 -O2 -g
    13resulting cxxflags after:  -O1 -g
    14
    15make -C depends DEBUG=1
    16cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug
    17resulting cxxflags before: -pipe -std=c++20 -O0 -ftrapv -O1 -g3 -g3
    18resulting cxxflags after:  -O0 -ftrapv -g3
    

    Basically, the “Depends” build type now comes from the debug/release flags, and the rest are mandatory that get passed to all build types.

    I think this is a much saner system, but it did change/break a few things, so it’s not ready to go in as-is. Will convert to draft. Curious to see if there are any Concept ACKs here though. It’s not ideal, but at least it’s not arbitrary clobbering as we have now.

    The issues specifically are:

    • -g is now off for a default depends Core build because it’s off for the “release” depends builds
    • only the “Depends” build works for Linux with DEBUG=1 because -D_GLIBCXX_DEBUG is then required unconditionally (Debug/Release/RelWithDebInfo won’t work)
    • Guix would probably need to be explicitly set to RelWithDebInfo so that it’s allowed to diverge from depends
    • ci builds that use the Debug type would need to be updated as that’s probably not necessary anymore (this one is actually a good thing)
12. theuni marked this as a draft on Feb 27, 2025
13. 
    ryanofsky commented at 4:08 pm on March 25, 2025: contributor

    It might be nice to move the first commit 40a01e9d30b0c7deabd5996867f248637f3d1a08 into a separate PR because it seems straightforwardly good (deduplicating code and moving flags that aren’t host specific out of host-specific configurations) and it would remove complexity from this PR.

    Yeah, I don’t think this is intended to address any of those concerns. This is intended to define a sane way of forwarding compile flags from depends.

    Can you explain what specific problems or concerns this PR is addressing?

    I can see looking at the current toolchain file that it does some odd things, but it looks to me like those things could be directly fixed using an approach like I described #30813 (comment), where we define a clear precedence order like “the depends toolchain file should be able to see and freely override cmake defaults, and users should be able to see and freely override depends defaults” and then implement it.

    In this PR, instead of choosing a precedence order, this adds a new “depends” build type that gives users a choice of whether to ignore cmake defaults and use depends defaults by using the new “depends” build type, or to ignore depends defaults and use cmake defaults by using an ordinary build type.

    This seems kludgy to me because the both choices seem suboptimal and the choice itself should be unnecessary when cmake seems to provide everything the depends system needs to set sane default values for existing build types without introducing a new build type.

    But it’s hard to evaluate this PR without knowing what specific problems it is trying to address, since maybe they are not the same as the ones I can see.

Contributors
theuni DrahtBot maflcko ryanofsky

Labels
Build system CI failed