diff options
26 files changed, 815 insertions, 114 deletions
diff --git a/SConstruct b/SConstruct index dbe813eede4..6ff444f2c43 100644 --- a/SConstruct +++ b/SConstruct @@ -2983,6 +2983,9 @@ def doLint( env , target , source ): if not buildscripts.clang_format.lint_all(None): raise Exception("clang-format lint errors") + import buildscripts.pylinters + buildscripts.pylinters.lint_all(None, {}, []) + import buildscripts.lint if not buildscripts.lint.run_lint( [ "src/mongo/" ] ): raise Exception( "lint errors" ) diff --git a/buildscripts/.pydocstyle b/buildscripts/.pydocstyle new file mode 100644 index 00000000000..fdff3f6c351 --- /dev/null +++ b/buildscripts/.pydocstyle @@ -0,0 +1,7 @@ +# See https://readthedocs.org/projects/pydocstyle/ +[pydocstyle] +inherit = false +# D202 - No blank lines allowed after function docstring +# D203 - 1 blank line required before class docstring +# D212 - Multi-line docstring summary should start at the first line +ignore = D202,D203,D212 diff --git a/buildscripts/.pylintrc b/buildscripts/.pylintrc new file mode 100644 index 00000000000..686543fb789 --- /dev/null +++ b/buildscripts/.pylintrc @@ -0,0 +1,10 @@ +# See https://www.pylint.org/ +[MESSAGES CONTROL] +# C0301 - line-too-long - some of the type annotations are longer then 100 columns +# E0401 - import-error - ignore imports that fail to load +# I0011 - locally-disabled - ignore warnings about disable pylint checks +# R0903 - too-few-public-method - pylint does not always know best +# W0511 - fixme - ignore TODOs in comments +# W0611 - unused-import - typing module is needed for mypy + +disable=fixme,import-error,line-too-long,locally-disabled,too-few-public-methods,unused-import diff --git a/buildscripts/.style.yapf b/buildscripts/.style.yapf new file mode 100644 index 00000000000..12534bdeead --- /dev/null +++ b/buildscripts/.style.yapf @@ -0,0 +1,6 @@ +# See https://github.com/google/yapf +[style] +based_on_style = pep8 +column_limit = 100 +indent_dictionary_value = True + diff --git a/buildscripts/idl/idl/ast.py b/buildscripts/idl/idl/ast.py index ff55418a7a6..1337958b718 100644 --- a/buildscripts/idl/idl/ast.py +++ b/buildscripts/idl/idl/ast.py @@ -23,7 +23,7 @@ the structs that need code generated for them, and just enough information to do from __future__ import absolute_import, print_function, unicode_literals -#from typing import List, Union, Any, Optional, Tuple +from typing import List, Union, Any, Optional, Tuple from . import common from . import errors diff --git a/buildscripts/idl/idl/binder.py b/buildscripts/idl/idl/binder.py index 067eea93364..39aa0ee4b63 100644 --- a/buildscripts/idl/idl/binder.py +++ b/buildscripts/idl/idl/binder.py @@ -17,7 +17,7 @@ from __future__ import absolute_import, print_function, unicode_literals import re -# from typing import Union +from typing import Union from . import ast from . import bson diff --git a/buildscripts/idl/idl/bson.py b/buildscripts/idl/idl/bson.py index 570a2cecab2..66106e0c8cd 100644 --- a/buildscripts/idl/idl/bson.py +++ b/buildscripts/idl/idl/bson.py @@ -20,7 +20,7 @@ Utilities for validating bson types, etc. from __future__ import absolute_import, print_function, unicode_literals -# from typing import Dict, List +from typing import Dict, List # Dictionary of BSON type Information # scalar: True if the type is not an array or object diff --git a/buildscripts/idl/idl/compiler.py b/buildscripts/idl/idl/compiler.py index f285f91e627..5dea3166f74 100644 --- a/buildscripts/idl/idl/compiler.py +++ b/buildscripts/idl/idl/compiler.py @@ -23,7 +23,7 @@ from __future__ import absolute_import, print_function, unicode_literals import io import logging import os -# from typing import Any, List +from typing import Any, List from . import binder from . import errors diff --git a/buildscripts/idl/idl/errors.py b/buildscripts/idl/idl/errors.py index 872f24641d8..86e3b119d88 100644 --- a/buildscripts/idl/idl/errors.py +++ b/buildscripts/idl/idl/errors.py @@ -23,7 +23,7 @@ from __future__ import absolute_import, print_function, unicode_literals import inspect import sys -# from typing import List, Union, Any +from typing import List, Union, Any from yaml import nodes import yaml diff --git a/buildscripts/idl/idl/generator.py b/buildscripts/idl/idl/generator.py index d8a34c1cc4c..d458572a717 100644 --- a/buildscripts/idl/idl/generator.py +++ b/buildscripts/idl/idl/generator.py @@ -21,7 +21,7 @@ import os import string import sys import textwrap -# from typing import List, Union +from typing import List, Union from . import ast from . import bson @@ -640,10 +640,9 @@ class _CppSourceFileWriter(_CppFileWriterBase): # type: (ast.Struct) -> None """Generate the C++ deserializer method definition.""" - with self._block( - '%s %s::parse(const IDLParserErrorContext& ctxt, const BSONObj& bsonObject) {' % - (_title_case(struct.name), _title_case(struct.name)), '}'): - + func_def = '%s %s::parse(const IDLParserErrorContext& ctxt, const BSONObj& bsonObject)' % ( + _title_case(struct.name), _title_case(struct.name)) + with self._block('%s {' % (func_def), '}'): self._writer.write_line('%s object;' % _title_case(struct.name)) field_usage_check = _FieldUsageChecker(self._writer) diff --git a/buildscripts/idl/idl/parser.py b/buildscripts/idl/idl/parser.py index c945142ce7f..ee37a0f600b 100644 --- a/buildscripts/idl/idl/parser.py +++ b/buildscripts/idl/idl/parser.py @@ -20,7 +20,7 @@ Only validates the document is syntatically correct, not semantically. """ from __future__ import absolute_import, print_function, unicode_literals -# from typing import Any, Callable, Dict, List, Set, Union +from typing import Any, Callable, Dict, List, Set, Union from yaml import nodes import yaml diff --git a/buildscripts/idl/idl/syntax.py b/buildscripts/idl/idl/syntax.py index bd0e255ff8f..0470a52e0cf 100644 --- a/buildscripts/idl/idl/syntax.py +++ b/buildscripts/idl/idl/syntax.py @@ -22,7 +22,7 @@ it follows the rules of the IDL, etc. from __future__ import absolute_import, print_function, unicode_literals -# from typing import List, Union, Any, Optional, Tuple +from typing import List, Union, Any, Optional, Tuple from . import common from . import errors diff --git a/buildscripts/idl/tests/context.py b/buildscripts/idl/tests/context.py index ee38a44e30f..bf83a640c81 100644 --- a/buildscripts/idl/tests/context.py +++ b/buildscripts/idl/tests/context.py @@ -19,9 +19,9 @@ import sys sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))) -import idl.ast -import idl.binder -import idl.errors -import idl.generator -import idl.parser -import idl.syntax +import idl.ast # pylint: disable=wrong-import-position +import idl.binder # pylint: disable=wrong-import-position +import idl.errors # pylint: disable=wrong-import-position +import idl.generator # pylint: disable=wrong-import-position +import idl.parser # pylint: disable=wrong-import-position +import idl.syntax # pylint: disable=wrong-import-position diff --git a/buildscripts/idl/tests/test_binder.py b/buildscripts/idl/tests/test_binder.py index 6b13e3598b9..612a2eb8960 100644 --- a/buildscripts/idl/tests/test_binder.py +++ b/buildscripts/idl/tests/test_binder.py @@ -241,7 +241,7 @@ class TestBinder(testcase.IDLTestcase): description: foo cpp_type: %s bson_serialization_type: int - """ % (cpp_type)), idl.errors.ERROR_ID_BAD_NUMERIC_CPP_TYPE) + """ % (std_cpp_type)), idl.errors.ERROR_ID_BAD_NUMERIC_CPP_TYPE) # Test bindata_subtype missing self.assert_bind_fail( diff --git a/buildscripts/linter/base.py b/buildscripts/linter/base.py new file mode 100644 index 00000000000..ae78d520664 --- /dev/null +++ b/buildscripts/linter/base.py @@ -0,0 +1,74 @@ +"""Base class and support functions for linters.""" +from __future__ import absolute_import +from __future__ import print_function + +from abc import ABCMeta, abstractmethod +from typing import Dict, List, Optional + + +class LinterBase(object): + """Base Class for all linters.""" + + __metaclass__ = ABCMeta + + def __init__(self, cmd_name, required_version): + # type: (str, str) -> None + """ + Create a linter. + + cmd_name - short friendly name + required_version - the required version string to check against + """ + self.cmd_name = cmd_name + self.required_version = required_version + + @abstractmethod + def get_lint_cmd_args(self, file_name): + # type: (str) -> List[str] + """Get the command to run a linter.""" + pass + + def get_fix_cmd_args(self, file_name): + # type: (str) -> Optional[List[str]] + # pylint: disable=no-self-use,unused-argument + """Get the command to run a linter fix.""" + return None + + @abstractmethod + def get_lint_version_cmd_args(self): + # type: () -> List[str] + """Get the command to run a linter version check.""" + pass + + def needs_file_diff(self): + # type: () -> bool + # pylint: disable=no-self-use + """ + Check if we need to diff the output of this linter with the original file. + + This applies to tools like clang-format and yapf which do not have a notion of linting. We + introduce the idea of linting by formatting a file with the tool to standard out and + comparing it to the original. + """ + return False + + def ignore_interpreter(self): + # type: () -> bool + # pylint: disable=no-self-use + """ + Check if we should ignore the interpreter when searching for the linter to run. + + This applies to mypy specifically since the pylinters are executed under Python 2 but mypy + is executed by python 3. + """ + return False + + +class LinterInstance(object): + """A pair of a Linter and the full path of the linter cmd to run.""" + + def __init__(self, linter, cmd_path): + # type: (LinterBase, List[str]) -> None + """Construct a LinterInstance.""" + self.linter = linter + self.cmd_path = cmd_path diff --git a/buildscripts/linter/git.py b/buildscripts/linter/git.py index c8f95149750..45b54bbd960 100644 --- a/buildscripts/linter/git.py +++ b/buildscripts/linter/git.py @@ -1,3 +1,4 @@ +"""Git Utility functions.""" from __future__ import absolute_import from __future__ import print_function @@ -5,6 +6,7 @@ import itertools import os import re import subprocess +from typing import Any, Callable, List, Tuple from buildscripts import moduleconfig from buildscripts.resmokelib.utils import globstar @@ -13,26 +15,31 @@ from buildscripts.resmokelib.utils import globstar # Has to match the string in SConstruct MODULE_DIR = "src/mongo/db/modules" + def get_base_dir(): - """Get the base directory for mongo repo. - This script assumes that it is running in buildscripts/, and uses - that to find the base directory. + # type: () -> str + """ + Get the base directory for mongo repo. + + This script assumes that it is running in buildscripts/, and uses + that to find the base directory. """ try: return subprocess.check_output(['git', 'rev-parse', '--show-toplevel']).rstrip() - except: + except subprocess.CalledProcessError: # We are not in a valid git directory. Use the script path instead. return os.path.dirname(os.path.dirname(os.path.realpath(__file__))) + def get_repos(): - """Get a list of Repos to check clang-format for - """ + # type: () -> List[Repo] + """Get a list of Repos to check linters for.""" base_dir = get_base_dir() # Get a list of modules # TODO: how do we filter rocks, does it matter? mongo_modules = moduleconfig.discover_module_directories( - os.path.join(base_dir, MODULE_DIR), None) + os.path.join(base_dir, MODULE_DIR), None) paths = [os.path.join(base_dir, MODULE_DIR, m) for m in mongo_modules] @@ -40,41 +47,50 @@ def get_repos(): return [Repo(p) for p in paths] + class Repo(object): - """Class encapsulates all knowledge about a git repository, and its metadata - to run clang-format. - """ + """Class encapsulates all knowledge about a git repository, and its metadata to run linters.""" + def __init__(self, path): + # type: (str) -> None + """Construct a repo object.""" self.path = path def _callgito(self, args): - """Call git for this repository, and return the captured output - """ + # type: (List[str]) -> str + """Call git for this repository, and return the captured output.""" # These two flags are the equivalent of -C in newer versions of Git # but we use these to support versions pre 1.8.5 but it depends on the command # and what the current directory is if "ls-files" in args: # This command depends on the current directory and works better if not run with # work-tree - return subprocess.check_output(['git', '--git-dir', os.path.join(self.path, ".git")] - + args) + return subprocess.check_output(['git', '--git-dir', os.path.join(self.path, ".git")] + + args) else: - return subprocess.check_output(['git', '--git-dir', os.path.join(self.path, ".git"), - '--work-tree', self.path] + args) + return subprocess.check_output([ + 'git', '--git-dir', os.path.join(self.path, ".git"), '--work-tree', self.path + ] + args) def _callgit(self, args): - """Call git for this repository without capturing output + # type: (List[str]) -> int + """ + Call git for this repository without capturing output. + This is designed to be used when git returns non-zero exit codes. """ # These two flags are the equivalent of -C in newer versions of Git # but we use these to support versions pre 1.8.5 but it depends on the command # and what the current directory is - return subprocess.call(['git', '--git-dir', os.path.join(self.path, ".git"), - '--work-tree', self.path] + args) + return subprocess.call([ + 'git', + '--git-dir', + os.path.join(self.path, ".git"), + ] + args) def _get_local_dir(self, path): - """Get a directory path relative to the git root directory - """ + # type: (str) -> str + """Get a directory path relative to the git root directory.""" if os.path.isabs(path): path = os.path.relpath(path, self.path) @@ -84,13 +100,16 @@ class Repo(object): return path def get_candidates(self, candidates, filter_function): - """Get the set of candidate files to check by querying the repository + # type: (List[str], Callable[[str], bool]) -> List[str] + """ + Get the set of candidate files to check by querying the repository. Returns the full path to the file for clang-format to consume. """ if candidates is not None and len(candidates) > 0: candidates = [self._get_local_dir(f) for f in candidates] - valid_files = list(set(candidates).intersection(self.get_candidate_files(filter_function))) + valid_files = list( + set(candidates).intersection(self.get_candidate_files(filter_function))) else: valid_files = list(self.get_candidate_files(filter_function)) @@ -100,29 +119,31 @@ class Repo(object): return valid_files def _git_ls_files(self, cmd, filter_function): - """Run git-ls-files and filter the list of files to a valid candidate list - """ + # type: (List[str], Callable[[str], bool]) -> List[str] + """Run git-ls-files and filter the list of files to a valid candidate list.""" gito = self._callgito(cmd) # This allows us to pick all the interesting files # in the mongo and mongo-enterprise repos - file_list = [line.rstrip() - for line in gito.splitlines() if filter_function(line.rstrip())] + file_list = [line.rstrip() for line in gito.splitlines() if filter_function(line.rstrip())] return file_list def get_candidate_files(self, filter_function): - """Query git to get a list of all files in the repo to consider for analysis - """ + # type: (Callable[[str], bool]) -> List[str] + """Query git to get a list of all files in the repo to consider for analysis.""" return self._git_ls_files(["ls-files", "--cached"], filter_function) def get_working_tree_candidate_files(self, filter_function): - """Query git to get a list of all files in the working tree to consider for analysis - """ + # type: (Callable[[str], bool]) -> List[str] + # pylint: disable=invalid-name + """Query git to get a list of all files in the working tree to consider for analysis.""" return self._git_ls_files(["ls-files", "--cached", "--others"], filter_function) def get_working_tree_candidates(self, filter_function): - """Get the set of candidate files to check by querying the repository + # type: (Callable[[str], bool]) -> List[str] + """ + Get the set of candidate files to check by querying the repository. Returns the full path to the file for clang-format to consume. """ @@ -137,44 +158,47 @@ class Repo(object): return valid_files def is_detached(self): - """Is the current working tree in a detached HEAD state? - """ + # type: () -> bool + """Return true if the current working tree in a detached HEAD state.""" # symbolic-ref returns 1 if the repo is in a detached HEAD state - return self._callgit(["symbolic-ref", "--quiet", "HEAD"]) + return self._callgit(["symbolic-ref", "--quiet", "HEAD"]) == 1 def is_ancestor(self, parent, child): - """Is the specified parent hash an ancestor of child hash? - """ + # type: (str, str) -> bool + """Return true if the specified parent hash an ancestor of child hash.""" # merge base returns 0 if parent is an ancestor of child return not self._callgit(["merge-base", "--is-ancestor", parent, child]) def is_commit(self, sha1): - """Is the specified hash a valid git commit? - """ + # type: (str) -> bool + """Return true if the specified hash is a valid git commit.""" # cat-file -e returns 0 if it is a valid hash return not self._callgit(["cat-file", "-e", "%s^{commit}" % sha1]) def is_working_tree_dirty(self): - """Does the current working tree have changes? - """ + # type: () -> bool + """Return true the current working tree have changes.""" # diff returns 1 if the working tree has local changes - return self._callgit(["diff", "--quiet"]) + return self._callgit(["diff", "--quiet"]) == 1 def does_branch_exist(self, branch): - """Does the branch exist? - """ + # type: (str) -> bool + """Return true if the branch exists.""" # rev-parse returns 0 if the branch exists return not self._callgit(["rev-parse", "--verify", branch]) def get_merge_base(self, commit): - """Get the merge base between 'commit' and HEAD - """ + # type: (str) -> str + """Get the merge base between 'commit' and HEAD.""" return self._callgito(["merge-base", "HEAD", commit]).rstrip() def get_branch_name(self): - """Get the current branch name, short form - This returns "master", not "refs/head/master" - Will not work if the current branch is detached + # type: () -> str + """ + Get the current branch name, short form. + + This returns "master", not "refs/head/master". + Will not work if the current branch is detached. """ branch = self.rev_parse(["--abbrev-ref", "HEAD"]) if branch == "HEAD": @@ -183,87 +207,100 @@ class Repo(object): return branch def add(self, command): - """git add wrapper - """ + # type: (List[str]) -> str + """Git add wrapper.""" return self._callgito(["add"] + command) def checkout(self, command): - """git checkout wrapper - """ + # type: (List[str]) -> str + """Git checkout wrapper.""" return self._callgito(["checkout"] + command) def commit(self, command): - """git commit wrapper - """ + # type: (List[str]) -> str + """Git commit wrapper.""" return self._callgito(["commit"] + command) def diff(self, command): - """git diff wrapper - """ + # type: (List[str]) -> str + """Git diff wrapper.""" return self._callgito(["diff"] + command) def log(self, command): - """git log wrapper - """ + # type: (List[str]) -> str + """Git log wrapper.""" return self._callgito(["log"] + command) def rev_parse(self, command): - """git rev-parse wrapper - """ + # type: (List[str]) -> str + """Git rev-parse wrapper.""" return self._callgito(["rev-parse"] + command).rstrip() def rm(self, command): - """git rm wrapper - """ + # type: (List[str]) -> str + # pylint: disable=invalid-name + """Git rm wrapper.""" return self._callgito(["rm"] + command) def show(self, command): - """git show wrapper - """ + # type: (List[str]) -> str + """Git show wrapper.""" return self._callgito(["show"] + command) def expand_file_string(glob_pattern): - """Expand a string that represents a set of files - """ + # type: (str) -> List[str] + """Expand a string that represents a set of files.""" return [os.path.abspath(f) for f in globstar.iglob(glob_pattern)] + def get_files_to_check_working_tree(filter_function): - """Get a list of files to check from the working tree. - This will pick up files not managed by git. + # type: (Callable[[str], bool]) -> List[str] + """ + Get a list of files to check from the working tree. + + This will pick up files not managed by git. """ repos = get_repos() - valid_files = list(itertools.chain.from_iterable([r.get_working_tree_candidates(filter_function) for r in repos])) + valid_files = list( + itertools.chain.from_iterable( + [r.get_working_tree_candidates(filter_function) for r in repos])) return valid_files + def get_files_to_check(files, filter_function): - """Get a list of files that need to be checked - based on which files are managed by git. - """ + # type: (List[str], Callable[[str], bool]) -> List[str] + """Get a list of files that need to be checked based on which files are managed by git.""" # Get a list of candidate_files - candidates = [expand_file_string(f) for f in files] - candidates = list(itertools.chain.from_iterable(candidates)) + candidates_nested = [expand_file_string(f) for f in files] + candidates = list(itertools.chain.from_iterable(candidates_nested)) if len(files) > 0 and len(candidates) == 0: - raise ValueError("Globs '%s' did not find any files." % (files)) + raise ValueError("Globs '%s' did not find any files with glob." % (files)) repos = get_repos() - valid_files = list(itertools.chain.from_iterable([r.get_candidates(candidates, filter_function) for r in repos])) + valid_files = list( + itertools.chain.from_iterable( + [r.get_candidates(candidates, filter_function) for r in repos])) + + if len(files) > 0 and len(valid_files) == 0: + raise ValueError("Globs '%s' did not find any files with glob in git." % (files)) return valid_files + def get_files_to_check_from_patch(patches, filter_function): - """Take a patch file generated by git diff, and scan the patch for a list of files to check. - """ - candidates = [] + # type: (List[str], Callable[[str], bool]) -> List[str] + """Take a patch file generated by git diff, and scan the patch for a list of files to check.""" + candidates = [] # type: List[str] # Get a list of candidate_files check = re.compile(r"^diff --git a\/([\w\/\.\-]+) b\/[\w\/\.\-]+") - lines = [] + lines = [] # type: List[str] for patch in patches: with open(patch, "rb") as infile: lines += infile.readlines() @@ -272,6 +309,8 @@ def get_files_to_check_from_patch(patches, filter_function): repos = get_repos() - valid_files = list(itertools.chain.from_iterable([r.get_candidates(candidates, filter_function) for r in repos])) + valid_files = list( + itertools.chain.from_iterable( + [r.get_candidates(candidates, filter_function) for r in repos])) return valid_files diff --git a/buildscripts/linter/mypy.py b/buildscripts/linter/mypy.py new file mode 100644 index 00000000000..9526b388c75 --- /dev/null +++ b/buildscripts/linter/mypy.py @@ -0,0 +1,47 @@ +"""Mypy linter support module.""" +from __future__ import absolute_import +from __future__ import print_function + +from typing import List + +from . import base + + +class MypyLinter(base.LinterBase): + """Mypy linter.""" + + def __init__(self): + # type: () -> None + """Create a mypy linter.""" + super(MypyLinter, self).__init__("mypy", "mypy 0.501") + + def get_lint_version_cmd_args(self): + # type: () -> List[str] + """Get the command to run a linter version check.""" + return ["--version"] + + def get_lint_cmd_args(self, file_name): + # type: (str) -> List[str] + """Get the command to run a linter.""" + # -py2 - Check Python 2 code for type annotations in comments + # --disallow-untyped-defs - Error if any code is missing type annotations + # --ignore-missing-imports - Do not error if imports are not found. This can be a problem + # with standalone scripts and relative imports. This will limit effectiveness but avoids + # mypy complaining about running code. + # --follow-imports=silent - Do not error on imported files since all imported files may not + # be mypy clean + return [ + "--py2", "--disallow-untyped-defs", "--ignore-missing-imports", + "--follow-imports=silent", file_name + ] + + def ignore_interpreter(self): + # type: () -> bool + # pylint: disable=no-self-use + """ + Check if we should ignore the interpreter when searching for the linter to run. + + This applies to mypy specifically since the pylinters are executed under Python 2 but mypy + is executed by python 3. + """ + return True diff --git a/buildscripts/linter/parallel.py b/buildscripts/linter/parallel.py index 95fee2c7c3b..0648bfb16e7 100644 --- a/buildscripts/linter/parallel.py +++ b/buildscripts/linter/parallel.py @@ -1,3 +1,4 @@ +"""Utility code to execute code in parallel.""" from __future__ import absolute_import from __future__ import print_function @@ -5,16 +6,18 @@ import Queue import threading import time from multiprocessing import cpu_count +from typing import Any, Callable, List + def parallel_process(items, func): - """Run a set of work items to completion - """ + # type: (List[Any], Callable[[Any], bool]) -> bool + """Run a set of work items to completion and wait.""" try: cpus = cpu_count() except NotImplementedError: cpus = 1 - task_queue = Queue.Queue() + task_queue = Queue.Queue() # type: Queue.Queue # Use a list so that worker function will capture this variable pp_event = threading.Event() @@ -22,8 +25,8 @@ def parallel_process(items, func): pp_lock = threading.Lock() def worker(): - """Worker thread to process work items in parallel - """ + # type: () -> None + """Worker thread to process work items in parallel.""" while not pp_event.is_set(): try: item = task_queue.get_nowait() @@ -52,7 +55,7 @@ def parallel_process(items, func): # Process all the work threads = [] - for cpu in range(cpus): + for _ in range(cpus): thread = threading.Thread(target=worker) thread.daemon = True diff --git a/buildscripts/linter/pydocstyle.py b/buildscripts/linter/pydocstyle.py new file mode 100644 index 00000000000..9420d42354c --- /dev/null +++ b/buildscripts/linter/pydocstyle.py @@ -0,0 +1,26 @@ +"""PyDocStyle linter support module.""" +from __future__ import absolute_import +from __future__ import print_function + +from typing import List + +from . import base + + +class PyDocstyleLinter(base.LinterBase): + """PyDocStyle linter.""" + + def __init__(self): + # type: () -> None + """Create a pydocstyle linter.""" + super(PyDocstyleLinter, self).__init__("pydocstyle", "1.1.1") + + def get_lint_version_cmd_args(self): + # type: () -> List[str] + """Get the command to run a linter version check.""" + return ["--version"] + + def get_lint_cmd_args(self, file_name): + # type: (str) -> List[str] + """Get the command to run a linter.""" + return [file_name] diff --git a/buildscripts/linter/pylint.py b/buildscripts/linter/pylint.py new file mode 100644 index 00000000000..056465501d8 --- /dev/null +++ b/buildscripts/linter/pylint.py @@ -0,0 +1,36 @@ +"""PyLint linter support module.""" +from __future__ import absolute_import +from __future__ import print_function + +import os +from typing import List + +from . import base +from . import git + + +class PyLintLinter(base.LinterBase): + """Pylint linter.""" + + def __init__(self): + # type: () -> None + """Create a pylint linter.""" + self._rc_file = os.path.join( + os.path.normpath(git.get_base_dir()), "buildscripts", ".pylintrc") + super(PyLintLinter, self).__init__("pylint", "pylint 1.6.5") + + def get_lint_version_cmd_args(self): + # type: () -> List[str] + """Get the command to run a linter version check.""" + return ["--version"] + + def get_lint_cmd_args(self, file_name): + # type: (str) -> List[str] + """Get the command to run a linter.""" + # pylintrc only searches parent directories if it is a part of a module, and since our code + # is split across different modules, and individual script files, we need to specify the + # path to the rcfile. + # See https://pylint.readthedocs.io/en/latest/user_guide/run.html + return [ + "--rcfile=%s" % (self._rc_file), "--output-format", "msvs", "--reports=n", file_name + ] diff --git a/buildscripts/linter/runner.py b/buildscripts/linter/runner.py new file mode 100644 index 00000000000..8b80f93cdbb --- /dev/null +++ b/buildscripts/linter/runner.py @@ -0,0 +1,195 @@ +"""Class to support running various linters in a common framework.""" +from __future__ import absolute_import +from __future__ import print_function + +import difflib +import logging +import os +import re +import subprocess +import sys +import threading +from typing import Dict, List, Optional + +from . import base + + +def _check_version(linter, cmd_path, args): + # type: (base.LinterBase, List[str], List[str]) -> bool + """Check if the given linter has the correct version.""" + + try: + cmd = cmd_path + args + logging.debug(str(cmd)) + output = subprocess.check_output(cmd) + + required_version = re.escape(linter.required_version) + pattern = r"\b%s\b" % (required_version) + if not re.search(pattern, output): + logging.info("Linter %s has wrong version for '%s'. Expected '%s', received '%s'", + linter.cmd_name, cmd, required_version, output) + return False + except OSError as os_error: + # The WindowsError exception is thrown if the command is not found. + # We catch OSError since WindowsError does not exist on non-Windows platforms. + logging.info("Version check command [%s] failed: %s", cmd, os_error) + return False + except subprocess.CalledProcessError as cpe: + logging.info("Version check command [%s] failed:\n%s", cmd, cpe.output) + return False + + return True + + +def _find_linter(linter, config_dict): + # type: (base.LinterBase, Dict[str,str]) -> Optional[base.LinterInstance] + """ + Look for a linter command with the required version. + + Return a LinterInstance with the location of the linter binary if a linter binary with the + matching version is found. None otherwise. + """ + + if linter.cmd_name in config_dict and config_dict[linter.cmd_name] is not None: + cmd = [config_dict[linter.cmd_name]] + + # If the user specified a tool location, we do not search any further + if _check_version(linter, cmd, linter.get_lint_version_cmd_args()): + return base.LinterInstance(linter, cmd) + return None + + # Search for tool + # 1. In the same directory as the interpreter + # 2. The current path + python_dir = os.path.dirname(sys.executable) + if sys.platform == "win32": + # On Windows, these scripts are installed in %PYTHONDIR%\scripts like + # 'C:\Python27\scripts', and have .exe extensions. + python_dir = os.path.join(python_dir, "scripts") + + cmd_str = os.path.join(python_dir, linter.cmd_name) + cmd_str += ".exe" + cmd = [cmd_str] + else: + # On Mac and with Homebrew, check for the binaries in /usr/local instead of sys.executable. + if sys.platform == 'darwin' and python_dir.startswith('/usr/local/opt'): + python_dir = '/usr/local/bin' + + # On Linux, these scripts are installed in %PYTHONDIR%\bin like + # '/opt/mongodbtoolchain/v2/bin', but they may point to the wrong interpreter. + cmd_str = os.path.join(python_dir, linter.cmd_name) + + if linter.ignore_interpreter(): + # Some linters use a different interpreter then the current interpreter. + cmd = [cmd_str] + else: + cmd = [sys.executable, cmd_str] + + # Check 1: interpreter location + if _check_version(linter, cmd, linter.get_lint_version_cmd_args()): + return base.LinterInstance(linter, cmd) + + # Check 2: current path + cmd = [linter.cmd_name] + if _check_version(linter, cmd, linter.get_lint_version_cmd_args()): + return base.LinterInstance(linter, cmd) + + return None + + +def find_linters(linter_list, config_dict): + # type: (List[base.LinterBase], Dict[str,str]) -> List[base.LinterInstance] + """Find the location of all linters.""" + + linter_instances = [] # type: List[base.LinterInstance] + for linter in linter_list: + linter_instance = _find_linter(linter, config_dict) + if not linter_instance: + logging.error("Could not find correct version of linter '%s', expected '%s'", + linter.cmd_name, linter.required_version) + return None + + linter_instances.append(linter_instance) + + return linter_instances + + +class LintRunner(object): + """Run a linter and print results in a thread safe manner.""" + + def __init__(self): + # type: () -> None + """Create a Lint Runner.""" + self.print_lock = threading.Lock() + + def _safe_print(self, line): + # type: (str) -> None + """ + Print a line of text under a lock. + + Take a lock to ensure diffs do not get mixed when printed to the screen. + """ + with self.print_lock: + print(line) + + def run_lint(self, linter, file_name): + # type: (base.LinterInstance, str) -> bool + """Run the specified linter for the file.""" + + cmd = linter.cmd_path + linter.linter.get_lint_cmd_args(file_name) + logging.debug(str(cmd)) + + try: + if linter.linter.needs_file_diff(): + # Need a file diff + with open(file_name, 'rb') as original_text: + original_file = original_text.read() + + formatted_file = subprocess.check_output(cmd) + if original_file != formatted_file: + original_lines = original_file.splitlines() + formatted_lines = formatted_file.splitlines() + result = difflib.unified_diff(original_lines, formatted_lines) + + # Take a lock to ensure diffs do not get mixed when printed to the screen + with self.print_lock: + print("ERROR: Found diff for " + file_name) + print("To fix formatting errors, run pylinters.py fix %s" % (file_name)) + + count = 0 + for line in result: + print(line.rstrip()) + count += 1 + + if count == 0: + print("ERROR: The files only differ in trailing whitespace? LF vs CRLF") + + return False + else: + output = subprocess.check_output(cmd) + + # On Windows, mypy.bat returns 0 even if there are length failures so we need to + # check if there was any output + if output and sys.platform == "win32": + self._safe_print("CMD [%s] output:\n%s" % (cmd, output)) + return False + + except subprocess.CalledProcessError as cpe: + self._safe_print("CMD [%s] failed:\n%s" % (cmd, cpe.output)) + return False + + return True + + def run(self, cmd): + # type: (List[str]) -> bool + """Check the specified cmd succeeds.""" + + logging.debug(str(cmd)) + + try: + subprocess.check_output(cmd) + except subprocess.CalledProcessError as cpe: + self._safe_print("CMD [%s] failed:\n%s" % (cmd, cpe.output)) + return False + + return True diff --git a/buildscripts/linter/yapf.py b/buildscripts/linter/yapf.py new file mode 100644 index 00000000000..86955981092 --- /dev/null +++ b/buildscripts/linter/yapf.py @@ -0,0 +1,36 @@ +"""YAPF linter support module.""" +from __future__ import absolute_import +from __future__ import print_function + +from typing import List + +from . import base + + +class YapfLinter(base.LinterBase): + """Yapf linter.""" + + def __init__(self): + # type: () -> None + """Create a yapf linter.""" + super(YapfLinter, self).__init__("yapf", "yapf 0.16.0") + + def get_lint_version_cmd_args(self): + # type: () -> List[str] + """Get the command to run a linter version check.""" + return ["--version"] + + def needs_file_diff(self): + # type: () -> bool + """See comment in base class.""" + return True + + def get_lint_cmd_args(self, file_name): + # type: (str) -> List[str] + """Get the command to run a linter.""" + return [file_name] + + def get_fix_cmd_args(self, file_name): + # type: (str) -> List[str] + """Get the command to run a linter fix.""" + return ["-i", file_name] diff --git a/buildscripts/moduleconfig.py b/buildscripts/moduleconfig.py index 7f98667b6b5..eece68bcdc3 100644 --- a/buildscripts/moduleconfig.py +++ b/buildscripts/moduleconfig.py @@ -23,6 +23,7 @@ alter those programs' behavior. MongoDB module SConscript files can describe libraries, programs and unit tests, just as other MongoDB SConscript files do. """ +from __future__ import print_function __all__ = ('discover_modules', 'discover_module_directories', 'configure_modules', 'register_module_test') @@ -53,11 +54,11 @@ def discover_modules(module_root, allowed_modules): module = None if allowed_modules is not None and name not in allowed_modules: - print "skipping module: %s" % name + print("skipping module: %s" % (name)) continue if os.path.isfile(build_py): - print "adding module: %s" % name + print("adding module: %s" % (name)) fp = open(build_py, "r") try: module = imp.load_module("module_" + name, fp, build_py, @@ -91,11 +92,11 @@ def discover_module_directories(module_root, allowed_modules): build_py = os.path.join(root, 'build.py') if allowed_modules is not None and name not in allowed_modules: - print "skipping module: %s" % name + print("skipping module: %s" % (name)) continue if os.path.isfile(build_py): - print "adding module: %s" % name + print("adding module: %s" % (name)) found_modules.append(name) return found_modules @@ -108,7 +109,7 @@ def configure_modules(modules, conf): """ for module in modules: name = module.name - print "configuring module: %s" % name + print("configuring module: %s" % (name)) root = os.path.dirname(module.__file__) module.configure(conf, conf.env) diff --git a/buildscripts/pylinters.py b/buildscripts/pylinters.py new file mode 100755 index 00000000000..539979e7dfe --- /dev/null +++ b/buildscripts/pylinters.py @@ -0,0 +1,210 @@ +#!/usr/bin/env python2 +"""Extensible script to run one or more Python Linters across a subset of files in parallel.""" +from __future__ import absolute_import +from __future__ import print_function + +import argparse +import logging +import os +import sys +import threading +from abc import ABCMeta, abstractmethod +from typing import Any, Dict, List + +# Get relative imports to work when the package is not installed on the PYTHONPATH. +if __name__ == "__main__" and __package__ is None: + sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(os.path.realpath(__file__))))) + +from buildscripts.linter import base # pylint: disable=wrong-import-position +from buildscripts.linter import git # pylint: disable=wrong-import-position +from buildscripts.linter import mypy # pylint: disable=wrong-import-position +from buildscripts.linter import parallel # pylint: disable=wrong-import-position +from buildscripts.linter import pydocstyle # pylint: disable=wrong-import-position +from buildscripts.linter import pylint # pylint: disable=wrong-import-position +from buildscripts.linter import runner # pylint: disable=wrong-import-position +from buildscripts.linter import yapf # pylint: disable=wrong-import-position + +# List of supported linters +_LINTERS = [ + yapf.YapfLinter(), + pylint.PyLintLinter(), + pydocstyle.PyDocstyleLinter(), + mypy.MypyLinter(), +] + + +def get_py_linter(linter_filter): + # type: (str) -> List[base.LinterBase] + """ + Get a list of linters to use. + + 'all' or None - select all linters + 'a,b,c' - a comma delimited list is describes a list of linters to choose + """ + if linter_filter is None or linter_filter == "all": + return _LINTERS + + linter_list = linter_filter.split(",") + + linter_candidates = [linter for linter in _LINTERS if linter.cmd_name in linter_list] + + if len(linter_candidates) == 0: + raise ValueError("No linters found for filter '%s'" % (linter_filter)) + + return linter_candidates + + +def is_interesting_file(file_name): + # type: (str) -> bool + """"Return true if this file should be checked.""" + return file_name.endswith(".py") and (file_name.startswith("buildscripts/idl") or + file_name.startswith("buildscripts/linter") or + file_name.startswith("buildscripts/pylinters.py")) + + +def _get_build_dir(): + # type: () -> str + """Get the location of the scons' build directory in case we need to download clang-format.""" + return os.path.join(git.get_base_dir(), "build") + + +def _lint_files(linters, config_dict, file_names): + # type: (str, Dict[str, str], List[str]) -> None + """Lint a list of files with clang-format.""" + linter_list = get_py_linter(linters) + + lint_runner = runner.LintRunner() + + linter_instances = runner.find_linters(linter_list, config_dict) + if not linter_instances: + sys.exit(1) + + for linter in linter_instances: + run_fix = lambda param1: lint_runner.run_lint(linter, param1) # pylint: disable=cell-var-from-loop + lint_clean = parallel.parallel_process([os.path.abspath(f) for f in file_names], run_fix) + + if not lint_clean: + print("ERROR: Code Style does not match coding style") + sys.exit(1) + + +def lint_patch(linters, config_dict, file_name): + # type: (str, Dict[str, str], List[str]) -> None + """Lint patch command entry point.""" + file_names = git.get_files_to_check_from_patch(file_name, is_interesting_file) + + # Patch may have files that we do not want to check which is fine + if file_names: + _lint_files(linters, config_dict, file_names) + + +def lint(linters, config_dict, file_names): + # type: (str, Dict[str, str], List[str]) -> None + """Lint files command entry point.""" + all_file_names = git.get_files_to_check(file_names, is_interesting_file) + + _lint_files(linters, config_dict, all_file_names) + + +def lint_all(linters, config_dict, file_names): + # type: (str, Dict[str, str], List[str]) -> None + # pylint: disable=unused-argument + """Lint files command entry point based on working tree.""" + all_file_names = git.get_files_to_check_working_tree(is_interesting_file) + + _lint_files(linters, config_dict, all_file_names) + + +def _fix_files(linters, config_dict, file_names): + # type: (str, Dict[str, str], List[str]) -> None + """Fix a list of files with linters if possible.""" + linter_list = get_py_linter(linters) + + # Get a list of linters which return a valid command for get_fix_cmd() + fix_list = [fixer for fixer in linter_list if fixer.get_fix_cmd_args("ignore")] + + if len(fix_list) == 0: + raise ValueError("Cannot find any linters '%s' that support fixing." % (linters)) + + lint_runner = runner.LintRunner() + + linter_instances = runner.find_linters(fix_list, config_dict) + if not linter_instances: + sys.exit(1) + + for linter in linter_instances: + run_linter = lambda param1: lint_runner.run(linter.cmd_path + linter.linter.get_fix_cmd_args(param1)) # pylint: disable=cell-var-from-loop + + lint_clean = parallel.parallel_process([os.path.abspath(f) for f in file_names], run_linter) + + if not lint_clean: + print("ERROR: Code Style does not match coding style") + sys.exit(1) + + +def fix_func(linters, config_dict, file_names): + # type: (str, Dict[str, str], List[str]) -> None + """Fix files command entry point.""" + all_file_names = git.get_files_to_check(file_names, is_interesting_file) + + _fix_files(linters, config_dict, all_file_names) + + +def main(): + # type: () -> None + """Main entry point.""" + + parser = argparse.ArgumentParser(description='PyLinter frontend.') + + linters = get_py_linter(None) + + dest_prefix = "linter_" + for linter1 in linters: + msg = 'Path to linter %s' % (linter1.cmd_name) + parser.add_argument( + '--' + linter1.cmd_name, type=str, help=msg, dest=dest_prefix + linter1.cmd_name) + + parser.add_argument( + '--linters', + type=str, + help="Comma separated list of filters to use, defaults to 'all'", + default="all") + + parser.add_argument('-v', "--verbose", action='store_true', help="Enable verbose logging") + + sub = parser.add_subparsers(title="Linter subcommands", help="sub-command help") + + parser_lint = sub.add_parser('lint', help='Lint only Git files') + parser_lint.add_argument("file_names", nargs="*", help="Globs of files to check") + parser_lint.set_defaults(func=lint) + + parser_lint_all = sub.add_parser('lint-all', help='Lint All files') + parser_lint_all.add_argument("file_names", nargs="*", help="Globs of files to check") + parser_lint_all.set_defaults(func=lint_all) + + parser_lint_patch = sub.add_parser('lint-patch', help='Lint the files in a patch') + parser_lint_patch.add_argument("file_names", nargs="*", help="Globs of files to check") + parser_lint_patch.set_defaults(func=lint_patch) + + parser_fix = sub.add_parser('fix', help='Fix files if possible') + parser_fix.add_argument("file_names", nargs="*", help="Globs of files to check") + parser_fix.set_defaults(func=fix_func) + + args = parser.parse_args() + + # Create a dictionary of linter locations if the user needs to override the location of a + # linter. This is common for mypy on Windows for instance. + config_dict = {} + for key in args.__dict__: + if key.startswith("linter_"): + name = key.replace(dest_prefix, "") + config_dict[name] = args.__dict__[key] + + if args.verbose: + logging.basicConfig(level=logging.DEBUG) + + args.func(args.linters, config_dict, args.file_names) + + +if __name__ == "__main__": + main() diff --git a/buildscripts/requirements.txt b/buildscripts/requirements.txt index 1b6662698d5..39f5814987d 100644 --- a/buildscripts/requirements.txt +++ b/buildscripts/requirements.txt @@ -1,3 +1,11 @@ pyyaml == 3.11 -typing == 3.5 unittest-xml-reporting == 2.1.0 +# Linters +yapf == 0.16.0 +mypy == 0.501 ; python_version > "3" +# typing in Python 2 for mypy +typing == 3.6.1; python_version < "3" +pylint == 1.6.5 +pydocstyle == 1.1.1 +# resmoke.py +-r resmokelib/requirements.txt diff --git a/buildscripts/resmokelib/testing/testcases.py b/buildscripts/resmokelib/testing/testcases.py index 731226e181b..0f665a8f06e 100644 --- a/buildscripts/resmokelib/testing/testcases.py +++ b/buildscripts/resmokelib/testing/testcases.py @@ -299,7 +299,8 @@ class JSTestCase(TestCase): def run(self): try: threading.Thread.run(self) - except Exception as self.err: + except Exception as e1: + self.err = e1 raise else: self.err = None |