pre-upload: Verify the bug reference based on the upstream project.
When uploading patches to AOSP directly from a Chromium OS checkout, we
need to specify the bug number in a different format. This patch checks
the upstream project the branch is tracking and decides what format
should be used for the bug number line.
BUG=None
TEST=Added unittests.
Change-Id: I2084b4de7c55051bb229f1642384788916d1253b
Reviewed-on: https://chromium-review.googlesource.com/297173
Commit-Ready: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index 5432d40..40bc5c7 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -10,6 +10,7 @@
from __future__ import print_function
+import collections
import ConfigParser
import fnmatch
import functools
@@ -97,6 +98,9 @@
# General Helpers
+Project = collections.namedtuple('Project', ['name', 'dir', 'remote'])
+
+
# pylint: disable=redefined-builtin
def _run_command(cmd, cwd=None, input=None,
redirect_stderr=False, combine_stdout_stderr=False):
@@ -490,7 +494,7 @@
return HookFailure(msg, [example, str(ex)])
-def _check_change_has_bug_field(_project, commit):
+def _check_change_has_bug_field(project, commit):
"""Check for a correctly formatted 'BUG=' field in the commit message."""
OLD_BUG_RE = r'\nBUG=.*chromium-os'
if re.search(OLD_BUG_RE, _get_commit_desc(commit)):
@@ -498,15 +502,29 @@
'the chromium tracker in your BUG= line now.')
return HookFailure(msg)
- BUG_RE = r'\nBUG=([Nn]one|(chrome-os-partner|chromium|brillo|b):\d+)'
- if not re.search(BUG_RE, _get_commit_desc(commit)):
- msg = ('Changelist description needs BUG field (after first line):\n'
- 'BUG=brillo:9999 (for Brillo tracker)\n'
- 'BUG=chromium:9999 (for public tracker)\n'
- 'BUG=chrome-os-partner:9999 (for partner tracker)\n'
- 'BUG=b:9999 (for buganizer)\n'
- 'BUG=None')
- return HookFailure(msg)
+ # Android internal and external projects use "Bug: " to track bugs in
+ # buganizer.
+ BUG_COLON_REMOTES = (
+ 'aosp',
+ 'goog',
+ )
+ if project.remote in BUG_COLON_REMOTES:
+ BUG_RE = r'\nBug: ?([Nn]one|\d+)'
+ if not re.search(BUG_RE, _get_commit_desc(commit)):
+ msg = ('Changelist description needs BUG field (after first line):\n'
+ 'Bug: 9999 (for buganizer)\n'
+ 'BUG=None')
+ return HookFailure(msg)
+ else:
+ BUG_RE = r'\nBUG=([Nn]one|(chrome-os-partner|chromium|brillo|b):\d+)'
+ if not re.search(BUG_RE, _get_commit_desc(commit)):
+ msg = ('Changelist description needs BUG field (after first line):\n'
+ 'BUG=brillo:9999 (for Brillo tracker)\n'
+ 'BUG=chromium:9999 (for public tracker)\n'
+ 'BUG=chrome-os-partner:9999 (for partner tracker)\n'
+ 'BUG=b:9999 (for buganizer)\n'
+ 'BUG=None')
+ return HookFailure(msg)
def _check_for_uprev(project, commit, project_top=None):
@@ -532,7 +550,7 @@
still better off than without this check.
Args:
- project: The project to look at
+ project: The Project to look at
commit: The commit to look at
project_top: Top dir to process commits in
@@ -545,7 +563,7 @@
whitelist = (
'chromiumos/overlays/portage-stable',
)
- if project in whitelist:
+ if project.name in whitelist:
return None
def FinalName(obj):
@@ -628,7 +646,7 @@
have less builtin error checking.
Args:
- project: The project to look at
+ project: The Project to look at
commit: The commit to look at
Returns:
@@ -640,7 +658,7 @@
whitelist = (
'chromiumos/overlays/portage-stable',
)
- if project in whitelist:
+ if project.name in whitelist:
return None
BAD_EAPIS = ('0', '1', '2', '3')
@@ -697,7 +715,7 @@
KEYWORDS="-* ..." # Is known to only work on specific arches.
Args:
- project: The project to look at
+ project: The Project to look at
commit: The commit to look at
Returns:
@@ -773,12 +791,12 @@
whitelist = (
'chromiumos/overlays/portage-stable',
)
- if project in whitelist:
+ if project.name in whitelist:
return None
# We assume the repo name is the same as the dir name on disk.
# It would be dumb to not have them match though.
- project = os.path.basename(project)
+ project_base = os.path.basename(project.name)
is_variant = lambda x: x.startswith('overlay-variant-')
is_board = lambda x: x.startswith('overlay-')
@@ -796,7 +814,7 @@
if m:
overlay = m.group(1)
if not overlay or not is_board(overlay):
- overlay = project
+ overlay = project_base
pv = m.group(3).split('-', 1)[0]
@@ -1161,7 +1179,7 @@
write an error message to stdout.
"""
env = dict(os.environ)
- env['PRESUBMIT_PROJECT'] = project
+ env['PRESUBMIT_PROJECT'] = project.name
env['PRESUBMIT_COMMIT'] = commit
# Put affected files in an environment variable
@@ -1370,12 +1388,12 @@
return hooks
-def _run_project_hooks(project, proj_dir=None,
+def _run_project_hooks(project_name, proj_dir=None,
commit_list=None, presubmit=False):
"""For each project run its project specific hook from the hooks dictionary.
Args:
- project: The name of project to run hooks for.
+ project_name: The name of project to run hooks for.
proj_dir: If non-None, this is the directory the project is in. If None,
we'll ask repo.
commit_list: A list of commits to run hooks against. If None or empty list
@@ -1386,13 +1404,14 @@
Boolean value of whether any errors were ecountered while running the hooks.
"""
if proj_dir is None:
- proj_dirs = _run_command(['repo', 'forall', project, '-c', 'pwd']).split()
+ proj_dirs = _run_command(
+ ['repo', 'forall', project_name, '-c', 'pwd']).split()
if len(proj_dirs) == 0:
- print('%s cannot be found.' % project, file=sys.stderr)
+ print('%s cannot be found.' % project_name, file=sys.stderr)
print('Please specify a valid project.', file=sys.stderr)
return True
if len(proj_dirs) > 1:
- print('%s is associated with multiple directories.' % project,
+ print('%s is associated with multiple directories.' % project_name,
file=sys.stderr)
print('Please specify a directory to help disambiguate.', file=sys.stderr)
return True
@@ -1402,15 +1421,26 @@
# hooks assume they are run from the root of the project
os.chdir(proj_dir)
+ remote_branch = _run_command(['git', 'rev-parse', '--abbrev-ref',
+ '--symbolic-full-name', '@{u}']).strip()
+ if not remote_branch:
+ print('Your project %s doesn\'t track any remote repo.' % project_name,
+ file=sys.stderr)
+ remote = None
+ else:
+ remote, _branch = remote_branch.split('/', 1)
+
+ project = Project(name=project_name, dir=proj_dir, remote=remote)
+
if not commit_list:
try:
commit_list = _get_commits()
except VerifyException as e:
- PrintErrorForProject(project, HookFailure(str(e)))
+ PrintErrorForProject(project.name, HookFailure(str(e)))
os.chdir(pwd)
return True
- hooks = _get_project_hooks(project, presubmit)
+ hooks = _get_project_hooks(project.name, presubmit)
error_found = False
for commit in commit_list:
error_list = []
@@ -1420,7 +1450,7 @@
error_list.append(hook_error)
error_found = True
if error_list:
- PrintErrorsForCommit(project, commit, _get_commit_desc(commit),
+ PrintErrorsForCommit(project.name, commit, _get_commit_desc(commit),
error_list)
os.chdir(pwd)
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 890a3e5..d11ee3e 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -32,6 +32,11 @@
pre_upload = __import__('pre-upload')
+def ProjectNamed(project_name):
+ """Wrapper to create a Project with just the name"""
+ return pre_upload.Project(project_name, None, None)
+
+
class TryUTF8DecodeTest(cros_test_lib.TestCase):
"""Verify we sanely handle unicode content."""
@@ -64,7 +69,7 @@
(7, u"# define " + (u"x" * 80)), # OK (compiler directive)
(8, u"#define" + (u"x" * 74)), # Too long
]
- failure = pre_upload._check_no_long_lines('PROJECT', 'COMMIT')
+ failure = pre_upload._check_no_long_lines(ProjectNamed('PROJECT'), 'COMMIT')
self.assertTrue(failure)
self.assertEquals('Found lines longer than 80 characters (first 5 shown):',
failure.msg)
@@ -94,7 +99,8 @@
"""Report an error when the prefix doesn't match the base directory."""
self.file_mock.return_value = ['foo/foo.cc', 'foo/subdir/baz.cc']
self.desc_mock.return_value = 'bar: Some commit'
- failure = pre_upload._check_project_prefix('PROJECT', 'COMMIT')
+ failure = pre_upload._check_project_prefix(ProjectNamed('PROJECT'),
+ 'COMMIT')
self.assertTrue(failure)
self.assertEquals(('The commit title for changes affecting only foo' +
' should start with "foo: "'), failure.msg)
@@ -103,14 +109,16 @@
"""Use a prefix that matches the base directory."""
self.file_mock.return_value = ['foo/foo.cc', 'foo/subdir/baz.cc']
self.desc_mock.return_value = 'foo: Change some files.'
- self.assertFalse(pre_upload._check_project_prefix('PROJECT', 'COMMIT'))
+ self.assertFalse(
+ pre_upload._check_project_prefix(ProjectNamed('PROJECT'), 'COMMIT'))
def testAliasFile(self):
"""Use .project_alias to override the project name."""
self._WriteAliasFile('foo/.project_alias', 'project')
self.file_mock.return_value = ['foo/foo.cc', 'foo/subdir/bar.cc']
self.desc_mock.return_value = 'project: Use an alias.'
- self.assertFalse(pre_upload._check_project_prefix('PROJECT', 'COMMIT'))
+ self.assertFalse(
+ pre_upload._check_project_prefix(ProjectNamed('PROJECT'), 'COMMIT'))
def testAliasFileWithSubdirs(self):
"""Check that .project_alias is used when only modifying subdirectories."""
@@ -121,7 +129,8 @@
'foo/subdir/blah/baz.cc'
]
self.desc_mock.return_value = 'project: Alias with subdirs.'
- self.assertFalse(pre_upload._check_project_prefix('PROJECT', 'COMMIT'))
+ self.assertFalse(
+ pre_upload._check_project_prefix(ProjectNamed('PROJECT'), 'COMMIT'))
class CheckKernelConfig(cros_test_lib.MockTestCase):
@@ -208,7 +217,7 @@
class CheckEbuildEapi(cros_test_lib.MockTestCase):
"""Tests for _check_ebuild_eapi."""
- PORTAGE_STABLE = 'chromiumos/overlays/portage-stable'
+ PORTAGE_STABLE = ProjectNamed('chromiumos/overlays/portage-stable')
def setUp(self):
self.file_mock = self.PatchObject(pre_upload, '_get_affected_files')
@@ -230,18 +239,19 @@
self.content_mock.side_effect = Exception()
self.file_mock.return_value = ['some-file', 'ebuild/dir', 'an.ebuild~']
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
# Make sure our condition above triggers.
self.file_mock.return_value.append('a/real.ebuild')
- self.assertRaises(Exception, pre_upload._check_ebuild_eapi, 'o', 'HEAD')
+ self.assertRaises(Exception, pre_upload._check_ebuild_eapi,
+ ProjectNamed('o'), 'HEAD')
def testSkipSymlink(self):
"""Skip files that are just symlinks."""
self.file_mock.return_value = ['a-r1.ebuild']
self.content_mock.return_value = 'a.ebuild'
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
def testRejectEapiImplicit0Content(self):
@@ -252,7 +262,7 @@
IUSE="foo"
src_compile() { }
"""
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertTrue(isinstance(ret, errors.HookFailure))
def testRejectExplicitEapi1Content(self):
@@ -266,17 +276,17 @@
"""
# Make sure we only check the first EAPI= setting.
self.content_mock.return_value = template % '1\nEAPI=4'
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertTrue(isinstance(ret, errors.HookFailure))
# Verify we handle double quotes too.
self.content_mock.return_value = template % '"1"'
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertTrue(isinstance(ret, errors.HookFailure))
# Verify we handle single quotes too.
self.content_mock.return_value = template % "'1'"
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertTrue(isinstance(ret, errors.HookFailure))
def testAcceptExplicitEapi4Content(self):
@@ -290,17 +300,17 @@
"""
# Make sure we only check the first EAPI= setting.
self.content_mock.return_value = template % '4\nEAPI=1'
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
# Verify we handle double quotes too.
self.content_mock.return_value = template % '"5"'
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
# Verify we handle single quotes too.
self.content_mock.return_value = template % "'5-hdepend'"
- ret = pre_upload._check_ebuild_eapi('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_eapi(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
@@ -315,7 +325,7 @@
"""If no ebuilds are found, do not scan."""
self.file_mock.return_value = ['a.file', 'ebuild-is-not.foo']
- ret = pre_upload._check_ebuild_keywords('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_keywords(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
self.assertEqual(self.content_mock.call_count, 0)
@@ -325,7 +335,7 @@
self.file_mock.return_value = ['a.file', 'blah', 'foo.ebuild', 'cow']
self.content_mock.return_value = ''
- ret = pre_upload._check_ebuild_keywords('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_keywords(ProjectNamed('overlay'), 'HEAD')
self.assertEqual(ret, None)
self.assertEqual(self.content_mock.call_count, 1)
@@ -340,7 +350,7 @@
self.file_mock.return_value = ['a.ebuild']
self.content_mock.return_value = content
- ret = pre_upload._check_ebuild_keywords('overlay', 'HEAD')
+ ret = pre_upload._check_ebuild_keywords(ProjectNamed('overlay'), 'HEAD')
if fails:
self.assertTrue(isinstance(ret, errors.HookFailure))
else:
@@ -376,12 +386,12 @@
class CheckEbuildVirtualPv(cros_test_lib.MockTestCase):
"""Tests for _check_ebuild_virtual_pv."""
- PORTAGE_STABLE = 'chromiumos/overlays/portage-stable'
- CHROMIUMOS_OVERLAY = 'chromiumos/overlays/chromiumos'
- BOARD_OVERLAY = 'chromiumos/overlays/board-overlays'
- PRIVATE_OVERLAY = 'chromeos/overlays/overlay-link-private'
- PRIVATE_VARIANT_OVERLAY = ('chromeos/overlays/'
- 'overlay-variant-daisy-spring-private')
+ PORTAGE_STABLE = ProjectNamed('chromiumos/overlays/portage-stable')
+ CHROMIUMOS_OVERLAY = ProjectNamed('chromiumos/overlays/chromiumos')
+ BOARD_OVERLAY = ProjectNamed('chromiumos/overlays/board-overlays')
+ PRIVATE_OVERLAY = ProjectNamed('chromeos/overlays/overlay-link-private')
+ PRIVATE_VARIANT_OVERLAY = ProjectNamed('chromeos/overlays/'
+ 'overlay-variant-daisy-spring-private')
def setUp(self):
self.file_mock = self.PatchObject(pre_upload, '_get_affected_files')
@@ -389,7 +399,7 @@
def testNoVirtuals(self):
"""Skip non virtual packages."""
self.file_mock.return_value = ['some/package/package-3.ebuild']
- ret = pre_upload._check_ebuild_virtual_pv('overlay', 'H')
+ ret = pre_upload._check_ebuild_virtual_pv(ProjectNamed('overlay'), 'H')
self.assertEqual(ret, None)
def testCommonVirtuals(self):
@@ -663,13 +673,15 @@
# enable it for all the call sites below.
return 1 # pylint: disable=W0101
- def assertMessageAccepted(self, msg, project='project', commit='1234'):
+ def assertMessageAccepted(self, msg, project=ProjectNamed('project'),
+ commit='1234'):
"""Assert _check_change_has_bug_field accepts |msg|."""
self.msg_mock.return_value = msg
ret = self.CheckMessage(project, commit)
self.assertEqual(ret, None)
- def assertMessageRejected(self, msg, project='project', commit='1234'):
+ def assertMessageRejected(self, msg, project=ProjectNamed('project'),
+ commit='1234'):
"""Assert _check_change_has_bug_field rejects |msg|."""
self.msg_mock.return_value = msg
ret = self.CheckMessage(project, commit)
@@ -679,44 +691,60 @@
class CheckCommitMessageBug(CommitMessageTestCase):
"""Tests for _check_change_has_bug_field."""
+ AOSP_PROJECT = pre_upload.Project(name='overlay', dir='', remote='aosp')
+ CROS_PROJECT = pre_upload.Project(name='overlay', dir='', remote='cros')
+
@staticmethod
def CheckMessage(project, commit):
return pre_upload._check_change_has_bug_field(project, commit)
def testNormal(self):
"""Accept a commit message w/a valid BUG."""
- self.assertMessageAccepted('\nBUG=chromium:1234\n')
- self.assertMessageAccepted('\nBUG=chrome-os-partner:1234\n')
- self.assertMessageAccepted('\nBUG=b:1234\n')
+ self.assertMessageAccepted('\nBUG=chromium:1234\n', self.CROS_PROJECT)
+ self.assertMessageAccepted('\nBUG=chrome-os-partner:1234\n',
+ self.CROS_PROJECT)
+ self.assertMessageAccepted('\nBUG=b:1234\n', self.CROS_PROJECT)
+
+ self.assertMessageAccepted('\nBug: 1234\n', self.AOSP_PROJECT)
+ self.assertMessageAccepted('\nBug:1234\n', self.AOSP_PROJECT)
def testNone(self):
"""Accept BUG=None."""
- self.assertMessageAccepted('\nBUG=None\n')
- self.assertMessageAccepted('\nBUG=none\n')
- self.assertMessageRejected('\nBUG=NONE\n')
+ self.assertMessageAccepted('\nBUG=None\n', self.CROS_PROJECT)
+ self.assertMessageAccepted('\nBUG=none\n', self.CROS_PROJECT)
+ self.assertMessageAccepted('\nBug: None\n', self.AOSP_PROJECT)
+ self.assertMessageAccepted('\nBug:none\n', self.AOSP_PROJECT)
+
+ for project in (self.AOSP_PROJECT, self.CROS_PROJECT):
+ self.assertMessageRejected('\nBUG=NONE\n', project)
def testBlank(self):
"""Reject blank values."""
- self.assertMessageRejected('\nBUG=\n')
- self.assertMessageRejected('\nBUG= \n')
+ for project in (self.AOSP_PROJECT, self.CROS_PROJECT):
+ self.assertMessageRejected('\nBUG=\n', project)
+ self.assertMessageRejected('\nBUG= \n', project)
+ self.assertMessageRejected('\nBug:\n', project)
+ self.assertMessageRejected('\nBug: \n', project)
def testNotFirstLine(self):
"""Reject the first line."""
- self.assertMessageRejected('BUG=None\n\n\n')
+ for project in (self.AOSP_PROJECT, self.CROS_PROJECT):
+ self.assertMessageRejected('BUG=None\n\n\n', project)
def testNotInline(self):
"""Reject not at the start of line."""
- self.assertMessageRejected('\n BUG=None\n')
- self.assertMessageRejected('\n\tBUG=None\n')
+ for project in (self.AOSP_PROJECT, self.CROS_PROJECT):
+ self.assertMessageRejected('\n BUG=None\n', project)
+ self.assertMessageRejected('\n\tBUG=None\n', project)
def testOldTrackers(self):
"""Reject commit messages using old trackers."""
- self.assertMessageRejected('\nBUG=chromium-os:1234\n')
+ self.assertMessageRejected('\nBUG=chromium-os:1234\n', self.CROS_PROJECT)
def testNoTrackers(self):
"""Reject commit messages w/invalid trackers."""
- self.assertMessageRejected('\nBUG=booga:1234\n')
- self.assertMessageRejected('\nBUG=br:1234\n')
+ self.assertMessageRejected('\nBUG=booga:1234\n', self.CROS_PROJECT)
+ self.assertMessageRejected('\nBUG=br:1234\n', self.CROS_PROJECT)
def testMissing(self):
"""Reject commit messages w/no BUG line."""
@@ -996,13 +1024,15 @@
def assertAccepted(self, files, project='project', commit='fake sha1'):
"""Assert _check_for_uprev accepts |files|."""
self.file_mock.return_value = self._Files(files)
- ret = pre_upload._check_for_uprev(project, commit, project_top=self.tempdir)
+ ret = pre_upload._check_for_uprev(ProjectNamed(project), commit,
+ project_top=self.tempdir)
self.assertEqual(ret, None)
def assertRejected(self, files, project='project', commit='fake sha1'):
"""Assert _check_for_uprev rejects |files|."""
self.file_mock.return_value = self._Files(files)
- ret = pre_upload._check_for_uprev(project, commit, project_top=self.tempdir)
+ ret = pre_upload._check_for_uprev(ProjectNamed(project), commit,
+ project_top=self.tempdir)
self.assertTrue(isinstance(ret, errors.HookFailure))
def testWhitelistOverlay(self):
