flake8: no wildcards in core; only `import *` from spack in packages

There are now separate flake8 configs for core vs. packages:
- core has a smaller set of flake8 exceptions
- packages allow `from spack import *` and module globals
- Allows core to take advantage of static checking for undefined names
- Allows packages to keep using Spack tricks like `from spack import *`
  and dependencies setting globals for dependents.

4 files changed, 78 insertions, 29 deletions

diff --git a/.flake8 b/.flake8
index e697d5ea04..49199b35c8 100644
--- a/.flake8
+++ b/.flake8
@@ -1,8 +1,8 @@
 # -*- conf -*-
-# flake8 settings for Spack.
+# flake8 settings for Spack core files.
 #
-# Below we describe which flake8 checks Spack ignores and what the
-# rationale is.
+# These exceptions ar for Spack core files. We're slightly more lenient
+# with packages.  See .flake8_packages for that.
 #
 # Let people line things up nicely:
 # - E129: visually indented line with same indent as next logical line
@@ -13,14 +13,9 @@
 # Let people use terse Python features:
 # - E731: lambda expressions
 #
-# Spack allows wildcard imports:
-# - F403: disable wildcard import
-#
 # These are required to get the package.py files to test clean:
-# - F405: `name` may be undefined, or undefined from star imports: `module`
-# - F821: undefined name `name` (needed for cmake, configure, etc.)
 # - F999: syntax error in doctest
 #
 [flake8]
-ignore = E129,E221,E241,E272,E731,F403,F405,F821,F999
+ignore = E129,E221,E241,E272,E731,F999
 max-line-length = 79
diff --git a/.flake8_packages b/.flake8_packages
new file mode 100644
index 0000000000..9fcc3b86d4
--- /dev/null
+++ b/.flake8_packages
@@ -0,0 +1,22 @@
+# -*- conf -*-
+# flake8 settings for Spack package files.
+#
+# This should include all the same exceptions that we use for core files.
+#
+# In Spack packages, we also allow the single `from spack import *`
+# wildcard import and dependencies can set globals for their
+# dependents. So we add exceptions for checks related to undefined names.
+#
+# Note that we also add *per-line* exemptions for certain patters in the
+# `spack flake8` command.  This is where F403 for `from spack import *`
+# is added (beause we *only* allow that wildcard).
+#
+# See .flake8 for regular exceptions.
+#
+# Redefinition exceptions:
+# - F405: `name` may be undefined, or undefined from star imports: `module`
+# - F821: undefined name `name` (needed for cmake, configure, etc.)
+#
+[flake8]
+ignore = E129,E221,E241,E272,E731,F999,F405,F821
+max-line-length = 79
diff --git a/lib/spack/spack/cmd/flake8.py b/lib/spack/spack/cmd/flake8.py
index 8f9e311734..01e642235d 100644
--- a/lib/spack/spack/cmd/flake8.py
+++ b/lib/spack/spack/cmd/flake8.py
@@ -42,18 +42,34 @@ section = "developer"
 level = "long"
 
 
-"""List of directories to exclude from checks."""
+def is_package(f):
+    """Whether flake8 should consider a file as a core file or a package.
+
+    We run flake8 with different exceptions for the core and for
+    packages, since we allow `from spack import *` and poking globals
+    into packages.
+    """
+    return f.startswith('var/spack/repos/') or 'docs/tutorial/examples' in f
+
+
+#: List of directories to exclude from checks.
 exclude_directories = [spack.external_path]
 
-"""
-This is a dict that maps:
- filename pattern ->
-    a flake8 exemption code ->
-       list of patterns, for which matching lines should have codes applied.
-"""
-exemptions = {
+
+#: This is a dict that maps:
+#:  filename pattern ->
+#:     flake8 exemption code ->
+#:        list of patterns, for which matching lines should have codes applied.
+#:
+#: For each file, if the filename pattern matches, we'll add per-line
+#: exemptions if any patterns in the sub-dict match.
+pattern_exemptions = {
     # exemptions applied only to package.py files.
     r'package.py$': {
+        # Allow 'from spack import *' in packages, but no other wildcards
+        'F403': [
+            r'^from spack import \*$'
+        ],
         # Exempt lines with urls and descriptions from overlong line errors.
         'E501': [
             r'^\s*homepage\s*=',
@@ -87,10 +103,11 @@ exemptions = {
 }
 
 # compile all regular expressions.
-exemptions = dict((re.compile(file_pattern),
-                   dict((code, [re.compile(p) for p in patterns])
-                        for code, patterns in error_dict.items()))
-                  for file_pattern, error_dict in exemptions.items())
+pattern_exemptions = dict(
+    (re.compile(file_pattern),
+     dict((code, [re.compile(p) for p in patterns])
+          for code, patterns in error_dict.items()))
+    for file_pattern, error_dict in pattern_exemptions.items())
 
 
 def changed_files(args):
@@ -135,7 +152,7 @@ def changed_files(args):
     return sorted(changed)
 
 
-def add_exemptions(line, codes):
+def add_pattern_exemptions(line, codes):
     """Add a flake8 exemption to a line."""
     if line.startswith('#'):
         return line
@@ -163,7 +180,7 @@ def add_exemptions(line, codes):
 
 
 def filter_file(source, dest, output=False):
-    """Filter a single file through all the patterns in exemptions."""
+    """Filter a single file through all the patterns in pattern_exemptions."""
     with open(source) as infile:
         parent = os.path.dirname(dest)
         mkdirp(parent)
@@ -171,7 +188,9 @@ def filter_file(source, dest, output=False):
         with open(dest, 'w') as outfile:
             for line in infile:
                 line_errors = []
-                for file_pattern, errors in exemptions.items():
+
+                # pattern exemptions
+                for file_pattern, errors in pattern_exemptions.items():
                     if not file_pattern.search(source):
                         continue
 
@@ -182,7 +201,7 @@ def filter_file(source, dest, output=False):
                                 break
 
                 if line_errors:
-                    line = add_exemptions(line, line_errors)
+                    line = add_pattern_exemptions(line, line_errors)
 
                 outfile.write(line)
                 if output:
@@ -226,7 +245,6 @@ def flake8(parser, args):
         with working_dir(spack.prefix):
             if not file_list:
                 file_list = changed_files(args)
-            shutil.copy('.flake8', os.path.join(temp, '.flake8'))
 
         print('=======================================================')
         print('flake8: running flake8 code checks on spack.')
@@ -242,10 +260,23 @@ def flake8(parser, args):
             dest_path = os.path.join(temp, filename)
             filter_file(src_path, dest_path, args.output)
 
-        # run flake8 on the temporary tree.
+        # run flake8 on the temporary tree, once for core, once for pkgs
+        package_file_list = [f for f in file_list if is_package(f)]
+        file_list         = [f for f in file_list if not is_package(f)]
+
         with working_dir(temp):
-            output = flake8('--format', 'pylint', *file_list,
-                            fail_on_error=False, output=str)
+            output = ''
+            if file_list:
+                output += flake8(
+                    '--format', 'pylint',
+                    '--config=%s' % os.path.join(spack.prefix, '.flake8'),
+                    *file_list, fail_on_error=False, output=str)
+            if package_file_list:
+                output += flake8(
+                    '--format', 'pylint',
+                    '--config=%s' % os.path.join(spack.prefix,
+                                                 '.flake8_packages'),
+                    *package_file_list, fail_on_error=False, output=str)
 
         if args.root_relative:
             # print results relative to repo root.
diff --git a/lib/spack/spack/test/cmd/flake8.py b/lib/spack/spack/test/cmd/flake8.py
index 8556a0a036..b6433e95c9 100644
--- a/lib/spack/spack/test/cmd/flake8.py
+++ b/lib/spack/spack/test/cmd/flake8.py
@@ -34,6 +34,7 @@ from spack.cmd.flake8 import *
 from spack.repository import Repo
 
 
+
 @pytest.fixture(scope='module')
 def parser():
     """Returns the parser for the ``flake8`` command"""