Directory (--directory) argument for |mach warnings-list| and |mach warnings-summary|

RESOLVED FIXED in Firefox 42

Status

()

Core
mach
--
enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Mitch, Assigned: Mitch)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
mach's warnings-list and warnings-summary commands could have a --directory argument to narrow down which warnings are where.
(Assignee)

Comment 1

3 years ago
Created attachment 8555172 [details] [diff] [review]
Patch v1

This patch adds a directory argument (--directory, -C) for |mach warnings-list| and |mach warnings-summary|.
It's a bit rough; directories are always recursed and relative paths aren't handled.
I've also added a warning flags argument (--flags) for warnings-list while I was at it. Example for MSVC warning C4267 in nsprpub:

>mach warnings-list --directory nsprpub --flags C4267
Assignee: nobody → mitchell.field
Status: NEW → ASSIGNED
Attachment #8555172 - Flags: review?(gps)

Comment 2

3 years ago
Comment on attachment 8555172 [details] [diff] [review]
Patch v1

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

Nice feature addition!

Just minor nits in the code.

Should be a rubber stamp on next review.

::: python/mozbuild/mozbuild/compilation/warnings.py
@@ +153,5 @@
>          types = {}
>          for value in self._files.values():
>              for warning in value['warnings']:
> +                if dirpath is not None and not warning['filename'].startswith(dirpath):
> +                    continue

if dirpath and not ...

I can't remember if the paths on Windows are normalized to posix style or not. You should use mozpack.path to do path normalization is a platform independent way. https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/path.py

::: python/mozbuild/mozbuild/mach_commands.py
@@ +626,3 @@
>          database = self.database
>  
> +        if directory is not None:

if directory:

@@ +626,5 @@
>          database = self.database
>  
> +        if directory is not None:
> +            if directory.startswith('/'):
> +                directory = directory[1:]

This assumes posix line endings. But it's a command argument, so that's probably fine since we develop in a posix-like shell on windows.

@@ +629,5 @@
> +            if directory.startswith('/'):
> +                directory = directory[1:]
> +
> +            dirpath = os.path.join(self.topsrcdir, directory)
> +            print(dirpath)

did you mean for this print statement to be in the final output?

@@ +669,5 @@
> +
> +            dirpath = mozpath.join(self.topsrcdir, directory)
> +            if not os.path.isdir(dirpath):
> +                print('Specified directory not found.')
> +                return 1

There is a bit of duplicate code here. Extract into its own function?

@@ +671,5 @@
> +            if not os.path.isdir(dirpath):
> +                print('Specified directory not found.')
> +                return 1
> +
> +        if flags is not None:

if flags

@@ +673,5 @@
> +                return 1
> +
> +        if flags is not None:
> +            # Flatten lists of flags
> +            flags = list(itertools.chain(*[flaglist.split(',') for flaglist in flags]))

You probably want a set() here. Membership testing in sets is much, much faster in Python.

@@ +681,5 @@
>  
>              if filename.startswith(self.topsrcdir):
>                  filename = filename[len(self.topsrcdir) + 1:]
>  
> +            if directory is not None:

if directory:

@@ +685,5 @@
> +            if directory is not None:
> +                if not filename.startswith(directory):
> +                    continue
> +
> +            if flags is not None and warning['flag'] not in flags:

if flags and ...
Attachment #8555172 - Flags: review?(gps) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8556395 [details] [diff] [review]
Patch v2

I believe the nits have been fixed.
Attachment #8555172 - Attachment is obsolete: true
Attachment #8556395 - Flags: review?(gps)
(Assignee)

Updated

3 years ago
Attachment #8556395 - Attachment description: bug_1126228_v2.patch → Patch v2

Updated

3 years ago
Attachment #8556395 - Flags: review?(gps) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3baacbe31b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e3baacbe31b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.