Revert "backport #25972 (ensure WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior) - #6642
Revert "backport #25972 (ensure WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior)#6642knst wants to merge 2 commits into
WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior)#6642Conversation
WalkthroughThe changes remove the export of the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 06a998b2e5af20a460b54eb9868b66ff4e671080 and 9e88380. 📒 Files selected for processing (6)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between db5d000 and 06a998b2e5af20a460b54eb9868b66ff4e671080.
📒 Files selected for processing (13)
ci/dash/build_src.sh(1 hunks)ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(0 hunks)ci/test/00_setup_env_native_qt5.sh(0 hunks)ci/test/00_setup_env_native_sqlite.sh(0 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(2 hunks)doc/developer-notes.md(1 hunks)src/wallet/walletdb.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- ci/test/00_setup_env_native_sqlite.sh
- ci/test/00_setup_env_native_nowallet.sh
- ci/test/00_setup_env_native_qt5.sh
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
🔇 Additional comments (13)
src/wallet/walletdb.cpp (1)
1163-1164: Fixed preprocessor directive structure for database backend handlingThe changes properly close the
#ifdefblocks for both SQLite and BDB database support, ensuring that the error handling code is outside the conditional blocks. This makes the error handling cleaner and more logical, as errors for unsupported database formats are now reported consistently regardless of the specific missing backend.Also applies to: 1171-1172
configure.ac (4)
213-217: Reversed the default behavior for external warnings suppressionThe flag has been changed from
--disable-suppress-external-warnings(default: enabled) to--enable-suppress-external-warnings(default: disabled). This reverses the default behavior to not suppress warnings from external dependencies unless explicitly requested.
455-460: Added workaround for GCC return-type warning bug on MinGW-w64This addition detects and disables the problematic
-Werror=return-typewarning when using GCC on MinGW-w64, which has a known issue with this warning. The fix includes a test case using assertions that would incorrectly trigger the warning.
466-494: Improved compiler warning flags handlingThe code now:
- Only applies warning flags when CXXFLAGS are not user-overridden
- Tests for broken
-Wsuggest-overridein GCC before 9.2 and only adds it if working- Only enables
-Wdocumentationwarnings when external warnings are suppressedThis provides more robust handling of compiler warnings across different environments.
501-503: Conditional handling of warning suppression flagsThe code now only suppresses certain warnings (like
-Wdeprecated-copy) when external warning suppression is not enabled. This provides more consistent control over which warnings are shown or hidden depending on the configuration.ci/dash/build_src.sh (1)
36-36: Added external warnings suppression flag to build configurationAdded
--enable-suppress-external-warningsto the default build configuration to maintain the previous behavior of suppressing warnings from external dependencies. This aligns with the changes in configure.ac where the default was reversed to require explicit opt-in.doc/developer-notes.md (2)
217-217: Updated clang-tidy configuration command with external warnings suppressionAdded the
--enable-suppress-external-warningsflag to the configure command in the documentation for running clang-tidy, aligning with the changes made to the build system.
221-223: Added explanation for external warnings suppression optionsAdded documentation explaining how the
--enable-suppress-external-warningsand--config src/.bear-tidy-configoptions help denoise clang-tidy output by suppressing warnings from external dependencies, with a note that these can be omitted to see all warnings.ci/test/00_setup_env_native_fuzz_with_valgrind.sh (1)
17-17: Approve addition of--enable-suppress-external-warnings
Adding--enable-suppress-external-warningstoBITCOIN_CONFIGhere ensures that external dependency warnings are opt‑in suppressed, matching the other native fuzz environments.ci/test/00_setup_env_native_valgrind.sh (1)
14-14: Approve suppress‑external‑warnings flag for Valgrind
The new--enable-suppress-external-warningsflag is correctly appended alongside existing options.ci/test/00_setup_env_native_fuzz.sh (1)
13-13: Approve newCXXFLAGSfor fuzz environment
DefiningCXXFLAGS="-Werror -Wno-unused-command-line-argument -Wno-unused-value -Wno-deprecated-builtins -Wno-deprecated-volatile"correctly configures Clang to treat warnings as errors while suppressing known benign warnings.ci/test/00_setup_env_arm.sh (1)
28-28: Approve addition of--enable-suppress-external-warningsfor ARM
Including the new flag alongside--enable-reduce-exportsensures external warnings are opt‑in suppressed in the ARM cross‑build environment.ci/test/00_setup_env_win64.sh (1)
16-16: Simplifying BITCOIN_CONFIG by removing manual warning suppressionsThe custom
CXXFLAGSwarnings flags and theNO_WERRORexport have been stripped out here, aligning with the revert’s goal to restore a working GCC build. Please verify that:
- All other
ci/test/00_setup_env_*.shscripts have been updated to use the same trimmed-downBITCOIN_CONFIG.- The Windows CI job now completes successfully under GCC without the suppressed warning flags.
There was a problem hiding this comment.
Quoting inside BITCOIN_CONFIG may not be interpreted as intended
Embedding single‐quoted CFLAGS='-O0 -g0' and CXXFLAGS='-O0 -g0' inside a double‑quoted BITCOIN_CONFIG string will leave literal quotes in the value and break tokenization when passed to ./configure. Consider exporting these environment variables separately for clarity and correctness:
- export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0'"
+ export CFLAGS="-O0 -g0"
+ export CXXFLAGS="-O0 -g0"
+ export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0'" | |
| export CFLAGS="-O0 -g0" | |
| export CXXFLAGS="-O0 -g0" | |
| export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening" |
There was a problem hiding this comment.
Quoting CC/CXX assignments in BITCOIN_CONFIG will be literal
Using CC='clang-18 -ftrivial-auto-var-init=pattern' and CXX='clang++-18 -ftrivial-auto-var-init=pattern' inside a double‑quoted string will not group the two arguments as intended. Instead, export the tools separately and simplify BITCOIN_CONFIG:
- export BITCOIN_CONFIG="--enable-zmq --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer --enable-suppress-external-warnings CC='clang-18 -ftrivial-auto-var-init=pattern' CXX='clang++-18 -ftrivial-auto-var-init=pattern' --with-boost-process"
+ export CC="clang-18 -ftrivial-auto-var-init=pattern"
+ export CXX="clang++-18 -ftrivial-auto-var-init=pattern"
+ export BITCOIN_CONFIG="--enable-zmq --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer --enable-suppress-external-warnings --with-boost-process"06a998b to
9e88380
Compare
WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior)WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior)
|
I reduced scope of this PR to only one commit (backport bitcoin#25972), just in case #6639 will be delayed longer. |
Issue being fixed or feature implemented
This reverts only commit enabled extra warnings 9e88380
developis broken with gcc nowAlternate solution is #6639 [and it's almost ready but not yet]
What was done?
Revert breaking changes.
How Has This Been Tested?
Build succeed with gcc
Breaking Changes
N/A
Checklist: