Bug 1367092 - [flake8] Take exclusion handling away from flake8, r=egao
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Fri, 22 Feb 2019 22:16:57 +0000
changeset 460906 41fdb372e22cf213eb76b966e357772f89a77710
parent 460905 cd46126bc5924f041570d7adc889c394921930ba
child 460907 4624c850f711417ae3cc95f3e69ce4cb6c07f70b
push id35613
push usernerli@mozilla.com
push dateTue, 26 Feb 2019 03:52:35 +0000
treeherdermozilla-central@faec87a80ed1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersegao
bugs1367092
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1367092 - [flake8] Take exclusion handling away from flake8, r=egao 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
python/mozlint/mozlint/pathutils.py
tools/lint/python/flake8.py
tools/lint/test/files/flake8/.flake8
tools/lint/test/files/flake8/subdir/exclude/bad.py
tools/lint/test/test_flake8.py
--- a/python/mozlint/mozlint/pathutils.py
+++ b/python/mozlint/mozlint/pathutils.py
@@ -119,17 +119,17 @@ def collapse(paths, base=None, dotfiles=
         elif os.path.isdir(path):
             new_paths = [p for p in paths if p.startswith(path)]
             covered.update(collapse(new_paths, base=path, dotfiles=dotfiles))
 
     if full == covered:
         # Every file under this base was covered, so we can collapse them all
         # up into the base path.
         return [base]
-    return covered
+    return list(covered)
 
 
 def filterpaths(root, paths, include, exclude=None, extensions=None):
     """Filters a list of paths.
 
     Given a list of paths and some filtering rules, return the set of paths
     that should be linted.
 
--- a/tools/lint/python/flake8.py
+++ b/tools/lint/python/flake8.py
@@ -8,16 +8,17 @@ import json
 import os
 import platform
 import subprocess
 import sys
 
 import mozfile
 
 from mozlint import result
+from mozlint.pathutils import expand_exclusions
 from mozlint.util import pip
 
 here = os.path.abspath(os.path.dirname(__file__))
 FLAKE8_REQUIREMENTS_PATH = os.path.join(here, 'flake8_requirements.txt')
 
 FLAKE8_NOT_FOUND = """
 Could not find flake8! Install flake8 and try again.
 
@@ -58,54 +59,91 @@ to the lineoffset property of an `Issue`
 # We use sys.prefix to find executables as that gets modified with
 # virtualenv's activate_this.py, whereas sys.executable doesn't.
 if platform.system() == 'Windows':
     bindir = os.path.join(sys.prefix, 'Scripts')
 else:
     bindir = os.path.join(sys.prefix, 'bin')
 
 
+class NothingToLint(Exception):
+    """Exception used to bail out of flake8's internals if all the specified
+    files were excluded.
+    """
+
+
 def setup(root):
     if not pip.reinstall_program(FLAKE8_REQUIREMENTS_PATH):
         print(FLAKE8_INSTALL_ERROR)
         return 1
 
 
 def lint(paths, config, **lintargs):
     from flake8.main.application import Application
 
-    config_path = os.path.join(lintargs['root'], '.flake8')
-    exclude = config.get('exclude', [])
+    root = lintargs['root']
+    config_path = os.path.join(root, '.flake8')
 
     if lintargs.get('fix'):
         fix_cmd = [
             os.path.join(bindir, 'autopep8'),
             '--global-config', config_path,
             '--in-place', '--recursive',
         ]
 
-        if exclude:
-            fix_cmd.extend(['--exclude', ','.join(exclude)])
+        if config.get('exclude'):
+            fix_cmd.extend(['--exclude', ','.join(config['exclude'])])
 
         subprocess.call(fix_cmd + paths)
 
+    # Run flake8.
+    app = Application()
+
     output_file = mozfile.NamedTemporaryFile()
     flake8_cmd = [
-        os.path.join(bindir, 'flake8'),
         '--config', config_path,
         '--output-file', output_file.name,
         '--format', '{"path":"%(path)s","lineno":%(row)s,'
                     '"column":%(col)s,"rule":"%(code)s","message":"%(text)s"}',
         '--filename', ','.join(['*.{}'.format(e) for e in config['extensions']]),
-    ] + paths
+    ]
+
+    orig_make_file_checker_manager = app.make_file_checker_manager
+
+    def wrap_make_file_checker_manager(self):
+        """Flake8 is very inefficient when it comes to applying exclusion
+        rules, using `expand_exclusions` to turn directories into a list of
+        relevant python files is an order of magnitude faster.
+
+        Hooking into flake8 here also gives us a convenient place to merge the
+        `exclude` rules specified in the root .flake8 with the ones added by
+        tools/lint/mach_commands.py.
+        """
+        config.setdefault('exclude', []).extend(self.options.exclude)
+        self.options.exclude = None
+        self.args = self.args + list(expand_exclusions(paths, config, root))
 
-    # Run flake8.
+        if not self.args:
+            raise NothingToLint
+        return orig_make_file_checker_manager()
+
+    app.make_file_checker_manager = wrap_make_file_checker_manager.__get__(app, Application)
+
+    # Make sure to run from repository root so exlusions are joined to the
+    # repository root and not the current working directory.
+    oldcwd = os.getcwd()
+    os.chdir(root)
+    try:
+        app.run(flake8_cmd)
+    except NothingToLint:
+        pass
+    finally:
+        os.chdir(oldcwd)
+
     results = []
-    app = Application()
-    app.run(flake8_cmd)
 
     def process_line(line):
         # Escape slashes otherwise JSON conversion will not work
         line = line.replace('\\', '\\\\')
         try:
             res = json.loads(line)
         except ValueError:
             print('Non JSON output from linter, will not be processed: {}'.format(line))
--- a/tools/lint/test/files/flake8/.flake8
+++ b/tools/lint/test/files/flake8/.flake8
@@ -1,2 +1,4 @@
 [flake8]
 max-line-length = 100
+exclude =
+    subdir/exclude,
copy from tools/lint/test/files/flake8/bad.py
copy to tools/lint/test/files/flake8/subdir/exclude/bad.py
--- a/tools/lint/test/test_flake8.py
+++ b/tools/lint/test/test_flake8.py
@@ -1,12 +1,11 @@
 import os
 
 import mozunit
-import pytest
 
 LINTER = 'flake8'
 
 
 def test_lint_single_file(lint, paths):
     results = lint(paths('bad.py'))
     assert len(results) == 2
     assert results[0].rule == 'F401'
@@ -59,26 +58,29 @@ foo = ['A list of strings', 'that go ove
     lint([path], fix=True)
 
     # Make sure autopep8 reads the global config under lintargs['root']. If it
     # didn't, then the line-length over 80 would get fixed.
     with open(path, 'r') as fh:
         assert fh.read() == contents
 
 
-@pytest.mark.xfail(reason="Bug 1277851 - custom configs are ignored if specifying a parent path")
-def test_lint_custom_config_from_parent_path(lint, paths):
-    results = lint(paths(), collapse_results=True)
-    assert paths('custom/good.py')[0] not in results
+def test_lint_excluded_file(lint, paths, config):
+    # First file is globally excluded, second one is from .flake8 config.
+    files = paths('bad.py', 'subdir/exclude/bad.py')
+    config['exclude'] = paths('bad.py')
+    results = lint(files, config)
+    print(results)
+    assert len(results) == 0
 
+    # Make sure excludes also apply when running from a different cwd.
+    cwd = paths('subdir')[0]
+    os.chdir(cwd)
 
-@pytest.mark.xfail(reason="Bug 1277851 - 'exclude' argument is ignored")
-def test_lint_excluded_file(lint, paths):
-    paths = paths('bad.py')
-    results = lint(paths, exclude=paths)
+    results = lint(paths('subdir/exclude'))
     assert len(results) == 0
 
 
 def test_lint_uses_custom_extensions(lint, paths):
     assert len(lint(paths('ext'))) == 1
     assert len(lint(paths('ext/bad.configure'))) == 1