Currently there are 3 things that can impact the result of a lint run:
1. The list of lint issues found
2. The set of failures that happened during the setup phase
3. The set of failures that happened during the execution phase
All three of these things are stored as instance variables on the LintRoller
object, and then passed into a formatter when it comes time to print the
results. I'd like to add even more things that can impact the result, and it
became clear that the current scenario does not scale well.
This patch moves all data that could impact the end result of a lint run off of
the LintRoller object and onto a new 'result.ResultSummary' class. To avoid
confusion, this patch also renames the 'result.ResultContainer' class to
'result.Issue'.
With this new nomenclature:
result -> overall state of an entire lint run (can comprise multiple linters)
issue -> one specific lint infraction (at either 'warning' or 'error' level)
failure -> a non-recoverable error in the linter implementation itself
A "result" is comprised of 0 or more "issues" and 0 or more "failures".
Differential Revision: https://phabricator.services.mozilla.com/D3819
--HG--
extra : moz-landing-system : lando
Files returned from version control (i.e via --outgoing or --workdir), are currently joined to
cwd. This will cause failures if |mach lint| is run from anywhere other than topsrcdir.
However we *do* want to join manually specified paths to cwd so things like:
cd devtools && mach lint client
continue to work. This patch makes sure we join the proper kind of path to the proper place.
MozReview-Commit-ID: EQmRhAr3Oog
--HG--
extra : rebase_source : 2629cc27f79059e44369d46d4f8278f83923582c
Previously, using --workdir or --outgoing could miss errors when modifying a
support file since those could affect unmodified files.
This patch allows linters to define support-files in their .yml configuration.
If using --outgoing or --workdir and a file matching one of those patterns was
modified, we'll lint the entire tree.
MozReview-Commit-ID: CuGLYwQwiWr
--HG--
extra : rebase_source : 00d4107c41404f5e6ab05e0106d5cd377e25652f
There a few pieces needed here to properly handle KeyboardInterrupts.
1. All in-progress work needs to abort. Ideally the underlying linters will be
able to catch KeyboardInterrupt, and return partial results (like the flake8
linter does). Linters may alternatively allow the KeyboardInterrupt to
propagate up. Mozlint will catch and handle this appropriately, though any
results found will be lost. The only change to this behaviour was fixing a bug
in the flake8 linter.
2. Any unstarted jobs need to be canceled. In concurrent.futures, there are two
different queues. First, jobs are placed on the work queue, which is just a list
maintained by the parent process. As workers become available, jobs are moved
off the work queue, and onto the call queue (which is a multiprocessing.Queue).
Jobs that live on the work queue can be canceled with 'future.cancel()', whereas
jobs that live on the call queue cannot. The number of extra jobs that are stored
on the call queue is determined by this variable:
https://hg.mozilla.org/mozilla-central/file/deb7714a7bcd/third_party/python/futures/concurrent/futures/process.py#l86
In this patch, the parent process' sigint handler (which will be called on Ctrl-C)
is responsible for canceling all the jobs on the work queue. For the jobs on the
call queue, the best we can do is set a global variable that tells workers to
abort early.
3. Idle workers should exit gracefully. When there are no more jobs left, workers
will block on the call queue (either waiting for more jobs, or waiting for the
executor to send the shutdown signal). If a KeyboardInterrupt is received while a
worker is blocking, it isn't possible to intercept that anywhere (due to quirks
of how concurrent.futures is implemented). The InterruptableQueue class was
created to solve this problem. It will return None instead of propagating
KeyboardInterrupt. A None value will wake the worker up and tell it to gracefully
shutdown. This way, we avoid cryptic tracebacks in the output.
With all of these various pieces solved, pressing Ctrl-C appears to always exit
gracefully, sometimes even printing partial results.
MozReview-Commit-ID: 36Pe3bbUKmk
--HG--
extra : rebase_source : d4c312ee5cc3679eeee1407c5521aed679f84ad4
extra : source : a93a00141bf62f6bc9e30934c0e56f6b2e434bf0
This commit doesn't change any behaviour, just attempts to make this a little
more readable. The workers will call '_collect_results' for each WorkItem they
process (either because it is finished or because it was canceled).
This also differentiates between setup failures and run failures.
MozReview-Commit-ID: 36Pe3bbUKmk
--HG--
extra : rebase_source : 873167512b745ccdc52de7e7f1ecf66b094e063d
This allows linters to define a 'setup' method which will automatically be
called by |mach lint| before running the linter. Users can also explicitly run
these methods (without doing any actual linting) by running |mach lint --setup|.
MozReview-Commit-ID: 74aY1pfsaX1
--HG--
extra : rebase_source : e6a7d769ba14c996c7a77316928063fa46358c5a
The initial motivation for this patch, was to prevent command lines that are
too long on Windows. To that end, there is a cap to the number of paths that
can be run per job. For now that cap is set to 50. This will allow for an
average path length of 160 characters, which should be sufficient with room to
spare.
But another big benefit of this patch is that we are running more things in
parallel. Previously, mozlint ran each linter in its own subprocess, but that's
it. If running eslint is 90% of the work, it'll still only get a single
process. This means we are wasting cores as soon as the other linters are
finished.
This patch chunks the number of specified paths such that there will be N*L
jobs where 'N' is the number of cores and 'L' is the number of linters. This
means even when there's a dominant linter, we'll be making better use of our
resources. This isn't perfect of course, as some paths might contain a small
number of files, and some will contain a very large number of files. But it's
a start
A limitation to this approach is that if there are fewer paths specified than
there are cores, we won't schedule enough jobs per linter to use those extra
cores. One idea might be to expand specified directories and individually list
all the paths under the directory. But this has some hairy edge cases that
would be tough to catch. Doing this in a non-hacky way would also require a
medium scale refactor.
So I propose further parallelization efforts be destined for follow-ups.
MozReview-Commit-ID: JRRu13AFaii
--HG--
extra : rebase_source : 242fb71fe0af8bd2a981bd10a7216bb897fe00ac
The initial motivation for this patch, was to prevent command lines that are
too long on Windows. To that end, there is a cap to the number of paths that
can be run per job. For now that cap is set to 50. This will allow for an
average path length of 160 characters, which should be sufficient with room to
spare.
But another big benefit of this patch is that we are running more things in
parallel. Previously, mozlint ran each linter in its own subprocess, but that's
it. If running eslint is 90% of the work, it'll still only get a single
process. This means we are wasting cores as soon as the other linters are
finished.
This patch chunks the number of specified paths such that there will be N*L
jobs where 'N' is the number of cores and 'L' is the number of linters. This
means even when there's a dominant linter, we'll be making better use of our
resources. This isn't perfect of course, as some paths might contain a small
number of files, and some will contain a very large number of files. But it's
a start
A limitation to this approach is that if there are fewer paths specified than
there are cores, we won't schedule enough jobs per linter to use those extra
cores. One idea might be to expand specified directories and individually list
all the paths under the directory. But this has some hairy edge cases that
would be tough to catch. Doing this in a non-hacky way would also require a
medium scale refactor.
So I propose further parallelization efforts be destined for follow-ups.
MozReview-Commit-ID: JRRu13AFaii
--HG--
extra : rebase_source : 6cd73d8b6888723de3410df043f7ed042ba3349f
This switches most tests over to use pytest as the runner instead of unittest (taking
advantage of the fact that pytest can run unittest based tests).
There were a couple tests that had failures when swithing to pytest:
config/tests/unit-expandlibs.py
xpcom/idl-parser/xpidl/runtests.py
For these tests, I added a runwith='unittest' argument so that they still run the
same way as before. Once we fix them to use pytest, the unittest logic in mozunit.py
can be deleted.
MozReview-Commit-ID: Gcsz6z8MeOi
--HG--
extra : rebase_source : 3c762422ce0af54cbbe7d9fc20085a2d1ebe7057
Rather than using .lint.py files that contain a LINTER object, linter definitions are now in
standalone .yml files. In the case of external linters that need to run python code, the payload
is now of the form:
<module path>:<object path>
The <module path> is the import path to the module, and <object path> is the callable object to
use within that module. It is up to the consumer of mozlint to ensure the <module path> lives on
sys.path. For example, if an external lint's function lives in package 'foo', file 'bar.py' and
function 'lint', the payload would read:
foo.bar:lint
This mechanism was borrowed from taskcluster.
MozReview-Commit-ID: AIsfbVmozy4
--HG--
rename : python/mozlint/test/linters/badreturncode.lint.py => python/mozlint/test/linters/badreturncode.yml
rename : python/mozlint/test/linters/explicit_path.lint.py => python/mozlint/test/linters/explicit_path.yml
rename : python/mozlint/test/linters/external.lint.py => python/mozlint/test/linters/external.yml
rename : python/mozlint/test/linters/invalid_exclude.lint.py => python/mozlint/test/linters/invalid_exclude.yml
rename : python/mozlint/test/linters/invalid_extension.lnt => python/mozlint/test/linters/invalid_extension.ym
rename : python/mozlint/test/linters/invalid_include.lint.py => python/mozlint/test/linters/invalid_include.yml
rename : python/mozlint/test/linters/invalid_type.lint.py => python/mozlint/test/linters/invalid_type.yml
rename : python/mozlint/test/linters/missing_attrs.lint.py => python/mozlint/test/linters/missing_attrs.yml
rename : python/mozlint/test/linters/missing_definition.lint.py => python/mozlint/test/linters/missing_definition.yml
rename : python/mozlint/test/linters/raises.lint.py => python/mozlint/test/linters/raises.yml
rename : python/mozlint/test/linters/regex.lint.py => python/mozlint/test/linters/regex.yml
rename : python/mozlint/test/linters/string.lint.py => python/mozlint/test/linters/string.yml
rename : python/mozlint/test/linters/structured.lint.py => python/mozlint/test/linters/structured.yml
extra : rebase_source : bda3926712234123355c5af71c6453ce869b19fc
This replaces the "return_code" property on the LintRoller object with a list of "failed"
linters. This is a bit more useful as it lets us know exactly which linters had a problem
(whereas previously we just knew *something* went wrong). This patch pushes determining
the return code back into cli.py, which I think is fine.
In addition, we now pass the list of failed linters into the formatter. This allows us to
clarify exactly how many linters hit a failure which is a lot better than a seemingly
"successful" summary message.
Finally I also removed the "no files to lint" message because I've seen several people
confuse it for an error. I'll probably add it back as a debug log message when we switch
to using mozlog for output.
MozReview-Commit-ID: 4wyCeOZdOf8
--HG--
extra : rebase_source : 23810a8ab8dd9cbbad6b9e965ccff7214f947fbc
If a linter returns a status code instead of a list of results, mozlint will throw away
that status code and move on to the next linter. We should make sure that status code
bubbles all the way up to the cli.
MozReview-Commit-ID: 9fXpmtlUUT1
--HG--
extra : rebase_source : 84c80d1b24d7233d0c4c6e38b23e775110776d1f
Some linters, such as flake8, will lint invalid file extensions if you explicitly pass them in. E.g,
|flake8 foobar.js| will result in flake8 attempting to lint a JS file. This is a problem because passing
in files explicitly is exactly what the --rev/--workdir options do. If a developer modifies a JS file
then runs |mach lint -l flake8 -w|, that JS file will get linted.
To prevent this, mozlint needs to handle file extensions instead of relying on the underlying linter to
do it. This patch adds an "extensions" config option to the LINTER dict, and will filter these files out
as part of the 'filterpaths' steps.
MozReview-Commit-ID: KYhC6SEySC3
--HG--
extra : rebase_source : 6fea2942b2db1bea7deca1d6738546362b6ebd65
Previously, vcs related stuff was resolved in the cli.py module. But it's possible
for consumers to bypass the cli and instantiate a LintRoller directly. In fact this
is what the mozlint tests do.
Now that we always try to find the vcs root, calling into vcs is no longer optional.
This patch moves the VCSFiles class to a new vcs.py module and makes LintRoller
responsible for instantiating it instead of cli.py.
MozReview-Commit-ID: 5yA3gDZ1UGM
--HG--
extra : rebase_source : 96adc0ca01e8220b0432b64c96c478fc526db756
extra : source : 709c1e521ad3eb466c3434000aed7f71ebf37365
Some linters, such as flake8, will lint invalid file extensions if you explicitly pass them in. E.g,
|flake8 foobar.js| will result in flake8 attempting to lint a JS file. This is a problem because passing
in files explicitly is exactly what the --rev/--workdir options do. If a developer modifies a JS file
then runs |mach lint -l flake8 -w|, that JS file will get linted.
To prevent this, mozlint needs to handle file extensions instead of relying on the underlying linter to
do it. This patch adds an "extensions" config option to the LINTER dict, and will filter these files out
as part of the 'filterpaths' steps.
MozReview-Commit-ID: KYhC6SEySC3
--HG--
extra : rebase_source : 46807a4913660f33e691b864c2c59c448902dfa5
Mozlint provides two main benefits:
1. A common system for defining lints across multiple languages
2. A common interface and result format for running them
This commit only adds the core library, it does not add any consumers of mozlint just yet.
MozReview-Commit-ID: CSQzq5del5k
--HG--
extra : rebase_source : b520b96177281a1b1770edf53a01cbc2196f494f
Andrew Halberstadt
Gurzau Raul
Steve Armand
Mark Banner
Iris Hsiao