Bug 1429457 - [mozlint] Create formal 'setup' mechanism for bootstrapping lint dependencies, r=gbrown

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

python/mozlint/mozlint/cli.py

@@ -78,6 +78,11 @@ class MozlintParser(ArgumentParser):
           'help': "Each file containing lint errors will be opened in $EDITOR one after "
                   "the other."
           }],
+        [['--setup'],
+         {'action': 'store_true',
+          'default': False,
+          'help': "Bootstrap linter dependencies without running any of the linters."
+          }],
         [['extra_args'],
          {'nargs': REMAINDER,
           'help': "Extra arguments that will be forwarded to the underlying linter.",
@@ -139,7 +144,8 @@ def find_linters(linters=None):
     return lints
 
 
-def run(paths, linters, fmt, outgoing, workdir, edit, list_linters=None, **lintargs):
+def run(paths, linters, fmt, outgoing, workdir, edit,
+        setup=False, list_linters=False, **lintargs):
     from mozlint import LintRoller, formatters
     from mozlint.editor import edit_results
 
@@ -153,6 +159,11 @@ def run(paths, linters, fmt, outgoing, workdir, edit, list_linters=None, **linta
     lint = LintRoller(**lintargs)
     lint.read(find_linters(linters))
 
+    # Always run bootstrapping, but return early if --setup was passed in.
+    ret = lint.setup()
+    if setup:
+        return ret
+
     # run all linters
     results = lint.roll(paths, outgoing=outgoing, workdir=workdir)
 

python/mozlint/mozlint/parser.py

@@ -8,8 +8,8 @@ import os
 
 import yaml
 
-from .types import supported_types
 from .errors import LinterNotFound, LinterParseError
+from .types import supported_types
 
 
 class Parser(object):
@@ -43,6 +43,12 @@ class Parser(object):
                 raise LinterParseError(linter['path'], "The {} directive must be a "
                                                        "list of strings!".format(attr))
 
+        if 'setup' in linter:
+            if linter['setup'].count(':') != 1:
+                raise LinterParseError(linter['path'], "The setup attribute '{!r}' must have the "
+                                                       "form 'module:object'".format(
+                                                           linter['setup']))
+
         if 'extensions' in linter:
             linter['extensions'] = [e.strip('.') for e in linter['extensions']]
 

python/mozlint/mozlint/roller.py

@@ -18,6 +18,7 @@ from mozversioncontrol import get_repository_object, MissingUpstreamRepo, Invali
 
 from .errors import LintersNotConfigured
 from .parser import Parser
+from .pathutils import findobject
 from .types import supported_types
 
 
@@ -71,7 +72,8 @@ class LintRoller(object):
         self.lintargs['root'] = root
 
         # linters that return non-zero
-        self.failed = None
+        self.failed = set()
+        self.root = root
 
     def read(self, paths):
         """Parse one or more linters and add them to the registry.
@@ -84,6 +86,32 @@ class LintRoller(object):
         for path in paths:
             self.linters.extend(self.parse(path))
 
+    def setup(self):
+        """Run setup for applicable linters"""
+        if not self.linters:
+            raise LintersNotConfigured
+
+        failed = set()
+        for linter in self.linters:
+            if 'setup' not in linter:
+                continue
+
+            try:
+                res = findobject(linter['setup'])(self.root)
+            except Exception:
+                traceback.print_exc()
+                res = 1
+
+            if res:
+                failed.add(linter['name'])
+
+        if failed:
+            print("error: problem with lint setup, skipping {}".format(', '.join(sorted(failed))))
+            self.linters = [l for l in self.linters if l['name'] not in failed]
+            self.failed.update(failed)
+            return 1
+        return 0
+
     def _generate_jobs(self, 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)))
@@ -102,6 +130,9 @@ class LintRoller(object):
         :return: A dictionary with file names as the key, and a list of
                  :class:`~result.ResultContainer`s as the value.
         """
+        if not self.linters:
+            raise LintersNotConfigured
+
         # Need to use a set in case vcs operations specify the same file
         # more than once.
         paths = paths or set()
@@ -110,9 +141,6 @@ class LintRoller(object):
         elif isinstance(paths, (list, tuple)):
             paths = set(paths)
 
-        if not self.linters:
-            raise LintersNotConfigured
-
         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']))
@@ -149,11 +177,10 @@ class LintRoller(object):
             # ignore SIGINT in parent so we can still get partial results
             # from child processes. These should shutdown quickly anyway.
             orig_sigint = signal.signal(signal.SIGINT, signal.SIG_IGN)
-            self.failed = []
             for future in futures:
                 results, failed = future.result()
                 if failed:
-                    self.failed.extend(failed)
+                    self.failed.update(set(failed))
                 for k, v in results.iteritems():
                     all_results[k].extend(v)
 

python/mozlint/test/conftest.py

@@ -42,6 +42,6 @@ def lintdir():
 
 @pytest.fixture(scope='module')
 def linters(lintdir, request):
-    suffix_filter = getattr(request.module, 'linters', ['.lint.py'])
+    suffix_filter = getattr(request.module, 'linters', ['.yml'])
     return [os.path.join(lintdir, p) for p in os.listdir(lintdir)
             if any(p.endswith(suffix) for suffix in suffix_filter)]

python/mozlint/test/linters/external.py

@@ -2,7 +2,7 @@
 # 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
+from __future__ import absolute_import, print_function
 
 import os
 
@@ -48,3 +48,17 @@ def structured(files, config, logger, **kwargs):
                                       lineno=i+1,
                                       column=1,
                                       rule="no-foobar")
+
+
+def setup(root):
+    print('setup passed')
+
+
+def setupfailed(root):
+    print('setup failed')
+    return 1
+
+
+def setupraised(root):
+    print('setup raised')
+    raise LintException('oh no setup failed')

python/mozlint/test/linters/setup.yml

@@ -0,0 +1,9 @@
+---
+SetupLinter:
+    description: It's bad to have the string foobar in js files.
+    include:
+        - files
+    type: external
+    extensions: ['.js', '.jsm']
+    payload: external:external
+    setup: external:setup

python/mozlint/test/linters/setupfailed.yml

@@ -0,0 +1,9 @@
+---
+SetupFailedLinter:
+    description: It's bad to have the string foobar in js files.
+    include:
+        - files
+    type: external
+    extensions: ['.js', '.jsm']
+    payload: external:external
+    setup: external:setupfailed

python/mozlint/test/linters/setupraised.yml

@@ -0,0 +1,9 @@
+---
+SetupRaisedLinter:
+    description: It's bad to have the string foobar in js files.
+    include:
+        - files
+    type: external
+    extensions: ['.js', '.jsm']
+    payload: external:external
+    setup: external:setupraised

python/mozlint/test/test_cli.py

@@ -59,5 +59,23 @@ def test_cli_run_with_edit(run, parser, capfd):
         parser.parse_args(['--edit'])
 
 
+def test_cli_run_with_setup(run, capfd):
+    # implicitly call setup
+    ret = run(['-l', 'setup', '-l', 'setupfailed', '-l', 'setupraised'])
+    out, err = capfd.readouterr()
+    assert 'setup passed' in out
+    assert 'setup failed' in out
+    assert 'setup raised' in out
+    assert ret == 1
+
+    # explicitly call setup
+    ret = run(['-l', 'setup', '-l', 'setupfailed', '-l', 'setupraised', '--setup'])
+    out, err = capfd.readouterr()
+    assert 'setup passed' in out
+    assert 'setup failed' in out
+    assert 'setup raised' in out
+    assert ret == 1
+
+
 if __name__ == '__main__':
     mozunit.main()

python/mozlint/test/test_roller.py

@@ -31,7 +31,7 @@ def test_roll_successful(lint, linters, files):
 
     result = lint.roll(files)
     assert len(result) == 1
-    assert lint.failed == []
+    assert lint.failed == set([])
 
     path = result.keys()[0]
     assert os.path.basename(path) == 'foobar.js'
@@ -63,23 +63,23 @@ def test_roll_with_excluded_path(lint, linters, files):
     result = lint.roll(files)
 
     assert len(result) == 0
-    assert lint.failed == []
+    assert lint.failed == set([])
 
 
 def test_roll_with_invalid_extension(lint, lintdir, filedir):
     lint.read(os.path.join(lintdir, 'external.yml'))
     result = lint.roll(os.path.join(filedir, 'foobar.py'))
     assert len(result) == 0
-    assert lint.failed == []
+    assert lint.failed == set([])
 
 
 def test_roll_with_failure_code(lint, lintdir, files):
     lint.read(os.path.join(lintdir, 'badreturncode.yml'))
 
-    assert lint.failed is None
+    assert lint.failed == set([])
     result = lint.roll(files, num_procs=1)
     assert len(result) == 0
-    assert lint.failed == ['BadReturnCodeLinter']
+    assert lint.failed == set(['BadReturnCodeLinter'])
 
 
 def fake_run_linters(config, paths, **lintargs):
@@ -119,5 +119,22 @@ def test_max_paths_per_job(monkeypatch, lint, linters, files, max_paths, expecte
     assert num_jobs == expected_jobs
 
 
+linters = ('setup.yml', 'setupfailed.yml', 'setupraised.yml')
+
+
+def test_setup(lint, linters, filedir, capfd):
+    with pytest.raises(LintersNotConfigured):
+        lint.setup()
+
+    lint.read(linters)
+    lint.setup()
+    out, err = capfd.readouterr()
+    assert 'setup passed' in out
+    assert 'setup failed' in out
+    assert 'setup raised' in out
+    assert 'error: problem with lint setup, skipping' in out
+    assert lint.failed == set(['SetupFailedLinter', 'SetupRaisedLinter'])
+
+
 if __name__ == '__main__':
     mozunit.main()

tools/lint/docs/create.rst

@@ -160,16 +160,60 @@ let's call the file ``flake8_lint.py``:
 
 Now here is the linter definition that would call it:
 
-.. code-block::
+.. code-block:: yml
 
     flake8:
         description: Python linter
         include:
             - '**/*.py'
         type: external
-        payload: flake8_lint:lint
+        payload: py.flake8:lint
 
-Notice the payload has two parts, delimited by ':'. The first is the module path, which
-``mozlint`` will attempt to import (e.g, the name of a function to call). The second is
-the object path within that module. It is up to consumers of ``mozlint`` to ensure the
-module is in ``sys.path``. Structured log linters use the same import mechanism.
+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.
+
+
+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.
+
+Either way, to reduce the burden on users, linters should strive to provide
+automated bootstrapping of all their dependencies. To help with this,
+``mozlint`` allows linters to define a ``setup`` config, which has the same
+path object format as an external payload. For example:
+
+.. code-block:: yml
+
+    flake8:
+        description: Python linter
+        include:
+            - '**/*.py'
+        type: external
+        payload: py.flake8:lint
+        setup: py.flake8:setup
+
+The setup function takes a single argument, the root of the repository being
+linted. In the case of ``flake8``, it might look like:
+
+.. code-block:: python
+
+    import subprocess
+    from distutils.spawn import find_executable
+
+    def setup(root):
+        if not find_executable('flake8'):
+            subprocess.call(['pip', 'install', 'flake8'])
+
+The setup function will be called implicitly before running the linter. This
+means it should return fast and not produce any output if there is no setup to
+be performed.
+
+The setup functions can also be called explicitly by running ``mach lint
+--setup``. This will only perform setup and not perform any linting. It is
+mainly useful for other tools like ``mach bootstrap`` to call into.

tools/lint/eslint.yml

@@ -7,3 +7,4 @@ eslint:
     extensions: ['js', 'jsm', 'jsx', 'xml', 'html', 'xhtml']
     type: external
     payload: eslint:lint
+    setup: eslint:setup

tools/lint/eslint/__init__.py

@@ -32,28 +32,24 @@ and try again.
 """.strip()
 
 
-def lint(paths, config, binary=None, fix=None, setup=None, **lintargs):
-    """Run eslint."""
-    setup_helper.set_project_root(lintargs['root'])
-
-    module_path = setup_helper.get_project_root()
+def setup(root):
+    setup_helper.set_project_root(root)
 
     if not setup_helper.check_node_executables_valid():
         return 1
 
-    if setup:
-        return setup_helper.eslint_setup()
+    return setup_helper.eslint_maybe_setup()
 
-    setup_helper.eslint_maybe_setup()
+
+def lint(paths, config, binary=None, fix=None, setup=None, **lintargs):
+    """Run eslint."""
+    setup_helper.set_project_root(lintargs['root'])
+    module_path = setup_helper.get_project_root()
 
     # Valid binaries are:
     #  - Any provided by the binary argument.
     #  - Any pointed at by the ESLINT environmental variable.
-    #  - Those provided by mach eslint --setup.
-    #
-    #  eslint --setup installs some mozilla specific plugins and installs
-    #  all node modules locally. This is the preferred method of
-    #  installation.
+    #  - Those provided by |mach lint --setup|.
 
     if not binary:
         binary = os.environ.get('ESLINT', None)

tools/lint/flake8.yml

@@ -47,3 +47,4 @@ flake8:
     extensions: ['configure', 'py']
     type: external
     payload: python.flake8:lint
+    setup: python.flake8:setup

tools/lint/python/flake8.py

@@ -136,14 +136,14 @@ def run_process(config, cmd):
         proc.kill()
 
 
-def lint(paths, config, **lintargs):
-
+def setup(root):
     if not reinstall_flake8():
         print(FLAKE8_INSTALL_ERROR)
         return 1
 
-    binary = get_flake8_binary()
 
+def lint(paths, config, **lintargs):
+    binary = get_flake8_binary()
     cmdargs = [
         binary,
         '--format', '{"path":"%(path)s","lineno":%(row)s,'