Bug 1373368 - [mozlint] Lint whole tree if using --workdir/--outgoing and support-file was modified, r?Standard8 draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Fri, 16 Feb 2018 17:46:04 -0500
changeset 778814 9c1b9925a59d255453f8f24ba963b40342d3e010
parent 778813 1ad59ef89046d1a1697597dc6d489290fa546a27
child 778815 203cc38f61cbb766dc98856880f2e6561042f514
push id105585
push userahalberstadt@mozilla.com
push dateFri, 06 Apr 2018 21:49:25 +0000
reviewersStandard8
bugs1373368
milestone61.0a1
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
python/mozlint/mozlint/parser.py
python/mozlint/mozlint/roller.py
python/mozlint/test/conftest.py
python/mozlint/test/linters/external.py
python/mozlint/test/linters/invalid_support_files.yml
python/mozlint/test/linters/support_files.yml
python/mozlint/test/test_parser.py
python/mozlint/test/test_roller.py
tools/lint/docs/create.rst
--- 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.