build: create Depends build type for depends and use it by default for depends builds #31920
Conversation
…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.
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.
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31920. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
|
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. |
|
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. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
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.
With this, building depends with DEBUG simply means that the "Depends" type now holds the debug flags (currently Here's a quick before/after reference: 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:
|
|
It might be nice to move the first commit 40a01e9 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.
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. |
I've done that in #32584, as I've got some changes that will build on top of this. |
…host The per-host flag variables hold platform-specific defaults that are ignored when flag environment variables are set, so it was wrong for them to contain -std options relied on by package definitions. Additionally, these flags (-pipe and -std=xx) will no longer be passed into the CMake build, meaning less duplication in the build summary. Pulled out of bitcoin#31920.
…ting them per-host 9954d6c depends: hard-code necessary c(xx)flags rather than setting them per-host (Cory Fields) Pull request description: The per-host flag variables hold platform-specific defaults that are ignored when flag environment variables are set, so it was wrong for them to contain -std options relied on by package definitions. Additionally, these flags (-pipe and -std=xx) will no longer be passed into the CMake build, meaning less duplication in the build summary. Pulled out of #31920. ACKs for top commit: achow101: ACK 9954d6c ryanofsky: Code review ACK 9954d6c. No changes since last review other than improving the commit message. Change overall makes sense because it deduplicates host definitions, stops dropping `-std` flags from package builds when custom CFLAGS/CXXFLAGS environment variables are set, and stops passing duplicate flags to cmake that have no effect. theuni: ACK 9954d6c Tree-SHA512: 62a2a21b741893cf708ca71fb5f0626c30d52685c845f9016f428a5e38fc8515acd4bf2c83635d6505b63830d1c296472026ec3d341c8069f1e490a991b6b5ef
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.