Allow warnings conditionally.

When building in CI, we want to see all potential problems noted by
compilers & static analysers that masquerade as a compiler, rather than
halting on the first one.
This is especially important as we tend to implement
different/stricter/expensive checks compared to a typical compiler, and
as such, these may not be visible when performing a normal build.

This patch conditionally allows compiler warnings by passing
ALLOW_WARNINGS=1 as a make variable. The default behavior remains to
fault on the first warning.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 Makefile.main              | 6 ++++--
 external/gard/rules.mk     | 5 ++++-
 external/opal-prd/Makefile | 5 ++++-
 external/shared/Makefile   | 5 ++++-
 4 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.5.5

On 20/04/16 10:34, Alastair D'Silva wrote:
> When building in CI, we want to see all potential problems noted by
> compilers & static analysers that masquerade as a compiler, rather than
> halting on the first one.
> This is especially important as we tend to implement
> different/stricter/expensive checks compared to a typical compiler, and
> as such, these may not be visible when performing a normal build.
> 
> This patch conditionally allows compiler warnings by passing
> ALLOW_WARNINGS=1 as a make variable. The default behavior remains to
> fault on the first warning.

Aren't such runs one-off. One would expect by induction that builds not
create warnings, if they do the build is broken and fixed right away.

Balbir Singh

On Wed, 2016-04-20 at 13:21 +1000, Balbir Singh wrote:
> 
> On 20/04/16 10:34, Alastair D'Silva wrote:
> > 
> > When building in CI, we want to see all potential problems noted by
> > compilers & static analysers that masquerade as a compiler, rather
> > than
> > halting on the first one.
<snip>
> Aren't such runs one-off. One would expect by induction that builds
> not
> create warnings, if they do the build is broken and fixed right away.
> 
> Balbir Singh

They are done automatically on every commit, using tools (and emitting
warnings) beyond what developers are currently utilising. For example,
we are currently using PMD's copy & paste detector, cppcheck & clang-
analyzer, and I expect to add more in future (where reasonable, I'll
provide integration patches so these can be run manually).

The intent here is to provide information back to the (local Ozlabs)
devs about problems that their own development environment may not have
caught, before patches are committed. To do that effectively, and to
evaluate trends/progress, we need to see all the potential problems
with a build, not just a binary compiles/doesn't compile.

Also, failing a build in CI for warnings results in the tests not being run against that commit. This is problematic in the following scenario:
 - Commit 1 - Builds & passes all tests fine
 - Commit 2-10 - Fails to build
 - Commit 11 - Builds fine, test 13 fails
In this situation, the information about which commit breaks test 13
has been lost, and would require investigation in an environment with
those warnings permitted/disabled to track it down.

On Wed, 2016-04-20 at 10:34 +1000, Alastair D'Silva wrote:
> When building in CI, we want to see all potential problems noted by
> compilers & static analysers that masquerade as a compiler, rather than
> halting on the first one.
> This is especially important as we tend to implement
> different/stricter/expensive checks compared to a typical compiler, and
> as such, these may not be visible when performing a normal build.
> 
> This patch conditionally allows compiler warnings by passing
> ALLOW_WARNINGS=1 as a make variable. The default behavior remains to
> fault on the first warning.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---

This patch doesn't apply to master or stable:

	Applying: Allow warnings conditionally.
	fatal: corrupt patch at line 13

which is strange.  I like the idea, though.

- Russell

On 20/04/16 15:52, Russell Currey wrote:
> This patch doesn't apply to master or stable:
>
> 	Applying: Allow warnings conditionally.
> 	fatal: corrupt patch at line 13
>
> which is strange.  I like the idea, though.

emacs tells me the spacing on line 13 onwards consists of a lot of c2a0:

00000170: 4357 4152 4e53 203a 3d20 2d57 616c 6c20  CWARNS := -Wall
00000180: 2d57 756e 6465 6620 2d57 7374 7269 6374  -Wundef -Wstrict
00000190: 2d70 726f 746f 7479 7065 7320 2d57 6e6f  -prototypes -Wno
000001a0: 2d0a 7472 6967 7261 7068 7320 5c0a c2a0  -.trigraphs \...
000001b0: c2a0 c2a0 c2a0 c2a0 c2a0 c2a0 c2a0 c2a0  ................
000001c0: c2a0 2d57 6d69 7373 696e 672d 7072 6f74  ..-Wmissing-prot
000001d0: 6f74 7970 6573 202d 576d 6973 7369 6e67  otypes -Wmissing
000001e0: 2d64 6563 6c61 7261 7469 6f6e 7320 5c0a  -declarations \.
000001f0: c2a0 c2a0 c2a0 c2a0 c2a0 c2a0 c2a0 c2a0  ................
00000200: c2a0 c2a0 2d57 7772 6974 652d 7374 7269  ....-Wwrite-stri

According to the interwebs, c2a0, in UTF-8, is a non-breaking space.

Alastair: please resend in ASCII :)


Andrew

On 20/04/16 16:15, Andrew Donnellan wrote:
> On 20/04/16 15:52, Russell Currey wrote:
>> This patch doesn't apply to master or stable:
>>
>>     Applying: Allow warnings conditionally.
>>     fatal: corrupt patch at line 13
>>
>> which is strange.  I like the idea, though.

FWIW I like the idea too.

On Wed, 2016-04-20 at 15:52 +1000, Russell Currey wrote:
> On Wed, 2016-04-20 at 10:34 +1000, Alastair D'Silva wrote:
> > When building in CI, we want to see all potential problems noted by
> > compilers & static analysers that masquerade as a compiler, rather than
> > halting on the first one.
> > This is especially important as we tend to implement
> > different/stricter/expensive checks compared to a typical compiler, and
> > as such, these may not be visible when performing a normal build.
> > 
> > This patch conditionally allows compiler warnings by passing
> > ALLOW_WARNINGS=1 as a make variable. The default behavior remains to
> > fault on the first warning.
> > 
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> 
> This patch doesn't apply to master or stable:
> 
> 	Applying: Allow warnings conditionally.
> 	fatal: corrupt patch at line 13
> 
> which is strange.  I like the idea, though.

The headers suggest it was sent via IBM mail routers, so it could
be them playing white space tricks again.

Mikey

> 
> - Russell
>

Michael Neuling <mikey@neuling.org> writes:
> On Wed, 2016-04-20 at 15:52 +1000, Russell Currey wrote:
>> On Wed, 2016-04-20 at 10:34 +1000, Alastair D'Silva wrote:
>> > When building in CI, we want to see all potential problems noted by
>> > compilers & static analysers that masquerade as a compiler, rather than
>> > halting on the first one.
>> > This is especially important as we tend to implement
>> > different/stricter/expensive checks compared to a typical compiler, and
>> > as such, these may not be visible when performing a normal build.
>> > 
>> > This patch conditionally allows compiler warnings by passing
>> > ALLOW_WARNINGS=1 as a make variable. The default behavior remains to
>> > fault on the first warning.
>> > 
>> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>> > ---
>> 
>> This patch doesn't apply to master or stable:
>> 
>> 	Applying: Allow warnings conditionally.
>> 	fatal: corrupt patch at line 13
>> 
>> which is strange.  I like the idea, though.
>
> The headers suggest it was sent via IBM mail routers, so it could
> be them playing white space tricks again.

While blaming IBM mail infrastructure is almost by default correct, in
this case it may be the MUA - Evolution seems to have been butchering
things recently - there were a few corrupted ones from benh a little
while ago.

I'd suggest git-send-email :)