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

--HG--
extra : rebase_source : 00d4107c41404f5e6ab05e0106d5cd377e25652f

python/mozlint/mozlint/parser.py

@@ -42,7 +42,7 @@ class Parser(object):
         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
 
@@ -100,5 +100,6 @@ class Parser(object):
             linter['name'] = name
             linter['path'] = path
             self._validate(linter)
+            linter.setdefault('support-files', []).append(path)
             linters.append(linter)
         return linters

python/mozlint/mozlint/roller.py

@@ -15,6 +15,7 @@ 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
@@ -144,13 +145,25 @@ class LintRoller(object):
             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():
@@ -192,12 +205,13 @@ class LintRoller(object):
                   "--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:
@@ -205,21 +219,15 @@ class LintRoller(object):
             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)

python/mozlint/test/conftest.py

@@ -6,6 +6,7 @@ from __future__ import absolute_import
 
 import os
 import sys
+from argparse import Namespace
 
 import pytest
 
@@ -18,7 +19,19 @@ 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')

python/mozlint/test/linters/external.py

@@ -56,6 +56,10 @@ def structured(files, config, logger, **kwargs):
                                       rule="no-foobar")
 
 
+def passes(files, config, **lintargs):
+    return []
+
+
 def setup(root):
     print('setup passed')
 

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

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

python/mozlint/test/test_parser.py

@@ -48,6 +48,7 @@ def test_parse_valid_linter(parse):
     'invalid_extension.ym',
     'invalid_include.yml',
     'invalid_exclude.yml',
+    'invalid_support_files.yml',
     'missing_attrs.yml',
     'missing_definition.yml',
     'non_existing_include.yml',

python/mozlint/test/test_roller.py

@@ -138,6 +138,44 @@ def test_keyboard_interrupt():
     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')
 
 

tools/lint/docs/create.rst

@@ -79,6 +79,7 @@ Each ``.yml`` file must have at least one linter defined in it. Here are the sup
 * 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:
@@ -168,6 +169,8 @@ Now here is the linter definition that would call it:
             - '**/*.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
@@ -175,6 +178,11 @@ 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
 --------------------------