I want to enable WARNINGS_AS_ERRORS for browser builds on TBPL

[Nicholas Nethercote]

Hi,

I have a long-term goal of turning on WARNINGS_AS_ERRORS for browser
builds on TBPL. Obviously this will require fixing a lot of existing
warnings, but I think it can be done, esp. as it's possible to turn
WARNINGS_AS_ERRORS on one module at a time. (There may be a handful
of modules, e.g. third-party code, for which it's not feasible.)
https://bugzilla.mozilla.org/show_bug.cgi?id=703121 is open for this.

As it happens, the JS engine is a great example for this, because we
already almost do this. We have WARNINGS_AS_ERRORS on for shell
builds, and to see those builds on TBPL you have to add something
extra (I can't remember what) to the URL. Our current process is that
warnings on TBPL are unacceptable, but they aren't grounds for
immediate fixing/backout -- the pusher has some
reasonable-but-unspecified amount of time to fix things.

I propose that we turn on WARNINGS_AS_ERRORS for the JS engine for
browser builds as well. I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=706976 for this. This
would effectively tighten up our process so that warnings on TBPL are
grounds for immediate fixing/backout. I don't think this will cause
any problems in practice, because it's only a small change from the
current process. And once we have the JS engine doing this, it's a
great precedent and makes it easier to add WARNINGS_AS_ERRORS for
other modules.

Nick

[David Mandelin]

On 12/1/2011 3:01 PM, Nicholas Nethercote wrote:
> I have a long-term goal of turning on WARNINGS_AS_ERRORS for browser
> builds on TBPL.

Could you lay out the rationale for this? I'm not necessarily opposed,
but my gut reaction is "but WARNINGS_AS_ERRORS is really annoying", so
I'd appreciate hearing about the benefits (and perhaps why the downsides
might not be as bad as I think). (Plus I think we should generally be
offering rationales for significant changes, more than we do now.)

Dave

[Nicholas Nethercote]

On Thu, Dec 1, 2011 at 6:58 PM, David Mandelin <dman...@mozilla.com> wrote:
>
> Could you lay out the rationale for this? I'm not necessarily opposed, but
> my gut reaction is "but WARNINGS_AS_ERRORS is really annoying", so I'd
> appreciate hearing about the benefits (and perhaps why the downsides might
> not be as bad as I think). (Plus I think we should generally be offering
> rationales for significant changes, more than we do now.)

The rationale:

- Warnings are bad.  Some are false positives, but many aren't, so
leaving them unfixed is a recipe for bugs. So zero warnings is a good goal.

- You'll never reach zero warnings without automated help.  Partly
because people don't test their changes on all important platforms
(fair enough), and partly because some people don't notice or don't
care when they introduce warnings on the platforms they do develop on
(not as good).

If you think zero warnings is a valid goal, this is the only practical
way to get there.  If you don't think zero warnings is a valid goal,
you won't like this.  I'm assuming there's group buy-in in the JS team
on the idea that zero warnings is valid goal, since that's what we
currently have (via a marginally different process).

Nick

[Bobby Holley]

I'm not totally convinced that WARNINGS_AS_ERRORS is the most efficient way
to achieve a warning-free (or mostly warning-free) code base.

The set of warnings generated on different platforms is quite divergent. So
if there's a commonly-triggered warning on MSVC that doesn't show up on
GCC, many, many patches will bounce from try on questionable grounds. This
wastes a lot of cumulative time (both machine time and developer time),
because developers can't easily check compliance locally. They have to keep
pushing to try until they uncover everything.

Alternatively, we can just assign 1 person per platform to periodically fix
all the warnings they see in their local builds. This doesn't take much
time if done regularly, is much more efficient, and gets us close to the
same result.

If we can identify a set of warnings that show up consistently on all
platforms, I'd be happy to make _those_ warnings fatal on tinderbox. Not
sure if the tooling supports that though.

-bholley

On Fri, Dec 2, 2011 at 1:11 PM, Nicholas Nethercote
<n.neth...@gmail.com>wrote:

> On Thu, Dec 1, 2011 at 6:58 PM, David Mandelin <dman...@mozilla.com>
> wrote:
> >

> > Could you lay out the rationale for this? I'm not necessarily opposed,
> but
> > my gut reaction is "but WARNINGS_AS_ERRORS is really annoying", so I'd
> > appreciate hearing about the benefits (and perhaps why the downsides
> might
> > not be as bad as I think). (Plus I think we should generally be offering
> > rationales for significant changes, more than we do now.)
>

> The rationale:
>
> - Warnings are bad. Some are false positives, but many aren't, so
> leaving them unfixed is a recipe for bugs. So zero warnings is a good
> goal.
>
> - You'll never reach zero warnings without automated help. Partly
> because people don't test their changes on all important platforms
> (fair enough), and partly because some people don't notice or don't
> care when they introduce warnings on the platforms they do develop on
> (not as good).
>
> If you think zero warnings is a valid goal, this is the only practical
> way to get there. If you don't think zero warnings is a valid goal,
> you won't like this. I'm assuming there's group buy-in in the JS team
> on the idea that zero warnings is valid goal, since that's what we
> currently have (via a marginally different process).
>
> Nick

> _______________________________________________
> dev-tech-js-engine-internals mailing list
> dev-tech-js-en...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
>

[Nicholas Nethercote]

On Sat, Dec 3, 2011 at 8:45 AM, Bobby Holley <bobby...@gmail.com> wrote:
>
> Alternatively, we can just assign 1 person per platform to periodically fix
> all the warnings they see in their local builds. This doesn't take much time
> if done regularly, is much more efficient, and gets us close to the same
> result.

So you're volunteering to be one of these people? And for the whole
browser, eventually?

Nick

[Bobby Holley]

Not necessarily, no. But I might prefer doing it (at least in the
components where I work) to having my patches constantly bounced for
spurious reasons.

There's also the question of whether to keep it on or off in local builds
(as opposed to tinderbox). If we keep it off, developers don't notice that
they broke something until they push. If we keep it on, then new
contributors with rare compiler versions get totally pooched the first time
they try to build mozilla.

I've seen WARNINGS_AS_ERRORS work well in proprietary environments where
the build configuration matrix is small and known. But in a diverse
ecosystem like Mozilla, I'm not convinced it's worth it.

There are a number of important things in software development that are
impossible to enforce programatically, such as stylistic consistency and
commenting. Of these, I think warnings are one of the easier issues to stay
on top of. If you care about them, it's not too hard to keep them out of
the modules where you work regularly.

-bholley

On Fri, Dec 2, 2011 at 7:09 PM, Nicholas Nethercote
<n.neth...@gmail.com>wrote:

[Brendan Eich]

I used to grep js shell build logs for warnings and nag folks, or fix 'em myself. But I cared only about my platform's warnings (Mac OS X GCC). Some of the MSVC warnings were, I thought, pedantic crap and not worth fixing. Of course you could suppress them by warning number.

I agree with you that this system won't be pleasant unless everyone has the same warning as error filter, which seems impractical. Having a champion per platform run down warnings is what we've done until now. If we can't find champions, then how important is this, really? It was to me, when I was generating lots of patches and keeping up with tip, so I acted as self-selected champion -- and only for Mac OS X GCC's idea of warnings.

/be

[Luke Wagner]

Since this seems like a group buy-in sort of deal, here is my opinion:

Right now: I usually see no warnings and when one appears it goes away soonish. I build with make -s so when I get a warning I see it immediately (no grep needed).

When we had warnings-as-errors: I was annoyed by the seemingly-urgent need to drop what I was doing to fix a warning, often costing 30 minutes (suspend what i was doing, switch to windows, hg pull (slow), reconfigure (slow), ack on irc that 'yes i was on it', ...). I would also be annoyed by seeing other people's red on tbpl since then I'd have to click it and wait to see if it was something real (and I should hold off landing) or just warnings-as-errors.

I know I'm getting the best of it since I'm on Linux. I grudgingly accepted warnings-as-errors the first time since I thought it wasn't fair to people on Windows. But if they aren't rioting to turn it on, I see no material benefit; only cost.

Also, I reject the premise that if you don't have warnings-as-errors then you get a flood of warnings. We're not velociraptors. As long as you (1) have people actively working on the codebase for a given build config and (2) have an accepted module policy of lets-strive-for-zero-warnings, warnings get fixed on that build config. The only hard spot is if you have only one person with a given build config: they may get stuck fixing a bunch of warnings (a possible solution being to turn off whatever pedantic warning their build config seems to enjoy producing).

[Brendan Eich]

On Dec 3, 2011, at 9:44 PM, Luke Wagner wrote:

> We're not velociraptors.

LOL. And manly <3.

/be

[Nicholas Nethercote]

On Sun, Dec 4, 2011 at 4:44 PM, Luke Wagner <lu...@mozilla.com> wrote:
>
> Also, I reject the premise that if you don't have warnings-as-errors then you get a flood of warnings.  We're not velociraptors.  As long as you (1) have people actively working on the codebase for a given build config and (2) have an accepted module policy of lets-strive-for-zero-warnings, warnings get fixed on that build config.

I think the slight delays in fixing warnings works pretty well for the
JS engine, where (1) and (2) are in place. (The JS engine also has
lots of people working on it compared to most other parts of the
browser.)

But my long-term goal here is to get zero warnings, or as close as
feasible, for the entire browser, and I don't think the lenient
approach has a chance of working in that case -- just see the hundreds
(thousands?) of warnings we currently get. (The rest of the browser
also doesn't have the luxury of not-visible-by-default shell builds
which can have non-standard build parameters.)

I figured that if a strict process could be made to work for the JS
engine, it would serve as precedent and my long-term goal would have
some chance of success. If I can't get buy-in from JS engine
developers then my long-term goal has zero chance.

(FWIW, my current interest in this topic is due to my fixing a very
real bug (https://bugzilla.mozilla.org/show_bug.cgi?id=703113) that
was caused by an overshadowed variable that GCC warned about but
nobody prior to me noticed or cared about.)

Nick

[Brendan Eich]

On Dec 3, 2011, at 11:58 PM, Nicholas Nethercote wrote:

> But my long-term goal here is to get zero warnings, or as close as
> feasible, for the entire browser, and I don't think the lenient
> approach has a chance of working in that case -- just see the hundreds
> (thousands?) of warnings we currently get. (The rest of the browser
> also doesn't have the luxury of not-visible-by-default shell builds
> which can have non-standard build parameters.)

You want a wider channel, probably m.d.platform.

FWIW, we've been here before. The project goes back 13+ years and we have had several attempts at zero-warning tolerance (the one I remember best, Steve Lamm drove -- don't have time to bug-dive for details). These attempts failed. It's hard to get a large group acting coherently, especially with warnings sometimes crying wolf, especially with minority-share platforms (GCC-based, in the old days also a freakshow of proprietary Unix compilers) not seeing the same warnings the majority-share one (Windows) sees. (Majority-share now may be Mac OS X for all I know; doesn't affect the argument.)

The problem remains the different compilers don't agree on warnings.

Can we prioritize the "bad" warnings that all agree on.

> (FWIW, my current interest in this topic is due to my fixing a very
> real bug (https://bugzilla.mozilla.org/show_bug.cgi?id=703113) that
> was caused by an overshadowed variable that GCC warned about but
> nobody prior to me noticed or cared about.)

Unused variables are "bad" -- can we get a count of those for all top tier platforms for a Firefox build, to see how bad the problem is?

/be

[Boris Zbarsky]

On 12/4/11 3:24 AM, Brendan Eich wrote:
> Can we prioritize the "bad" warnings that all agree on.

This. Making warnings about shadowing variables fatal by default would
be great; for one thing that's something compilers actually agree on, right?

Making the various insane warnings that depend on optimization level and
what a particular version of gcc is smoking that day be fatal by default
seems like a non-starter to me.

The hard part is that gcc does not support warnings-as-errors at
per-warning-type granularity, right?

-Boris

[Igor Bukanov]

On 4 December 2011 18:13, Boris Zbarsky <bzba...@mit.edu> wrote:
> The hard part is that gcc does not support warnings-as-errors at
> per-warning-type granularity, right?

At least GCC 4.6 supports it via #pragma GCC diagnostics,
http://gcc.gnu.org/onlinedocs/gcc-4.6.2/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas
, that can be put in a common header.

[Nicholas Nethercote]

On Sun, Dec 4, 2011 at 12:24 AM, Brendan Eich <bre...@mozilla.com> wrote:
>
>> But my long-term goal here is to get zero warnings, or as close as
>> feasible, for the entire browser
>

> You want a wider channel, probably m.d.platform.

I deliberately avoided that because I figure if I can't get this
happening in the JS engine where the situation is already good, the
broader effort is doomed.

> FWIW, we've been here before. The project goes back 13+ years and we have had several attempts at zero-warning tolerance (the one I remember best, Steve Lamm drove -- don't have time to bug-dive for details). These attempts failed.
>
> It's hard to get a large group acting coherently, especially with warnings sometimes crying wolf, especially with minority-share platforms (GCC-based, in the old days also a freakshow of proprietary Unix compilers) not seeing the same warnings the majority-share one (Windows) sees. (Majority-share now may be Mac OS X for all I know; doesn't affect the argument.)

Yep, https://bugzilla.mozilla.org/show_bug.cgi?id=187528 is a sad bug
that will never be closed if a new approach isn't taken. That's why I
want it automated, to force the group to act coherently, like they do
with things like compile errors and test failures. And that's why I
only want it on TBPL, so unusual platforms aren't affected.

> The problem remains the different compilers don't agree on warnings.

They don't even agree on compile errors, but we live with that -- you
just test patches on try server before landing them. In my mind the
warning situation is no different, once you get down to zero warnings.
I guess you think that warnings on platforms other than yours will be
much more common than errors on platforms other than yours? Maybe
you're right, but I don't think we can ever know without trying it,
which is why I suggested the two week trial period.

> Can we prioritize the "bad" warnings that all agree on.
>

> Unused variables are "bad" -- can we get a count of those for all top tier platforms for a Firefox build, to see how bad the problem is?

I realized I'm tilting at a large windmill here, so I'm happy to be
pragmatic. If we can make one useful warning fatal on one platform
(e.g. -Wshadow on Mac/GCC) that would be a useful toe-hold and
precedent. I originally was aiming for a larger initial toe-hold that
covered the entire JS engine on all TBPL platforms, but clearly I'm
not getting buy-in on that.

(BTW, if you forget a return statement in a non-void function that's
an error in GCC Mozilla builds, but I thought it was a warning... has
the default GCC behaviour changed, or have we made that warning fatal
somehow?)

Nick

[Boris Zbarsky]

On 12/4/11 7:50 PM, Nicholas Nethercote wrote:
> They don't even agree on compile errors, but we live with that

The set of such disagreements is small and has been getting smaller over
time (modulo the introduction of clang, of course, which gave a
temporary bump to the set size) as compilers actually converge on the
spec. Our C++ compatibility guidelines covered just about all such
disagreements, for "sane" code.

For warnings, the situation is much less reasonable; compilers add new
warnings all the time and there is no spec for warnings...

> I guess you think that warnings on platforms other than yours will be
> much more common than errors on platforms other than yours?

I think that given the way compilers treat warnings (a compared to
errors) at the moment that's nearly guaranteed.

Doing this for specific warnings is definitely better in terms of not
surprising people; it's a lot more likely to get buy-in for sure, based
on my sample size of 1. ;)

-Boris

[Brendan Eich]

On Dec 4, 2011, at 4:50 PM, Nicholas Nethercote wrote:

>> FWIW, we've been here before. The project goes back 13+ years and we have had several attempts at zero-warning tolerance (the one I remember best, Steve Lamm drove -- don't have time to bug-dive for details). These attempts failed.
>>
>> It's hard to get a large group acting coherently, especially with warnings sometimes crying wolf, especially with minority-share platforms (GCC-based, in the old days also a freakshow of proprietary Unix compilers) not seeing the same warnings the majority-share one (Windows) sees. (Majority-share now may be Mac OS X for all I know; doesn't affect the argument.)
>
> Yep, https://bugzilla.mozilla.org/show_bug.cgi?id=187528 is a sad bug
> that will never be closed if a new approach isn't taken. That's why I
> want it automated, to force the group to act coherently,

You can't force a group to act coherently, unless you start pointing guns.

If you make everyone stop work when anyone regresses the pickiest compiler's warning-as-error threshold, you'll get a revolt.

> like they do
> with things like compile errors and test failures.

We have to deal with compile errors and they've a much larger common cross-platform intersection (SWAG) in proportion to all errors, than the situation with warnings.

Tests were an optional thing in the old days. We've learned our lesson but we don't have infinite money. Everything costs. If there's a better way to be nearly zero-warnings-that-matter, we should find it.

> And that's why I
> only want it on TBPL, so unusual platforms aren't affected.

Luke cited Linux already. Let's not go in circles. There's an issue even with tier 1.

> I realized I'm tilting at a large windmill here, so I'm happy to be
> pragmatic.

Great, because being otherwise is really the wrong thing, in a large group. Requires force, and open source doesn't have that gun in its belt. People go along with compiler errors and test failures stopping checkins because of enlightened self-interest. Anyone who matters could fork the whole project, or a module such as SpiderMonkey, and go back to the bad old days (no tests) or even give up continuous tier-1 buildability as requirement. No one of merit has.

> If we can make one useful warning fatal on one platform
> (e.g. -Wshadow on Mac/GCC) that would be a useful toe-hold and
> precedent.

Now you are talking.

> I originally was aiming for a larger initial toe-hold that
> covered the entire JS engine on all TBPL platforms, but clearly I'm
> not getting buy-in on that.
>
> (BTW, if you forget a return statement in a non-void function that's
> an error in GCC Mozilla builds, but I thought it was a warning... has
> the default GCC behaviour changed, or have we made that warning fatal
> somehow?)

If my aging memory serves, GCC changed.

/be

[Patrick Walton]

On 12/03/2011 11:58 PM, Nicholas Nethercote wrote:
> I figured that if a strict process could be made to work for the JS
> engine, it would serve as precedent and my long-term goal would have
> some chance of success. If I can't get buy-in from JS engine

> developers then my long-term goal has zero chance.

We do -Werror on Rust. It is a huge pain, unfortunately. Half of my
patches that touch the C++ codebase tend to bounce, since I develop on
clang and the bots run on gcc. If and when we use MSVC on Windows the
situation will probably be hopeless.

Patrick

[Jeff Walden]

On 12/04/2011 12:13 PM, Boris Zbarsky wrote:
> The hard part is that gcc does not support warnings-as-errors at per-warning-type granularity, right?

I think at least newer gcc supports -Werror=<type>.

Jeff

[Jeff Walden]

On 12/04/2011 12:44 AM, Luke Wagner wrote:
> The only hard spot is if you have only one person with a given build config: they may get stuck fixing a bunch of warnings (a possible solution being to turn off whatever pedantic warning their build config seems to enjoy producing).

This is a very, very real problem with the current setup. Every time I do a Windows build, I invariably end up getting stuck fixing a bunch of compiler warnings. And we wonder why no one wants to develop on Windows.

"I feel like the maid; I just cleaned up this mess! Can we keep it clean for... for ten minutes?"
-- Mr. Incredible

We're rarely warning-free on Windows in my experience. That is directly correlated with there being no consequence to introducing a warning.

Jeff