882824 - Draw more attention to warnings in FAIL_ON_WARNINGS directories

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Someone (I'm not sure who) voiced a terrific idea over IRC a few days ago: the build system should draw more attention to compiler warnings in FAIL_ON_WARNINGS directories. Since many local builds don't use fatal warnings (only the official build infrastructure and a few daring individuals), it's apparently pretty common to have code build properly locally (with a warning) only to have it fail when built with FAIL_ON_WARNINGS.

I'm thinking the warnings database could reconcile warnings against directories with FAIL_ON_WARNINGS and highlight them some how. Perhaps we display a message in the build summary, etc.

This will require FAIL_ON_WARNINGS to be ported to moz.build so we can easily look up which directories are fatal. I hear Ms2ger is looking into that...

Comment 1

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #0)
> Someone (I'm not sure who) voiced a terrific idea over IRC a few days ago:
> the build system should draw more attention to compiler warnings in
> FAIL_ON_WARNINGS directories. Since many local builds don't use fatal
> warnings (only the official build infrastructure and a few daring
> individuals), it's apparently pretty common to have code build properly
> locally (with a warning) only to have it fail when built with
> FAIL_ON_WARNINGS.

Err, why aren't they on by default? The easiest way to draw attention to it is to report it as an error, so it can be fixed as soon as it is introduced.

Comment 2

[Gregory Szorc [:gps]]

Short version is the disparity between local toolchains and official toolchains makes it difficult to ensure the set of compiler warnings is consistent. This is one of the reasons why I would love for us to make it possible for developers to run a bit-exact copy of the official toolchain locally.

Comment 3

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #2)
> Short version is the disparity between local toolchains and official
> toolchains makes it difficult to ensure the set of compiler warnings is
> consistent. This is one of the reasons why I would love for us to make it
> possible for developers to run a bit-exact copy of the official toolchain
> locally.

That's only true with gcc, right?  With MSVC and clang we (should) have a much more deterministic build system.  We should have more people using MSVC and clang than gcc...

Comment 4

[Daniel Holbert [:dholbert]]

It's true of MSVC and clang as well.

Back when I naively turned on --enable-warnings-as-errors by default a few years ago, it broke Vlad's local MSVC build because he had a newer MSVC version than we used on our builders, with new warnings (treated as errors) in some FAIL_ON_WARNINGS directories.

And I think I've heard of the same situation with clang, too -- new versions sometimes introduce new warnings, which trigger build errors in --enable-warnings-as-errors configurations.

So if we turn --enable-warnings-as-errors on by default, then new contributors who are trying to get a local build going and are using the Latest And Greatest Compiler may end up hitting mysterious unexpected build-failures, be unable to build, and be turned off from contributing.  We don't want that, which is why we opted to leave it off by default but on for our builders. (and encourage devs to turn it on locally and also use TryServer)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> new versions
> sometimes introduce new warnings, which trigger build errors in
> --enable-warnings-as-errors configurations.

(See bug 851237 for a real-world example of where this recently happened & broke my local --enable-warnings-as-errors build when I upgraded to GCC 4.8.  This was GCC, but other compilers could just as easily hit the same sort of thing, if they add warnings or modify their list of enabled-by-default warnings.)

Comment 6

[Michael Shal [:mshal]]

It looks like gcc/clang/MSVC all support some way of turning only particular warnings into errors (eg: -Werror=switch, or /we1234). Might it make sense to use some of these instead of the blanket -Werror flag? Perhaps the more stable warning flags could default to errors, leaving any unknown future warning flags as just warnings. Though this might not work so great if a particular warning flag changes meaning over time, rather than just getting new warning flags in new versions. Anyone know if these issues are generally from "-Wfoo now warns about things it didn't before", or from "-Wnew-warning is now included by -Wall"? Using -Werror=foo would help fix the second case, but not the first.

Comment 7

[Daniel Holbert [:dholbert]]

> (eg: -Werror=switch, or /we1234). Might it make sense to use some of these
> instead of the blanket -Werror flag

It might make sense to migrate to something like that, but that discussion is getting a bit off-topic for the scope of this bug & is probably better-suited to a newsgroup thread and/or a dedicated bug.

(My brief take: If a warning is important enough to have it enabled and spamming people's build-spew by default, then it's probably a good thing to have it fatal, so that it actually gets noticed & fixed.  In cases where false positives are an issue, we can opt the warning out of -Werror in a targeted way (as we did in bug 716541) or disable it entirely (as we did in bug 857863).)

> Anyone know if these issues are generally from "-Wfoo now warns about things
> it didn't before", or from "-Wnew-warning is now included by -Wall"? 

(I've only seen the latter, FWIW.)

Comment 8

[(no longer active)]

My point was that currently there are only two MSVC versions that can build our code, and we can also enforce checks which prevent anything older from clang3.3 from building us.  This is very different than the situation from gcc where we can use anything from gcc 4.4 to 4.8.  Or am I missing something?

Comment 9

[(not currently active) Ted Mielczarek]

We could take a middle ground approach here: declare a set of compiler versions (perhaps just the set we build official builds with) that get the warnings-as-errors treatment.

Comment 10

[(no longer active)]

(In reply to comment #9)
> We could take a middle ground approach here: declare a set of compiler versions
> (perhaps just the set we build official builds with) that get the
> warnings-as-errors treatment.

Sounds good to me!

Comment 11

[Daniel Holbert [:dholbert]]

Ted suggested the compiler whitelisting approach back in bug 557566 comment 32, too; I think it's a good idea -- just, no one's implemented it yet. :)

If we do add support for that, we should still preserve --enable-warnings-as-errors (or something like it) so that people with non-whitelisted compilers (e.g. me, on GCC 4.8) can still opt in to that treatment.

Comment 12

[(not currently active) Ted Mielczarek]

Good to know I repeat myself every few years. If we implemented this I'd like to to just influence the defaults, and allow overriding that via the option. I would probably implement this as a very strict whitelist, like:

(psuedocoding)

compiler="unknown"
if CLANG then;
  compiler="clang";
elif GCC then;
  compiler="gcc";
elif _MSC_VER then;
  compiler="msvc";
fi
  

case "$OS_TARGET" in
   Linux)
      case "$compiler-$GCC_VERSION$CLANG_VERSION" in
         gcc-4.7); WARNINGS_AS_ERRORS=1;
         ...

Comment 13

[Takanori MATSUURA]

All bugs fixed?