Add warnings module to mozbuild

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 645646 [details] [diff] [review]
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.
Attachment #645646 - Flags: review?(mh+mozilla)
Attachment #645646 - Flags: feedback?(jhammel)
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.
Attachment #645646 - Flags: review?(mh+mozilla) → review?(ted.mielczarek)
(Assignee)

Comment 2

5 years ago
(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.
(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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 773398
(Assignee)

Comment 5

5 years ago
(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?
(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

5 years ago
> 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

5 years ago
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.
Attachment #645646 - Flags: feedback?(jhammel)

Updated

5 years ago
Assignee: nobody → gps
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
Attachment #645646 - Flags: review+
(Assignee)

Comment 10

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

I have bug 756804 on file to support GCC.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Attachment #645646 - Flags: review?(ted.mielczarek)
https://hg.mozilla.org/mozilla-central/rev/d6deca9f6666
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.