diff options
-rw-r--r-- | PREUPLOAD.cfg | 19 | ||||
-rw-r--r-- | README.md | 28 | ||||
-rwxr-xr-x | pre-upload.py | 29 | ||||
-rw-r--r-- | rh/config.py | 249 | ||||
-rwxr-xr-x | rh/config_unittest.py | 102 | ||||
-rw-r--r-- | rh/git.py | 6 | ||||
-rw-r--r-- | rh/hooks.py | 65 | ||||
-rwxr-xr-x | rh/hooks_unittest.py | 43 | ||||
-rwxr-xr-x | tools/android_test_mapping_format.py | 2 | ||||
-rwxr-xr-x | tools/clang-format.py | 2 | ||||
-rwxr-xr-x | tools/cpplint.py | 56 | ||||
-rwxr-xr-x | tools/google-java-format.py | 2 |
12 files changed, 431 insertions, 172 deletions
diff --git a/PREUPLOAD.cfg b/PREUPLOAD.cfg index 03350c7..81f505d 100644 --- a/PREUPLOAD.cfg +++ b/PREUPLOAD.cfg @@ -1,21 +1,14 @@ [Hook Scripts] # Only list fast unittests here. -py2_config_unittest = python2 ./rh/config_unittest.py -py3_config_unittest = python3 ./rh/config_unittest.py -py2_hooks_unittest = python2 ./rh/hooks_unittest.py -py3_hooks_unittest = python3 ./rh/hooks_unittest.py -py2_shell_unittest = python2 ./rh/shell_unittest.py -py3_shell_unittest = python3 ./rh/shell_unittest.py -py2_utils_unittest = python2 ./rh/utils_unittest.py -py3_utils_unittest = python3 ./rh/utils_unittest.py -py2_android_test_mapping_format_unittest = python2 ./tools/android_test_mapping_format_unittest.py -py3_android_test_mapping_format_unittest = python3 ./tools/android_test_mapping_format_unittest.py -py2_config_test = python2 ./rh/config_test.py --check-env --commit-id ${PREUPLOAD_COMMIT} --commit-msg ${PREUPLOAD_COMMIT_MESSAGE} --repo-root ${REPO_ROOT} -- ${PREUPLOAD_FILES} -py3_config_test = python3 ./rh/config_test.py --check-env --commit-id ${PREUPLOAD_COMMIT} --commit-msg ${PREUPLOAD_COMMIT_MESSAGE} --repo-root ${REPO_ROOT} -- ${PREUPLOAD_FILES} +config_unittest = ./rh/config_unittest.py +hooks_unittest = ./rh/hooks_unittest.py +shell_unittest = ./rh/shell_unittest.py +utils_unittest = ./rh/utils_unittest.py +android_test_mapping_format_unittest = ./tools/android_test_mapping_format_unittest.py +config_test = ./rh/config_test.py --check-env --commit-id ${PREUPLOAD_COMMIT} --commit-msg ${PREUPLOAD_COMMIT_MESSAGE} --repo-root ${REPO_ROOT} -- ${PREUPLOAD_FILES} [Builtin Hooks] commit_msg_bug_field = true commit_msg_changeid_field = true commit_msg_test_field = true -pylint2 = true pylint3 = true @@ -227,6 +227,34 @@ See [Placeholders](#Placeholders) for variables you can expand automatically. cpplint = --filter=-x ${PREUPLOAD_FILES} ``` +## [Builtin Hooks Exclude Paths] + +*** note +This section can only be added to the repo project-wide settings +[GLOBAL-PREUPLOAD.cfg]. +*** + +Used to explicitly exclude some projects when processing a hook. With this +section, it is possible to define a hook that should apply to the majority of +projects except a few. + +An entry must completely match the project's `REPO_PATH`. The paths can use the +[shell-style wildcards](https://docs.python.org/library/fnmatch.html) and +quotes. For advanced cases, it is possible to use a [regular +expression](https://docs.python.org/howto/regex.html) by using the `^` prefix. + +``` +[Builtin Hooks Exclude Paths] +# Run cpplint on all projects except ones under external/ and vendor/. +# The "external" and "vendor" projects, if they exist, will still run cpplint. +cpplint = external/* vendor/* + +# Run rustfmt on all projects except ones under external/. All projects under +# hardware/ will be excluded except for ones starting with hardware/google (due to +# the negative regex match). +rustfmt = external/ ^hardware/(!?google) +``` + ## [Tool Paths] Some builtin hooks need to call external executables to work correctly. By diff --git a/pre-upload.py b/pre-upload.py index 0d0afb6..e7ef564 100755 --- a/pre-upload.py +++ b/pre-upload.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python3 # -*- coding:utf-8 -*- # Copyright 2016 The Android Open Source Project # @@ -29,19 +29,9 @@ import sys # Assert some minimum Python versions as we don't test or support any others. -# We only support Python 2.7, and require 2.7.5+/3.4+ to include signal fix: -# https://bugs.python.org/issue14173 -if sys.version_info < (2, 7, 5): - print('repohooks: error: Python-2.7.5+ is required', file=sys.stderr) +if sys.version_info < (3, 5): + print('repohooks: error: Python-3.5+ is required', file=sys.stderr) sys.exit(1) -elif sys.version_info.major == 3 and sys.version_info < (3, 4): - # We don't actually test <Python-3.6. Hope for the best! - print('repohooks: error: Python-3.4+ is required', file=sys.stderr) - sys.exit(1) -elif sys.version_info < (3, 6): - # We want to get people off of old versions of Python. - print('repohooks: warning: Python-3.6+ is going to be required; ' - 'please upgrade soon to maintain support.', file=sys.stderr) _path = os.path.dirname(os.path.realpath(__file__)) @@ -213,7 +203,7 @@ def _get_project_config(): # Load the config for this git repo. '.', ) - return rh.config.PreUploadConfig(paths=paths, global_paths=global_paths) + return rh.config.PreUploadSettings(paths=paths, global_paths=global_paths) def _attempt_fixes(fixup_func_list, commit_list): @@ -284,16 +274,17 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, commit_list=None): (e,)) return False + project = rh.Project(name=project_name, dir=proj_dir, remote=remote) + rel_proj_dir = os.path.relpath(proj_dir, rh.git.find_repo_root()) + os.environ.update({ 'REPO_LREV': rh.git.get_commit_for_ref(upstream_branch), - 'REPO_PATH': os.path.relpath(proj_dir, rh.git.find_repo_root()), + 'REPO_PATH': rel_proj_dir, 'REPO_PROJECT': project_name, 'REPO_REMOTE': remote, 'REPO_RREV': rh.git.get_remote_revision(upstream_branch, remote), }) - project = rh.Project(name=project_name, dir=proj_dir, remote=remote) - if not commit_list: commit_list = rh.git.get_commits( ignore_merged_commits=config.ignore_merged_commits) @@ -311,8 +302,10 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, commit_list=None): commit_summary = desc.split('\n', 1)[0] output.commit_start(commit=commit, commit_summary=commit_summary) - for name, hook in hooks: + for name, hook, exclusion_scope in hooks: output.hook_start(name) + if rel_proj_dir in exclusion_scope: + break hook_results = hook(project, commit, desc, diff) (error, warning) = _process_hook_results(hook_results) if error is not None or warning is not None: diff --git a/rh/config.py b/rh/config.py index 6e96fc7..e2ad713 100644 --- a/rh/config.py +++ b/rh/config.py @@ -18,6 +18,7 @@ from __future__ import print_function import functools +import itertools import os import shlex import sys @@ -41,99 +42,97 @@ class ValidationError(Error): """Config file has unknown sections/keys or other values.""" +# Sentinel so we can handle None-vs-unspecified. +_UNSET = object() + + class RawConfigParser(configparser.RawConfigParser): """Like RawConfigParser but with some default helpers.""" - @staticmethod - def _check_args(name, cnt_min, cnt_max, args): - cnt = len(args) - if cnt not in (0, cnt_max - cnt_min): - raise TypeError('%s() takes %i or %i arguments (got %i)' % - (name, cnt_min, cnt_max, cnt,)) - return cnt - - # pylint can't seem to grok our use of *args here. + # pylint doesn't like it when we extend the API. # pylint: disable=arguments-differ - def options(self, section, *args): - """Return the options in |section| (with default |args|). + def options(self, section, default=_UNSET): + """Return the options in |section|. Args: section: The section to look up. - args: What to return if |section| does not exist. + default: What to return if |section| does not exist. """ - cnt = self._check_args('options', 2, 3, args) try: return configparser.RawConfigParser.options(self, section) except configparser.NoSectionError: - if cnt == 1: - return args[0] + if default is not _UNSET: + return default raise - def get(self, section, option, *args): - """Return the value for |option| in |section| (with default |args|).""" - cnt = self._check_args('get', 3, 4, args) + def get(self, section, option, default=_UNSET): + """Return the value for |option| in |section| (with |default|).""" try: return configparser.RawConfigParser.get(self, section, option) except (configparser.NoSectionError, configparser.NoOptionError): - if cnt == 1: - return args[0] + if default is not _UNSET: + return default raise - def items(self, section, *args): + def items(self, section=_UNSET, default=_UNSET): """Return a list of (key, value) tuples for the options in |section|.""" - cnt = self._check_args('items', 2, 3, args) + if section is _UNSET: + # Python 3 compat logic. Return a dict of section-to-options. + if sys.version_info.major < 3: + return [(x, self.items(x)) for x in self.sections()] + return super(RawConfigParser, self).items() + try: return configparser.RawConfigParser.items(self, section) except configparser.NoSectionError: - if cnt == 1: - return args[0] + if default is not _UNSET: + return default raise + if sys.version_info.major < 3: + def read_dict(self, dictionary): + """Store |dictionary| into ourselves.""" + for section, settings in dictionary.items(): + for option, value in settings: + if not self.has_section(section): + self.add_section(section) + self.set(section, option, value) -class PreUploadConfig(object): - """Config file used for per-project `repo upload` hooks.""" - FILENAME = 'PREUPLOAD.cfg' - GLOBAL_FILENAME = 'GLOBAL-PREUPLOAD.cfg' +class PreUploadConfig(object): + """A single (abstract) config used for `repo upload` hooks.""" CUSTOM_HOOKS_SECTION = 'Hook Scripts' BUILTIN_HOOKS_SECTION = 'Builtin Hooks' BUILTIN_HOOKS_OPTIONS_SECTION = 'Builtin Hooks Options' + BUILTIN_HOOKS_EXCLUDE_SECTION = 'Builtin Hooks Exclude Paths' TOOL_PATHS_SECTION = 'Tool Paths' OPTIONS_SECTION = 'Options' + VALID_SECTIONS = { + CUSTOM_HOOKS_SECTION, + BUILTIN_HOOKS_SECTION, + BUILTIN_HOOKS_OPTIONS_SECTION, + BUILTIN_HOOKS_EXCLUDE_SECTION, + TOOL_PATHS_SECTION, + OPTIONS_SECTION, + } OPTION_IGNORE_MERGED_COMMITS = 'ignore_merged_commits' - VALID_OPTIONS = (OPTION_IGNORE_MERGED_COMMITS,) + VALID_OPTIONS = {OPTION_IGNORE_MERGED_COMMITS} - def __init__(self, paths=('',), global_paths=()): + def __init__(self, config=None, source=None): """Initialize. - All the config files found will be merged together in order. - Args: - paths: The directories to look for config files. - global_paths: The directories to look for global config files. + config: A configparse.ConfigParser instance. + source: Where this config came from. This is used in error messages to + facilitate debugging. It is not necessarily a valid path. """ - config = RawConfigParser() - - def _search(paths, filename): - for path in paths: - path = os.path.join(path, filename) - if os.path.exists(path): - self.paths.append(path) - try: - config.read(path) - except configparser.ParsingError as e: - raise ValidationError('%s: %s' % (path, e)) - - self.paths = [] - _search(global_paths, self.GLOBAL_FILENAME) - _search(paths, self.FILENAME) - - self.config = config - - self._validate() + self.config = config if config else RawConfigParser() + self.source = source + if config: + self._validate() @property def custom_hooks(self): @@ -155,26 +154,35 @@ class PreUploadConfig(object): return shlex.split(self.config.get(self.BUILTIN_HOOKS_OPTIONS_SECTION, hook, '')) + def builtin_hook_exclude_paths(self, hook): + """List of paths for which |hook| should not be executed.""" + return shlex.split(self.config.get(self.BUILTIN_HOOKS_EXCLUDE_SECTION, + hook, '')) + @property def tool_paths(self): """List of all tool paths.""" return dict(self.config.items(self.TOOL_PATHS_SECTION, ())) def callable_hooks(self): - """Yield a name and callback for each hook to be executed.""" + """Yield a CallableHook for each hook to be executed.""" + scope = rh.hooks.ExclusionScope([]) for hook in self.custom_hooks: options = rh.hooks.HookOptions(hook, self.custom_hook(hook), self.tool_paths) - yield (hook, functools.partial(rh.hooks.check_custom, - options=options)) + func = functools.partial(rh.hooks.check_custom, options=options) + yield rh.hooks.CallableHook(hook, func, scope) for hook in self.builtin_hooks: options = rh.hooks.HookOptions(hook, self.builtin_hook_option(hook), self.tool_paths) - yield (hook, functools.partial(rh.hooks.BUILTIN_HOOKS[hook], - options=options)) + func = functools.partial(rh.hooks.BUILTIN_HOOKS[hook], + options=options) + scope = rh.hooks.ExclusionScope( + self.builtin_hook_exclude_paths(hook)) + yield rh.hooks.CallableHook(hook, func, scope) @property def ignore_merged_commits(self): @@ -184,28 +192,25 @@ class PreUploadConfig(object): self.OPTION_IGNORE_MERGED_COMMITS, None), False) + def update(self, preupload_config): + """Merge settings from |preupload_config| into ourself.""" + self.config.read_dict(preupload_config.config) + def _validate(self): """Run consistency checks on the config settings.""" config = self.config # Reject unknown sections. - valid_sections = set(( - self.CUSTOM_HOOKS_SECTION, - self.BUILTIN_HOOKS_SECTION, - self.BUILTIN_HOOKS_OPTIONS_SECTION, - self.TOOL_PATHS_SECTION, - self.OPTIONS_SECTION, - )) - bad_sections = set(config.sections()) - valid_sections + bad_sections = set(config.sections()) - self.VALID_SECTIONS if bad_sections: raise ValidationError('%s: unknown sections: %s' % - (self.paths, bad_sections)) + (self.source, bad_sections)) # Reject blank custom hooks. for hook in self.custom_hooks: if not config.get(self.CUSTOM_HOOKS_SECTION, hook): raise ValidationError('%s: custom hook "%s" cannot be blank' % - (self.paths, hook)) + (self.source, hook)) # Reject unknown builtin hooks. valid_builtin_hooks = set(rh.hooks.BUILTIN_HOOKS.keys()) @@ -214,7 +219,7 @@ class PreUploadConfig(object): bad_hooks = hooks - valid_builtin_hooks if bad_hooks: raise ValidationError('%s: unknown builtin hooks: %s' % - (self.paths, bad_hooks)) + (self.source, bad_hooks)) elif config.has_section(self.BUILTIN_HOOKS_OPTIONS_SECTION): raise ValidationError('Builtin hook options specified, but missing ' 'builtin hook settings') @@ -224,7 +229,7 @@ class PreUploadConfig(object): bad_hooks = hooks - valid_builtin_hooks if bad_hooks: raise ValidationError('%s: unknown builtin hook options: %s' % - (self.paths, bad_hooks)) + (self.source, bad_hooks)) # Verify hooks are valid shell strings. for hook in self.custom_hooks: @@ -232,7 +237,7 @@ class PreUploadConfig(object): self.custom_hook(hook) except ValueError as e: raise ValidationError('%s: hook "%s" command line is invalid: ' - '%s' % (self.paths, hook, e)) + '%s' % (self.source, hook, e)) # Verify hook options are valid shell strings. for hook in self.builtin_hooks: @@ -240,7 +245,7 @@ class PreUploadConfig(object): self.builtin_hook_option(hook) except ValueError as e: raise ValidationError('%s: hook options "%s" are invalid: %s' % - (self.paths, hook, e)) + (self.source, hook, e)) # Reject unknown tools. valid_tools = set(rh.hooks.TOOL_PATHS.keys()) @@ -249,13 +254,105 @@ class PreUploadConfig(object): bad_tools = tools - valid_tools if bad_tools: raise ValidationError('%s: unknown tools: %s' % - (self.paths, bad_tools)) + (self.source, bad_tools)) # Reject unknown options. - valid_options = set(self.VALID_OPTIONS) if config.has_section(self.OPTIONS_SECTION): options = set(config.options(self.OPTIONS_SECTION)) - bad_options = options - valid_options + bad_options = options - self.VALID_OPTIONS if bad_options: raise ValidationError('%s: unknown options: %s' % - (self.paths, bad_options)) + (self.source, bad_options)) + + +class PreUploadFile(PreUploadConfig): + """A single config (file) used for `repo upload` hooks. + + This is an abstract class that requires subclasses to define the FILENAME + constant. + + Attributes: + path: The path of the file. + """ + FILENAME = None + + def __init__(self, path): + """Initialize. + + Args: + path: The config file to load. + """ + super(PreUploadFile, self).__init__(source=path) + + self.path = path + try: + self.config.read(path) + except configparser.ParsingError as e: + raise ValidationError('%s: %s' % (path, e)) + + self._validate() + + @classmethod + def from_paths(cls, paths): + """Search for files within paths that matches the class FILENAME. + + Args: + paths: List of directories to look for config files. + + Yields: + For each valid file found, an instance is created and returned. + """ + for path in paths: + path = os.path.join(path, cls.FILENAME) + if os.path.exists(path): + yield cls(path) + + +class LocalPreUploadFile(PreUploadFile): + """A single config file for a project (PREUPLOAD.cfg).""" + FILENAME = 'PREUPLOAD.cfg' + + def _validate(self): + super(LocalPreUploadFile, self)._validate() + + # Reject Exclude Paths section for local config. + if self.config.has_section(self.BUILTIN_HOOKS_EXCLUDE_SECTION): + raise ValidationError('%s: [%s] is not valid in local files' % + (self.path, + self.BUILTIN_HOOKS_EXCLUDE_SECTION)) + + +class GlobalPreUploadFile(PreUploadFile): + """A single config file for a repo (GLOBAL-PREUPLOAD.cfg).""" + FILENAME = 'GLOBAL-PREUPLOAD.cfg' + + +class PreUploadSettings(PreUploadConfig): + """Settings for `repo upload` hooks. + + This encompasses multiple config files and provides the final (merged) + settings for a particular project. + """ + + def __init__(self, paths=('',), global_paths=()): + """Initialize. + + All the config files found will be merged together in order. + + Args: + paths: The directories to look for config files. + global_paths: The directories to look for global config files. + """ + super(PreUploadSettings, self).__init__() + + self.paths = [] + for config in itertools.chain( + GlobalPreUploadFile.from_paths(global_paths), + LocalPreUploadFile.from_paths(paths)): + self.paths.append(config.path) + self.update(config) + + + # We validated configs in isolation, now do one final pass altogether. + self.source = '{%s}' % '|'.join(self.paths) + self._validate() diff --git a/rh/config_unittest.py b/rh/config_unittest.py index d51afdc..4b27c5a 100755 --- a/rh/config_unittest.py +++ b/rh/config_unittest.py @@ -39,37 +39,53 @@ import rh.config class PreUploadConfigTests(unittest.TestCase): """Tests for the PreUploadConfig class.""" + def testMissing(self): + """Instantiating a non-existent config file should be fine.""" + rh.config.PreUploadConfig() + + +class FileTestCase(unittest.TestCase): + """Helper class for tests cases to setup configuration files.""" + def setUp(self): self.tempdir = tempfile.mkdtemp() def tearDown(self): shutil.rmtree(self.tempdir) - def _write_config(self, data, filename=None): - """Helper to write out a config file for testing.""" - if filename is None: - filename = rh.config.PreUploadConfig.FILENAME + def _write_config(self, data, filename='temp.cfg'): + """Helper to write out a config file for testing. + + Returns: + Path to the file where the configuration was written. + """ path = os.path.join(self.tempdir, filename) with open(path, 'w') as fp: fp.write(data) + return path + + def _write_local_config(self, data): + """Helper to write out a local config file for testing.""" + return self._write_config( + data, filename=rh.config.LocalPreUploadFile.FILENAME) def _write_global_config(self, data): """Helper to write out a global config file for testing.""" - self._write_config( - data, filename=rh.config.PreUploadConfig.GLOBAL_FILENAME) + return self._write_config( + data, filename=rh.config.GlobalPreUploadFile.FILENAME) - def testMissing(self): - """Instantiating a non-existent config file should be fine.""" - rh.config.PreUploadConfig() + +class PreUploadFileTests(FileTestCase): + """Tests for the PreUploadFile class.""" def testEmpty(self): """Instantiating an empty config file should be fine.""" - self._write_config('') - rh.config.PreUploadConfig(paths=(self.tempdir,)) + path = self._write_config('') + rh.config.PreUploadFile(path) def testValid(self): """Verify a fully valid file works.""" - self._write_config("""# This be a comment me matey. + path = self._write_config("""# This be a comment me matey. [Hook Scripts] name = script --with "some args" @@ -82,39 +98,56 @@ cpplint = --some 'more args' [Options] ignore_merged_commits = true """) - rh.config.PreUploadConfig(paths=(self.tempdir,)) + rh.config.PreUploadFile(path) def testUnknownSection(self): """Reject unknown sections.""" - self._write_config('[BOOGA]') - self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig, - paths=(self.tempdir,)) + path = self._write_config('[BOOGA]') + self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile, + path) def testUnknownBuiltin(self): """Reject unknown builtin hooks.""" - self._write_config('[Builtin Hooks]\nbooga = borg!') - self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig, - paths=(self.tempdir,)) + path = self._write_config('[Builtin Hooks]\nbooga = borg!') + self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile, + path) def testEmptyCustomHook(self): """Reject empty custom hooks.""" - self._write_config('[Hook Scripts]\nbooga = \t \n') - self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig, - paths=(self.tempdir,)) + path = self._write_config('[Hook Scripts]\nbooga = \t \n') + self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile, + path) def testInvalidIni(self): """Reject invalid ini files.""" - self._write_config('[Hook Scripts]\n =') - self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig, - paths=(self.tempdir,)) + path = self._write_config('[Hook Scripts]\n =') + self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile, + path) def testInvalidString(self): """Catch invalid string quoting.""" - self._write_config("""[Hook Scripts] + path = self._write_config("""[Hook Scripts] name = script --'bad-quotes """) - self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig, - paths=(self.tempdir,)) + self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile, + path) + + +class LocalPreUploadFileTests(FileTestCase): + """Test for the LocalPreUploadFile class.""" + + def testInvalidSectionConfig(self): + """Reject local config that uses invalid sections.""" + path = self._write_config("""[Builtin Hooks Exclude Paths] +cpplint = external/ 'test directory' ^vendor/(?!google/) +""") + self.assertRaises(rh.config.ValidationError, + rh.config.LocalPreUploadFile, + path) + + +class PreUploadSettingsTests(FileTestCase): + """Tests for the PreUploadSettings class.""" def testGlobalConfigs(self): """Verify global configs stack properly.""" @@ -122,14 +155,21 @@ name = script --'bad-quotes commit_msg_bug_field = true commit_msg_changeid_field = true commit_msg_test_field = false""") - self._write_config("""[Builtin Hooks] + self._write_local_config("""[Builtin Hooks] commit_msg_bug_field = false commit_msg_test_field = true""") - config = rh.config.PreUploadConfig(paths=(self.tempdir,), - global_paths=(self.tempdir,)) + config = rh.config.PreUploadSettings(paths=(self.tempdir,), + global_paths=(self.tempdir,)) self.assertEqual(config.builtin_hooks, ['commit_msg_changeid_field', 'commit_msg_test_field']) + def testGlobalExcludeScope(self): + """Verify exclude scope is valid for global config.""" + self._write_global_config("""[Builtin Hooks Exclude Paths] +cpplint = external/ 'test directory' ^vendor/(?!google/) +""") + rh.config.PreUploadSettings(global_paths=(self.tempdir,)) + if __name__ == '__main__': unittest.main() @@ -167,12 +167,12 @@ def get_affected_files(commit): Returns: A list of modified/added (and perhaps deleted) files """ - return raw_diff(os.getcwd(), '%s^!' % commit) + return raw_diff(os.getcwd(), '%s^-' % commit) def get_commits(ignore_merged_commits=False): """Returns a list of commits for this review.""" - cmd = ['git', 'log', '%s..' % get_upstream_branch(), '--format=%H'] + cmd = ['git', 'rev-list', '%s..' % get_upstream_branch()] if ignore_merged_commits: cmd.append('--first-parent') return rh.utils.run(cmd, capture_output=True).stdout.split() @@ -180,7 +180,7 @@ def get_commits(ignore_merged_commits=False): def get_commit_desc(commit): """Returns the full commit message of a commit.""" - cmd = ['git', 'log', '--format=%B', commit + '^!'] + cmd = ['git', 'diff-tree', '-s', '--always', '--format=%B', commit] return rh.utils.run(cmd, capture_output=True).stdout diff --git a/rh/hooks.py b/rh/hooks.py index 7c13bf1..491da91 100644 --- a/rh/hooks.py +++ b/rh/hooks.py @@ -17,6 +17,8 @@ from __future__ import print_function +import collections +import fnmatch import json import os import platform @@ -141,6 +143,42 @@ class Placeholders(object): return _get_build_os_name() +class ExclusionScope(object): + """Exclusion scope for a hook. + + An exclusion scope can be used to determine if a hook has been disabled for + a specific project. + """ + + def __init__(self, scope): + """Initialize. + + Args: + scope: A list of shell-style wildcards (fnmatch) or regular + expression. Regular expressions must start with the ^ character. + """ + self._scope = [] + for path in scope: + if path.startswith('^'): + self._scope.append(re.compile(path)) + else: + self._scope.append(path) + + def __contains__(self, proj_dir): + """Checks if |proj_dir| matches the excluded paths. + + Args: + proj_dir: The relative path of the project. + """ + for exclusion_path in self._scope: + if hasattr(exclusion_path, 'match'): + if exclusion_path.match(proj_dir): + return True + elif fnmatch.fnmatch(proj_dir, exclusion_path): + return True + return False + + class HookOptions(object): """Holder class for hook options.""" @@ -199,6 +237,10 @@ class HookOptions(object): return self.expand_vars([tool_path])[0] +# A callable hook. +CallableHook = collections.namedtuple('CallableHook', ('name', 'hook', 'scope')) + + def _run(cmd, **kwargs): """Helper command for checks that tend to gather output.""" kwargs.setdefault('combine_stdout_stderr', True) @@ -792,8 +834,27 @@ def check_rustfmt(project, commit, _desc, diff, options=None): return None rustfmt = options.tool_path('rustfmt') - cmd = [rustfmt] + options.args(('--check', '${PREUPLOAD_FILES}',), filtered) - return _check_cmd('rustfmt', project, commit, cmd) + cmd = [rustfmt] + options.args((), filtered) + ret = [] + for d in filtered: + data = rh.git.get_file_content(commit, d.file) + result = _run(cmd, input=data) + # If the parsing failed, stdout will contain enough details on the + # location of the error. + if result.returncode: + ret.append(rh.results.HookResult( + 'rustfmt', project, commit, error=result.stdout, + files=(d.file,))) + continue + # TODO(b/164111102): rustfmt stable does not support --check on stdin. + # If no error is reported, compare stdin with stdout. + if data != result.stdout: + msg = ('To fix, please run: %s' % + rh.shell.cmd_to_str(cmd + [d.file])) + ret.append(rh.results.HookResult( + 'rustfmt', project, commit, error=msg, + files=(d.file,))) + return ret def check_xmllint(project, commit, _desc, diff, options=None): diff --git a/rh/hooks_unittest.py b/rh/hooks_unittest.py index 373e09f..12059f8 100755 --- a/rh/hooks_unittest.py +++ b/rh/hooks_unittest.py @@ -182,6 +182,28 @@ class PlaceholderTests(unittest.TestCase): self.assertEqual(self.replacer.get('BUILD_OS'), m.return_value) +class ExclusionScopeTests(unittest.TestCase): + """Verify behavior of ExclusionScope class.""" + + def testEmpty(self): + """Verify the in operator for an empty scope.""" + scope = rh.hooks.ExclusionScope([]) + self.assertNotIn('external/*', scope) + + def testGlob(self): + """Verify the in operator for a scope using wildcards.""" + scope = rh.hooks.ExclusionScope(['vendor/*', 'external/*']) + self.assertIn('external/tools', scope) + + def testRegex(self): + """Verify the in operator for a scope using regular expressions.""" + scope = rh.hooks.ExclusionScope(['^vendor/(?!google)', + 'external/*']) + self.assertIn('vendor/', scope) + self.assertNotIn('vendor/google/', scope) + self.assertIn('vendor/other/', scope) + + class HookOptionsTests(unittest.TestCase): """Verify behavior of HookOptions object.""" @@ -293,7 +315,7 @@ class BuiltinHooksTests(unittest.TestCase): """ # First call should do nothing as there are no files to check. ret = func(self.project, 'commit', 'desc', (), options=self.options) - self.assertEqual(ret, None) + self.assertIsNone(ret) self.assertFalse(mock_check.called) # Second call should include some checks. @@ -690,21 +712,21 @@ class BuiltinHooksTests(unittest.TestCase): # First call should do nothing as there are no files to check. ret = rh.hooks.check_gofmt( self.project, 'commit', 'desc', (), options=self.options) - self.assertEqual(ret, None) + self.assertIsNone(ret) self.assertFalse(mock_check.called) # Second call will have some results. diff = [rh.git.RawDiffEntry(file='foo.go')] ret = rh.hooks.check_gofmt( self.project, 'commit', 'desc', diff, options=self.options) - self.assertNotEqual(ret, None) + self.assertIsNotNone(ret) def test_jsonlint(self, mock_check, _mock_run): """Verify the jsonlint builtin hook.""" # First call should do nothing as there are no files to check. ret = rh.hooks.check_json( self.project, 'commit', 'desc', (), options=self.options) - self.assertEqual(ret, None) + self.assertIsNone(ret) self.assertFalse(mock_check.called) # TODO: Actually pass some valid/invalid json data down. @@ -725,8 +747,17 @@ class BuiltinHooksTests(unittest.TestCase): ('foo.py',)) def test_rustfmt(self, mock_check, _mock_run): - self._test_file_filter(mock_check, rh.hooks.check_rustfmt, - ('foo.rs',)) + # First call should do nothing as there are no files to check. + ret = rh.hooks.check_rustfmt( + self.project, 'commit', 'desc', (), options=self.options) + self.assertEqual(ret, None) + self.assertFalse(mock_check.called) + + # Second call will have some results. + diff = [rh.git.RawDiffEntry(file='lib.rs')] + ret = rh.hooks.check_rustfmt( + self.project, 'commit', 'desc', diff, options=self.options) + self.assertNotEqual(ret, None) def test_xmllint(self, mock_check, _mock_run): """Verify the xmllint builtin hook.""" diff --git a/tools/android_test_mapping_format.py b/tools/android_test_mapping_format.py index 14b02f5..47e09c5 100755 --- a/tools/android_test_mapping_format.py +++ b/tools/android_test_mapping_format.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python3 # -*- coding:utf-8 -*- # Copyright 2018 The Android Open Source Project # diff --git a/tools/clang-format.py b/tools/clang-format.py index 7fa5b10..958f543 100755 --- a/tools/clang-format.py +++ b/tools/clang-format.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python3 # -*- coding:utf-8 -*- # Copyright 2016 The Android Open Source Project # diff --git a/tools/cpplint.py b/tools/cpplint.py index c08cf6f..e99d661 100755 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -52,6 +52,12 @@ import sre_compile import string import sys import unicodedata +import sysconfig + +try: + xrange # Python 2 +except NameError: + xrange = range # Python 3 _USAGE = """ @@ -570,7 +576,7 @@ def ProcessHppHeadersOption(val): # Automatically append to extensions list so it does not have to be set 2 times _valid_extensions.update(_hpp_headers) except ValueError: - PrintUsage('Header extensions must be comma seperated list.') + PrintUsage('Header extensions must be comma separated list.') def IsHeaderExtension(file_extension): return file_extension in _hpp_headers @@ -1378,7 +1384,7 @@ def FindNextMultiLineCommentEnd(lines, lineix): def RemoveMultiLineCommentsFromRange(lines, begin, end): """Clears a range of lines for multi-line comments.""" - # Having // dummy comments makes the lines non-empty, so we will not get + # Having // <empty> comments makes the lines non-empty, so we will not get # unnecessary blank line warnings later in the code. for i in range(begin, end): lines[i] = '/**/' @@ -1752,7 +1758,7 @@ def CheckForCopyright(filename, lines, error): """Logs an error if no Copyright message appears at the top of the file.""" # We'll say it should occur by line 10. Don't forget there's a - # dummy line at the front. + # placeholder line at the front. for line in xrange(1, min(len(lines), 11)): if re.search(r'Copyright', lines[line], re.I): break else: # means no copyright line was found @@ -1847,8 +1853,8 @@ def GetHeaderGuardCPPVariable(filename): PathSplitToList(_root)) if _root_debug: - sys.stderr.write("_root lstrip (maybe_path=%s, file_path_from_root=%s," + - " _root=%s)\n" %(maybe_path, file_path_from_root, _root)) + sys.stderr.write(("_root lstrip (maybe_path=%s, file_path_from_root=%s," + + " _root=%s)\n") %(maybe_path, file_path_from_root, _root)) if maybe_path: return os.path.join(*maybe_path) @@ -1861,8 +1867,8 @@ def GetHeaderGuardCPPVariable(filename): PathSplitToList(root_abspath)) if _root_debug: - sys.stderr.write("_root prepend (maybe_path=%s, full_path=%s, " + - "root_abspath=%s)\n" %(maybe_path, full_path, root_abspath)) + sys.stderr.write(("_root prepend (maybe_path=%s, full_path=%s, " + + "root_abspath=%s)\n") %(maybe_path, full_path, root_abspath)) if maybe_path: return os.path.join(*maybe_path) @@ -3278,8 +3284,8 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): line = clean_lines.elided[linenum] # You shouldn't have spaces before your brackets, except maybe after - # 'delete []' or 'return []() {};' - if Search(r'\w\s+\[', line) and not Search(r'(?:delete|return)\s+\[', line): + # 'delete []', 'return []() {};', or 'auto [abc, ...] = ...;'. + if Search(r'\w\s+\[', line) and not Search(r'(?:auto&?|delete|return)\s+\[', line): error(filename, linenum, 'whitespace/braces', 5, 'Extra space before [') @@ -3861,9 +3867,9 @@ def CheckTrailingSemicolon(filename, clean_lines, linenum, error): # Block bodies should not be followed by a semicolon. Due to C++11 # brace initialization, there are more places where semicolons are - # required than not, so we use a whitelist approach to check these - # rather than a blacklist. These are the places where "};" should - # be replaced by just "}": + # required than not, so we explicitly list the allowed rules rather + # than listing the disallowed ones. These are the places where "};" + # should be replaced by just "}": # 1. Some flavor of block following closing parenthesis: # for (;;) {}; # while (...) {}; @@ -3919,11 +3925,11 @@ def CheckTrailingSemicolon(filename, clean_lines, linenum, error): # - INTERFACE_DEF # - EXCLUSIVE_LOCKS_REQUIRED, SHARED_LOCKS_REQUIRED, LOCKS_EXCLUDED: # - # We implement a whitelist of safe macros instead of a blacklist of + # We implement a list of safe macros instead of a list of # unsafe macros, even though the latter appears less frequently in # google code and would have been easier to implement. This is because - # the downside for getting the whitelist wrong means some extra - # semicolons, while the downside for getting the blacklist wrong + # the downside for getting the allowed checks wrong means some extra + # semicolons, while the downside for getting disallowed checks wrong # would result in compile errors. # # In addition to macros, we also don't want to warn on @@ -4287,6 +4293,16 @@ def GetLineWidth(line): if unicodedata.east_asian_width(uc) in ('W', 'F'): width += 2 elif not unicodedata.combining(uc): + # Issue 337 + # https://mail.python.org/pipermail/python-list/2012-August/628809.html + if (sys.version_info.major, sys.version_info.minor) <= (3, 2): + # https://github.com/python/cpython/blob/2.7/Include/unicodeobject.h#L81 + is_wide_build = sysconfig.get_config_var("Py_UNICODE_SIZE") >= 4 + # https://github.com/python/cpython/blob/2.7/Objects/unicodeobject.c#L564 + is_low_surrogate = 0xDC00 <= ord(uc) <= 0xDFFF + if not is_wide_build and is_low_surrogate: + width -= 1 + width += 1 return width else: @@ -5109,19 +5125,19 @@ def CheckForNonConstReference(filename, clean_lines, linenum, # # We also accept & in static_assert, which looks like a function but # it's actually a declaration expression. - whitelisted_functions = (r'(?:[sS]wap(?:<\w:+>)?|' + allowed_functions = (r'(?:[sS]wap(?:<\w:+>)?|' r'operator\s*[<>][<>]|' r'static_assert|COMPILE_ASSERT' r')\s*\(') - if Search(whitelisted_functions, line): + if Search(allowed_functions, line): return elif not Search(r'\S+\([^)]*$', line): - # Don't see a whitelisted function on this line. Actually we + # Don't see an allowed function on this line. Actually we # didn't see any function name on this line, so this is likely a # multi-line parameter list. Try a bit harder to catch this case. for i in xrange(2): if (linenum > i and - Search(whitelisted_functions, clean_lines.elided[linenum - i - 1])): + Search(allowed_functions, clean_lines.elided[linenum - i - 1])): return decls = ReplaceAll(r'{[^}]*}', ' ', line) # exclude function body @@ -6189,7 +6205,7 @@ def ParseArguments(args): try: _valid_extensions = set(val.split(',')) except ValueError: - PrintUsage('Extensions must be comma seperated list.') + PrintUsage('Extensions must be comma separated list.') elif opt == '--headers': ProcessHppHeadersOption(val) diff --git a/tools/google-java-format.py b/tools/google-java-format.py index ed5ce28..5a537c0 100755 --- a/tools/google-java-format.py +++ b/tools/google-java-format.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python3 # -*- coding:utf-8 -*- # Copyright 2016 The Android Open Source Project # |