1301688 - Hide warnings from log in ALLOW_COMPILER_WARNINGS=True modules

Status: NEW

(Firefox Build System :: General, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

We currently always pass warning flags to gcc whether or not -Werror is set for the given input file, which means that the output is clogged full of errors which nobody reads, and which makes it hard to find the actual build failure.

It would be nice to have an option to disable passing these warning flags (silencing all of these warnings) unless -Werror is also passed (due to --warnings-as-errors). That way I can more easily find the actual problem and don't have to wade through hundreds of lines of warnings to find the error I actually care about.

This spam is especially frustrating when building with icecc, as the build failure may be 100+ files up in the log, and each file can produce pages of warnings sometimes.

Comment 1

[Gregory Szorc [:gps]]

We want developers to see warnings. Unless a warning is explicitly disabled in the build config, that is potentially something actionable someone should fix.

I agree sorting through logs to find compiler warnings can be annoying. That's why `mach warnings-list` exists. If that doesn't work for you, we should improve it instead of hiding warnings. Or we could potentially implement a --quiet flag on `mach build` to silence warnings from terminal output. But I'm generally opposed to a build system mode to disable warnings completely.

That being said, I don't hack on C++. So maybe I'm missing something obvious. If you could provide a concrete example of the workflow that's failing you, it would help me understand where you're coming from.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

The reasoning which I am following is that when I build, I build with an icecream cluster in the Toronto office. This means that at any point in time, I have ~100 or so files building. As mach doesn't abort immediately when it sees its first error, but instead finishes any in-flight operations, this means that the error which I am trying to find would already be a little bit up, but because of the number of warnings which fire in some parts of the codebase (which are completely unrelated to what I am working on), I have to scroll through pages and pages before I can find the actual error I'm trying to fix. My goal with this is simply to make it easier to find that actual error after the fact, without wading through the warnings first.

This problem would also be fixed by bug 1288431 but this seemed like a more tractable solution.

Comment 3

[Mike Hommey [:glandium] OoO until Jun 18]

If you're seeing warnings in your build, it means one or more of the following:

- You're seeing warnings in code from a directory with ALLOW_COMPILER_WARNINGS=True.

  Most of that code is third part, and yes, in that case, I'd agree it's noise

- You're seeing warnings that exist in your version of the compiler, but not the one used on automation.

  You could view that as being self-inflicted, because you're not using the same compiler as what's on automation in the first place, but you could also view that as the fact that those warnings would turn into errors when we upgrade the compiler on automation. The sooner they are fixed, the better for everyone.

- You're seeing -Wno-error warnings. 

  I'd say it's a case by case thing. Some, like -Wno-error=shadow are disabling errors from code constructs that are commonplace in some parts of the code. Maybe some if not all of the -Wno-error=foo flags should be -Wno-foo instead.

- You added them.

  You definitely should fix them.


All in all I can see value in completely disabling warnings for ALLOW_COMPILER_WARNINGS=True and -Wno-error. I'm even tempted to say that should probably be the default.

Chris, since you're the person who's doing most of the warnings-related things, do you have thoughts?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Hommey [:glandium] from comment #3)
> All in all I can see value in completely disabling warnings for
> ALLOW_COMPILER_WARNINGS=True and -Wno-error. I'm even tempted to say that
> should probably be the default.

I build with `ac_add_options --enable-warnings-as-errors`, and use a sane version of GCC (The one in Ubuntu's repo), so I am almost 100% sure that the errors I'm seeing are either `-Wno-error` warnings or warnings from ALLOW_COMPILER_WARNINGS=True sections of code.

I'm generally of the same opinion as you. I immediately fix the other types of warnings, because they fail my builds :), but these warnings just fill up my log, and are in third party code so I can't even fix them if I wanted to.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Mike Hommey [:glandium] from comment #3)
> All in all I can see value in completely disabling warnings for
> ALLOW_COMPILER_WARNINGS=True and -Wno-error. I'm even tempted to say that
> should probably be the default.

I don't have a strong opinion. Hiding warnings in ALLOW_COMPILER_WARNINGS=True directories sounds fine to me. Most of the remaining warnings are in third-party C code. Those warnings are unlikely to get fixed by someone watching Firefox build output. We could lessen this problem by removing the noisier warning flags from CFLAGS.

Before we hide all these warnings, we should consider whether any particular warnings are serious enough to block landing of new third-party code. Perhaps we remove all warning flags from CFLAGS and/or ALLOW_COMPILER_WARNINGS=True directories except a few serious warnings-as-errors like -Wreturn-type.