777233 - Add warnings module to mozbuild

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Attachment: mozbuild warnings module, v1

This adds a warning module to mozbuild. It contains code for representing and collecting compiler warnings. It is quite handy. It will eventually be used to collect compiler warnings from build output and to persist them after compiler runs so people can easily query the set of compiler warnings.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 645646 [details] [diff] [review]
mozbuild warnings module, v1

Review of attachment 645646 [details] [diff] [review]:
-----------------------------------------------------------------

I have a concern about the usefulness of this module, when plenty of warnings are split on multiple lines in the compiler output.

::: python/mozbuild/mozbuild/compilation/warnings.py
@@ +16,5 @@
> +# It assumes ANSI escape sequences.
> +RE_STRIP_COLORS = re.compile(r'\x1b\[[\d;]+m')
> +
> +# This captures Clang diagnostics with the standard formatting.
> +RE_CLANG_WARNING = re.compile(r"""

Nothing for gcc?

@@ +238,5 @@
> +
> +    def load_from_file(self, filename):
> +        """Load the database from a file."""
> +        with open(filename, 'rb') as fh:
> +            self.deserialize(fh)

I'd merge load_from_file with deserialize, and likewise for save_to_file with serialize, and use the argument type to decide whether you want to open a file or just use the given file object.

::: python/mozbuild/mozbuild/test/test_util.py
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import hashlib
> +import unittest

You want to use mozunit (see bug 776035). You may also use MockedOpen instead of writing temporary files.

Comment 2

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #1)
> I have a concern about the usefulness of this module, when plenty of
> warnings are split on multiple lines in the compiler output.

It is true that parsing falls apart during parallel builds when output from multiple processes is interleaved. We can't solve that in this module. We can only solve that on the execution side.

If that's not what you meant, I've had no problems running this locally. It seems to work just fine. Clang's first line is always the bulk of the warning. Subsequent lines are formatting foo or notes (which would be nice to capture in a follow-up). I will concede that I haven't tested MSVC thoroughly. It seems to work though.

> Nothing for gcc?

I think anything we don't capture in the initial land can be the territory of a follow-up. Something is better than nothing.
 
> @@ +238,5 @@
> > +
> > +    def load_from_file(self, filename):
> > +        """Load the database from a file."""
> > +        with open(filename, 'rb') as fh:
> > +            self.deserialize(fh)
> 
> I'd merge load_from_file with deserialize, and likewise for save_to_file
> with serialize, and use the argument type to decide whether you want to open
> a file or just use the given file object.

I hear both sides of this argument from different Python camps. I agree your proposal seems cleaner because there are fewer methods. But, it opens the door for bugs regarding detection of the argument type. How exactly do you do that? isinstance(arg, file)? What if the thing isn't a file object but implements write()? hasattr(arg, 'write')? You are left implementing a heuristic. This seems prone to failure.

Maybe I should just nuke the *_from_file methods and make the caller pass a file object.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #2)
> It is true that parsing falls apart during parallel builds when output
> from multiple processesis interleaved. We can't solve that in this module.
> We can only solve that on the execution side.

I was assuming the module would only be used in a wrapper script for the compiler. OTOH, if each compiler call is going to deserialize and reserialize the database, with concurrency taken into account, that may end up not being very efficient.

> Subsequent lines are formatting foo or notes (which would be nice
> to capture in a follow-up).

That's what i was referring to.

> > Nothing for gcc?
> 
> I think anything we don't capture in the initial land can be the territory
> of a follow-up. Something is better than nothing.

Arguably, adding gcc should be a matter of adding a regexp and a re.match call...

> > @@ +238,5 @@
> > > +
> > > +    def load_from_file(self, filename):
> > > +        """Load the database from a file."""
> > > +        with open(filename, 'rb') as fh:
> > > +            self.deserialize(fh)
> > 
> > I'd merge load_from_file with deserialize, and likewise for save_to_file
> > with serialize, and use the argument type to decide whether you want to open
> > a file or just use the given file object.
> 
> I hear both sides of this argument from different Python camps. I agree your
> proposal seems cleaner because there are fewer methods. But, it opens the
> door for bugs regarding detection of the argument type. How exactly do you
> do that? isinstance(arg, file)? What if the thing isn't a file object but
> implements write()? hasattr(arg, 'write')? You are left implementing a
> heuristic. This seems prone to failure.
> 
> Maybe I should just nuke the *_from_file methods and make the caller pass a
> file object.

What is done in Preprocessor.py, for example, is, in essence:
  if type(arg) == str or type(arg) == unicode:
    file = open(arg, 'r')
  else:
    file = arg

Comment 5

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #3)
> > > Nothing for gcc?
> > 
> > I think anything we don't capture in the initial land can be the territory
> > of a follow-up. Something is better than nothing.
> 
> Arguably, adding gcc should be a matter of adding a regexp and a re.match
> call...

I'm lazy. Perhaps this is also my inner voice encouraging people to use Clang :)

I also have bug 756804 on file to track this explicitly.
 
> What is done in Preprocessor.py, for example, is, in essence:
>   if type(arg) == str or type(arg) == unicode:
>     file = open(arg, 'r')
>   else:
>     file = arg

It's good to have a reference point! FWIW, isn't "isinstance(arg, basestring)" a better way to write the above?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> It's good to have a reference point! FWIW, isn't "isinstance(arg,
> basestring)" a better way to write the above?

Probably.

Comment 7

[Jeff Hammel]

> What is done in Preprocessor.py, for example, is, in essence:
>   if type(arg) == str or type(arg) == unicode:
>     file = open(arg, 'r')
>   else:
>     file = arg

Probably better is 'if isinstance(arg, basestring)'

Comment 8

[Jeff Hammel]

Comment on attachment 645646 [details] [diff] [review]
mozbuild warnings module, v1

+                normalized = v2
+
+                if k2 == 'warnings':
+                    normalized = [w for w in v2]

Huh?

I'm not sure if I'm really qualified to give much feedback here.  I can see the need for this module, but its hard for me to figure out how it will ultimately fit together with everything else.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 645646 [details] [diff] [review]
mozbuild warnings module, v1

Looks good enough to go in, though +1 on the gcc comment -- I understand wanting to encourage clang use, but enough people use gcc that I think you don't want to throw up too many barriers in their way to trying out the new system

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6deca9f6666

I have bug 756804 on file to support GCC.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d6deca9f6666