cmake: Check user-defined APPEND_*FLAGS variables early

[hebasto]

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
...

[DrahtBot]

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 <!--meta-tag:bot-skip--> 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.

[janb84]

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 )

Details

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

[BrandonOdiwuor]

ACK eb2a943

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)

(commit: eb2a943)

[fanquake]

@hebasto was this drafted for any particular reason?

[hebasto]

@hebasto was this drafted for any particular reason?

Rebased and undrafted.

[hebasto]

Rebased to resolve conflicts.

[purpleKarrot]

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]

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]

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

[purpleKarrot]

The release notes for 29.0 contain:

7. 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]

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 hebasto#184 and superseded hebasto#157 after a discussion here hebasto#157 (comment) .

[fanquake]

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]

The release notes for 29.0 contain:

7. 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]

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:

bitcoin/cmake/module/ProcessConfigurations.cmake

Lines 81 to 85 in ff7cdf6

	if(NOT CMAKE_BUILD_TYPE)
	message(STATUS "Setting build type to \"${config}\" as none was specified")
	get_property(help_string CACHE CMAKE_BUILD_TYPE PROPERTY HELPSTRING)
	set(CMAKE_BUILD_TYPE "${config}" CACHE STRING "${help_string}" FORCE)
	endif()

[fanquake]

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]

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 (review)

[purpleKarrot]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.