From 58f2a947db98bdecb061133a7b6780fa405fbd33 Mon Sep 17 00:00:00 2001
From: "Adam J. Stewart" <ajstewart426@gmail.com>
Date: Tue, 25 Apr 2017 13:01:25 -0500
Subject: [PATCH] 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
---
.flake8 | 4 +-
lib/spack/spack/cmd/flake8.py | 91 +++++++++++-------
lib/spack/spack/test/cmd/flake8.py | 95 +++++++++++++++++++
.../builtin.mock/packages/boost/package.py | 83 ++++++++++++++++
...-that-probably-only-affects-one-user.patch | 1 +
.../builtin.mock/packages/flake8/package.py | 79 +++++++++++++++
.../repos/builtin/packages/metis/package.py | 4 +-
.../repos/builtin/packages/qt/package.py | 8 +-
8 files changed, 324 insertions(+), 41 deletions(-)
create mode 100644 lib/spack/spack/test/cmd/flake8.py
create mode 100644 var/spack/repos/builtin.mock/packages/boost/package.py
create mode 100644 var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch
create mode 100644 var/spack/repos/builtin.mock/packages/flake8/package.py
diff --git a/.flake8 b/.flake8
index 023f392952..e697d5ea04 100644
--- a/.flake8
+++ b/.flake8
@@ -11,12 +11,12 @@
# - E272: multiple spaces before keyword
#
# Let people use terse Python features:
-# - E731 : lambda expressions
+# - E731: lambda expressions
#
# Spack allows wildcard imports:
# - F403: disable wildcard import
#
-# These are required to get the package.py files to test clean.
+# 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
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 @@
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 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 @@
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 prefix_relative(path):
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 prefix_relative(path):
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 prefix_relative(path):
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)
diff --git a/var/spack/repos/builtin.mock/packages/boost/package.py b/var/spack/repos/builtin.mock/packages/boost/package.py
new file mode 100644
index 0000000000..5d86c4559f
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/boost/package.py
@@ -0,0 +1,83 @@
+##############################################################################
+# 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
+##############################################################################
+from spack import *
+
+
+class Boost(Package):
+ """Fake boost package."""
+
+ homepage = "http://www.boost.org"
+ url = "http://downloads.sourceforge.net/project/boost/boost/1.63.0/boost_1_63_0.tar.bz2"
+
+ version('1.63.0', '1c837ecd990bb022d07e7aab32b09847')
+
+ default_install_libs = set(['atomic',
+ 'chrono',
+ 'date_time',
+ 'filesystem',
+ 'graph',
+ 'iostreams',
+ 'locale',
+ 'log',
+ 'math',
+ 'program_options',
+ 'random',
+ 'regex',
+ 'serialization',
+ 'signals',
+ 'system',
+ 'test',
+ 'thread',
+ 'timer',
+ 'wave'])
+
+ # mpi/python are not installed by default because they pull in many
+ # dependencies and/or because there is a great deal of customization
+ # possible (and it would be difficult to choose sensible defaults)
+ default_noinstall_libs = set(['mpi', 'python'])
+
+ all_libs = default_install_libs | default_noinstall_libs
+
+ for lib in all_libs:
+ variant(lib, default=(lib not in default_noinstall_libs),
+ description="Compile with {0} library".format(lib))
+
+ variant('debug', default=False,
+ description='Switch to the debug version of Boost')
+ variant('shared', default=True,
+ description="Additionally build shared libraries")
+ variant('multithreaded', default=True,
+ description="Build multi-threaded versions of libraries")
+ variant('singlethreaded', default=False,
+ description="Build single-threaded versions of libraries")
+ variant('icu', default=False,
+ description="Build with Unicode and ICU suport")
+ variant('graph', default=False,
+ description="Build the Boost Graph library")
+ variant('taggedlayout', default=False,
+ description="Augment library names with build options")
+
+ def install(self, spec, prefix):
+ pass
diff --git a/var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch b/var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch
new file mode 100644
index 0000000000..b8c9065e4b
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch
@@ -0,0 +1 @@
+Fake patch
diff --git a/var/spack/repos/builtin.mock/packages/flake8/package.py b/var/spack/repos/builtin.mock/packages/flake8/package.py
new file mode 100644
index 0000000000..bd062d71bc
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/flake8/package.py
@@ -0,0 +1,79 @@
+##############################################################################
+# 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
+##############################################################################
+from spack import *
+
+
+class Flake8(Package):
+ """Package containing as many PEP 8 violations as possible.
+ All of these violations are exceptions that we allow in
+ package.py files."""
+
+ # Used to tell whether or not the package has been modified
+ state = 'unmodified'
+
+ # Make sure pre-existing noqa is not interfered with
+ blatant_violation = 'line-that-has-absolutely-no-execuse-for-being-over-79-characters' # noqa
+ blatant_violation = 'line-that-has-absolutely-no-execuse-for-being-over-79-characters' # noqa: E501
+
+ # Keywords exempt from line-length checks
+ homepage = '#####################################################################'
+ url = '#####################################################################'
+ git = '#####################################################################'
+ svn = '#####################################################################'
+ hg = '#####################################################################'
+ list_url = '#####################################################################'
+
+ # URL strings exempt from line-length checks
+ # http://########################################################################
+ # https://#######################################################################
+ # ftp://#########################################################################
+ # file://########################################################################
+
+ # Directives exempt from line-length checks
+ version('2.0', '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef')
+ version('1.0', '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef')
+
+ variant('super-awesome-feature', default=True, description='Enable super awesome feature')
+ variant('somewhat-awesome-feature', default=False, description='Enable somewhat awesome feature')
+
+ provides('lapack', when='@2.0+super-awesome-feature+somewhat-awesome-feature')
+
+ extends('python', ignore='bin/(why|does|every|package|that|depends|on|numpy|need|to|copy|f2py3?)')
+
+ depends_on('boost+atomic+chrono+date_time~debug+filesystem~graph~icu+iostreams+locale+log+math~mpi+multithreaded+program_options~python+random+regex+serialization+shared+signals~singlethreaded+system~taggedlayout+test+thread+timer+wave')
+
+ conflicts('+super-awesome-feature', when='%intel@16:17+somewhat-awesome-feature')
+
+ resource(name='Deez-Nuts', destination='White-House', placement='President', when='@2020', url='www.elect-deez-nuts.com')
+
+ patch('hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch', when='%gcc@3.2.2:3.2.3')
+
+ def install(self, spec, prefix):
+ pass
+
+ # '@when' decorated functions are exempt from redefinition errors
+ @when('@2.0')
+ def install(self, spec, prefix):
+ pass
diff --git a/var/spack/repos/builtin/packages/metis/package.py b/var/spack/repos/builtin/packages/metis/package.py
index c927c22a1b..454bb97037 100644
--- a/var/spack/repos/builtin/packages/metis/package.py
+++ b/var/spack/repos/builtin/packages/metis/package.py
@@ -84,7 +84,7 @@ def patch(self):
filter_file('#define MAX_JBUFS 128', '#define MAX_JBUFS 24',
join_path(source_path, 'GKlib', 'error.c'))
- @when('@:4') # noqa: F811
+ @when('@:4')
def install(self, spec, prefix):
# Process library spec and options
if any('+{0}'.format(v) in spec for v in ['gdb', 'int64', 'real64']):
@@ -175,7 +175,7 @@ def install(self, spec, prefix):
Executable(test_bin('mesh2dual'))(test_graph('metis.mesh'))
"""
- @when('@5:') # noqa: F811
+ @when('@5:')
def install(self, spec, prefix):
source_directory = self.stage.source_path
build_directory = join_path(source_directory, 'build')
diff --git a/var/spack/repos/builtin/packages/qt/package.py b/var/spack/repos/builtin/packages/qt/package.py
index b27bc3fe07..b9c7e1f5c3 100644
--- a/var/spack/repos/builtin/packages/qt/package.py
+++ b/var/spack/repos/builtin/packages/qt/package.py
@@ -251,7 +251,7 @@ def common_config_args(self):
# Don't disable all the database drivers, but should
# really get them into spack at some point.
- @when('@3') # noqa: F811
+ @when('@3')
def configure(self):
# A user reported that this was necessary to link Qt3 on ubuntu.
# However, if LD_LIBRARY_PATH is not set the qt build fails, check
@@ -268,7 +268,7 @@ def configure(self):
'-release',
'-fast')
- @when('@4') # noqa: F811
+ @when('@4')
def configure(self):
configure('-fast',
'-{0}gtkstyle'.format('' if '+gtk' in self.spec else 'no-'),
@@ -276,7 +276,7 @@ def configure(self):
'-arch', str(self.spec.architecture.target),
*self.common_config_args)
- @when('@5.0:5.6') # noqa: F811
+ @when('@5.0:5.6')
def configure(self):
webkit_args = [] if '+webkit' in self.spec else ['-skip', 'qtwebkit']
configure('-no-eglfs',
@@ -284,7 +284,7 @@ def configure(self):
'-{0}gtkstyle'.format('' if '+gtk' in self.spec else 'no-'),
*(webkit_args + self.common_config_args))
- @when('@5.7:') # noqa: F811
+ @when('@5.7:')
def configure(self):
config_args = self.common_config_args
--
GitLab