ci: Build with --enable-werror by default, and document exceptions by hebasto · Pull Request #20182 · bitcoin/bitcoin

hebasto · 2020-10-19T00:51:43Z

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

fanquake

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

ci/test/00_setup_env_i686_centos.sh

DrahtBot · 2020-10-19T07:07:57Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: Do not repeat warning names in -Werror=... options #20544 (build: Do not repeat warning names in -Werror=... options by hebasto)
Qt: Add Android packaging support #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.

configure.ac

practicalswift · 2020-10-20T07:37:09Z

Concept ACK: deduplication is good

vasild

Approach ACK

ci/test/06_script_a.sh

configure.ac

maflcko · 2020-10-21T09:07:15Z

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

vasild · 2020-10-21T09:14:05Z

[@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 · 2020-10-21T09:16:07Z

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

CI changes require build system changes.

vasild

ACK 81ade9b

vasild · 2020-10-21T10:02:36Z

configure.ac

+  dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780
+  AX_CHECK_COMPILE_FLAG([-Wunused-command-line-argument],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-command-line-argument"],,[[$CXXFLAG_WERROR]])


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.

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

vasild · 2020-11-05T15:44:13Z

ACK 3a409f6

hebasto · 2020-11-26T20:16:05Z

@fanquake @practicalswift @vasild @MarcoFalke

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

practicalswift · 2020-11-26T21:29:41Z

cr ACK 2ca5156: patch looks correct!

hebasto · 2020-12-02T08:52:45Z

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.

vasild · 2020-12-03T06:36:29Z

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

This PR should drop its first commit (the configure.ac changes, contained in build: Do not repeat warning names in -Werror=... options #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).
build: Do not repeat warning names in -Werror=... options #20544 should remove the changes to ci/test/06_script_a.sh which are contained in this PR and already merged.
build: Do not repeat warning names in -Werror=... options #20544 merged (it will not cause any breakage because ci is now prepared by the already merged PR).

hebasto · 2020-12-03T09:00:56Z

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

This PR should drop its first commit (the configure.ac changes, contained in build: Do not repeat warning names in -Werror=... options #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).

build: Do not repeat warning names in -Werror=... options #20544 should remove the changes to ci/test/06_script_a.sh which are contained in this PR and already merged.

build: Do not repeat warning names in -Werror=... options #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 · 2020-12-03T09:05:24Z

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

maflcko

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

ci/test/00_setup_env_s390x.sh

ci/test/00_setup_env_native_nowallet.sh

hebasto · 2020-12-03T14:19:38Z

Updated cbb234c -> 0e2d9ef (pr20182.10 -> pr20182.11):

removed unrelated changes
rebased on top of the recent CI changes

vasild

ACK 0e2d9ef

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

maflcko · 2020-12-03T16:50:55Z

ci/test/00_setup_env_win64.sh

 export RUN_SECURITY_TESTS="true"
 export GOAL="deploy"
 export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --without-boost-process"
+export NO_WERROR=1


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

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

script/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&)’: script/interpreter.cpp:429:1: error: control reaches end of non-void function [-Werror=return-type] } ^ cc1plus: some warnings being treated as errors

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

Thanks. Can confirm locally:

Could mention with a comment?

maflcko · 2020-12-03T17:34:41Z

ACK 0e2d9ef 👈

Show signature and timestamp

Signature:

Timestamp of file with hash bf23f2c7778536b97f325b2e84fde80d36846b764324b5b39cd6d0f4c94e45fa -

hebasto · 2020-12-03T18:56:05Z

Updated 0e2d9ef -> 2f6fe4e (pr20182.11 -> pr20182.12, diff):

addressed @MarcoFalke's comment:

Could mention with a comment?

practicalswift · 2020-12-03T21:21:29Z

cr ACK 2f6fe4e: patch looks correct

vasild

ACK 2f6fe4e

maflcko · 2020-12-04T06:23:39Z

re-ACK 2f6fe4e 🏏

Show signature and timestamp

Signature:

Timestamp of file with hash f9a4b582f6420d97a683d6112df2a75c1b6bc10c52fbb86286062edb48eef81d -

…ocument…

Github-Pull: bitcoin#20182 Rebased-From: 2f6fe4e

55e941f test: Fix intermittent feature_taproot issue (MarcoFalke) 681f728 ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov) 89426c4 ci: Fix macOS brew install command (Hennadii Stepanov) Pull request description: This backports a few changes to fix CI failures we are seeing with the 0.21 branch. Backports #21663, this might be the easiest way to fix the macOS CI failures we're seeing. i.e in #22569. The underlying issue is that the older CI images are using a version of brew that without running `brew update` first, is trying to download packages like Boost, from bintray (which no-longer works). This also includes #20182, as by fixing macOS failure, via running `brew upgrade`, we end up using a newer version of miniupnpc, which emits a GNU extension related warning, and causes the build to fail, because we use `-Werror`. Backporting #20535 should fix #22581. ACKs for top commit: hebasto: ACK 55e941f, I verified changes by backporting locally. Tree-SHA512: 3ab2c5c73c707d0f5b862264f3a0179cdeee30ae55aae872f3c3e0bb81d71a5027c39ba830210c99a21f98cc86c4167c4f215e24d1a8891ec79ce512debf82df

Needed to remain compatible with backport commit 681f728 Github-Pull: bitcoin#20182 Rebased-From: 2f6fe4e

fanquake added the Build system label Oct 19, 2020

hebasto force-pushed the 201018-error branch from 24bfd4b to ea8f849 Compare October 19, 2020 01:09

fanquake reviewed Oct 19, 2020

View reviewed changes

ci/test/00_setup_env_i686_centos.sh Outdated Show resolved Hide resolved

hebasto force-pushed the 201018-error branch from ea8f849 to a90582c Compare October 19, 2020 08:26

This was referenced Oct 19, 2020

Bump minimum python version to 3.6 #19504

Merged

wallet: upgradewallet fixes and additional tests #18836

Merged

Qt: Add Android packaging support #17227

Merged

hebasto force-pushed the 201018-error branch from a90582c to 0179abe Compare October 19, 2020 11:43

practicalswift reviewed Oct 19, 2020

View reviewed changes

configure.ac Show resolved Hide resolved

vasild reviewed Oct 21, 2020

View reviewed changes

ci/test/06_script_a.sh Outdated Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

hebasto force-pushed the 201018-error branch from 0179abe to 81ade9b Compare October 21, 2020 09:33

vasild approved these changes Oct 21, 2020

View reviewed changes

hebasto force-pushed the 201018-error branch from 81ade9b to 3a409f6 Compare November 5, 2020 11:31

This was referenced Nov 8, 2020

ci: Run more ci configs on cirrus #20339

Merged

ci: Run ci configs on cirrus #19179

Merged

hebasto force-pushed the 201018-error branch 3 times, most recently from b9e778f to bd09b7b Compare November 26, 2020 18:41

hebasto changed the title ~~ci: Enable -Werror for Travis builds, and document exceptions~~ ci: Enable -Werror, and document exceptions Nov 26, 2020

hebasto marked this pull request as ready for review November 26, 2020 20:10

hebasto force-pushed the 201018-error branch from bd09b7b to 2ca5156 Compare November 26, 2020 20:14

DrahtBot mentioned this pull request Nov 27, 2020

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487

Merged

hebasto force-pushed the 201018-error branch from a00dd1e to e5f3d51 Compare December 2, 2020 09:43

hebasto force-pushed the 201018-error branch from e5f3d51 to cbb234c Compare December 3, 2020 08:59

maflcko removed the Build system label Dec 3, 2020

DrahtBot added the Tests label Dec 3, 2020

maflcko reviewed Dec 3, 2020

View reviewed changes

ci/test/00_setup_env_s390x.sh Outdated Show resolved Hide resolved

ci/test/00_setup_env_native_nowallet.sh Outdated Show resolved Hide resolved

hebasto force-pushed the 201018-error branch from cbb234c to 0e2d9ef Compare December 3, 2020 14:16

hebasto changed the title ~~ci: Enable -Werror, and document exceptions~~ ci: Build with --enable-werror by default, and document exceptions Dec 3, 2020

vasild approved these changes Dec 3, 2020

View reviewed changes

maflcko reviewed Dec 3, 2020

View reviewed changes

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

2f6fe4e

hebasto force-pushed the 201018-error branch from 0e2d9ef to 2f6fe4e Compare December 3, 2020 18:52

vasild approved these changes Dec 4, 2020

View reviewed changes

maflcko merged commit e2ae6a2 into bitcoin:master Dec 4, 2020

hebasto deleted the 201018-error branch December 4, 2020 09:40

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 4, 2020

Merge bitcoin#20182: ci: Build with --enable-werror by default, and d…

ca1f60c

…ocument…

fanquake mentioned this pull request Mar 23, 2021

build: miniupnpc 2.2.2 #20421

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jul 29, 2021

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

681f728

Github-Pull: bitcoin#20182 Rebased-From: 2f6fe4e

fanquake mentioned this pull request Jul 29, 2021

[0.21] Additional backports #22578

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 15, 2021

ci: Exempt Win32 from default --enable-werror

12bbfb6

Needed to remain compatible with backport commit 681f728 Github-Pull: bitcoin#20182 Rebased-From: 2f6fe4e

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		dnl This is a temporary workaround. See https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-674404780
		AX_CHECK_COMPILE_FLAG([-Wunused-command-line-argument],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-command-line-argument"],,[[$CXXFLAG_WERROR]])

Conversation

hebasto commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

practicalswift commented Oct 20, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Oct 21, 2020

Uh oh!

vasild commented Oct 21, 2020

Uh oh!

hebasto commented Oct 21, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

vasild commented Nov 5, 2020

Uh oh!

hebasto commented Nov 26, 2020

Uh oh!

practicalswift commented Nov 26, 2020

Uh oh!

hebasto commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 3, 2020

Uh oh!

hebasto commented Dec 3, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 3, 2020

Uh oh!

hebasto commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 19, 2020 •

edited

Loading

DrahtBot commented Oct 19, 2020 •

edited

Loading

hebasto commented Dec 2, 2020 •

edited

Loading

vasild commented Dec 3, 2020 •

edited

Loading

hebasto commented Dec 3, 2020 •

edited

Loading

hebasto Dec 3, 2020 •

edited

Loading

hebasto commented Dec 3, 2020 •

edited

Loading