ci: Build with --enable-werror by default, and document exceptions

hebasto commented at 0:51 am on October 19, 2020: member

This PR prevents introducing of new compiler warnings in the master branch, e.g., #19986, #20162.

fanquake added the label Build system on Oct 19, 2020

hebasto force-pushed on Oct 19, 2020

in ci/test/00_setup_env_i686_centos.sh:17 in ea8f849316 outdated

12@@ -13,3 +13,7 @@ export DOCKER_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-
13 export GOAL="install"
14 export BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-reduce-exports --with-boost-process"
15 export CONFIG_SHELL="/bin/dash"
16+
17+# gcc-c++-4.8.5 is buggy, e.g.,

fanquake commented at 2:41 am on October 19, 2020:

I’d rather wait until the next branch off so that we don’t have to worry about compilers we are going to drop support for anyways.

hebasto commented at 8:12 am on October 19, 2020:

That’s why this is a draft for now :)

fanquake commented at 2:43 am on October 19, 2020: member

Please fill out the PR description. What’s the motivation for this change.

DrahtBot commented at 7:07 am on October 19, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20544 (build: Do not repeat warning names in -Werror=… options by hebasto)
#17227 (Qt: Add Android packaging support by icota)

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 force-pushed on Oct 19, 2020

in configure.ac:386 in 0179abea02 outdated

381@@ -382,25 +382,10 @@ if test "x$enable_werror" = "xyes"; then
382   if test "x$CXXFLAG_WERROR" = "x"; then
383     AC_MSG_ERROR("enable-werror set but -Werror is not usable")
384   fi
385-  AX_CHECK_COMPILE_FLAG([-Werror=gnu],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=gnu"],,[[$CXXFLAG_WERROR]])

practicalswift commented at 6:55 pm on October 19, 2020:

I see -Werror being removed here, but where is it re-added? :)

hebasto commented at 10:19 pm on October 19, 2020:

Here: https://github.com/bitcoin/bitcoin/blob/0179abea02576b4e4291d36d14a2915d3d3c477d/configure.ac#L385

practicalswift commented at 7:36 am on October 20, 2020:

Heh, thanks! Literally on the same line :) Sorry.

practicalswift commented at 7:37 am on October 20, 2020: contributor

Concept ACK: deduplication is good

in ci/test/06_script_a.sh:27 in 0179abea02 outdated

23@@ -21,7 +24,7 @@ DOCKER_EXEC mkdir -p "${BASE_BUILD_DIR}"
24 export P_CI_DIR="${BASE_BUILD_DIR}"
25 
26 BEGIN_FOLD configure
27-DOCKER_EXEC "${BASE_ROOT_DIR}/configure" --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( (DOCKER_EXEC cat config.log) && false)
28+DOCKER_EXEC "${BASE_ROOT_DIR}/configure" --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG $WERROR_CONFIG || ( (DOCKER_EXEC cat config.log) && false)

vasild commented at 8:37 am on October 21, 2020:

Where does WERROR_CONFIG come from?

hebasto commented at 9:18 am on October 21, 2020:

Where does WERROR_CONFIG come from?

It’s from an previous version of this pull. Going to remove. Thanks!

in configure.ac:426 in 0179abea02 outdated

419@@ -433,6 +420,10 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
420   AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]])
421   AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
422   AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]])
423+  if test -z "${host##*darwin*}"; then
424+    dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780
425+    AX_CHECK_COMPILE_FLAG([-Wunused-command-line-argument],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-command-line-argument"],,[[$CXXFLAG_WERROR]])
426+  fi

vasild commented at 8:39 am on October 21, 2020:

Shouldn’t this be done if the compiler is clang, rather than if the OS is macos?

hebasto commented at 9:27 am on October 21, 2020:

Yes, -Wunused-command-line-argument is clang specific. So the if test... is not required at all, right?

vasild commented at 9:51 am on October 21, 2020:

Right!

vasild commented at 9:03 am on October 21, 2020: member

Approach ACK

MarcoFalke commented at 9:07 am on October 21, 2020: member

The build system changes should go separate from the test/ci changes. Are they required or orthogonal?

vasild commented at 9:14 am on October 21, 2020: member

[@hebasto, correct me if I am wrong] @MarcoFalke, if they are going to go as separate PRs, then I think the CI change must go first and after that the configure.ac change. Otherwise we will get CI trying to compile with full -Werror and without --enable-suppress-external-warnings which will likely cause build failures.

hebasto commented at 9:16 am on October 21, 2020: member

The build system changes should go separate from the test/ci changes. Are they required or orthogonal?

CI changes require build system changes.

hebasto force-pushed on Oct 21, 2020

in configure.ac:425 in 81ade9b338 outdated

419@@ -433,6 +420,8 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
420   AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]])
421   AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
422   AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]])
423+  dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780
424+  AX_CHECK_COMPILE_FLAG([-Wunused-command-line-argument],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-command-line-argument"],,[[$CXXFLAG_WERROR]])

vasild commented at 10:02 am on October 21, 2020:

When can this workaround be removed? It is not clear from the linked comment.

I think whenever -Wno-foo is added (silencing a class of warnings in Bitcoin Core code) that should link a PR that fixes those warnings or at least some concrete plan to ensure that the added -Wno-foo will not be forgotten and remain forever in configure.ac.

hebasto commented at 10:08 am on October 21, 2020:

Right, but we have no a PR with a fix now ((

vasild approved

vasild commented at 10:03 am on October 21, 2020: member

ACK 81ade9b33

hebasto force-pushed on Nov 5, 2020

vasild commented at 3:44 pm on November 5, 2020: member

ACK 3a409f6

hebasto force-pushed on Nov 26, 2020

hebasto renamed this:
~~ci: Enable -Werror for Travis builds, and document exceptions~~
ci: Enable -Werror, and document exceptions
on Nov 26, 2020

hebasto marked this as ready for review on Nov 26, 2020

hebasto force-pushed on Nov 26, 2020

hebasto commented at 8:16 pm on November 26, 2020: member

@fanquake @practicalswift @vasild @MarcoFalke

Mind (re)reviewing this PR (at the early stage of 22.0 cycle)?

practicalswift commented at 9:29 pm on November 26, 2020: contributor

cr ACK 2ca5156ab4309a0a1dd3e17e07a1f86a411589dd: patch looks correct!

in src/qt/guiutil.cpp:71 in 2ca5156ab4 outdated

67@@ -67,7 +68,7 @@ namespace GUIUtil {
68 
69 QString dateTimeStr(const QDateTime &date)
70 {
71-    return date.date().toString(Qt::SystemLocaleShortDate) + QString(" ") + date.toString("hh:mm");
72+    return QLocale::system().toString(date.date(), QLocale::ShortFormat) + QString(" ") + date.toString("hh:mm");

MarcoFalke commented at 7:43 am on November 27, 2020:

this is not a ci change, but a gui refactor change? Should probably be separate

hebasto commented at 8:13 am on November 27, 2020:

Done in https://github.com/bitcoin-core/gui/pull/137

hebasto commented at 8:24 pm on December 1, 2020:

^ merged :)

vasild commented at 12:43 pm on November 27, 2020: member

The non-QT changes look good. We now use Werror also on CentOS after ditching gcc 4, good!

I don’t feel confident enough to ACK the QT changes.

hebasto commented at 12:52 pm on November 27, 2020: member

I don’t feel confident enough to ACK the QT changes.

Qt changes split into https://github.com/bitcoin-core/gui/pull/137 as requested by @MarcoFalke.

vasild commented at 1:02 pm on November 27, 2020: member

QT changes are still in this PR.

hebasto commented at 1:23 pm on November 27, 2020: member

QT changes are still in this PR.

Yes, as this PR is based on https://github.com/bitcoin-core/gui/pull/137 (OP has been updated accordingly).

jonasschnelli referenced this in commit f2a673f15b on Dec 1, 2020

sidhujag referenced this in commit fad2b2fac8 on Dec 1, 2020

hebasto force-pushed on Dec 1, 2020

hebasto commented at 8:23 pm on December 1, 2020: member

Rebased 2ca5156ab4309a0a1dd3e17e07a1f86a411589dd -> a00dd1e1824f9cb38551e5be4bd1c7be10c9db16 (pr20182.07 -> pr20182.08) on top of the merged https://github.com/bitcoin-core/gui/pull/137.

Now this PR contains non-Qt changes only.

in ci/test/06_script_a.sh:9 in a00dd1e182 outdated

 5@@ -6,7 +6,10 @@
 6 
 7 export LC_ALL=C.UTF-8
 8 
 9-BITCOIN_CONFIG_ALL="--disable-dependency-tracking --prefix=$DEPENDS_DIR/$HOST --bindir=$BASE_OUTDIR/bin --libdir=$BASE_OUTDIR/lib"
10+BITCOIN_CONFIG_ALL="--enable-suppress-external-warnings --disable-dependency-tracking --prefix=$DEPENDS_DIR/$HOST --bindir=$BASE_OUTDIR/bin --libdir=$BASE_OUTDIR/lib"

MarcoFalke commented at 8:12 am on December 2, 2020:

should the diff in this line (9) be part of the first commit?

hebasto commented at 8:31 am on December 2, 2020:

Why do you think so? The first commit seems complete to me, and it shouldn’t fire any error on CI, no?

MarcoFalke commented at 8:34 am on December 2, 2020:

Ok, then I think they should be a separate pull request: #20182 (comment)

Doing build system changes under the ci: prefix seems confusing.

hebasto commented at 8:51 am on December 2, 2020:

Done in #20544.

MarcoFalke commented at 9:14 am on December 2, 2020:

20544 fails without --enable-suppress-external-warnings:

0/tmp/cirrus-ci-build/depends/arm-linux-gnueabihf/include/boost/asio/impl/error.ipp:102:15: error:   by ‘virtual std::__cxx11::string boost::asio::error::detail::misc_category::message(int) const’ [-Werror=overloaded-virtual]
1   std::string message(int value) const
2               ^~~~~~~

hebasto commented at 9:45 am on December 2, 2020:

should the diff in this line (9) be part of the first commit?

You’re right. Some jobs already have --enable-werror :man_facepalming:

hebasto commented at 8:52 am on December 2, 2020: member

The build system changes should go separate from the test/ci changes.

Build system changes. i.e. the first commit, has been split out into a separated pr #20544.

hebasto force-pushed on Dec 2, 2020

vasild commented at 6:36 am on December 3, 2020: member

To resolve the dependency tangle with #20544 I think that:

This PR should drop its first commit (the configure.ac changes, contained in #20544). However the changes to ci/test/06_script_a.sh which are now part of that first commit should be moved to the second commit (ci changes).
This PR merged (it will not cause any ci breakage).
#20544 should remove the changes to ci/test/06_script_a.sh which are contained in this PR and already merged.
#20544 merged (it will not cause any breakage because ci is now prepared by the already merged PR).

hebasto force-pushed on Dec 3, 2020

hebasto commented at 9:00 am on December 3, 2020: member

To resolve the dependency tangle with #20544 I think that:

This PR should drop its first commit (the configure.ac changes, contained in #20544). However the changes to ci/test/06_script_a.sh which are now part of that first commit should be moved to the second commit (ci changes).

This PR merged (it will not cause any ci breakage).

#20544 should remove the changes to ci/test/06_script_a.sh which are contained in this PR and already merged.

#20544 merged (it will not cause any breakage because ci is now prepared by the already merged PR).

Something similar was in my mind, so done.

hebasto commented at 9:05 am on December 3, 2020: member

@MarcoFalke the assigned label could be switched from “Build system” to “Tests” now, right?

MarcoFalke removed the label Build system on Dec 3, 2020

DrahtBot added the label Tests on Dec 3, 2020

in ci/test/00_setup_env_s390x.sh:27 in cbb234c0da outdated

23@@ -24,3 +24,4 @@ export TEST_RUNNER_ENV="LC_ALL=C"
24 export RUN_FUNCTIONAL_TESTS=true
25 export GOAL="install"
26 export BITCOIN_CONFIG="--enable-reduce-exports --with-incompatible-bdb --with-boost-process"
27+export NO_WERROR=1

MarcoFalke commented at 10:41 am on December 3, 2020:

why? compiles fine for me

hebasto commented at 2:20 pm on December 3, 2020:

Thanks! Updated.

in ci/test/00_setup_env_native_nowallet.sh:18 in cbb234c0da outdated

11@@ -12,3 +12,7 @@ export PACKAGES="python3-zmq clang-5.0 llvm-5.0"  # Use clang-5 to test C++17 co
12 export DEP_OPTS="NO_WALLET=1"
13 export GOAL="install"
14 export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CC=clang-5.0 CXX=clang++-5.0 --with-boost-process"
15+
16+# clang < 6.0 emits the -Wmissing-braces warning when initializing an std::array with only one pair of braces:
17+#  - https://bugs.llvm.org/show_bug.cgi?id=21629
18+export NO_WERROR=1

MarcoFalke commented at 11:20 am on December 3, 2020:

I can’t reproduce locally

hebasto commented at 2:20 pm on December 3, 2020:

Updated.

MarcoFalke commented at 11:22 am on December 3, 2020: member

Not sure if this makes sense by itself in the current form. I failed to reproduce the compile failures locally.

hebasto force-pushed on Dec 3, 2020

hebasto renamed this:
~~ci: Enable -Werror, and document exceptions~~
ci: Build with --enable-werror by default, and document exceptions
on Dec 3, 2020

hebasto commented at 2:19 pm on December 3, 2020: member

Updated cbb234c0da2e98aa9fec06896a78d2ffb66ad6b7 -> 0e2d9ef54cdf21fddcfec1d8188633fb159ea993 (pr20182.10 -> pr20182.11):

removed unrelated changes
rebased on top of the recent CI changes

vasild approved

vasild commented at 2:22 pm on December 3, 2020: member

ACK 0e2d9ef5

I am fine with NO_WERROR=1 in some of the .sh files as long as those files did not contain --enable-werror in their BITCOIN_CONFIG before this PR. I.e. they did not use werror before this PR and will not use it after this PR either.

Surely, the less NO_WERROR=1 the better :)

in ci/test/00_setup_env_win64.sh:20 in 0e2d9ef54c outdated

13@@ -14,3 +14,4 @@ export RUN_FUNCTIONAL_TESTS=false
14 export RUN_SECURITY_TESTS="true"
15 export GOAL="deploy"
16 export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --without-boost-process"
17+export NO_WERROR=1

MarcoFalke commented at 4:50 pm on December 3, 2020:

I haven’t tried this one yet. What is the error here?

hebasto commented at 5:04 pm on December 3, 2020:

https://cirrus-ci.com/task/6268532532969472

0script/interpreter.cpp: In function ‘bool EvalChecksig(const valtype&, const valtype&, prevector<28, unsigned char>::const_iterator, prevector<28, unsigned char>::const_iterator, ScriptExecutionData&, unsigned int, const BaseSignatureChecker&, SigVersion, ScriptError*, bool&)’:
1script/interpreter.cpp:429:1: error: control reaches end of non-void function [-Werror=return-type]
2 }
3 ^
4cc1plus: some warnings being treated as errors

https://sourceforge.net/p/mingw-w64/bugs/306/

MarcoFalke commented at 5:30 pm on December 3, 2020:

Thanks. Can confirm locally:

Screenshot from 2020-12-03 18-29-59

MarcoFalke commented at 5:34 pm on December 3, 2020:

Could mention with a comment?

MarcoFalke commented at 5:34 pm on December 3, 2020: member

ACK 0e2d9ef54cdf21fddcfec1d8188633fb159ea993 👈

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 0e2d9ef54cdf21fddcfec1d8188633fb159ea993 👈
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjnwQv/f0RCPeajlgbPOL9U/Wqu/v50H3e8UfBTrKadvu4rz560W2Lhco2duJTz
 8E1U2KcKridi8Q/1Xgg75Jz0qm6X9S6EOI+brvc4IWMFu2IrKyZa3rDcrUTGoA6cB
 9LIBiDpoQPkFaWJH1bAgQLDwF1VOHoQx01qXQkcZRSNAC1yT8yCVuJ7OWrUDDusw8
10nY+X7MZpB01E70gfJhZWnJfBT1i3R21gk6Zv7iej0nEP81l3887V4IgtCqwCX+qS
11iOQxcsz/4GHZNZ1fiU5mz5GpZBcikdn9jRsqnLkJWGzZr+9z3hCY1pUsKx4tooHu
128L0s8x1Q/OH3+SUHivJNbIZ/h8HCaABZXlFZqULbK6abH4Zx4YKSeGPLJYhmIxcv
13H9hUUlBYlplWI3lPz0yNkGr4yC2aWCX6gpxJzLwhq+Nf3VKEXDtttp0tsUudvRBN
14YfCMqk4e0wzh3Ce7iaZymSdZeT3AMfDl3TSvSKcgxo9CQ5mqW3Yzdq215BWoX2dV
15xUa3Mvso
16=wpNQ
17-----END PGP SIGNATURE-----

Timestamp of file with hash bf23f2c7778536b97f325b2e84fde80d36846b764324b5b39cd6d0f4c94e45fa -

ci: Build with --enable-werror by default, and document exceptions 2f6fe4e4e9

hebasto force-pushed on Dec 3, 2020

hebasto commented at 6:56 pm on December 3, 2020: member

Updated 0e2d9ef54cdf21fddcfec1d8188633fb159ea993 -> 2f6fe4e4e9e9e35e713c0a20cf891b023592110a (pr20182.11 -> pr20182.12, diff):

addressed @MarcoFalke’s comment:

Could mention with a comment?

practicalswift commented at 9:21 pm on December 3, 2020: contributor

cr ACK 2f6fe4e4e9e9e35e713c0a20cf891b023592110a: patch looks correct

vasild approved

vasild commented at 4:57 am on December 4, 2020: member

ACK 2f6fe4e

MarcoFalke commented at 6:23 am on December 4, 2020: member

re-ACK 2f6fe4e4e9 🏏

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 2f6fe4e4e9 🏏
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhqegv7Bn3eV9QYxtVtRWO0JksQf+6awouqP9K4uHCR4Wkq/xG1yFw6+fMlm9yw
 8S/Zzv8tTdBfu3uwsqLGamAEd7lMrsN+nvqcnrl0gTOyaoOpKFfAZTOTD0kgF8zKI
 9n8323jIjYkOdCgLGsaDRgaHcQRZUoa9WRSr6SyavhXtBx1vKjeAGbWpVHt/knpG4
10hNhqDO3gTxF6S6Bt7nSj7WkG9CTj1ZzY1yJJe6tFE5xIfefRC5LHVBhwfFaRLa4t
11P4K9TPJnlbB7fJyf2/o80FKWNR4nLHsL/W218PQ58Iq8tEtvh2NcIwEcBHq0/sSV
12Nn/J8S6eghuR2bKwKS8XpkR/4Y0MfFBRHeFWDKhGCk3QY0KB52lFwWOpDcOPYnvN
13Gr79QqZJZs3Hghl5pha15yF+C8LPzWECs7Od7FAZJct+8Pf0J/M8lTDdZ4Y2VOpR
14kdQG+v5tdLAiMdqNndtBYvJ7aKJaKaYFGISz8r+JrVYlMaBfD4/GZwETzxGPm4tX
15GygUgrQE
16=gz7a
17-----END PGP SIGNATURE-----

Timestamp of file with hash f9a4b582f6420d97a683d6112df2a75c1b6bc10c52fbb86286062edb48eef81d -

MarcoFalke merged this on Dec 4, 2020

MarcoFalke closed this on Dec 4, 2020

hebasto deleted the branch on Dec 4, 2020

sidhujag referenced this in commit ca1f60cca0 on Dec 4, 2020

fanquake referenced this in commit 681f728a35 on Jul 29, 2021

MarcoFalke referenced this in commit 068ac69b56 on Jul 30, 2021

luke-jr referenced this in commit 12bbfb67b3 on Dec 15, 2021

DrahtBot locked this on Feb 15, 2022

ci: Build with –enable-werror by default, and document exceptions #20182

Conflicts