1367834 - Reduce volume of compiler warnings printed at end of the build

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

People are complaining about the volume of compiler warnings printed at the end of the build because they aren't actionable. Let's do something about it.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1367834 - Suppress compiler warnings for 3rd party projects;

Most complaints about compiler warnings being printed at the end of the
build (4abe611696ab - bug 1360764) center around the volume. And most
of the volume is due to 3rd party projects. Since work to suppress the
warnings in 3rd party projects has been slow, let's temporarily filter
out those warnings.

This commit adds a list of suppressed directories corresponding to
3rd party projects responsible for most compiler warnings. For
non-automation builds, we don't print the warnings. Instead, we
print a summary of how many warnings were suppressed.

Not all 3rd party projects were listed. The intent is to only list the
high volume ones so the warnings list is reasonable by default. I
believe the current implementation achieves that. But I wouldn't
be surprised if it requires refinement. Perfect is the enemy of good.

Review commit: https://reviewboard.mozilla.org/r/142842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142842/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8871370 [details]
Bug 1367834 - Suppress compiler warnings for 3rd party projects;

https://reviewboard.mozilla.org/r/142842/#review146596

I'm not a fan of the fixed list here, but I'm not totally sure if there's a good way around it.  Pushing back to try and stimulate other ideas.

The reporting loop thing needs to be fixed in some way.

::: python/mozbuild/mozbuild/mach_commands.py:482
(Diff revision 1)
>              if not status:
> +                # Suppress warnings for 3rd party projects in local builds
> +                # until we suppress them for real.
> +                # TODO remove entries/feature once we stop generating warnings
> +                # in these directories.
> +                LOCAL_SUPPRESS_DIRS = (

Could we not derive this from a BUILDSTATUS line about whether the directory permits compiler warnings or not?  I'd rather not maintain this list here if we can avoid it.  Or is this sort of a least-bad solution, because we still want to give people the option to fix warnings in their newly-imported third-party library or similar?

::: python/mozbuild/mozbuild/mach_commands.py:515
(Diff revision 1)
> +                if 'MOZ_AUTOMATION' not in os.environ:
> +                    for d in LOCAL_SUPPRESS_DIRS:
> +                        count = 0
> +                        for warning in sorted(monitor.instance_warnings):
> +                            if warning['normpath'].startswith(d):
> +                                count += 1
> +
> +                        if count:
> +                            self.log(logging.WARNING, 'suppressed_warning',
> +                                     {'dir': d, 'count': count},
> +                                     '(suppressed {count} warnings in {dir})')
> +

This loop is pretty inefficient in the local case, since you'll be sorting `monitor.instance_warnings` for each entry in `LOCAL_SUPPRESS_DIRS`.

`.startswith` accepts a tuple of prefixes, so you can just:

`if warning['normpath'].startswith(LOCAL_SUPPRESS_DIRS):`

though maybe it doesn't accept lists, so you might have to make `LOCAL_SUPPRESS_DIRS` a tuple right off the bat.

Also, WDYT about structuring things like:

```
// if we have to use a fixed set of dirs...
LOCAL_SUPPRESS_DIRS = (...)

sorted_warnings = list(sorted(monitor.instance_warnings))
...add `normpath` to all warnings...
relevant = [w for w in sorted_warnings
            if not w['normpath'].startswith(LOCAL_SUPPRESS_DIRS)]

in_automation = 'MOZ_AUTOMATION' in os.environ
if not in_automation:
  ...compute how many warnings we suppressed and print a message...

for warning in (relevant if not in_automation else sorted_warnings):
  ...stuff...
```

I think that makes the flow a little more obvious, fewer conditionals and whatnot.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8871370 [details]
Bug 1367834 - Suppress compiler warnings for 3rd party projects;

https://reviewboard.mozilla.org/r/142842/#review146596

> Could we not derive this from a BUILDSTATUS line about whether the directory permits compiler warnings or not?  I'd rather not maintain this list here if we can avoid it.  Or is this sort of a least-bad solution, because we still want to give people the option to fix warnings in their newly-imported third-party library or similar?

I'm not a fan of it either. I'm very much going for a quick fix to make people happy. I view this whole thing as temporary, so I'd prefer to not add too much infrastructure around tracking things unless it can be used to improve monitoring in some other way.

> This loop is pretty inefficient in the local case, since you'll be sorting `monitor.instance_warnings` for each entry in `LOCAL_SUPPRESS_DIRS`.
> 
> `.startswith` accepts a tuple of prefixes, so you can just:
> 
> `if warning['normpath'].startswith(LOCAL_SUPPRESS_DIRS):`
> 
> though maybe it doesn't accept lists, so you might have to make `LOCAL_SUPPRESS_DIRS` a tuple right off the bat.
> 
> Also, WDYT about structuring things like:
> 
> ```
> // if we have to use a fixed set of dirs...
> LOCAL_SUPPRESS_DIRS = (...)
> 
> sorted_warnings = list(sorted(monitor.instance_warnings))
> ...add `normpath` to all warnings...
> relevant = [w for w in sorted_warnings
>             if not w['normpath'].startswith(LOCAL_SUPPRESS_DIRS)]
> 
> in_automation = 'MOZ_AUTOMATION' in os.environ
> if not in_automation:
>   ...compute how many warnings we suppressed and print a message...
> 
> for warning in (relevant if not in_automation else sorted_warnings):
>   ...stuff...
> ```
> 
> I think that makes the flow a little more obvious, fewer conditionals and whatnot.

I agree. I'll make this much better in the next version.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8871370 [details]
Bug 1367834 - Suppress compiler warnings for 3rd party projects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142842/diff/1-2/

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8871370 [details]
Bug 1367834 - Suppress compiler warnings for 3rd party projects;

https://reviewboard.mozilla.org/r/142842/#review146614

Well, your plan worked, somebody did something about these warnings. :)

::: python/mozbuild/mozbuild/mach_commands.py:523
(Diff revision 2)
> +                for d, count in sorted(suppressed_by_dir.items()):
> +                    self.log(logging.WARNING, 'suppressed_warning',
> +                             {'dir': d, 'count': count},
> +                             '(suppressed {count} warnings in {dir})')

Willing to bet you a beverage that people will still complain about this. :)

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8871370 [details]
Bug 1367834 - Suppress compiler warnings for 3rd party projects;

https://reviewboard.mozilla.org/r/142842/#review146614

> Willing to bet you a beverage that people will still complain about this. :)

I'm not going to take that bet because you are probably correct. As I said in the commit message, let's take a big step and refine until people are generally happy.

FWIW, with the change to not print warnings after backend error, I don't think people will complain as much. The reason is that the warnings are printed after the last build backend output but before the rest of the summary information printed by mach. There usually isn't anything too valuable in the last lines of the build output. And since we only print warnings that occur during the current build, an incremental build shouldn't print many warnings, so there will be little output to scroll over.

But we'll see...

Thanks for the prompt reviews.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/508dbbb885fc
Suppress compiler warnings for 3rd party projects; r=froydnj

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/508dbbb885fc