From 58f2a947db98bdecb061133a7b6780fa405fbd33 Mon Sep 17 00:00:00 2001 From: "Adam J. Stewart" Date: Tue, 25 Apr 2017 13:01:25 -0500 Subject: 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 ++++++++++++++++++++++ .../repos/builtin.mock/packages/boost/package.py | 83 +++++++++++++++++++ ...m-bug-that-probably-only-affects-one-user.patch | 1 + .../repos/builtin.mock/packages/flake8/package.py | 79 ++++++++++++++++++ var/spack/repos/builtin/packages/metis/package.py | 4 +- var/spack/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 @@ 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) 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 @@ class Metis(Package): 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 @@ class Metis(Package): 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 @@ class Qt(Package): # 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 @@ class Qt(Package): '-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 @@ class Qt(Package): '-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 @@ class Qt(Package): '-{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 -- cgit v1.2.3-70-g09d2