build: make Travis catch unused variables by Sjors · Pull Request #17486 · bitcoin/bitcoin

Sjors · 2019-11-15T09:34:16Z

The two macOS Travis machines run with --enable-werror. This PR adds -Werror=unused-variable to the existing vla, switch and thread-safety-analysis checks. This should prevent the need for fixes like b07b07c, 26a93bc, dd777f3, 99be644, fa39f67, 16bcc1b, bb079a0, bdaed47 and ecf9b25 with minimal nuisance.

Thoughts for followups:

Travis starts these macOS machines fairly late, so we should consider setting --enable-werror on earlier machines as well.
We should encourage the use of --enable-werror by developers. Maybe switch it on by default for --enable-debug?
See practicalswift's overview of other checks to consider in RFC: Enabling some commonly enabled compiler diagnostics #17344

laanwj · 2019-11-15T09:38:31Z

Concept ACK. If we can trust the compiler to be consistent on this.

OTOH this warning was already enabled, and you're only promoting it to -Werror in the CI. I do agree that's the only good use of Werror. It should never be enabled by default by configure scripts. So it should be ok.

practicalswift · 2019-11-15T10:20:18Z

ACK ce47c11

maflcko · 2019-11-15T13:15:03Z

Are you sure this enabled by default? I couldn't find it here: https://clang.llvm.org/docs/DiagnosticsReference.html#wmost

What would be the downside of explicitly enabling it?

maflcko · 2019-11-15T13:16:04Z

Also, would be nice to call git revert on one of the commits and then give us a link to the travis run

Sjors · 2019-11-15T13:33:11Z

Here's a Travis build that should fail: https://travis-ci.org/Sjors/bitcoin/builds/612381391

@MarcoFalke wrote:

Are you sure this enabled by default?

Not sure I understand your question. I tested on my own machine with ./configure --enable-werror and it caught it. We set this for Travis here:

bitcoin/ci/test/00_setup_env_mac_host.sh

Line 16 in 21ee676

export BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --enable-werror"

maflcko · 2019-11-15T13:50:37Z

Oh, I am wondering if -Wunused-variable is enabled by default in clang. Couldn't find it mentioned in the docs.

practicalswift · 2019-11-15T14:02:03Z

@MarcoFalke I think it is enabled by -Wall in GCC, but not in Clang. I incorrectly claimed it being enabled by -Wall also in Clang in #17344. Sorry about that: I've now corrected that comment :)

Sjors · 2019-11-15T14:13:51Z

I normally do get unused variable warnings on macOS.

maflcko · 2019-11-15T14:20:10Z

So to ask my previous question again: What would be the downside of explicitly enabling it instead of relying on clangs undocumented default, which even differs for different oses.

Sjors · 2019-11-15T14:54:14Z

Enabling -Wunused-variable by default sounds good to me, but also orthogonal to this PR, which is about making sure Travis catches it as an error.

laanwj · 2019-11-15T15:10:14Z

I don't think it's entirely orthogonal. If you make a warning an error in the CI, I think it's reasonable to enable it by default (in non-WError mode) for normal builds. Otherwise people will realize it only when the CI fails and with the large turn-around cycle that's suboptimal.

ryanofsky · 2019-11-15T15:19:43Z

If you make a warning an error in the CI, I think it's reasonable to enable it by default (in non-WError mode)

Yes, please. The worst travis problems to deal with are the ones that happen remotely but not locally.

Turn corresponding warning on by default (not always covered by -Wall).

Sjors · 2019-11-15T16:39:57Z

Good point. Added.

New Travis build that should fail: https://travis-ci.org/Sjors/bitcoin/builds/612471016

maflcko · 2019-11-15T17:00:00Z

ACK 18b18f8

practicalswift · 2019-11-15T17:58:26Z

ACK 18b18f8 -- nice!

18b18f8 [build] ./configure --enable-werror: add unused-variable (Sjors Provoost) Pull request description: The two macOS Travis machines run with `--enable-werror`. This PR adds `-Werror=unused-variable` to the existing `vla`, `switch` and `thread-safety-analysis` checks. This should prevent the need for fixes like b07b07c, 26a93bc, dd777f3, 99be644, fa39f67, 16bcc1b, bb079a0, bdaed47 and ecf9b25 with minimal nuisance. Thoughts for followups: * Travis starts these macOS machines fairly late, so we should consider setting `--enable-werror` on earlier machines as well. * We should encourage the use of `--enable-werror` by developers. Maybe switch it on by default for `--enable-debug`? * See practicalswift's overview of other checks to consider in #17344 ACKs for top commit: MarcoFalke: ACK 18b18f8 practicalswift: ACK 18b18f8 -- nice! Tree-SHA512: 892b471ca5ea547f3c952ac88190cbebf8110cb7aec6f20466aeb312aeb0910bfe990f914e153c40ecb55709c03775ef30770412ad76f9d532ca77055596c582

Sjors · 2019-11-15T20:46:16Z

Travis catches the issue as expected (had to restart a few times for timeouts): https://travis-ci.org/Sjors/bitcoin/jobs/612471028#L2333

18b18f8 [build] ./configure --enable-werror: add unused-variable (Sjors Provoost) Pull request description: The two macOS Travis machines run with `--enable-werror`. This PR adds `-Werror=unused-variable` to the existing `vla`, `switch` and `thread-safety-analysis` checks. This should prevent the need for fixes like b07b07c, 26a93bc, dd777f3, 99be644, fa39f67, 16bcc1b, bb079a0, bdaed47 and ecf9b25 with minimal nuisance. Thoughts for followups: * Travis starts these macOS machines fairly late, so we should consider setting `--enable-werror` on earlier machines as well. * We should encourage the use of `--enable-werror` by developers. Maybe switch it on by default for `--enable-debug`? * See practicalswift's overview of other checks to consider in bitcoin#17344 ACKs for top commit: MarcoFalke: ACK 18b18f8 practicalswift: ACK 18b18f8 -- nice! Tree-SHA512: 892b471ca5ea547f3c952ac88190cbebf8110cb7aec6f20466aeb312aeb0910bfe990f914e153c40ecb55709c03775ef30770412ad76f9d532ca77055596c582

This reverts commit 5fe89ff.

Sjors · 2019-11-20T11:56:15Z

This turned out to be premature, reverting the error part in #17533

laanwj · 2019-11-20T12:00:46Z

yup, a textbook example of why to be careful with making things Werror
(didn't see this particular one coming either !)

18b18f8 [build] ./configure --enable-werror: add unused-variable (Sjors Provoost) Pull request description: The two macOS Travis machines run with `--enable-werror`. This PR adds `-Werror=unused-variable` to the existing `vla`, `switch` and `thread-safety-analysis` checks. This should prevent the need for fixes like b07b07c, 26a93bc, dd777f3, 99be644, fa39f67, 16bcc1b, bb079a0, bdaed47 and ecf9b25 with minimal nuisance. Thoughts for followups: * Travis starts these macOS machines fairly late, so we should consider setting `--enable-werror` on earlier machines as well. * We should encourage the use of `--enable-werror` by developers. Maybe switch it on by default for `--enable-debug`? * See practicalswift's overview of other checks to consider in bitcoin#17344 ACKs for top commit: MarcoFalke: ACK 18b18f8 practicalswift: ACK 18b18f8 -- nice! Tree-SHA512: 892b471ca5ea547f3c952ac88190cbebf8110cb7aec6f20466aeb312aeb0910bfe990f914e153c40ecb55709c03775ef30770412ad76f9d532ca77055596c582

This reverts commit 5fe89ff.

merge bitcoin#18914, bitcoin#13306, bitcoin#16424, bitcoin#13899, bitcoin#17486, bitcoin#17880, bitcoin#18145, bitcoin#18843, bitcoin#16710: split warnings out of CXXFLAGS, add more flags

DrahtBot added the Build system label Nov 15, 2019

[build] ./configure --enable-werror: add unused-variable

18b18f8

Turn corresponding warning on by default (not always covered by -Wall).

Sjors force-pushed the 2019/11/Werror-unused-variable branch from ce47c11 to 18b18f8 Compare November 15, 2019 16:37

maflcko merged commit 18b18f8 into bitcoin:master Nov 15, 2019

Sjors deleted the 2019/11/Werror-unused-variable branch November 15, 2019 20:46

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2019

Revert "Merge bitcoin#17486: build: make Travis catch unused variables"

53fa6e4

This reverts commit 5fe89ff.

This was referenced Nov 19, 2019

build: Mark depends headers as -isystem #17520

Closed

build: undo Werror=unused-variable #17533

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020

Revert "Merge bitcoin#17486: build: make Travis catch unused variables"

921e639

This reverts commit 5fe89ff.

kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021

bitcoin#17486: make Travis catch unused variables

52f3975

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

Sjors commented Nov 15, 2019 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2019

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

practicalswift commented Nov 15, 2019

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

laanwj commented Nov 15, 2019

Uh oh!

ryanofsky commented Nov 15, 2019

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

maflcko commented Nov 15, 2019

Uh oh!

practicalswift commented Nov 15, 2019

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

Sjors commented Nov 20, 2019

Uh oh!

laanwj commented Nov 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sjors commented Nov 15, 2019 •

edited by fanquake

Loading

laanwj commented Nov 15, 2019 •

edited

Loading