From ea52820de83a77d6f546d3dce682c616ada62950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thi=C3=A9baud=20Weksteen?= Date: Fri, 28 Aug 2020 15:54:29 +0200 Subject: Add [Builtin Hooks Exclude Paths] section There is currently no convenient option to enable a hook globally if some projects will fail the processing. The recommended setup is to enable the hook within each project's repository (using PREUPLOAD.cfg). This creates inconsistencies for large codebase. Adds a new configuration section to explicitly exclude some projects during the processing of a hook. The intent of this change is to enable rustfmt globally in AOSP, except for some paths (e.g. external/, vendor/). Test: Modified GLOBAL-PREUPLOAD.cfg to enable the new option, manually creates changes and review output of pre-upload.py Bug: 160223496 Change-Id: I94dbbf0ce2e6b58c4d4b4fc89c56a2a87543d878 --- README.md | 28 ++++++++++++++++++++++++++++ pre-upload.py | 11 +++++++---- rh/config.py | 30 +++++++++++++++++++++++++----- rh/config_unittest.py | 20 ++++++++++++++++++++ rh/hooks.py | 42 ++++++++++++++++++++++++++++++++++++++++++ rh/hooks_unittest.py | 22 ++++++++++++++++++++++ 6 files changed, 144 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index d35a4c4..dd6ffc6 100644 --- a/README.md +++ b/README.md @@ -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 94bb961..e7ef564 100755 --- a/pre-upload.py +++ b/pre-upload.py @@ -274,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) @@ -301,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 459d691..e2ad713 100644 --- a/rh/config.py +++ b/rh/config.py @@ -106,12 +106,14 @@ class PreUploadConfig(object): 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, } @@ -152,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): @@ -301,6 +312,15 @@ 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).""" diff --git a/rh/config_unittest.py b/rh/config_unittest.py index c8aceb9..4b27c5a 100755 --- a/rh/config_unittest.py +++ b/rh/config_unittest.py @@ -133,6 +133,19 @@ name = script --'bad-quotes 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.""" @@ -150,6 +163,13 @@ commit_msg_test_field = true""") 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() diff --git a/rh/hooks.py b/rh/hooks.py index 42427d5..b5350cd 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 isinstance(exclusion_path, re.Pattern): + 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) diff --git a/rh/hooks_unittest.py b/rh/hooks_unittest.py index daa240b..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.""" -- cgit v1.2.3