Properly ignore flake8 F811 redefinition errors (#3932)

* Properly ignore flake8 F811 redefinition errors
* Add unit tests for flake8 command
* Allow spack flake8 to work on systems with older git
* Skip flake8 unit tests for Python 2.6 and 3.3

2 files changed, 153 insertions, 33 deletions

diff --git a/lib/spack/spack/cmd/flake8.py b/lib/spack/spack/cmd/flake8.py
index 546a27c765..33c06155ae 100644
--- a/lib/spack/spack/cmd/flake8.py
+++ b/lib/spack/spack/cmd/flake8.py
@@ -37,8 +37,6 @@ import spack
 from spack.util.executable import *
 
 description = "runs source code style checks on Spack. requires flake8"
-flake8 = None
-include_untracked = True
 
 """List of directories to exclude from checks."""
 exclude_directories = [spack.external_path]
@@ -53,24 +51,34 @@ exemptions = {
     # exemptions applied only to package.py files.
     r'package.py$': {
         # Exempt lines with urls and descriptions from overlong line errors.
-        501: [r'^\s*homepage\s*=',
-              r'^\s*url\s*=',
-              r'^\s*git\s*=',
-              r'^\s*svn\s*=',
-              r'^\s*hg\s*=',
-              r'^\s*version\(.*\)',
-              r'^\s*variant\(.*\)',
-              r'^\s*depends_on\(.*\)',
-              r'^\s*extends\(.*\)',
-              r'^\s*patch\(.*\)'],
+        'E501': [
+            r'^\s*homepage\s*=',
+            r'^\s*url\s*=',
+            r'^\s*git\s*=',
+            r'^\s*svn\s*=',
+            r'^\s*hg\s*=',
+            r'^\s*list_url\s*=',
+            r'^\s*version\(.*\)',
+            r'^\s*variant\(.*\)',
+            r'^\s*provides\(.*\)',
+            r'^\s*extends\(.*\)',
+            r'^\s*depends_on\(.*\)',
+            r'^\s*conflicts\(.*\)',
+            r'^\s*resource\(.*\)',
+            r'^\s*patch\(.*\)',
+        ],
         # Exempt '@when' decorated functions from redefinition errors.
-        811: [r'^\s*\@when\(.*\)'],
+        'F811': [
+            r'^\s*@when\(.*\)',
+        ],
     },
 
     # exemptions applied to all files.
     r'.py$': {
         # Exempt lines with URLs from overlong line errors.
-        501: [r'(https?|ftp|file)\:']
+        'E501': [
+            r'(https?|ftp|file)\:',
+        ]
     },
 }
 
@@ -81,7 +89,7 @@ exemptions = dict((re.compile(file_pattern),
                   for file_pattern, error_dict in exemptions.items())
 
 
-def changed_files():
+def changed_files(args):
     """Get list of changed files in the Spack repository."""
 
     git = which('git', required=True)
@@ -92,23 +100,30 @@ def changed_files():
         # Add changed files that have been staged but not yet committed
         ['diff', '--name-only', '--diff-filter=ACMR', '--cached'],
         # Add changed files that are unstaged
-        ['diff', '--name-only', '--diff-filter=ACMR']]
+        ['diff', '--name-only', '--diff-filter=ACMR'],
+    ]
 
     # Add new files that are untracked
-    if include_untracked:
+    if args.untracked:
         git_args.append(['ls-files', '--exclude-standard', '--other'])
 
     excludes = [os.path.realpath(f) for f in exclude_directories]
     changed = set()
-    for git_arg_list in git_args:
-        arg_list = git_arg_list + ['--', '*.py']
 
-        files = [f for f in git(*arg_list, output=str).split('\n') if f]
+    for arg_list in git_args:
+        files = git(*arg_list, output=str).split('\n')
+
         for f in files:
-            # don't look at files that are in the exclude locations
+            # Ignore non-Python files
+            if not f.endswith('.py'):
+                continue
+
+            # Ignore files in the exclude locations
             if any(os.path.realpath(f).startswith(e) for e in excludes):
                 continue
+
             changed.add(f)
+
     return sorted(changed)
 
 
@@ -120,7 +135,17 @@ def filter_file(source, dest, output=False):
 
         with open(dest, 'w') as outfile:
             for line in infile:
-                line = line.rstrip()
+                # Only strip newline characters
+                # We still want to catch trailing whitespace warnings
+                line = line.rstrip('\n')
+
+                if line == '# flake8: noqa':
+                    # Entire file is ignored
+                    break
+
+                if line.endswith('# noqa'):
+                    # Line is already ignored
+                    continue
 
                 for file_pattern, errors in exemptions.items():
                     if not file_pattern.search(source):
@@ -129,7 +154,10 @@ def filter_file(source, dest, output=False):
                     for code, patterns in errors.items():
                         for pattern in patterns:
                             if pattern.search(line):
-                                line += ("  # NOQA: E%d" % code)
+                                if '# noqa: ' in line:
+                                    line += ',{0}'.format(code)
+                                else:
+                                    line += '  # noqa: {0}'.format(code)
                                 break
 
                 oline = line + '\n'
@@ -157,10 +185,7 @@ def setup_parser(subparser):
 
 
 def flake8(parser, args):
-    # Just use this to check for flake8 -- we actually execute it with Popen.
-    global flake8, include_untracked
     flake8 = which('flake8', required=True)
-    include_untracked = args.untracked
 
     temp = tempfile.mkdtemp()
     try:
@@ -174,7 +199,7 @@ def flake8(parser, args):
 
         with working_dir(spack.prefix):
             if not file_list:
-                file_list = changed_files()
+                file_list = changed_files(args)
             shutil.copy('.flake8', os.path.join(temp, '.flake8'))
 
         print('=======================================================')
@@ -182,7 +207,7 @@ def flake8(parser, args):
         print()
         print('Modified files:')
         for filename in file_list:
-            print("  %s" % filename.strip())
+            print('  {0}'.format(filename.strip()))
         print('=======================================================')
 
         # filter files into a temporary directory with exemptions added.
@@ -202,20 +227,20 @@ def flake8(parser, args):
         else:
             # print results relative to current working directory
             def cwd_relative(path):
-                return '%s: [' % os.path.relpath(
-                    os.path.join(spack.prefix, path.group(1)), os.getcwd())
+                return '{0}: ['.format(os.path.relpath(
+                    os.path.join(spack.prefix, path.group(1)), os.getcwd()))
 
             for line in output.split('\n'):
                 print(re.sub(r'^(.*): \[', cwd_relative, line))
 
         if flake8.returncode != 0:
-            print("Flake8 found errors.")
+            print('Flake8 found errors.')
             sys.exit(1)
         else:
-            print("Flake8 checks were clean.")
+            print('Flake8 checks were clean.')
 
     finally:
         if args.keep_temp:
-            print("temporary files are in ", temp)
+            print('Temporary files are in: ', temp)
         else:
             shutil.rmtree(temp, ignore_errors=True)
diff --git a/lib/spack/spack/test/cmd/flake8.py b/lib/spack/spack/test/cmd/flake8.py
new file mode 100644
index 0000000000..bde8d199a7
--- /dev/null
+++ b/lib/spack/spack/test/cmd/flake8.py
@@ -0,0 +1,95 @@
+##############################################################################
+# Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+import argparse
+import os
+import pytest
+import sys
+
+from llnl.util.filesystem import FileFilter
+
+import spack
+from spack.cmd.flake8 import *
+from spack.repository import Repo
+
+
+@pytest.fixture(scope='module')
+def parser():
+    """Returns the parser for the ``flake8`` command"""
+    parser = argparse.ArgumentParser()
+    setup_parser(parser)
+    return parser
+
+
+@pytest.fixture(scope='module')
+def flake8_package():
+    """Flake8 only checks files that have been modified.
+    This fixture makes a small change to the ``flake8``
+    mock package, yields the filename, then undoes the
+    change on cleanup.
+    """
+    repo = Repo(spack.mock_packages_path)
+    filename = repo.filename_for_package_name('flake8')
+    package = FileFilter(filename)
+
+    # Make the change
+    package.filter('unmodified', 'modified')
+
+    yield filename
+
+    # Undo the change
+    package.filter('modified', 'unmodified')
+
+
+def test_changed_files(parser, flake8_package):
+    args = parser.parse_args()
+
+    # changed_files returns file paths relative to the root
+    # directory of Spack. Convert to absolute file paths.
+    files = changed_files(args)
+    files = [os.path.join(spack.spack_root, path) for path in files]
+
+    # There will likely be other files that have changed
+    # when these tests are run
+    assert flake8_package in files
+
+
+# As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported
+# http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html
+@pytest.mark.skipif(
+    sys.version_info[:2] <= (2, 6) or
+    (3, 0) <= sys.version_info[:2] <= (3, 3),
+    reason='flake8 no longer supports Python 2.6 or 3.3 and older')
+def test_flake8(parser, flake8_package):
+    # Only test the flake8_package that we modified
+    # Otherwise, the unit tests would fail every time
+    # the flake8 tests fail
+    args = parser.parse_args([flake8_package])
+
+    flake8(parser, args)
+
+    # Get even more coverage
+    args = parser.parse_args(['--output', '--root-relative', flake8_package])
+
+    flake8(parser, args)