Bug 1395126 - Support cascading configuration for flake8, r?bc draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 29 Aug 2017 17:32:31 -0400
changeset 656064 0d82d6b2dd26561c5e6f815455baa84bd29fb3e3
parent 654592 1b4c59eef820b46eb0037aca68f83a15088db45f
child 728997 93b8f7277d21cf35b6d62a230fb8703e9f6dc228
push id77051
push userahalberstadt@mozilla.com
push dateWed, 30 Aug 2017 18:05:36 +0000
reviewersbc
bugs1395126
milestone57.0a1
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