Skip to content

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

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 3 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 28, 2025 •
edited
Loading

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 2 commits April 28, 2025 16:00
@hebasto
cmake: Wrap enable_language() command in macro
6d55894 
This change introduces the `bitcoincore_enable_language()` macro to wrap
CMake’s `enable_language()` command, applying Bitcoin Core-specific
settings and checks.
@hebasto
cmake: Move APPEND_* variables logic to bitcoincore_enable_language
8287a76 
This change is required for the following commit.
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 28, 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/32367.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK janb84, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #33101 (cmake: Proactively avoid use of SECP256K1_DISABLE_SHARED by hebasto)
  • #32865 (cmake: Use AUTHOR_WARNING for warnings by fanquake)

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 mentioned this pull request Apr 28, 2025
Open
@hebasto hebasto marked this pull request as draft April 28, 2025 16:38
@hebasto hebasto force-pushed the 250428-enable-lang branch from 43ccbb1 to 990067f Compare April 28, 2025 16:40
@hebasto hebasto marked this pull request as ready for review April 28, 2025 16:41
@hebasto
cmake: Check user-defined APPEND_* variables early
eb2a943 
This change improves usability in cases where the user provides
`APPEND_*` variables that are incompatible with the compiler for some
reason.
@hebasto hebasto force-pushed the 250428-enable-lang branch from 990067f to eb2a943 Compare April 28, 2025 16:44
janb84
janb84 reviewed Apr 29, 2025
View reviewed changes
Copy link
Contributor

@janb84 janb84 left a comment

Choose a reason for hiding this comment

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

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

@fanquake fanquake mentioned this pull request Apr 29, 2025
Closed
@DrahtBot DrahtBot mentioned this pull request May 7, 2025
Closed
@DrahtBot DrahtBot mentioned this pull request Jul 3, 2025
Open
@DrahtBot DrahtBot mentioned this pull request Jul 30, 2025
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 6, 2025

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

BrandonOdiwuor
BrandonOdiwuor reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

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

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)
Screenshot 2025-08-21 at 13 21 10

(commit: eb2a943)
Screenshot 2025-08-21 at 13 19 51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@janb84 janb84 janb84 left review comments

@BrandonOdiwuor BrandonOdiwuor BrandonOdiwuor left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system Needs rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build: CMake caching failing PIE check
4 participants
@hebasto @DrahtBot @janb84 @BrandonOdiwuor