backport: bitcoin#20182 ci: Build with --enable-werror by default, and document exceptions #5958
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
changed the title
UdjinM6
requested changes
Apr 3, 2024
There was a problem hiding this comment.
8d0da3d looks good but I don't like dropping check-symbols from CI in following commits. Let's maybe try to fix it by using -static-libstdc++ properly: 05a857e should help, CI https://gitlab.com/UdjinM6/dash/-/pipelines/1237445880.
…ocument exceptions 2f6fe4e ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov) Pull request description: This PR prevents introducing of new compiler warnings in the master branch, e.g., bitcoin#19986, bitcoin#20162. ACKs for top commit: practicalswift: cr ACK 2f6fe4e: patch looks correct MarcoFalke: re-ACK 2f6fe4e 🏏 vasild: ACK 2f6fe4e Tree-SHA512: 23b5feb5bc472658c992d882ef61af23496f25adaa19f9c79bfaef5d2db273d44981aa93b1631a7d37cb58755283c1dacf3f2d68e501522d3fa8c965ab646d19
Check new version of branch, please:
|
I accidentally rebased on the top of This commit contains new/changed code |
|
Guix Automation has began to build this PR tagged as v21.0.0-devpr5958.a47635ba. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.0.0-devpr5958.a47635ba. The image should be on dockerhub soon. |
PastaPastaPasta
approved these changes
Apr 3, 2024
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 23, 2024
…ackport bitcoin#25550, bitcoin#24624, bitcoin#24406, cleanup some gArgs usage in init 740d25c merge bitcoin#24406: Fix Wambiguous-reversed-operator compiler warnings (Kittywhiskers Van Gogh) 3265b54 merge bitcoin#24624: Avoid potential -Wdeprecated-enum-enum-conversion warnings (Kittywhiskers Van Gogh) 65585e6 merge bitcoin#25550: remove note on arm cross-compilation from build-unix.md (Kittywhiskers Van Gogh) fbc6bc8 init: use local ArgsManager variable instead of global when possible (Kittywhiskers Van Gogh) 9ae39f9 qt: drop leftover `boost::function` usage in Qt, drop header from list (Kittywhiskers Van Gogh) c47e9e7 bench: drop leftover `boost::lexical_cast` benches, drop header from list (Kittywhiskers Van Gogh) Pull request description: ## Additional Information When building Dash with `clang-16`, I use the following `CXXFLAGS`: ``` -Werror -Werror=reorder -Werror=thread-safety -Wno-unused-command-line-argument -Wno-deprecated-builtins -Wno-deprecated-volatile -Wno-ambiguous-reversed-operator -Wno-deprecated-enum-enum-conversion ``` The explanations of the `-Wno*` flags are as follows: | Argument | Reason | | -------------------------------------- | ------------------------------------------------------------ | | `-Wno-unused-command-line-argument` | Lingering use of `-static-libstdc++`, a GCC-ism that isn't recognized as a valid argument in Clang (no longer an issue thanks to #5958, thanks knst!) | | `-Wno-deprecated-builtins` | Due to usage of [deprecated builtins](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/bench/nanobench.h#L104-L110) in `nanobench` that exist even in the [most current version](https://github.com/martinus/nanobench/blob/a5a50c2b33eea2ff1fcb355cacdface43eb42b25/src/include/nanobench.h#L115-L121) (see below) | | `-Wno-deprecated-volatile` | The detection code for C++20 in the CI image (based on Ubuntu `focal`) relies on deprecated volatiles (see below) | | `-Wno-ambiguous-reversed-operator` | Due to an ambiguous operator in a Dash unit test ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/test/fuzz/addrman.cpp#L145)) (see below) | | `-Wno-deprecated-enum-enum-conversion` | Due to key combination code in Dash Qt ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/qt/bitcoingui.cpp#L376)) (see below) | <details> <summary>-Wno-deprecated-builtins</summary> ``` > make Making all in src make[1]: Entering directory '/src/dash/src' make[2]: Entering directory '/src/dash/src' make[3]: Entering directory '/src/dash' make[3]: Leaving directory '/src/dash' CXX bench/bench_dash-addrman.o In file included from bench/addrman.cpp:6: In file included from ./bench/bench.h:16: ./bench/nanobench.h:952:13: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins] return !ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(Decayed) || sizeof(Decayed) > sizeof(long) || std::is_pointer<Decayed>::value; ^ ./bench/nanobench.h:107:57: note: expanded from macro 'ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE' # define ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__) ^ 1 error generated. make[2]: *** [Makefile:14567: bench/bench_dash-addrman.o] Error 1 make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:18568: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:800: all-recursive] Error 1 ``` </details> <details> <summary>-Wno-deprecated-volatile</summary> ``` > ./configure [...] [...] checking whether clang++-16 supports C++20 features with -h std=c++20... no checking whether clang++-16 supports C++20 features with -std=c++2a... no checking whether clang++-16 supports C++20 features with +std=c++2a... no checking whether clang++-16 supports C++20 features with -h std=c++2a... no configure: error: *** A compiler with support for C++20 language features is required. > cat config.log This file contains any messages produced by compilers while running configure, to aid debugging if configure makes a mistake. It was created by Dash Core configure 21.0.0, which was generated by GNU Autoconf 2.69. Invocation command line was $ ./configure --prefix=/src/dash/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --disable-crash-hooks --disable-stacktraces --enable-c++20 --enable-suppress-external-warnings --enable-debug --disable-ccache --with-sanitizers=thread [...] configure:4450: checking whether clang++-16 accepts -g configure:4470: clang++-16 -c -g -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5 configure:4470: $? = 0 configure:4511: result: yes configure:4537: checking whether make supports the include directive configure:4552: make -f confmf.GNU && cat confinc.out this is the am__doit target configure:4555: $? = 0 configure:4574: result: yes (GNU style) configure:4599: checking dependency style of clang++-16 configure:4710: result: gcc3 configure:4756: checking whether clang++-16 supports C++20 features with -std=c++20 configure:5558: clang++-16 -std=c++20 -c -pipe -O2 -Werror -Werror=reorder -Werror=thread-safety -Wno-deprecated-builtins -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5 conftest.cpp:109:36: error: volatile-qualified parameter type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile] test(const int c, volatile int v) // 'volatile is deprecated in C++20' ^ 1 error generated. ``` </details> <details> <summary>-Wno-ambiguous-reversed-operator</summary> ``` > make Making all in src make[1]: Entering directory '/src/dash/src' make[2]: Entering directory '/src/dash/src' make[3]: Entering directory '/src/dash' make[3]: Leaving directory '/src/dash' [...] CXX test/fuzz/fuzz-addrman.o test/fuzz/addrman.cpp:329:22: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'CAddrManDeterministic' and 'CAddrManDeterministic') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator] assert(addr_man1 == addr_man2); ~~~~~~~~~ ^ ~~~~~~~~~ /usr/include/assert.h:93:27: note: expanded from macro 'assert' (static_cast <bool> (expr) \ ^~~~ test/fuzz/addrman.cpp:145:10: note: ambiguity is between a regular call to this operator and a call with the argument order reversed bool operator==(const CAddrManDeterministic& other) ^ test/fuzz/addrman.cpp:145:10: note: mark 'operator==' as const or add a matching 'operator!=' to resolve the ambiguity 1 error generated. make[2]: *** [Makefile:15393: test/fuzz/fuzz-addrman.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[3]: Leaving directory '/src/dash/src/dashbls' make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:18568: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:800: all-recursive] Error 1 ``` </details> <details> <summary>-Wno-deprecated-enum-enum-conversion</summary> ``` > make Making all in src make[1]: Entering directory '/src/dash/src' make[2]: Entering directory '/src/dash/src' make[3]: Entering directory '/src/dash' make[3]: Leaving directory '/src/dash' [...] CXX qt/libbitcoinqt_a-bitcoingui.o qt/bitcoingui.cpp:376:51: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion] quitAction->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_Q)); ~~~~~~~~ ^ ~~~~~~~~~ qt/bitcoingui.cpp:609:56: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion] minimize_action->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_M)); ~~~~~~~~ ^ ~~~~~~~~~ 2 errors generated. make[2]: *** [Makefile:12803: qt/libbitcoinqt_a-bitcoingui.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:18568: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:800: all-recursive] Error 1 ``` </details> This pull request removes the need for the last two `-Wno*` flags, removes some leftover `boost::`{`function`, `lexical_cast`} usage and some `gArgs` usage where a local variable would be more applicable. Additionally, in some builds, there is an deprecation warning (`-Wdeprecation`) for using `[=]` and relying on its implicit capture of `this` (see below), this can be seen during GCC builds but not Clang builds and correspond to a proposal to the C++20 standard ([proposal](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0806r2.html)). It has also been mentioned at https://en.cppreference.com/w/cpp/language/lambda, "The implicit capture of `*this` when the capture default is `=` is deprecated. _(since C++20)_". This has also been remedied as part of this PR. <details> <summary>-Wdeprecated</summary> ``` > make -j10 [...] CXX qt/libbitcoinqt_a-trafficgraphwidget.o qt/optionsdialog.cpp: In lambda function: qt/optionsdialog.cpp:195:63: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated] 195 | connect(appearance, &AppearanceWidget::appearanceChanged, [=](){ | ^ qt/optionsdialog.cpp:195:63: note: add explicit 'this' or '*this' capture qt/optionsdialog.cpp: In lambda function: qt/optionsdialog.cpp:277:55: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated] 277 | connect(ui->coinJoinEnabled, &QCheckBox::clicked, [=](bool fChecked) { | ^ qt/optionsdialog.cpp:277:55: note: add explicit 'this' or '*this' capture qt/optionsdialog.cpp: In lambda function: qt/optionsdialog.cpp:293:45: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated] 293 | connect(this, &OptionsDialog::rejected, [=]() { | ^ qt/optionsdialog.cpp:293:45: note: add explicit 'this' or '*this' capture CXX qt/libbitcoinqt_a-utilitydialog.o CXX qt/libbitcoinqt_a-addressbookpage.o [...] ``` </details> ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 740d25c knst: utACK 740d25c Tree-SHA512: 52793f9862192ccb556debcac2ec766c4f86e69ec4bedc2fdbaaff965ab4ac478b65b3983b1cb3a92610db85aee6f8668b0ded3494c617c5af7d8801284e461b
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
As discovered in #5957:
Jobs tsan/ubsan fails with backport bitcoin#20182
it fails, because both tsan and ubsan jobs use clang but all other jobs use gcc. Somehow, after configure there are set incorrect options:
clangdoesn't support-static-llibstdc++which is supposed to be gcc-only, it cause this failure:This failure make
autoconfto think that clang doesn't support-Werrorand fails with this error. So, you can't activate this flag.What was done?
Backport bitcoin#20182 and related fixes to make it works
How Has This Been Tested?
CI now succeed with bitcoin#20182. It means, that -Werror activated for clang; also there are not warnings such as:
https://gitlab.com/dashpay/dash/-/jobs/6494328698
Breaking Changes
N/A
Checklist: