build: make Travis catch unused variables

Sjors commented at 9:34 am on November 15, 2019: member

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 b07b07cd8779355ba1dd16e7eb4af42e0ae1c587, 26a93bce29fd813e1402b013f402869c25b656d1, dd777f3e1220dd1a76e8a29cafdd4fe6244c5c0f, 99be644966c63e9917161f97574905551e44360f, fa39f674aed8f2dc5a9bde6a84b0ec52fc49e695, 16bcc1b8237698c96b8ced2fa7eb76388c7ba85e, bb079a0e2c20beb22456c91ad9e11beeae7cdc34, bdaed4755846e8b1e533c14485faa5f9fd1cf18b and ecf9b25a03d8a29f16005ca4485b6533db6efc82 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

laanwj commented at 9:38 am on November 15, 2019: member

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.

DrahtBot added the label Build system on Nov 15, 2019

practicalswift commented at 10:20 am on November 15, 2019: contributor

ACK ce47c112642ab21fa21467bbdf109b668470c071

MarcoFalke commented at 1:15 pm on November 15, 2019: member

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?

MarcoFalke commented at 1:16 pm on November 15, 2019: member

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

Sjors commented at 1:33 pm on November 15, 2019: member

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: https://github.com/bitcoin/bitcoin/blob/21ee676dd6a7d9704367b6412bf8e1e443ec2b5b/ci/test/00_setup_env_mac_host.sh#L16

MarcoFalke commented at 1:50 pm on November 15, 2019: member

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

practicalswift commented at 2:02 pm on November 15, 2019: contributor

@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 commented at 2:13 pm on November 15, 2019: member

I normally do get unused variable warnings on macOS.

MarcoFalke commented at 2:20 pm on November 15, 2019: member

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 commented at 2:54 pm on November 15, 2019: member

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 commented at 3:10 pm on November 15, 2019: member

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 commented at 3:19 pm on November 15, 2019: member

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.

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

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

18b18f8e81

Sjors force-pushed on Nov 15, 2019

Sjors commented at 4:39 pm on November 15, 2019: member

Good point. Added.

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

MarcoFalke commented at 5:00 pm on November 15, 2019: member

ACK 18b18f8e813b332d0ce6b0af19c9cb265f043776

practicalswift commented at 5:58 pm on November 15, 2019: contributor

ACK 18b18f8e813b332d0ce6b0af19c9cb265f043776 – nice!

MarcoFalke referenced this in commit b90dad5143 on Nov 15, 2019

MarcoFalke merged this on Nov 15, 2019

MarcoFalke closed this on Nov 15, 2019

Sjors commented at 8:46 pm on November 15, 2019: member

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

Sjors deleted the branch on Nov 15, 2019

sidhujag referenced this in commit 5fe89ff6f0 on Nov 15, 2019

sidhujag referenced this in commit 53fa6e470b on Nov 16, 2019

Sjors commented at 11:56 am on November 20, 2019: member

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

laanwj commented at 12:00 pm on November 20, 2019: member

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

sidhujag referenced this in commit 55f16c9df7 on Nov 10, 2020

sidhujag referenced this in commit 921e6390aa on Nov 10, 2020

kittywhiskers referenced this in commit 5b525204cd on Jun 17, 2021

kittywhiskers referenced this in commit 2b49a4de9b on Jun 17, 2021

kittywhiskers referenced this in commit 52f3975cbc on Jun 17, 2021

UdjinM6 referenced this in commit 988fa6a235 on Jun 23, 2021

MarcoFalke locked this on Dec 16, 2021

build: make Travis catch unused variables #17486