Skip to content

ci: Replace CMAKE_CXX_FLAGS with APPEND_CXXFLAGS #31726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

ci: Replace CMAKE_CXX_FLAGS with APPEND_CXXFLAGS #31726

wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 23, 2025

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 hebasto added the Tests label Jan 23, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 23, 2025 •
edited
Loading

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 hebasto added this to the 29.0 milestone Jan 23, 2025
@fanquake
Copy link
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
Copy link
Member Author

hebasto commented Jan 23, 2025

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
Copy link
Member

theuni commented Jan 23, 2025

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 :(

This was referenced Jan 24, 2025
Closed
Merged
Merged
@hebasto
Copy link
Member Author

hebasto commented Jan 24, 2025

@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
Copy link
Member

maflcko commented Jan 24, 2025

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
Copy link
Member Author

hebasto commented Jan 24, 2025

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.

maflcko
maflcko reviewed Jan 24, 2025
View reviewed changes
ci/test/00_setup_env_i686_multiprocess.sh Outdated
@@ -18,7 +18,7 @@ export BITCOIN_CONFIG="\
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_C_COMPILER='clang;-m32' \
-DCMAKE_CXX_COMPILER='clang++;-m32' \
-DCMAKE_CXX_FLAGS='-Wno-error=documentation' \
-DAPPEND_CXXFLAGS='-Wno-error=documentation' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be dropped completely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Dropped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, could submit as a fresh pull?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Please see #31804.

@hebasto
ci: Remove no longer needed '-Wno-error=documentation'
5d53817
@hebasto
ci: Replace CMAKE_CXX_FLAGS with APPEND_CXXFLAGS
d4d5c3d 
This change ensures that compiler flags are applied exclusively during
compiler invocations.
This was referenced Jan 30, 2025
Merged
Open
@maflcko maflcko mentioned this pull request Feb 5, 2025
Closed
@hebasto hebasto closed this Feb 5, 2025
@hebasto hebasto mentioned this pull request Feb 5, 2025
Merged
fanquake added a commit that referenced this pull request Feb 6, 2025
@fanquake
Merge #31804: ci: Remove no longer needed -Wno-error=documentation
d6c229d 
f1d7a6d ci: Remove no longer needed '-Wno-error=documentation' (Hennadii Stepanov)

Pull request description:

  Picked from #31726.

ACKs for top commit:
  maflcko:
    lgtm ACK f1d7a6d

Tree-SHA512: 2c4b197da1a60662922341062f9c1fe43b0e35a50194f4757057a48d7538f2f68540ed56508193a5c6a81e3f7dfbca78b15e3c101aae08d8ccd1fc5df0535932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
29.0
Development

Successfully merging this pull request may close these issues.

build: compiler flags in linker flags output
5 participants
@hebasto @DrahtBot @fanquake @theuni @maflcko