Bug 1395126 - Support cascading configuration for flake8, r=bc
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 29 Aug 2017 17:32:31 -0400
changeset 377819 0145883aea3a74740298a2dc3072e4a4e80c8570
parent 377818 e54ed7547a3e9deec8c94f638c91eb5c587a584a
child 377820 99de93a9896b7e3075e488b4e33cc277e322a4d2
push id32416
push userarchaeopteryx@coole-files.de
push dateThu, 31 Aug 2017 12:35:23 +0000
treeherdermozilla-central@c9079d347aaa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbc
bugs1395126
milestone57.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 1395126 - Support cascading configuration for flake8, r=bc This allows .flake8 files to override one another, and fixes a pretty bad known bug with our flake8 implementation. For example, say we have a .flake8 file at: /foo/.flake8 Before this patch, if we ran |mach lint foo/bar|, the configuration defined in that .flake8 file wouldn't get picked up. It would only work if running the specific directory that contains it, e.g |mach lint foo|. This change additionally allows multiple .flake8 files to be used. So if there's one defined at both: /.flake8 /foo/.flake8 Then running |mach lint foo/bar| will first apply the root .flake8, then the one under /foo (overriding earlier configuration). This bug still doesn't make flake8 configuration perfect though. Any directory containing a .flake8 file still needs to be explicitly listed in the "include" section of /tools/lint/flake8.yml. Otherwise in the example above, if running |mach lint /|, it wouldn't be able to find /foo/.flake8. This is a hard problem and is likely best solved by fixing flake8's upstream configuration handling. Unfortunately this means we still can't switch from a whitelist to a blacklist. MozReview-Commit-ID: 3DZAi1QHYYo
.flake8
python/mozlint/mozlint/pathutils.py
testing/firefox-ui/.flake8
testing/marionette/client/.flake8
testing/marionette/harness/.flake8
testing/marionette/puppeteer/.flake8
toolkit/components/telemetry/.flake8
tools/lint/flake8.yml
tools/lint/flake8_/__init__.py
tools/lint/yamllint_/__init__.py
--- a/.flake8
+++ b/.flake8
@@ -1,4 +1,6 @@
 [flake8]
 # See http://pep8.readthedocs.io/en/latest/intro.html#configuration
 ignore = E121, E123, E126, E129, E133, E226, E241, E242, E704, W503, E402
 max-line-length = 99
+exclude =
+    testing/mochitest/pywebsocket,
--- a/python/mozlint/mozlint/pathutils.py
+++ b/python/mozlint/mozlint/pathutils.py
@@ -171,8 +171,31 @@ def findobject(path):
 
     modulepath, objectpath = path.split(':')
     obj = __import__(modulepath)
     for a in modulepath.split('.')[1:]:
         obj = getattr(obj, a)
     for a in objectpath.split('.'):
         obj = getattr(obj, a)
     return obj
+
+
+def ancestors(path):
+    while path:
+        yield path
+        (path, child) = os.path.split(path)
+        if child == "":
+            break
+
+
+def get_ancestors_by_name(name, path, root):
+    """Returns a list of files called `name` in `path`'s ancestors,
+    sorted from closest->furthest. This can be useful for finding
+    relevant configuration files.
+    """
+    configs = []
+    for path in ancestors(path):
+        config = os.path.join(path, name)
+        if os.path.isfile(config):
+            configs.append(config)
+        if path == root:
+            break
+    return configs
--- a/testing/firefox-ui/.flake8
+++ b/testing/firefox-ui/.flake8
@@ -1,3 +1,3 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,
+exclude =
+    __init__.py,
--- a/testing/marionette/client/.flake8
+++ b/testing/marionette/client/.flake8
@@ -1,3 +1,5 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,disti/*,build/*,
+exclude =
+    __init__.py,
+    disti/*,
+    build/*,
--- a/testing/marionette/harness/.flake8
+++ b/testing/marionette/harness/.flake8
@@ -1,3 +1,7 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,disti/*,build/*,marionette_harness/runner/mixins/*, marionette_harness/tests/*
+exclude =
+    __init__.py,
+    disti/*,
+    build/*,
+    marionette_harness/runner/mixins/*,
+    marionette_harness/tests/*,
--- a/testing/marionette/puppeteer/.flake8
+++ b/testing/marionette/puppeteer/.flake8
@@ -1,3 +1,3 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,
+exclude =
+    __init__.py,
deleted file mode 100644
--- a/toolkit/components/telemetry/.flake8
+++ /dev/null
@@ -1,4 +0,0 @@
-[flake8]
-# See http://pep8.readthedocs.io/en/latest/intro.html#configuration
-max-line-length = 99
-filename = *.py, +.lint
--- a/tools/lint/flake8.yml
+++ b/tools/lint/flake8.yml
@@ -14,13 +14,14 @@ flake8:
         - testing/mozbase
         - testing/mochitest
         - testing/talos/
         - tools/git
         - tools/lint
         - tools/mercurial
         - tools/tryselect
         - toolkit/components/telemetry
-    exclude:
-        - testing/mochitest/pywebsocket
+    # Excludes should be added to topsrcdir/.flake8 due to a bug in flake8 where
+    # specifying --exclude causes custom configuration files to be ignored.
+    exclude: []
     extensions: ['py']
     type: external
     payload: flake8_:lint
--- a/tools/lint/flake8_/__init__.py
+++ b/tools/lint/flake8_/__init__.py
@@ -1,21 +1,23 @@
 # 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/.
 
 import json
 import os
 import signal
 import subprocess
+from collections import defaultdict
 
 import which
 from mozprocess import ProcessHandlerMixin
 
 from mozlint import result
+from mozlint.pathutils import get_ancestors_by_name
 
 
 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.
 
@@ -133,17 +135,17 @@ def run_process(config, cmd):
     proc = Flake8Process(config, cmd)
     proc.run()
     try:
         proc.wait()
     except KeyboardInterrupt:
         proc.kill()
 
 
-def lint(files, config, **lintargs):
+def lint(paths, config, **lintargs):
 
     if not reinstall_flake8():
         print(FLAKE8_INSTALL_ERROR)
         return 1
 
     binary = get_flake8_binary()
 
     cmdargs = [
@@ -151,26 +153,24 @@ def lint(files, config, **lintargs):
         '--format', '{"path":"%(path)s","lineno":%(row)s,'
                     '"column":%(col)s,"rule":"%(code)s","message":"%(text)s"}',
     ]
 
     # Run any paths with a .flake8 file in the directory separately so
     # it gets picked up. This means only .flake8 files that live in
     # directories that are explicitly included will be considered.
     # See bug 1277851
-    no_config = []
-    for f in files:
-        if not os.path.isfile(os.path.join(f, '.flake8')):
-            no_config.append(f)
-            continue
-        run_process(config, cmdargs+[f])
+    paths_by_config = defaultdict(list)
+    for path in paths:
+        configs = get_ancestors_by_name('.flake8', path, lintargs['root'])
+        paths_by_config[os.pathsep.join(configs) if configs else 'default'].append(path)
 
-    # XXX For some reason passing in --exclude results in flake8 not using
-    # the local .flake8 file. So for now only pass in --exclude if there
-    # is no local config.
-    exclude = lintargs.get('exclude')
-    if exclude:
-        cmdargs += ['--exclude', ','.join(lintargs['exclude'])]
+    for configs, paths in paths_by_config.items():
+        cmd = cmdargs[:]
 
-    if no_config:
-        run_process(config, cmdargs+no_config)
+        if configs != 'default':
+            configs = reversed(configs.split(os.pathsep))
+            cmd.extend(['--append-config={}'.format(c) for c in configs])
+
+        cmd.extend(paths)
+        run_process(config, cmd)
 
     return results
--- a/tools/lint/yamllint_/__init__.py
+++ b/tools/lint/yamllint_/__init__.py
@@ -7,16 +7,17 @@ import os
 import signal
 import subprocess
 from collections import defaultdict
 
 import which
 from mozprocess import ProcessHandlerMixin
 
 from mozlint import result
+from mozlint.pathutils import get_ancestors_by_name
 
 
 here = os.path.abspath(os.path.dirname(__file__))
 YAMLLINT_REQUIREMENTS_PATH = os.path.join(here, 'yamllint_requirements.txt')
 
 
 YAMLLINT_INSTALL_ERROR = """
 Unable to install correct version of yamllint
@@ -115,39 +116,16 @@ def gen_yamllint_args(cmdargs, paths=Non
     args = cmdargs[:]
     if isinstance(paths, basestring):
         paths = [paths]
     if conf_file and conf_file != 'default':
         return args + ['-c', conf_file] + paths
     return args + paths
 
 
-def ancestors(path):
-    while path:
-        yield path
-        (path, child) = os.path.split(path)
-        if child == "":
-            break
-
-
-def get_relevant_configs(name, path, root):
-    """Returns a list of configuration files that exist in `path`'s ancestors,
-    sorted from closest->furthest.
-    """
-    configs = []
-    for path in ancestors(path):
-        if path == root:
-            break
-
-        config = os.path.join(path, name)
-        if os.path.isfile(config):
-            configs.append(config)
-    return configs
-
-
 def lint(files, config, **lintargs):
     if not reinstall_yamllint():
         print(YAMLLINT_INSTALL_ERROR)
         return 1
 
     binary = get_yamllint_binary()
 
     cmdargs = [
@@ -158,15 +136,15 @@ def lint(files, config, **lintargs):
     config = config.copy()
     config['root'] = lintargs['root']
 
     # Run any paths with a .yamllint file in the directory separately so
     # it gets picked up. This means only .yamllint files that live in
     # directories that are explicitly included will be considered.
     paths_by_config = defaultdict(list)
     for f in files:
-        conf_files = get_relevant_configs('.yamllint', f, config['root'])
+        conf_files = get_ancestors_by_name('.yamllint', f, config['root'])
         paths_by_config[conf_files[0] if conf_files else 'default'].append(f)
 
     for conf_file, paths in paths_by_config.items():
         run_process(config, gen_yamllint_args(cmdargs, conf_file=conf_file, paths=paths))
 
     return results