ci: Replace `CMAKE_CXX_FLAGS` with `APPEND

hebasto commented at 3:19 pm on January 23, 2025: member

This PR ensures that compiler flags are applied exclusively during compiler invocations.

Below is a summary of differences between CMAKE_CXX_FLAGS and APPEND_CXXFLAGS variables:

	`CMAKE_CXX_FLAGS`	`APPEND_CXXFLAGS`
Origin	CMake’s standard variable	Bitcoin Core’s custom variable
Context	Language-wide, applied during compiling and linking	Compiler only
Position	Prepends others flags	Appends other flags

Fixes #31487. However, during the development of the staging branch, the general consensus was to adhere to CMake’s standard variables as much as possible.

hebasto added the label Tests on Jan 23, 2025

DrahtBot commented at 3:19 pm on January 23, 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/31726.

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:

#31766 ([WIP] leveldb: pull upstream C++23 changes by fanquake)
#30975 (ci: build multiprocess on most jobs by Sjors)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#29881 (guix: use GCC 13 to build releases by fanquake)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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 added this to the milestone 29.0 on Jan 23, 2025

fanquake commented at 3:43 pm on January 23, 2025: member

This will “fix” #31487 for the CI, but just by hiding the underlying issue using APPEND_CXXFLAGS. Maybe this is better in any case, but it doesn’t fix it for general, or non-CI usage.

I’m also wondering if fixing it in this way, is worth introducing the inconsistency of using CMAKE_C_FLAGS but not CMAKE_CXX_FLAGS within the same CI job. I guess this would need to be documented, otherwise any changes or new jobs that don’t use the right variables, will be back to having the less useful output. I’m also wondering about if we start primarily using the APPEND_* flags in the CI we might miss/introduce other issues, for those using the CMAKE_*_FLAGS, given the difference in behaviour of the variable.

hebasto commented at 3:57 pm on January 23, 2025: member

This will “fix” #31487 for the CI, but just by hiding the underlying issue using APPEND_CXXFLAGS. Maybe this is better in any case, but it doesn’t fix it for general, or non-CI usage.

I don’t think there are any underlying issues with APPEND_CXXFLAGS. The summary correctly and completely reports flags passed to a compiler and to a linker, which was not the case with the Autotools-based system.

theuni commented at 7:20 pm on January 23, 2025: member

This will “fix” #31487 for the CI, but just by hiding the underlying issue using APPEND_CXXFLAGS. Maybe this is better in any case, but it doesn’t fix it for general, or non-CI usage.

For better or worse, this is just how CMake works. CMAKE_CXX_FLAGS and CXXFLAGS go to the linker. If there’s no conflict with the linker, I don’t see the harm in the warnings ending up there. But if you don’t want to see them there, I agree that APPEND_CXXFLAGS is the best solution :(

hebasto commented at 7:20 am on January 24, 2025: member

@theuni

Thank you for your feedback!

For better or worse, this is just how CMake works. … CXXFLAGS go to the linker.

This passage might give the impression that this behaviour is specific to CMake. However, it is not:

Passing {C,CXX}FLAGS variables when using Autotools exhibits the same behaviour.

maflcko commented at 7:35 am on January 24, 2025: member

Are there any flags that one could put into CMAKE_CXX_FLAGS and cause a link error (or other error in the resulting binary), when such error wouldn’t happen when putting the same flags in APPEND_CXXFLAGS? I couldn’t come up with one, but it is easier to come up with compiler flags that should also be passed to the linker. So on a quick glance this seems better to leave as-is and close this pull? But no strong opinion, the changes here look fine either way.

hebasto commented at 8:46 am on January 24, 2025: member

Are there any flags that one could put into CMAKE_CXX_FLAGS and cause a link error (or other error in the resulting binary), when such error wouldn’t happen when putting the same flags in APPEND_CXXFLAGS? I couldn’t come up with one

Neither could I.

but it is easier to come up with compiler flags that should also be passed to the linker. So on a quick glance this seems better to leave as-is and close this pull?

In that case, #31487 should be closed, as the summary is technically correct.

ci: Remove no longer needed '-Wno-error=documentation' 5d538171bb

ci: Replace `CMAKE_CXX_FLAGS` with `APPEND_CXXFLAGS`

This change ensures that compiler flags are applied exclusively during
compiler invocations.

d4d5c3d7b1

in ci/test/00_setup_env_i686_multiprocess.sh:21 in 1bd737df55 outdated

17@@ -18,7 +18,7 @@ export BITCOIN_CONFIG="\
18  -DCMAKE_BUILD_TYPE=Debug \
19  -DCMAKE_C_COMPILER='clang;-m32' \
20  -DCMAKE_CXX_COMPILER='clang++;-m32' \
21- -DCMAKE_CXX_FLAGS='-Wno-error=documentation' \
22+ -DAPPEND_CXXFLAGS='-Wno-error=documentation' \

maflcko commented at 8:57 am on January 24, 2025:

Can this be dropped completely?

hebasto commented at 10:58 am on January 24, 2025:

Sure! Dropped.

maflcko commented at 3:46 pm on February 5, 2025:

thx, could submit as a fresh pull?

hebasto commented at 5:26 pm on February 5, 2025:

Sure! Please see #31804.

hebasto force-pushed on Jan 24, 2025

DrahtBot added the label CI failed on Jan 26, 2025

DrahtBot removed the label CI failed on Jan 26, 2025

hebasto closed this on Feb 5, 2025

ci: Replace CMAKE_CXX_FLAGS with APPEND_CXXFLAGS #31726

Code Coverage & Benchmarks

Reviews

Conflicts

ci: Replace `CMAKE_CXX_FLAGS` with `APPEND_CXXFLAGS` #31726