Closed Bug 1230962 Opened 4 years ago Closed 4 years ago

Create arbitrary linter wrapper for running linters against various languages and dirs in the tree

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files, 4 obsolete files)

This bug is for tracking the proposal in the mozilla.tools newsgroup discussion here:
https://groups.google.com/d/topic/mozilla.tools/SZeDPTtsMiU/discussion
Bug 1230962 - Add prototype mozlint package
Add eslint to mozlint
These patches are very much in the prototype phase and will likely not look anything like the finished product. Putting them up so those interested can provide (very early) feedback.
Comment on attachment 8696486 [details]
MozReview Request: Bug 1230962 - Add prototype mozlint package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27333/diff/1-2/
Comment on attachment 8696487 [details]
MozReview Request: Add eslint to mozlint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27335/diff/1-2/
Comment on attachment 8696486 [details]
MozReview Request: Bug 1230962 - Add prototype mozlint package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27333/diff/2-3/
Attachment #8696487 - Attachment is obsolete: true
Comment on attachment 8696486 [details]
MozReview Request: Bug 1230962 - Add prototype mozlint package

I got a chance to push on this some more. I'm fairly happy with it so far. It has feature parity with |mach eslint| and also includes a flake8 linter. E.g, you can run |mach lint testing/mochitest| and get results from both flake8 and eslint in the same output format.

I still need to improve the docs and the tests. But if anyone wants to provide early feedback, I'd appreciate it!
Attachment #8696486 - Flags: feedback?
Comment on attachment 8696486 [details]
MozReview Request: Bug 1230962 - Add prototype mozlint package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27333/diff/3-4/
Attachment #8696486 - Flags: feedback?
Attachment #8723233 - Attachment description: MozReview Request: Add sphinx docs → MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?chmanchester
Attachment #8723233 - Flags: review?(cmanchester)
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36419/diff/1-2/
Attachment #8696486 - Attachment is obsolete: true
Attachment #8723235 - Attachment is obsolete: true
(In reply to Andrew Halberstadt [:ahal] from comment #13)
> Created attachment 8732317 [details]
> MozReview Request: Add mozlint integration for flake8
> 
> Review commit: https://reviewboard.mozilla.org/r/41089/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/41089/

I'm going to tackle this one in a follow-up bug but included it for now as an example of how this would look like, and in case anyone wanted to test it out locally.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Blocks: 1258341
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

https://reviewboard.mozilla.org/r/36419/#review39539

Sorry for the delay getting to this. Ultimately, I think I need to defer to someone who has been more involved with the recent linting work. Also, this is adding something that looks a bit like a "top-level" entity in the tree (a new metadata file type that will potentially end up throughout the tree), so I think module owner sign-off from a nearby module would be appropriate (Build Config or Testing Infrastructure, but probably Build Config). I have a couple of concerns I'll outline below, but if we have consumers who will find this beneficial for implementing their lints, it's probably worth landing despite those. When we get to that point I can do a more thorough line-by-line review.

It would be really nice to see a way to go from a declared lint to a TaskCluster task. We have some amount of logic around when to run a lint task (implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1245953#c52), unifying that would be better than duplicating it (linting certain files vs. linting when certain files change would seem to have the same effect).

I got a sense from the mailing list discussion that external lints are the essential use case. I agree, and actually I think in almost all cases a language specific lint would be preferable to a regex based lint. To that end, can we get to the point where someone can declare an external lint without writing any Python? The default implementation could just run some program with some files as input, and indicate failure if the program returns non-zero, and more advanced users could implement output parsers if necessary. Looking at the flake8 and eslint implementations, it seems like finding/installing external programs is a lot to ask of an individual lint implementation.

Related, my biggest concern here is the aspects of this that are re-inventing parts of the build system. Finding external programs to be used in the tree is something confgure should be well suited for (this was recently moved to python configure), and the declarations themselves look a lot like the the kind of metadata we're already incorporating into moz.build files.
Attachment #8723233 - Flags: review?(cmanchester)
Comment on attachment 8732316 [details]
MozReview Request: Bug 1230962 - Create mach/build system integration for mozlint, r=smacleod

https://reviewboard.mozilla.org/r/41087/#review39543
Attachment #8732316 - Flags: review?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #15)
> Comment on attachment 8723233 [details]
> MozReview Request: Bug 1230962 - Add python/mozlint for running several
> linters at once, r?chmanchester
> 
> https://reviewboard.mozilla.org/r/36419/#review39539
> 
> Sorry for the delay getting to this. Ultimately, I think I need to defer to
> someone who has been more involved with the recent linting work.

This is mostly the Firefox team. I don't think they really care much about the implementation as long as |mach eslint| and the ES taskcluster job keep working. And like you mentioned below, this is more  

> Also, this
> is adding something that looks a bit like a "top-level" entity in the tree
> (a new metadata file type that will potentially end up throughout the tree),
> so I think module owner sign-off from a nearby module would be appropriate
> (Build Config or Testing Infrastructure, but probably Build Config).

Any suggestions for another reviewer if not you?

Also my original intent (and gps' recommendation) was to just require all lint files in a single directory (tools/lint). I don't think there will be a terribly large number of them.. though if I'm wrong we could tackle that later.

> I have a couple of concerns I'll outline below, but if we have consumers who will find this beneficial for implementing
> their lints, it's probably worth landing despite those. When we get to that point I can do a more thorough line-by-line
> review.

I think the flake8 linter would qualify as an immediate benefit.

 
> It would be really nice to see a way to go from a declared lint to a
> TaskCluster task. We have some amount of logic around when to run a lint
> task (implemented in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1245953#c52), unifying that
> would be better than duplicating it (linting certain files vs. linting when
> certain files change would seem to have the same effect).

I agree, I was planning to tackle this in a separate bug. It's about step 3 on my list after this one gets landed.


> I got a sense from the mailing list discussion that external lints are the
> essential use case. I agree, and actually I think in almost all cases a
> language specific lint would be preferable to a regex based lint.

Also agree here, though I think it's still worth supporting simple linters for e.g non-programming language files, or less common languages with no easy 3rd party lint libraries.


>To that
> end, can we get to the point where someone can declare an external lint
> without writing any Python? The default implementation could just run some
> program with some files as input, and indicate failure if the program
> returns non-zero, and more advanced users could implement output parsers if
> necessary. Looking at the flake8 and eslint implementations, it seems like
> finding/installing external programs is a lot to ask of an individual lint
> implementation.

I would love to be able to do this, but I think it would be very hard. Each linter needs to be invoked slightly differently and produces results in a slightly different format. For example, linterA might say 'lineno', and linterB might say 'row'. Something will need to massage all the outputs to the same standard format. Given


> Related, my biggest concern here is the aspects of this that are
> re-inventing parts of the build system. Finding external programs to be used
> in the tree is something confgure should be well suited for (this was
> recently moved to python configure), and the declarations themselves look a
> lot like the the kind of metadata we're already incorporating into moz.build
> files.

I sort of wanted to avoid the complexity of build system integration, as this only needs to work with source files. But I'd be open to integrating.. I'm just not sure how. I guess from a high level my thinking is that adding new linters will be a relatively rare occurence, so optimizing for lint authors was not a huge priority.
Gps, are you interested in doing either feedback/review on this? If not, know of anyone who has some stake in linting and would be capable of doing reviews?
Flags: needinfo?(gps)
I'm in Toronto this week and will be busy during the day today and tomorrow.

smacleod would be interested in this from the perspective of MozReview (which is also planning to deploy its own linting integration at some time). We want as much stuff as possible to be in the tree to make it easier for developers to run. Even though I haven't looked at this approach yet, it being in tree and easily runnable makes me a fan.
Flags: needinfo?(gps)
Good point. I want mozreview to be able to use this, but I don't have a good understanding of how that integration would look. Sorry smacleod, you lose the review roulette ;).
Attachment #8723233 - Flags: review?(smacleod)
Attachment #8732316 - Flags: review?(smacleod)
(In reply to Andrew Halberstadt [:ahal] from comment #20)
> Good point. I want mozreview to be able to use this, but I don't have a good
> understanding of how that integration would look. Sorry smacleod, you lose
> the review roulette ;).

I'll try to get to this ASAP. Hopefully sometime today, but maybe tomorrow.
Attachment #8723233 - Flags: review?(smacleod)
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

https://reviewboard.mozilla.org/r/36419/#review41807

This is awesome :D

::: python/mozlint/mozlint/formatters/stylish.py:17
(Diff revision 2)
> +    A drop-in replacement for `blessings.Terminal()` that doesn't
> +    apply any formatting.

single line summary

::: python/mozlint/mozlint/result.py:10
(Diff revision 2)
> +    Represents a single lint error and its related metadata.
> +
> +    :param linter: name of the linter that flagged this error
> +    :param path: path to the file containing the error
> +    :param lineno: line number containing the error
> +    :param message: text describing the error
> +    :param column: Column containing the error (default 1)
> +    :param level: severity of the error, either 'warning' or 'error' (default 'error')
> +    :param hint: Suggestion on how to fix the error (optional)
> +    :param source: source code context of the error (optional)
> +    :param rule: name of the rule that was violated (optional)

Something we supported when we were previously running flake8 on MozReview requests was adjusting the range of the comment, depednent on the error[1]. This meant results involving multiple lines would display as a comment on all the relevant lines.

It would be really nice if MozReview could provide the same functionality when this is enabled. It might be possible to infer this information from the `source` parameter if the linter provides it, but an explicit system such as the one I linked would make things much simpler on our end.

Would you be open to introducing something like that to the result? Maybe some sort of context offset and length information based around lineno.


[1] https://hg.mozilla.org/hgcustom/version-control-tools/file/09a4c0d8c57a/pylib/mozreviewbots/pylintbot/__main__.py#l19

::: python/mozlint/mozlint/result.py:23
(Diff revision 2)
> +    :param hint: Suggestion on how to fix the error (optional)
> +    :param source: source code context of the error (optional)
> +    :param rule: name of the rule that was violated (optional)
> +    """
> +
> +    __slots__ = (

interesting, this is the first time I've seen `__slots__` used.

::: python/mozlint/mozlint/result.py:54
(Diff revision 2)
> +    Class for encoding `ResultContainer`s to json. Usage:
> +
> +        json.dumps(results, cls=ResultEncoder)

Shove the "Usage:" down into its own line.

::: python/mozlint/mozlint/result.py:66
(Diff revision 2)
> +    Convenience method that creates a ResultContainer from a LINTER
> +    definition, providing defaults pulled from the lint object.

Summary should fit on a single line. I'd suggest "Create a ResultContainer from a LINTER definition" and then stick the stuff about defaults on another line.

::: python/mozlint/mozlint/result.py:73
(Diff revision 2)
> +
> +    :param lintobj: LINTER obj as defined in a .lint file
> +    :param kwargs: same as `ResultContainer`
> +    :returns: ResultContainer object
> +    """
> +

no need for this blank

::: python/mozlint/mozlint/roller.py:51
(Diff revision 2)
> +
> +class LintRoller(object):
> +    """
> +    Registers and runs linters.
> +
> +    :param lintargs: Arguments to pass to the underlying linter.

"underlying linter(s)."

::: python/mozlint/mozlint/roller.py:63
(Diff revision 2)
> +
> +    def read(self, paths):
> +        """
> +        Parse one or more linters and add them to the registry.
> +
> +        :param paths: A path or list of paths to linter definitions.

"A path or iterable of paths"

::: python/mozlint/mozlint/roller.py:73
(Diff revision 2)
> +        Run all of the registered linters against the specified file
> +        paths.

this should be on one line (and fits :D)

::: python/mozlint/mozlint/roller.py:76
(Diff revision 2)
> +    def roll(self, paths, num_procs=None):
> +        """
> +        Run all of the registered linters against the specified file
> +        paths.
> +
> +        :param paths: A list of files and/or directories to lint.

"An iterable"

::: python/mozlint/mozlint/roller.py:79
(Diff revision 2)
> +        paths.
> +
> +        :param paths: A list of files and/or directories to lint.
> +        :param num_procs: The number of processes to use. Default: cpu count
> +        :return: A dictionary with file names as the key, and a list of
> +                 `ResultContainer`'s as the value.

no apostrophe

::: python/mozlint/mozlint/types.py:17
(Diff revision 2)
> +from . import result
> +
> +
> +class BaseType(object):
> +    """
> +    Base class for all types of linters.

"Abstract base class"

::: python/mozlint/mozlint/types.py:87
(Diff revision 2)
> +    Base class for linters that open a file, iterate line
> +    by line, and run a payload agains each line.

Single line summary. Suggestion: 
```
"""
Abstract base class for linter types that check each line individually.

Subclasses of this linter type will read each file and check the provided
payload against each line one by one.
"""
```

::: python/mozlint/mozlint/types.py:88
(Diff revision 2)
> +
> +
> +class LineType(BaseType):
> +    """
> +    Base class for linters that open a file, iterate line
> +    by line, and run a payload agains each line.

"against"

::: python/mozlint/mozlint/types.py:130
(Diff revision 2)
> +        return re.search(payload, line)
> +
> +
> +class ExternalType(BaseType):
> +    """
> +    Linter type that runs an external function. The function is

put a blank line after the first sentence

::: python/mozlint/mozlint/types.py:141
(Diff revision 2)
> +    def _lint(self, files, linter, **lintargs):
> +        payload = linter['payload']
> +        return payload(files, **lintargs)
> +
> +
> +builtin_types = {

The name `builtin_types` made me initially think that there would be a way to extend the types externally (which is now obviously incorrect). `supported_types` might be more representative?

::: python/mozlint/test/files/has_foobar.js:1
(Diff revision 2)
> +// Oh no.. this we called this variable foobar, bad!

heh

::: python/mozlint/test/test_roller.py:24
(Diff revision 2)
> +
> +    def __init__(self, *args, **kwargs):
> +        TestCase.__init__(self, *args, **kwargs)
> +
> +        filedir = os.path.join(here, 'files')
> +        self.files = [os.path.join(filedir, f) for f in os.listdir(filedir)]

This test relies on the order of `self.files` but according to the docs on `os.listdir()` "The list is in arbitrary order."

It also seems fragile that adding a single file to that directory (say for a new test or something) could force you to update a bunch of offsets here.

::: python/mozlint/test/test_types.py:24
(Diff revision 2)
> +    def __init__(self, *args, **kwargs):
> +        TestCase.__init__(self, *args, **kwargs)
> +
> +        self.lintdir = os.path.join(here, 'linters')
> +        self.filedir = os.path.join(here, 'files')
> +        self.files = [os.path.join(self.filedir, f) for f in os.listdir(self.filedir)]

same problem with listdir ordering.
Comment on attachment 8732316 [details]
MozReview Request: Bug 1230962 - Create mach/build system integration for mozlint, r=smacleod

https://reviewboard.mozilla.org/r/41087/#review41831

LGTM. I only skimmed the generated files very quickly, and I'm not familiar with the mozilla-central conventions around sphinx documentation, so if getting a thorough review on those pieces is important to you you'll need to ask another reviewer. That being said, I see no problems.
Attachment #8732316 - Flags: review?(smacleod) → review+
(In reply to Steven MacLeod [:smacleod] from comment #23)
> Comment on attachment 8732316 [details]
> MozReview Request: Bug 1230962 - Create mach/build system integration for
> mozlint, r?chmanchester
> 
> https://reviewboard.mozilla.org/r/41087/#review41831
> 
> LGTM. I only skimmed the generated files very quickly, and I'm not familiar
> with the mozilla-central conventions around sphinx documentation, so if
> getting a thorough review on those pieces is important to you you'll need to
> ask another reviewer. That being said, I see no problems.

We don't technically need the generated files, I could probably just get rid of them. Having them means these docs can be generated independently from the overall build documentation, but maybe that's not important. There are a fair bit of sphinx auto-generated files in the tree already though:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Adocs%2Fconf.py&redirect=false&case=false
https://reviewboard.mozilla.org/r/36419/#review41807

> Something we supported when we were previously running flake8 on MozReview requests was adjusting the range of the comment, depednent on the error[1]. This meant results involving multiple lines would display as a comment on all the relevant lines.
> 
> It would be really nice if MozReview could provide the same functionality when this is enabled. It might be possible to infer this information from the `source` parameter if the linter provides it, but an explicit system such as the one I linked would make things much simpler on our end.
> 
> Would you be open to introducing something like that to the result? Maybe some sort of context offset and length information based around lineno.
> 
> 
> [1] https://hg.mozilla.org/hgcustom/version-control-tools/file/09a4c0d8c57a/pylib/mozreviewbots/pylintbot/__main__.py#l19

Good idea! I'm thinking a 'numlines' parameter that is the number of lines the error spans (defaulting to 1). So the range would be lineno:lineno+numlines-1.
https://reviewboard.mozilla.org/r/36419/#review41807

> Good idea! I'm thinking a 'numlines' parameter that is the number of lines the error spans (defaulting to 1). So the range would be lineno:lineno+numlines-1.

While that works for most cases, there are instances where 3 parameters provide a slightly improved experience. The specific instances I'm thinking of is where the actual identified line is not the first in the context range. So while you can provide context by moving the line, and expanding the number of lines, you lose the exact line which the tool spit out itself (In the flake8 case for MozReview, most of our context expansions were actually to add the line above, so `lineno` no longer would represent the line the tool gave)
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36419/diff/2-3/
Attachment #8723233 - Attachment description: MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?chmanchester → MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod
Attachment #8732316 - Attachment description: MozReview Request: Bug 1230962 - Create mach/build system integration for mozlint, r?chmanchester → MozReview Request: Bug 1230962 - Create mach/build system integration for mozlint, r=smacleod
Attachment #8723233 - Flags: review?(smacleod)
Comment on attachment 8732316 [details]
MozReview Request: Bug 1230962 - Create mach/build system integration for mozlint, r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41087/diff/1-2/
Attachment #8732317 - Attachment is obsolete: true
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

https://reviewboard.mozilla.org/r/36419/#review46467

LGTM!
Attachment #8723233 - Flags: review?(smacleod) → review+
Looks like one of the tests I added fails in Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a5f24acedb

Follow-up coming shortly.
Attached file testcase.py
Maybe not so shortly. This is turning out to be super hairy thanks to windows and multiprocessing.

The reason for the previous failures was that Queue was defined globally, and since windows has no fork(), the subprocesses weren't sharing that state. The solution is to use a multiprocessing.Mananger() and pass the Queue object as an argument to the workers.

However, in this particular case doing that also fails. Presumably because I'm trying to sync a function that was defined in a dynamically created module (the payload function in the various .lint files that are loaded with imp.load_source). This testcase demonstrates what I mean more clearly.

I'm pretty stumped, this problem is niche and I can't find anyone on the internets who were trying to do the same thing. I'm leaning towards making windows run in single process mode for the time being.. which sucks.
I worked around the problem by only passing the linter path across processes, and re-parsing the linter in the subprocesses. Parsing the linter twice isn't ideal, but isn't a very big deal, and is much preferable to using a single process. Here's a passing try run on all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6992996045f9fbc6b967be5541d115ace81341f

The change is small, but not trivial.. so I'll ask for a quick review.
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36419/diff/3-4/
Comment on attachment 8732316 [details]
MozReview Request: Bug 1230962 - Create mach/build system integration for mozlint, r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41087/diff/2-3/
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

I don't know how to unset the r+ in mozreview, smacleod could you take a quick look at this interdiff?
https://reviewboard.mozilla.org/r/36419/diff/3-4/
Attachment #8723233 - Flags: review+ → review?(smacleod)
Comment on attachment 8723233 [details]
MozReview Request: Bug 1230962 - Add python/mozlint for running several linters at once, r?smacleod

https://reviewboard.mozilla.org/r/36419/#review47373

::: python/mozlint/mozlint/roller.py:86
(Diff revisions 3 - 4)
>              raise LintersNotConfigured
>  
>          if isinstance(paths, basestring):
>              paths = [paths]
>  
> -        num_jobs = len(self.linters)
> +        m = Manager()

You're not using the `Manager`. (Don't forget to remove the import)
Attachment #8723233 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/36419/#review47373

> You're not using the `Manager`. (Don't forget to remove the import)

I'm using it on the next line :). Using Manager().Queue() is needed instead of multiprocessing.Queue(), as the latter can't be passed across processes on Windows. The former spawns a little server to pass data between processes. The queue object it returns is only a proxy.
Blocks: 1270506
https://hg.mozilla.org/mozilla-central/rev/6b825d526501
https://hg.mozilla.org/mozilla-central/rev/b8c6b6769bf6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.