Bug 1373368 - [mozlint] Lint whole tree if using --workdir/--outgoing and support-file was modified, r?Standard8
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
--- a/python/mozlint/mozlint/parser.py
+++ b/python/mozlint/mozlint/parser.py
@@ -37,17 +37,17 @@ class Parser(object):
if missing_attrs:
raise LinterParseError(relpath, "Missing required attribute(s): "
"{}".format(','.join(missing_attrs)))
if linter['type'] not in supported_types:
raise LinterParseError(relpath, "Invalid type '{}'".format(linter['type']))
- for attr in ('include', 'exclude'):
+ for attr in ('include', 'exclude', 'support-files'):
if attr not in linter:
continue
if not isinstance(linter[attr], list) or \
not all(isinstance(a, basestring) for a in linter[attr]):
raise LinterParseError(relpath, "The {} directive must be a "
"list of strings!".format(attr))
invalid_paths = set()
@@ -95,10 +95,11 @@ class Parser(object):
if not config:
raise LinterParseError(path, "No lint definitions found!")
linters = []
for name, linter in config.iteritems():
linter['name'] = name
linter['path'] = path
self._validate(linter)
+ linter.setdefault('support-files', []).append(path)
linters.append(linter)
return linters
--- a/python/mozlint/mozlint/roller.py
+++ b/python/mozlint/mozlint/roller.py
@@ -10,16 +10,17 @@ import sys
import traceback
from collections import defaultdict
from concurrent.futures import ProcessPoolExecutor
from math import ceil
from multiprocessing import cpu_count
from multiprocessing.queues import Queue
from subprocess import CalledProcessError
+import mozpack.path as mozpath
from mozversioncontrol import get_repository_object, MissingUpstreamRepo, InvalidRepoPath
from .errors import LintersNotConfigured
from .parser import Parser
from .pathutils import findobject
from .types import supported_types
SHUTDOWN = False
@@ -139,23 +140,35 @@ class LintRoller(object):
if self.failed_setup:
print("error: problem with lint setup, skipping {}".format(
', '.join(sorted(self.failed_setup))))
self.linters = [l for l in self.linters if l['name'] not in self.failed_setup]
return 1
return 0
- def _generate_jobs(self, paths, num_procs):
+ def _generate_jobs(self, paths, vcs_paths, num_procs):
"""A job is of the form (<linter:dict>, <paths:list>)."""
- chunk_size = min(self.MAX_PATHS_PER_JOB, int(ceil(float(len(paths)) / num_procs)))
- while paths:
- for linter in self.linters:
- yield linter, paths[:chunk_size]
- paths = paths[chunk_size:]
+ for linter in self.linters:
+ if any(os.path.isfile(p) and mozpath.match(p, pattern)
+ for pattern in linter.get('support-files', []) for p in vcs_paths):
+ lpaths = [self.root]
+ print("warning: {} support-file modified, linting entire tree "
+ "(press ctrl-c to cancel)".format(linter['name']))
+ else:
+ lpaths = paths.union(vcs_paths)
+
+ lpaths = lpaths or ['.']
+ lpaths = map(os.path.abspath, lpaths)
+ chunk_size = min(self.MAX_PATHS_PER_JOB, int(ceil(len(lpaths) / num_procs))) or 1
+ assert chunk_size > 0
+
+ while lpaths:
+ yield linter, lpaths[:chunk_size]
+ lpaths = lpaths[chunk_size:]
def _collect_results(self, future):
if future.cancelled():
return
results, failed = future.result()
if failed:
self.failed.update(set(failed))
@@ -187,44 +200,39 @@ class LintRoller(object):
elif isinstance(paths, (list, tuple)):
paths = set(paths)
if not self.vcs and (workdir or outgoing):
print("error: '{}' is not a known repository, can't use "
"--workdir or --outgoing".format(self.lintargs['root']))
# Calculate files from VCS
+ vcs_paths = set()
try:
if workdir:
- paths.update(self.vcs.get_changed_files('AM', mode=workdir))
+ vcs_paths.update(self.vcs.get_changed_files('AM', mode=workdir))
if outgoing:
try:
- paths.update(self.vcs.get_outgoing_files('AM', upstream=outgoing))
+ vcs_paths.update(self.vcs.get_outgoing_files('AM', upstream=outgoing))
except MissingUpstreamRepo:
print("warning: could not find default push, specify a remote for --outgoing")
except CalledProcessError as e:
print("error running: {}".format(' '.join(e.cmd)))
if e.output:
print(e.output)
- if not paths and (workdir or outgoing):
+ if not (paths or vcs_paths) and (workdir or outgoing):
print("warning: no files linted")
return {}
- paths = paths or ['.']
-
- # This will convert paths back to a list, but that's ok since
- # we're done adding to it.
- paths = map(os.path.abspath, paths)
-
num_procs = num_procs or cpu_count()
- jobs = list(self._generate_jobs(paths, num_procs))
+ jobs = list(self._generate_jobs(paths, vcs_paths, num_procs))
# Make sure we never spawn more processes than we have jobs.
- num_procs = min(len(jobs), num_procs)
+ num_procs = min(len(jobs), num_procs) or 1
signal.signal(signal.SIGINT, _worker_sigint_handler)
executor = ProcessPoolExecutor(num_procs)
executor._call_queue = InterruptableQueue(executor._call_queue._maxsize)
# Submit jobs to the worker pool. The _collect_results method will be
# called when a job is finished. We store the futures so that they can
# be canceled in the event of a KeyboardInterrupt.
--- a/python/mozlint/test/conftest.py
+++ b/python/mozlint/test/conftest.py
@@ -1,29 +1,42 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# 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/.
from __future__ import absolute_import
import os
import sys
+from argparse import Namespace
import pytest
from mozlint import LintRoller
here = os.path.abspath(os.path.dirname(__file__))
@pytest.fixture
def lint(request):
lintargs = getattr(request.module, 'lintargs', {})
- return LintRoller(root=here, **lintargs)
+ lint = LintRoller(root=here, **lintargs)
+
+ # Add a few super powers to our lint instance
+ def mock_vcs(files):
+ def _fake_vcs_files(*args, **kwargs):
+ return files
+
+ setattr(lint.vcs, 'get_changed_files', _fake_vcs_files)
+ setattr(lint.vcs, 'get_outgoing_files', _fake_vcs_files)
+
+ setattr(lint, 'vcs', Namespace())
+ setattr(lint, 'mock_vcs', mock_vcs)
+ return lint
@pytest.fixture(scope='session')
def filedir():
return os.path.join(here, 'files')
@pytest.fixture(scope='module')
--- a/python/mozlint/test/linters/external.py
+++ b/python/mozlint/test/linters/external.py
@@ -51,16 +51,20 @@ def structured(files, config, logger, **
for i, line in enumerate(fh.readlines()):
if 'foobar' in line:
logger.lint_error(path=path,
lineno=i+1,
column=1,
rule="no-foobar")
+def passes(files, config, **lintargs):
+ return []
+
+
def setup(root):
print('setup passed')
def setupfailed(root):
print('setup failed')
return 1
new file mode 100644
--- /dev/null
+++ b/python/mozlint/test/linters/invalid_support_files.yml
@@ -0,0 +1,6 @@
+---
+BadSupportFilesLinter:
+ description: Has an invalid support files directive.
+ support-files: should be a list
+ type: string
+ payload: foobar
new file mode 100644
--- /dev/null
+++ b/python/mozlint/test/linters/support_files.yml
@@ -0,0 +1,10 @@
+---
+SupportFilesLinter:
+ description: Linter that has a few support files
+ include:
+ - files
+ support-files:
+ - '**/*.py'
+ type: external
+ extensions: ['.js', '.jsm']
+ payload: external:passes
--- a/python/mozlint/test/test_parser.py
+++ b/python/mozlint/test/test_parser.py
@@ -43,16 +43,17 @@ def test_parse_valid_linter(parse):
assert set(lintobj['extensions']) == set(['js', 'jsm'])
@pytest.mark.parametrize('linter', [
'invalid_type.yml',
'invalid_extension.ym',
'invalid_include.yml',
'invalid_exclude.yml',
+ 'invalid_support_files.yml',
'missing_attrs.yml',
'missing_definition.yml',
'non_existing_include.yml',
'non_existing_exclude.yml',
'non_existing_support_files.yml',
])
def test_parse_invalid_linter(parse, linter):
with pytest.raises(LinterParseError):
--- a/python/mozlint/test/test_roller.py
+++ b/python/mozlint/test/test_roller.py
@@ -133,16 +133,54 @@ def test_keyboard_interrupt():
time.sleep(1)
proc.send_signal(signal.SIGINT)
out = proc.communicate()[0]
assert 'warning: not all files were linted' in out
assert 'Traceback' not in out
+def test_support_files(lint, lintdir, filedir, monkeypatch):
+ jobs = []
+
+ # Replace the original _generate_jobs with a new one that simply
+ # adds jobs to a list (and then doesn't return anything).
+ orig_generate_jobs = lint._generate_jobs
+
+ def fake_generate_jobs(*args, **kwargs):
+ jobs.extend([job[1] for job in orig_generate_jobs(*args, **kwargs)])
+ return []
+
+ monkeypatch.setattr(lint, '_generate_jobs', fake_generate_jobs)
+
+ linter_path = os.path.join(lintdir, 'support_files.yml')
+ lint.read(linter_path)
+
+ # Modified support files only lint entire root if --outgoing or --workdir
+ # are used.
+ path = os.path.join(filedir, 'foobar.js')
+ lint.mock_vcs([os.path.join(filedir, 'foobar.py')])
+ lint.roll(path)
+ assert jobs[0] == [path]
+
+ jobs = []
+ lint.roll(path, workdir=True)
+ assert jobs[0] == [lint.root]
+
+ jobs = []
+ lint.roll(path, outgoing=True)
+ assert jobs[0] == [lint.root]
+
+ # Lint config file is implicitly added as a support file
+ lint.mock_vcs([linter_path])
+ jobs = []
+ lint.roll(path, outgoing=True, workdir=True)
+ assert jobs[0] == [lint.root]
+
+
linters = ('setup.yml', 'setupfailed.yml', 'setupraised.yml')
def test_setup(lint, linters, filedir, capfd):
with pytest.raises(LintersNotConfigured):
lint.setup()
lint.read(linters)
--- a/tools/lint/docs/create.rst
+++ b/tools/lint/docs/create.rst
@@ -74,16 +74,17 @@ Each ``.yml`` file must have at least on
* description - A brief description of the linter's purpose (required)
* type - One of 'string', 'regex' or 'external' (required)
* payload - The actual linting logic, depends on the type (required)
* include - A list of glob patterns that must be matched (optional)
* exclude - A list of glob patterns that must not be matched (optional)
* extensions - A list of file extensions to be considered (optional)
* setup - A function that sets up external dependencies (optional)
+* support-files - A list of glob patterns matching configuration files (optional)
In addition to the above, some ``.yml`` files correspond to a single lint rule. For these, the
following additional keys may be specified:
* message - A string to print on infraction (optional)
* hint - A string with a clue on how to fix the infraction (optional)
* rule - An id string for the lint rule (optional)
* level - The severity of the infraction, either 'error' or 'warning' (optional)
@@ -163,23 +164,30 @@ Now here is the linter definition that w
.. code-block:: yml
flake8:
description: Python linter
include:
- '**/*.py'
type: external
payload: py.flake8:lint
+ support-files:
+ - '**/.flake8'
Notice the payload has two parts, delimited by ':'. The first is the module
path, which ``mozlint`` will attempt to import. The second is the object path
within that module (e.g, the name of a function to call). It is up to consumers
of ``mozlint`` to ensure the module is in ``sys.path``. Structured log linters
use the same import mechanism.
+The ``support-files`` key is used to list configuration files or files related
+to the running of the linter itself. If using ``--outgoing`` or ``--workdir``
+and one of these files was modified, the entire tree will be linted instead of
+just the modified files.
+
Bootstrapping Dependencies
--------------------------
Many linters, especially 3rd party ones, will require a set of dependencies. It
could be as simple as installing a binary from a package manager, or as
complicated as pulling a whole graph of tools, plugins and their dependencies.