cmake: Check user-defined `APPEND_*FLAGS` variables early #32367

hebasto commented at 4:35 PM on April 28, 2025: member

For the purpose of checks performed by the build system, we strive to handle user-defined APPEND_*FLAGS in the same way as flags provided by other standard means. In particular, they are considered when the try_compile() command is used.

However, these flags are not considered during enable_language() command invocation due to design limitations, which might cause issues, such as #32323.

This PR addresses the issue by introducing an additional compiler check that does consider the user-defined APPEND_*FLAGS.

Fixes #32323:

$ cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init"
-- The CXX compiler identification is GNU 14.2.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test CXX_COMPILER_WORKS
-- Performing Test CXX_COMPILER_WORKS - Failed
CMake Error at cmake/module/EnableLanguage.cmake:42 (message):
  The CXX compiler is not able to compile a simple test program.

  Check that the "APPEND_*FLAGS" variables are set correctly.



Call Stack (most recent call first):
  CMakeLists.txt:71 (bitcoincore_enable_language)


-- Configuring incomplete, errors occurred!
hebasto@TS-P340:~/dev/bitcoin$ cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init=zero"
-- Performing Test CXX_COMPILER_WORKS
-- Performing Test CXX_COMPILER_WORKS - Success
...

hebasto added the label Build system on Apr 28, 2025

DrahtBot commented at 4:35 PM on April 28, 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/32367.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	janb84, BrandonOdiwuor

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34580 (build: Add a compiler minimum feature check by polespinasa)
#33974 (cmake: Check dependencies after build option interaction 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.

hebasto marked this as a draft on Apr 28, 2025

hebasto force-pushed on Apr 28, 2025

hebasto marked this as ready for review on Apr 28, 2025

hebasto force-pushed on Apr 28, 2025

janb84 commented at 9:16 AM on April 29, 2025: contributor

ACK eb2a943

PR fixes #32323 (reproduced on main, checked it was solved by this PR) ✅
Code review looks good to me. Adds a new macro with additional checks and some optimisations.✅ (There is one reference to enable_languages() in secp256k1 but that is separate and not relevant to this PR )

Main:

Configure with faulty flag: cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init"

expected config BREAK

Configure with correct flag: cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init=zero" not expected: STILL BREAKS

This PR:

Configure with faulty flag cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init" expected BREAK

Configure with correct flag cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init=zero" config WORKS

</details>

DrahtBot added the label CI failed on Jul 30, 2025

DrahtBot removed the label CI failed on Jul 30, 2025

DrahtBot added the label Needs rebase on Aug 6, 2025

BrandonOdiwuor commented at 10:43 AM on August 21, 2025: contributor

ACK eb2a9435044ac91442fafc606c5af2f473bff3c5

Confirmed that the new bitcoincore_enable_language(...) macro properly validates user-defined APPEND_*FLAGS early via check_source_compiles(...), fixing #32323 by catching bad flags during enable_language(...), and removing the need a clean build directory.

(system: macOS 15.6)

(branch: master) <img width="1512" height="917" alt="Screenshot 2025-08-21 at 13 21 10" src="https://github.com/user-attachments/assets/be10daae-31c4-4eb7-8504-51fb0a0d72ec" />

(commit: eb2a9435044ac91442fafc606c5af2f473bff3c5) <img width="1512" height="914" alt="Screenshot 2025-08-21 at 13 19 51" src="https://github.com/user-attachments/assets/59de425d-6a48-4ce1-86c2-8378feedbd77" />

hebasto marked this as a draft on Oct 6, 2025

fanquake commented at 11:43 AM on February 13, 2026: member

@hebasto was this drafted for any particular reason?

hebasto force-pushed on Feb 13, 2026

hebasto marked this as ready for review on Feb 13, 2026

hebasto commented at 12:12 PM on February 13, 2026: member

@hebasto was this drafted for any particular reason?

Rebased and undrafted.

DrahtBot removed the label Needs rebase on Feb 13, 2026

DrahtBot added the label Needs rebase on Mar 5, 2026

cmake: Wrap `enable_language()` command in macro

This change introduces the `bitcoincore_enable_language()` macro to wrap
CMake’s `enable_language()` command, applying Bitcoin Core-specific
settings and checks.

9ef9b9d999

cmake: Move `APPEND_*` variables logic to `bitcoincore_enable_language`

This change is required for the following commit.

e83fb84b15

cmake: Check user-defined `APPEND_*` variables early

This change improves usability in cases where the user provides
`APPEND_*` variables that are incompatible with the compiler for some
reason.

cd9367c872

hebasto force-pushed on Mar 5, 2026

hebasto commented at 1:50 PM on March 5, 2026: member

Rebased to resolve conflicts.

DrahtBot removed the label Needs rebase on Mar 5, 2026

sedited requested review from purpleKarrot on Mar 11, 2026

purpleKarrot commented at 4:43 PM on March 11, 2026: contributor

What you are observing here is the so-called intervention spiral. You want to improve one aspect of cmake's builtin behavior, there are unintended side effects, you add a workaround, which have side effects of their own and you need to add more workarounds until the whole system is complex beyond recognition.

The simple approach would be to respect user provided compile flags. Period.

A project that does not modify user provided compile flags does not need any additional mechanism to let the user "append" flags and it also does not need a custom way to enable languages. We should go back to the scratch board and discuss what that original intervention was that pulled in all those other workarounds.

fanquake commented at 4:55 PM on March 11, 2026: member

A project that does not modify user provided compile flags does not need any additional mechanism to let the user "append" flags

What about something like #31843 (https://gitlab.kitware.com/cmake/cmake/-/issues/26757), where the mechanism for the user to pass the flags they want (CMAKE_EXE_LINKER_FLAGS) doesn't work?

purpleKarrot commented at 8:52 PM on March 11, 2026: contributor

@fanquake, is #31843 the reason why APPEND_*FLAGS were introduced? Is CMAKE_EXE_LINKER_FLAGS ignored?

purpleKarrot commented at 9:08 PM on March 11, 2026: contributor

The release notes for 29.0 contain:

If any of the CPPFLAGS, CFLAGS, CXXFLAGS or LDFLAGS environment variables were used in your Autotools-based build process, you should instead use the corresponding CMake variables (APPEND_CPPFLAGS, APPEND_CFLAGS, APPEND_CXXFLAGS and APPEND_LDFLAGS).

CMake does not use the APPEND_*FLAGS by default, but it actually does use the mentioned environment variables. But why is it mentioned that they should not be used?

sedited commented at 8:06 AM on March 12, 2026: contributor

CMake does not use the APPEND_*FLAGS by default, but it actually does use the mentioned environment variables. But why is it mentioned that they should not be used?

We extensively debated the introduction during the migration. The APPEND_ flags were introduced to always have the last say, over build type, or any other cmake configuration. For more context, it was introduced here https://github.com/hebasto/bitcoin/pull/184 and superseded https://github.com/hebasto/bitcoin/pull/157 after a discussion here https://github.com/hebasto/bitcoin/pull/157#issuecomment-2090205960 .

fanquake commented at 8:21 AM on March 12, 2026: member

Is CMAKE_EXE_LINKER_FLAGS ignored?

CMake doesn't put the user supplied flags, last on the link line, meaning they are overriden by CMakes own PIC/PIE flags. So in this case, the user flags are effectively ignored by CMake.

hebasto commented at 9:30 AM on March 12, 2026: member

The release notes for 29.0 contain:

If any of the CPPFLAGS, CFLAGS, CXXFLAGS or LDFLAGS environment variables were used in your Autotools-based build process, you should instead use the corresponding CMake variables (APPEND_CPPFLAGS, APPEND_CFLAGS, APPEND_CXXFLAGS and APPEND_LDFLAGS).

CMake does not use the APPEND_*FLAGS by default, but it actually does use the mentioned environment variables. But why is it mentioned that they should not be used?

Migrating from Autotools to CMake presented several challenges. Perhaps the most significant was that, over the years, users relied on the fact that any user-provided flags override whatever is set by the build system. This is generally not the case with CMake. Another minor issue is that CMake does not support CPPFLAGS.

purpleKarrot commented at 8:54 AM on March 16, 2026: contributor

We extensively debated the introduction during the migration. The APPEND_ flags were introduced to always have the last say, over build type, or any other cmake configuration. For more context, it was introduced here hebasto#184 and superseded hebasto#157 after a discussion here hebasto#157 (comment) .

Those are links to the actual commits that introduced them. Is there a way to reason about the "extensive debate" that led to their introduction? Are they intended as temporary workarounds, long term technical debt, features that should be upstreamed, ... ?

CMake doesn't put the user supplied flags, last on the link line, meaning they are overriden by CMakes own PIC/PIE flags.

CMake does not add any PIC/PIE flags by default. It is the project's configuration that causes the additional flags. The precedence rules (file flags > target flags > directory flags > project flags > env flags) make sense. We are in agreement that CMake#26757 is an issue, but we could just as well simply not use CheckPIESupported.

CMake's default build type "" also does not add any optimization flags that users might disagree with. A problem may be that this build type is explicitly prevented here: https://github.com/bitcoin/bitcoin/blob/ff7cdf633e375f151cccbcc78c7add161b3d29b8/cmake/module/ProcessConfigurations.cmake#L81-L85

fanquake commented at 9:05 AM on March 16, 2026: member

but we could just as well simply not use CheckPIESupported.

If by this you mean, apply the flags we want, manually by default (required because we are shipping with hardening by default), and in a way that user applied flags can override them, then sure, that could be an option (not clear how involved that is). Especially given we are already working around bugs in CMake PIE logic anyways, i.e: https://gitlab.kitware.com/cmake/cmake/-/issues/26463 (fixed in https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10034).

maflcko commented at 9:06 AM on March 16, 2026: member

Those are links to the actual commits that introduced them. Is there a way to reason about the "extensive debate" that led to their introduction? Are they intended as temporary workarounds, long term technical debt, features that should be upstreamed, ... ?

I can't answer those questions, but my understanding is that the feature is needed, because it is not possible otherwise to say: "Thanks for setting all the compiler options in the Bitcoin Core build system, which seem like sane defaults, but I really want to force this one myself, and I know what I am doing".

E.g. ci/test/00_setup_env_native_asan.sh: -DAPPEND_CXXFLAGS='-std=c++23' \

Maybe this is similar to the AUTHOR_WARNING hack, which is the easiest way to achieve the goal, and anything more correct would require considering all use-cases and adjusting the build system to handle (or not handle) all of them one-by-one: #33144#pullrequestreview-3174447010

purpleKarrot commented at 2:51 PM on March 16, 2026: contributor

Thanks for setting all the compiler options in the Bitcoin Core build system, which seem like sane defaults, but I really want to force this one myself, and I know what I am doing

Here lies an important detail. The precedence rules are: "file flags > target flags > directory flags > project flags > env flags". The CMakeLists.txt files are not the right place to set the project's "sane defaults"; rather, they should describe the structure of the source tree, focusing on the what rather than the how. They are the appropriate place to specify required compile flags, but it's crucial that these requirements are dictated by the needs of the source code, not by our conventions or personal preference.

Conventions and personal preferences such as compiler warnings, optimization settings, hardening, etc. are better placed in toolchain files, CI scripts, presets, etc. Keeping these concerns out of CMakeLists.txt files has the following advantages:

Far less code. Easy to read and maintain.
Declarative. No procedural logic like try_append_linker_flag.
Fastererer.
Knowledge transfer. Users who know how to customize the build of one project can customize any project.
Reuse.

Ideally, toolchains and CI scripts should live in a separate repo. Consider stress-testing them on a large, complex CMake project like VTK. If they work there, they will probably work anywhere.

hebasto commented at 3:27 PM on March 16, 2026: member

Conventions and personal preferences such as ... hardening, etc. are better placed in toolchain files, CI scripts, presets, etc.

I strongly believe we need to provide an out-of-the-box option for regular (non-programmer) users to build Bitcoin Core from source using default flags that ensure the safety of their funds. If doing so requires them to pass an additional --toolchain or --preset command-line option, the build process becomes too fragile.

Another case I'm curious about is more developer-specific. Let's say I'm testing the code under the assumption that SSE4.1 is unavailable. On master branch, I can currently run the following:

cmake -B build -DAPPEND_CXXFLAGS="-mno-sse4.1"

How would this be achieved in your proposed alternative?

maflcko commented at 3:35 PM on March 16, 2026: member

dictated by the needs of the source code

I can totally see that point. Though,

I think developers on this project want to have all those flags enabled by default, so that they can compile and then with reasonable assurance know that their code is fine according to the flags picked by the project. If the flags were moved to another place (like a preset), there is no easy way for a dev to append a flag of their choice, unless the APPEND_* feature was kept.
If APPEND_ is kept, then moving the flags to another place seems lower priority, or possibly even orthogonal to this pull request. (Though, personally I wouldn't mind moving them to another place, if the main build system reviewers are on board with it as well)
Some flags will remain to be set in CMakeLists.txt, if removing them would open users to risks, so the additional overhead of having the other flags is discounted.

purpleKarrot commented at 8:48 PM on March 16, 2026: contributor

I strongly believe we need to provide an out-of-the-box option for regular (non-programmer) users to build Bitcoin Core from source using default flags that ensure the safety of their funds. If doing so requires them to pass an additional --toolchain or --preset command-line option, the build process becomes too fragile.

For situations like this, a top level Makefile or justfile could be appropriate, suggested here. So people could run:

just build

In the justfile, it would be possible to append the value of CXXFLAGS to "sane defaults":

cxxflags := "-Wall -Wextra " + env_var_or_default("CXXFLAGS", "")

[private]
configure:
    cmake -B build -S . -DCMAKE_CXX_FLAGS="{{cxxflags}}"

build target="all": configure
    cmake --build build --target {{target}}

test pattern="": build
    ctest --test-dir build -R "{{pattern}}" --output-on-failure

Another case I'm curious about is more developer-specific. Let's say I'm testing the code under the assumption that SSE4.1 is unavailable. On master branch, I can currently run the following:
cmake -B build -DAPPEND_CXXFLAGS="-mno-sse4.1"
How would this be achieved in your proposed alternative?

CXXFLAGS="-mno-sse4.1" just test

hebasto commented at 8:57 PM on March 16, 2026: member

Another case I'm curious about is more developer-specific. Let's say I'm testing the code under the assumption that SSE4.1 is unavailable. On master branch, I can currently run the following:
cmake -B build -DAPPEND_CXXFLAGS="-mno-sse4.1"
How would this be achieved in your proposed alternative?
CXXFLAGS="-mno-sse4.1" just test

Will it worK?

purpleKarrot commented at 9:01 PM on March 16, 2026: contributor

Will it worK?

Try it. Throw the above justfile into the toplevel directory of a CMake project and run the command. You need to have just installed.

hebasto commented at 9:10 PM on March 16, 2026: member

Will it worK?

Try it. Throw the above justfile into the toplevel directory of a CMake project and run the command. You need to have just installed.

This results in -DCMAKE_CXX_FLAGS="-mno-sse4.1". Therefore, it's broken, as it fails to achieve the goal of disabling SSE4.1.

purpleKarrot commented at 9:18 PM on March 16, 2026: contributor

It ends up with -DCMAKE_CXX_FLAGS="-mno-sse4.1", therefore, it's broken because the goal, which is not using SSE4.1, is not achieved.

and -DAPPEND_CXXFLAGS="-mno-sse4.1" has a different behavor? I thought your intention was to append that flag. With the above justfile, I get:

cmake -B build -S . -DCMAKE_CXX_FLAGS="-Wall -Wextra -mno-sse4.1"

so the flag is appended. Did you want to remove it instead?

hebasto commented at 8:26 AM on March 17, 2026: member

It ends up with -DCMAKE_CXX_FLAGS="-mno-sse4.1", therefore, it's broken because the goal, which is not using SSE4.1, is not achieved.

and -DAPPEND_CXXFLAGS="-mno-sse4.1" has a different behavor? I thought your intention was to append that flag. With the above justfile, I get:
cmake -B build -S . -DCMAKE_CXX_FLAGS="-Wall -Wextra -mno-sse4.1"
so the flag is appended. Did you want to remove it instead?

The expected behavior is as follows:

$ cmake -B build -DAPPEND_CXXFLAGS="-mno-sse4.1"
<snip>
-- Performing Test HAVE_SSE41
-- Performing Test HAVE_SSE41 - Failed
<snip>

purpleKarrot commented at 10:58 AM on March 17, 2026: contributor

The expected behavior is as follows:

$ cmake -B build -DAPPEND_CXXFLAGS="-mno-sse4.1"
<snip>
-- Performing Test HAVE_SSE41
-- Performing Test HAVE_SSE41 - Failed
<snip>

I see, you want CXXFLAGS to affect the SIMD capability detection. But is that the right approach?

We can search for the existence of Qt and based on the result we can enable building of the GUI. Or, we can provide an option whether the GUI should be built and based on that setting, we make the existence of Qt a requirement. But we would not use CMake to search for Qt headers and libraries in the paths that the user passes in -I and -L flags of CXXFLAGS and LDFLAGS, right?

In the same sense, we should check for SIMD capabilities and based on the result provide options to use them. Or, provide the options unconditionally and based on the setting make the capability a requirement. But relying on CXXFLAGS or APPEND_CXXFLAGS does not seem the right approach to me.

Also, it requires custom procedural logic like CheckSourceCompilesWithFlags that would not be necessary if we used a more standard approach.