pre-upload.py: Add hook to run cargo clippy
Add a hook `check_cargo_clippy` to run cargo clippy in pre-upload.py.
This hook is opt-in and clippy runs only when files in specified Cargo
projects were modified.
BUG=chromium:1105466
TEST=repo upload a Rust CL
TEST=pre-upload_unittest.py
Change-Id: I6cd642457125d1f76c87b0f6db19a2c36ab28c95
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/repohooks/+/2297362
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Tested-by: Keiichi Watanabe <keiichiw@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index e22a032..ec1ce24 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -646,6 +646,108 @@
return None
+class CargoClippyArgumentParserError(Exception):
+ """An exception indicating an invalid check_cargo_clippy option."""
+
+
+class CargoClippyArgumentParser(argparse.ArgumentParser):
+ """A argument parser for check_cargo_clippy."""
+
+ def error(self, message):
+ raise CargoClippyArgumentParserError(message)
+
+
+# A cargo project in which clippy runs.
+ClippyProject = collections.namedtuple('ClippyProject', ('root', 'script'))
+
+
+class _AddClippyProjectAction(argparse.Action):
+ """A callback that adds a cargo clippy setting.
+
+ It accepts a value which is in the form of "ROOT[:SCRIPT]".
+ """
+
+ def __call__(self, parser, namespace, values, option_string=None):
+ if getattr(namespace, self.dest, None) is None:
+ setattr(namespace, self.dest, [])
+ spec = values.split(':', 1)
+ if len(spec) == 1:
+ spec += [None]
+
+ if spec[0].startswith('/'):
+ raise CargoClippyArgumentParserError('root path must not start with "/"'
+ f' but "{spec[0]}"')
+
+ clippy = ClippyProject(root=spec[0], script=spec[1])
+ getattr(namespace, self.dest).append(clippy)
+
+
+def _get_cargo_clippy_parser():
+ """Creates a parser for check_cargo_clippy options."""
+
+ parser = CargoClippyArgumentParser()
+ parser.add_argument('--project', action=_AddClippyProjectAction, default=[])
+
+ return parser
+
+
+def _check_cargo_clippy(project, commit, options=()):
+ """Checks that a change doesn't produce cargo-clippy errors."""
+
+ options = list(options)
+ if not options:
+ return []
+ parser = _get_cargo_clippy_parser()
+
+ try:
+ opts = parser.parse_args(options)
+ except CargoClippyArgumentParserError as e:
+ return [HookFailure('invalid check_cargo_clippy option is given.'
+ f' Please check PRESUBMIT.cfg is correct: {e}')]
+ files = _get_affected_files(commit)
+
+ errors = []
+ for clippy in opts.project:
+ root = os.path.normpath(os.path.join(project.dir, clippy.root))
+
+ # Check if any file under `root` was modified.
+ modified = False
+ for f in files:
+ if f.startswith(root):
+ modified = True
+ break
+ if not modified:
+ continue
+
+ # Clean cargo's cache when we run clippy for this `root` for the first time.
+ # We don't want to clean the cache between commits to save time when
+ # multiple commits are checked.
+ if root not in _check_cargo_clippy.cleaned_root:
+ _run_command(['cargo', 'clean',
+ f'--manifest-path={root}/Cargo.toml'],
+ stderr=subprocess.STDOUT)
+ _check_cargo_clippy.cleaned_root.add(root)
+
+ cmd = ['cargo', 'clippy', '--all-features', '--all-targets',
+ f'--manifest-path={root}/Cargo.toml',
+ '--', '-D', 'warnings']
+ # Overwrite the clippy command if a project-specific script is specified.
+ if clippy.script:
+ cmd = [os.path.join(project.dir, clippy.script)]
+
+ output = _run_command(cmd, stderr=subprocess.STDOUT)
+ error = re.search(r'^error: ', output, flags=re.MULTILINE)
+ if error:
+ msg = output[error.start():]
+ errors.append(HookFailure(msg))
+
+ return errors
+
+
+# Stores cargo projects in which `cargo clean` ran.
+_check_cargo_clippy.cleaned_root = set()
+
+
def _check_change_has_test_field(_project, commit):
"""Check for a non-empty 'TEST=' field in the commit message."""
SEE_ALSO = 'Please review the documentation:\n%s' % (DOC_COMMIT_MSG_URL,)
@@ -1834,6 +1936,7 @@
# A list of hooks that are not project-specific
_COMMON_HOOKS = [
+ _check_cargo_clippy,
_check_cros_license,
_check_ebuild_eapi,
_check_ebuild_keywords,
@@ -1883,6 +1986,7 @@
'contribution_check': _check_change_is_contribution,
'project_prefix_check': _check_project_prefix,
'filepath_chartype_check': _check_filepath_chartype,
+ 'cargo_clippy_check': _check_cargo_clippy,
}
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 71b4337..c3886ba 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -1703,5 +1703,90 @@
self.assertIsNone(failure)
+class GetCargoClippyParserTest(cros_test_lib.TestCase):
+ """Tests for _get_cargo_clippy_parser."""
+
+ def testSingleProject(self):
+ parser = pre_upload._get_cargo_clippy_parser()
+ args = parser.parse_args(['--project', 'foo'])
+ self.assertEqual(args.project,
+ [pre_upload.ClippyProject(root='foo', script=None)])
+
+ def testMultipleProjects(self):
+ parser = pre_upload._get_cargo_clippy_parser()
+ args = parser.parse_args(['--project', 'foo:bar',
+ '--project', 'baz'])
+ self.assertCountEqual(args.project,
+ [pre_upload.ClippyProject(root='foo', script='bar'),
+ pre_upload.ClippyProject(root='baz', script=None)])
+
+
+class CheckCargoClippyTest(PreUploadTestCase, cros_test_lib.TempDirTestCase):
+ """Tests for _check_cargo_clippy."""
+
+ def setUp(self):
+ self.project = pre_upload.Project(name='PROJECT', dir=self.tempdir,
+ remote=None)
+
+ def testClippy(self):
+ """Verify clippy is called when a monitored file was changed."""
+ rc_mock = self.PatchObject(pre_upload, '_run_command', return_value='')
+
+ self.PatchObject(pre_upload, '_get_affected_files',
+ return_value=[f'{self.project.dir}/repo_a/a.rs'])
+
+ ret = pre_upload._check_cargo_clippy(self.project, 'COMMIT',
+ options=['--project=repo_a',
+ '--project=repo_b:foo'])
+ self.assertFalse(ret)
+
+ # Check if `cargo clippy` ran.
+ called = False
+ for args, _ in rc_mock.call_args_list:
+ cmd = args[0]
+ if len(cmd) > 1 and cmd[0] == 'cargo' and cmd[1] == 'clippy':
+ called = True
+ break
+
+ self.assertTrue(called)
+
+ def testDontRun(self):
+ """Skip checks when no monitored files are modified."""
+ rc_mock = self.PatchObject(pre_upload, '_run_command', return_value='')
+
+ # A file under `repo_a` was monitored.
+ self.PatchObject(pre_upload, '_get_affected_files',
+ return_value=[f'{self.project.dir}/repo_a/a.rs'])
+ # But, we only care about files under `repo_b`.
+ errs = pre_upload._check_cargo_clippy(self.project, 'COMMIT',
+ options=['--project=repo_b:foo'])
+
+ self.assertFalse(errs)
+
+ rc_mock.assert_not_called()
+
+ def testCustomScript(self):
+ """Verify project-specific script is used."""
+ rc_mock = self.PatchObject(pre_upload, '_run_command', return_value='')
+
+ self.PatchObject(pre_upload, '_get_affected_files',
+ return_value=[f'{self.project.dir}/repo_b/b.rs'])
+
+ errs = pre_upload._check_cargo_clippy(self.project, 'COMMIT',
+ options=['--project=repo_a',
+ '--project=repo_b:foo'])
+ self.assertFalse(errs)
+
+ # Check if the script `foo` ran.
+ called = False
+ for args, _ in rc_mock.call_args_list:
+ cmd = args[0]
+ if len(cmd) > 0 and cmd[0] == os.path.join(self.project.dir, 'foo'):
+ called = True
+ break
+
+ self.assertTrue(called)
+
+
if __name__ == '__main__':
cros_test_lib.main(module=__name__)
