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

--HG--
extra : rebase_source : 51298c5847f6c2792581d9b312c87b70fa716ee1

.flake8

@@ -2,3 +2,5 @@
 # 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,

python/mozlint/mozlint/pathutils.py

@@ -176,3 +176,26 @@ def findobject(path):
     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

testing/firefox-ui/.flake8

@@ -1,3 +1,3 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,
+exclude =
+    __init__.py,

testing/marionette/client/.flake8

@@ -1,3 +1,5 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,disti/*,build/*,
+exclude =
+    __init__.py,
+    disti/*,
+    build/*,

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/*,

testing/marionette/puppeteer/.flake8

@@ -1,3 +1,3 @@
 [flake8]
-max-line-length = 99
-exclude = __init__.py,
+exclude =
+    __init__.py,

toolkit/components/telemetry/.flake8

@@ -1,4 +0,0 @@
-[flake8]
-# See http://pep8.readthedocs.io/en/latest/intro.html#configuration
-max-line-length = 99
-filename = *.py, +.lint

tools/lint/flake8.yml

@@ -19,8 +19,9 @@ flake8:
         - 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

tools/lint/flake8_/__init__.py

@@ -6,11 +6,13 @@ 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__))
@@ -138,7 +140,7 @@ def run_process(config, cmd):
         proc.kill()
 
 
-def lint(files, config, **lintargs):
+def lint(paths, config, **lintargs):
 
     if not reinstall_flake8():
         print(FLAKE8_INSTALL_ERROR)
@@ -156,21 +158,19 @@ def lint(files, config, **lintargs):
     # 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

tools/lint/yamllint_/__init__.py

@@ -12,6 +12,7 @@ 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__))
@@ -120,29 +121,6 @@ def gen_yamllint_args(cmdargs, paths=None, conf_file=None):
     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)
@@ -163,7 +141,7 @@ def lint(files, config, **lintargs):
     # 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():