Flake8 ignores the 'exclude' section of the .flake8.yml if you pass in a direct
path to a file. To get around this we have some custom logic to handle these
exclusions for us, but this custom logic didn't account for globs.
Differential Revision: https://phabricator.services.mozilla.com/D23145
--HG--
extra : moz-landing-system : lando
The motivations for this are:
1) Apply global excludes. This merges the exclusion rules defined in
tools/lint/mach_commands.py with the ones in .flake8.
2) Improve performance. Switching to a blacklist will result in a much longer
runtime for linting the entire tree and flake8 handles exclusions incredibly
slowly. Without this patch (and the blacklist change applied), I gave up
waiting after 30 minutes. With this patch, it takes 30 seconds.
Depends on D20495
Differential Revision: https://phabricator.services.mozilla.com/D20496
--HG--
rename : tools/lint/test/files/flake8/bad.py => tools/lint/test/files/flake8/subdir/exclude/bad.py
extra : moz-landing-system : lando
This will be re-used by the flake8 linter, so moving it into mozlint for
re-useability.
Depends on D20493
Differential Revision: https://phabricator.services.mozilla.com/D20494
--HG--
extra : moz-landing-system : lando
The following python-test paths are being moved out of 'make check' and into their own task:
- python/mozlint
- testing/mozbase
- tools/lint
The following python-test paths previously did not run on Windows:
- python/mozterm
- testing/marionette
- testing/raptor
- tools/tryselect
Differential Revision: https://phabricator.services.mozilla.com/D10759
--HG--
extra : moz-landing-system : lando
The following python-test paths are being moved out of 'make check' and into their own task:
- python/mozlint
- testing/mozbase
- tools/lint
The following python-test paths previously did not run on Windows:
- python/mozterm
- testing/marionette
- testing/raptor
- tools/tryselect
MozReview-Commit-ID: C07FANaYzf7
Depends on D10758
Differential Revision: https://phabricator.services.mozilla.com/D10759
--HG--
extra : moz-landing-system : lando
This assumes that a lineno of 0 denotes a "file-level" issue, e.g an issue
associated with the filename. In the future it might be better to treat these
"file-level" issues as first class citizens. This would involve updating things
like formatters, editor integrations and review bot to not assume a lineno.
Differential Revision: https://phabricator.services.mozilla.com/D10383
--HG--
extra : moz-landing-system : lando
We were currently adding exclude paths to the "discard" set if the path contains
the include, but we weren't adding them if the include contains the path.
Depends on D5863
Differential Revision: https://phabricator.services.mozilla.com/D8845
--HG--
extra : moz-landing-system : lando
Right now there are excludes defined in the linter definition (via the .yml
files), as well as excludes defined in lintargs (via the mach_commands.py).
This is a minor simplification that extends each linter definition's local
excludes with the global ones right off the bat. This just makes it a bit
easier to keep track of.
Depends on D5863
Differential Revision: https://phabricator.services.mozilla.com/D8844
--HG--
extra : moz-landing-system : lando
Currently pathutils.filterpaths computes some extra paths that the underlying linter
should try to exclude if it can. It sets these on lintargs['exclude'], and relies on
the fact that it was passed by reference to work.
However, it seems like pytest does some magic under the hood that prevents this value
from being propagated back. It will also fail if 'filterpaths' was invoked in a
subprocess.
It's much simpler and cleaner to pass this value back directly, rather than rely on a
reference.
Differential Revision: https://phabricator.services.mozilla.com/D8850
--HG--
extra : moz-landing-system : lando
This fixes a latent bug that is currently not being hit (by sheer luck). Basically
the 'ignore' argument of a FileFinder object needs all paths to be relative to the
base. As luck would have it, most of the time it worked out that way if you were
running |mach lint| from the root of the repo.
However there are edge cases where this will cause an 'exclude' directive to get
missed. Plus this bug is about to be exposed 100% of the time in the next commit :).
Depends on D8842
Differential Revision: https://phabricator.services.mozilla.com/D8843
--HG--
extra : moz-landing-system : lando
I missed an edge case where the computed base itself was specified in the
paths input.
Differential Revision: https://phabricator.services.mozilla.com/D8842
--HG--
extra : moz-landing-system : lando
Often we specify globs in our exclude patterns, e.g:
exclude:
- **/node_modules
- obj*
However, these globs get expanded out to *every* file that matches them. This
can sometimes be thousands or even tens of thousands of files.
We then pass these paths on to the underlying linters and tell them to
exclude them all. This causes a lot of overhead and slows down performance.
This commit implements a "collapse" function. Given a set of paths, it'll
collapse them into the smallest set of parent directories that contain the
original set, and that don't contain any extra files.
For example, given a directory structure like this:
a
-- foo.txt
-- b
-- bar.txt
-- baz.txt
-- c
-- ham.txt
-- d
-- spam.txt
Then the following will happen:
>>> collapse(['a/foo.txt', 'a/b/bar.txt', 'a/c/ham.txt', 'a/c/d/spam.txt'])
['a/foo.txt', 'b/bar.txt', 'c']
Since all files under directory 'c' are specified by the original set (both
'c/ham.txt' and 'c/d/spam.txt'), we can collapse it down to just 'c'. However
not all files under 'b' are specified (we're missing 'a/b/baz.txt'), so we
can't collapse 'b' (and by extension also can't collapse 'a').
If we had included 'a/b/baz.txt':
>>> collapse(['a/foo.txt', 'a/b/bar.txt', 'a/b/baz.txt', 'a/c/ham.txt', 'a/c/d/spam.txt'])
['a']
In both cases, the smallest set of paths that contains the original set (and
only the original set) is computed.
The collapse function has a little bit of overhead but it's not too bad.
For example collapsing all files matched by '**/node_modules' takes ~0.015s.
Collapsing two full objdirs, takes ~0.6s. But a follow up commit is planned to
make sure we stop using 'obj*' to reduce that overhead.
Depends on D7738
Differential Revision: https://phabricator.services.mozilla.com/D7739
--HG--
extra : moz-landing-system : lando
If scandir is already present on the system the attempt to import the
c helper library will currently find the c helper from the system
install which may well be an outdated verion, so causing mach to
break. To solve this this patch does two things:
* Stops importing scandir in files that are run unconditionally when
invoking mach. This is generally considered good for performance
reasons.
* Installs the vendored scandir into the virtualenv for `mach lint`
rather than trying to import it directly from the source tree and so
not getting the c helper library.
Differential Revision: https://phabricator.services.mozilla.com/D8379
--HG--
extra : moz-landing-system : lando
Often we specify globs in our exclude patterns, e.g:
exclude:
- **/node_modules
- obj*
However, these globs get expanded out to *every* file that matches them. This
can sometimes be thousands or even tens of thousands of files.
We then pass these paths on to the underlying linters and tell them to
exclude them all. This causes a lot of overhead and slows down performance.
This commit implements a "collapse" function. Given a set of paths, it'll
collapse them into the smallest set of parent directories that contain the
original set, and that don't contain any extra files.
For example, given a directory structure like this:
a
-- foo.txt
-- b
-- bar.txt
-- baz.txt
-- c
-- ham.txt
-- d
-- spam.txt
Then the following will happen:
>>> collapse(['a/foo.txt', 'a/b/bar.txt', 'a/c/ham.txt', 'a/c/d/spam.txt'])
['a/foo.txt', 'b/bar.txt', 'c']
Since all files under directory 'c' are specified by the original set (both
'c/ham.txt' and 'c/d/spam.txt'), we can collapse it down to just 'c'. However
not all files under 'b' are specified (we're missing 'a/b/baz.txt'), so we
can't collapse 'b' (and by extension also can't collapse 'a').
If we had included 'a/b/baz.txt':
>>> collapse(['a/foo.txt', 'a/b/bar.txt', 'a/b/baz.txt', 'a/c/ham.txt', 'a/c/d/spam.txt'])
['a']
In both cases, the smallest set of paths that contains the original set (and
only the original set) is computed.
The collapse function has a little bit of overhead but it's not too bad.
For example collapsing all files matched by '**/node_modules' takes ~0.015s.
Collapsing two full objdirs, takes ~0.6s. But a follow up commit is planned to
make sure we stop using 'obj*' to reduce that overhead.
Depends on D7738
Differential Revision: https://phabricator.services.mozilla.com/D7739
--HG--
extra : moz-landing-system : lando
There is only a single linter (test-disable.yml) that uses a glob in any
include path, and that usage is easily replaced by using the 'extensions' key
instead.
Since globs in include directives aren't very useful, let's disallow them. This
will allow us to simplify the 'filterpaths' logic quite substantially and make
future refactorings in this area easier.
Differential Revision: https://phabricator.services.mozilla.com/D6798
--HG--
extra : moz-landing-system : lando
This makes things more explicit. Previously we were relying on those magic
global "linters" variables, and it turned out that one of the tests was
actually linting a completely different set of linters than I was expecting.
This changes things so each test needs to explicitly define which linters it
wants to use.
Depends on D6410
Differential Revision: https://phabricator.services.mozilla.com/D6412
--HG--
extra : moz-landing-system : lando
This makes this test match all the other tests (which are named after the module
they are testing).
Also rename the test function to 'test_filterpaths'.
Differential Revision: https://phabricator.services.mozilla.com/D6410
--HG--
rename : python/mozlint/test/test_filterpaths.py => python/mozlint/test/test_pathutils.py
extra : moz-landing-system : lando
This will make it easier to add new test cases in the future. It'll
also make it easier to print debug, as only the output for each
specific test case will be printed on failure.
Differential Revision: https://phabricator.services.mozilla.com/D5862
--HG--
extra : moz-landing-system : lando
After fixing the absolute path issue in codespell, I noticed that the stylish
formatter doesn't indent lint issues that don't have a column properly. This
was never noticed before since most other linters have a column attribute.
Depends on D4012
Differential Revision: https://phabricator.services.mozilla.com/D4013
--HG--
extra : moz-landing-system : lando
As of this patch, any lint issue at the "warning" level will *only* be displayed
if --warnings is passed in. This gives us some flexibility to track issues that
are "recommended to fix" but that aren't required (aka don't cause a backout).
I think it would be ideal if the reviewbot ran with warnings enabled, and CI
ran without warnings. This way these issues will be surfaced to developers
(and hopefully get fixed prior to landing), but developers will always be able
to ignore them if the situation warrants it.
Since the last change converted everything to use errors, this patch should
be a no-op for now. However as we move forward (and potentially decide that
certain types of lint issues should result in a backout), this feature will
start seeing more and more use.
Depends on D3820
Differential Revision: https://phabricator.services.mozilla.com/D3821
--HG--
extra : moz-landing-system : lando
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
This will make sure that when running |mach python-test --python 3| locally,
we only run the tests that also run in CI with python 3 (and therefore pass
presumably).
MozReview-Commit-ID: 3OBr9yLSlSq
--HG--
extra : rebase_source : 456340d0ecdddf1078f2b5b4ebb1eddf3813b26a
yaml.load() is unsafe and can lead to arbitrary code execution via
syntax like `!!python/object/apply:os.system`. yaml.safe_load() is
more reasonable.
Differential Revision: https://phabricator.services.mozilla.com/D1738
--HG--
extra : rebase_source : 597c07b3c1538dc27ad6f46e01cdb7f48755d0bc
extra : histedit_source : 131d570f8ac1ee047487cba54822dbf20abf6681
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
Since I left the next two patches to bitrot, I realized that a path I had added
didn't exist anymore. We should definitely error out if non-existant paths are
specified, otherwise the lists will become outdated and it will be possible to
accidentally disable linting on some files.
I discovered a few instances of this already in our existing definitions.
MozReview-Commit-ID: 8jsTKLI0nFE
--HG--
extra : rebase_source : acceb0b129fc472fb456ff527e4c8c52228edd59
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 formatter is useful for triaging paths when enabling new linters or
expanding existing ones. It works well with the -n/--no-filter option.
For example, if I wanted to look for candidates of new directories to enable
the codespell linter on, I could run:
./mach lint -l codespell -nf summary
This will print something like:
accessible: 429
dom: 142
layout: 15
testing: 53
etc..
If desired, you can also specify a depth by setting MOZLINT_SUMMARY_DEPTH. A
depth of 2 means results will be aggregated further down, e.g:
accesible/build: 129
accesible/ipc: 300
dom/indexedDB: 100
dom/workers: 42
etc..
The depth is always relative to the common path prefix of all results, so
running:
./mach lint -l codespell -nf python/mozbuild
Would expand all the directories under python/mozbuild (not topsrdir).
MozReview-Commit-ID: OiihLTpULA
--HG--
extra : rebase_source : eaaabc1d5cdc8e3942808d01b24e22081fea752e
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
Sylvestre Ledru
Robert Bartlensky
Bastien Abadie
Andrew Halberstadt
Axel Hecht
Justin Wood
Andreea Pavel
Csoregi Natalia
Narcis Beleuzu
James Graham
Gregory Szorc
Dorel Luca