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
diff --git a/README.md b/README.md
index d35a4c4..dd6ffc6 100644
--- a/README.md
+++ b/README.md
@@ -227,6 +227,34 @@
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 @@
(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 @@
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 @@
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 @@
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 @@
"""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 @@
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 @@
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 @@
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 @@
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 @@
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."""
