pre-upload: fix uprev checks with new packages & symlink renames
Handle a few more edge cases:
- If we're creating a package from scratch, an uprev is not needed.
- If the user has renamed the existing symlink and modified files/,
then they don't need to do anything else.
- If files/ are modified, but the ebuilds haven't been updated, throw
an error so the uprev is forced.
BUG=chromium:397714
TEST=created a CL that added an ebuild -> was not rejected
TEST=created a CL that modified an ebuild -> was rejected
Change-Id: I38ea5051d14fd2b68ea04ee97300c40cf604558f
Reviewed-on: https://chromium-review.googlesource.com/231614
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index a619cac..ffa6eae 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -240,17 +240,25 @@
return new_lines
-def _get_affected_files(commit, include_deletes=False, relative=False):
+def _get_affected_files(commit, include_deletes=False, relative=False,
+ include_symlinks=False, include_adds=True,
+ full_details=False):
"""Returns list of file paths that were modified/added, excluding symlinks.
Args:
commit: The commit
include_deletes: If true, we'll include deleted files in the result
relative: Whether to return relative or full paths to files
+ include_symlinks: If true, we'll include symlinks in the result
+ include_adds: If true, we'll include new files in the result
+ full_details: If False, return filenames, else return structured results.
Returns:
A list of modified/added (and perhaps deleted) files
"""
+ if not relative and full_details:
+ raise ValueError('full_details only supports relative paths currently')
+
if commit == PRE_SUBMIT:
return _run_command(['git', 'diff-index', '--cached',
'--name-only', 'HEAD']).split()
@@ -259,17 +267,25 @@
files = git.RawDiff(path, '%s^!' % commit)
# Filter out symlinks.
- files = [x for x in files if not stat.S_ISLNK(int(x.dst_mode, 8))]
+ if not include_symlinks:
+ files = [x for x in files if not stat.S_ISLNK(int(x.dst_mode, 8))]
if not include_deletes:
files = [x for x in files if x.status != 'D']
- # Caller only cares about filenames.
- files = [x.dst_file if x.dst_file else x.src_file for x in files]
- if relative:
+ if not include_adds:
+ files = [x for x in files if x.status != 'A']
+
+ if full_details:
+ # Caller wants the raw objects to parse status/etc... themselves.
return files
else:
- return [os.path.join(path, x) for x in files]
+ # Caller only cares about filenames.
+ files = [x.dst_file if x.dst_file else x.src_file for x in files]
+ if relative:
+ return files
+ else:
+ return [os.path.join(path, x) for x in files]
def _get_commits():
@@ -390,7 +406,7 @@
return HookFailure(msg)
-def _check_for_uprev(project, commit):
+def _check_for_uprev(project, commit, project_top=None):
"""Check that we're not missing a revbump of an ebuild in the given commit.
If the given commit touches files in a directory that has ebuilds somewhere
@@ -415,6 +431,7 @@
Args:
project: The project to look at
commit: The commit to look at
+ project_top: Top dir to process commits in
Returns:
A HookFailure or None.
@@ -428,38 +445,59 @@
if project in whitelist:
return None
- affected_paths = _get_affected_files(commit, include_deletes=True)
+ def FinalName(obj):
+ # If the file is being deleted, then the dst_file is not set.
+ if obj.dst_file is None:
+ return obj.src_file
+ else:
+ return obj.dst_file
+
+ affected_path_objs = _get_affected_files(
+ commit, include_deletes=True, include_symlinks=True, relative=True,
+ full_details=True)
# Don't yell about changes to whitelisted files...
whitelist = ('ChangeLog', 'Manifest', 'metadata.xml')
- affected_paths = [path for path in affected_paths
- if os.path.basename(path) not in whitelist]
- if not affected_paths:
+ affected_path_objs = [x for x in affected_path_objs
+ if os.path.basename(FinalName(x)) not in whitelist]
+ if not affected_path_objs:
return None
# If we've touched any file named with a -rN.ebuild then we'll say we're
# OK right away. See TODO above about enhancing this.
- touched_revved_ebuild = any(re.search(r'-r\d*\.ebuild$', path)
- for path in affected_paths)
+ touched_revved_ebuild = any(re.search(r'-r\d*\.ebuild$', FinalName(x))
+ for x in affected_path_objs)
if touched_revved_ebuild:
return None
+ # If we're creating new ebuilds from scratch, then we don't need an uprev.
+ # Find all the dirs that new ebuilds and ignore their files/.
+ ebuild_dirs = [os.path.dirname(FinalName(x)) + '/' for x in affected_path_objs
+ if FinalName(x).endswith('.ebuild') and x.status == 'A']
+ affected_path_objs = [obj for obj in affected_path_objs
+ if not any(FinalName(obj).startswith(x)
+ for x in ebuild_dirs)]
+ if not affected_path_objs:
+ return
+
# We want to examine the current contents of all directories that are parents
# of files that were touched (up to the top of the project).
#
# ...note: we use the current directory contents even though it may have
# changed since the commit we're looking at. This is just a heuristic after
# all. Worst case we don't flag a missing revbump.
- project_top = os.getcwd()
+ if project_top is None:
+ project_top = os.getcwd()
dirs_to_check = set([project_top])
- for path in affected_paths:
- path = os.path.dirname(path)
+ for obj in affected_path_objs:
+ path = os.path.join(project_top, os.path.dirname(FinalName(obj)))
while os.path.exists(path) and not os.path.samefile(path, project_top):
dirs_to_check.add(path)
path = os.path.dirname(path)
# Look through each directory. If it's got an ebuild in it then we'll
# consider this as a case when we need a revbump.
+ affected_paths = set([FinalName(x) for x in affected_path_objs])
for dir_path in dirs_to_check:
contents = os.listdir(dir_path)
ebuilds = [os.path.join(dir_path, path)
@@ -468,12 +506,13 @@
# If the -9999.ebuild file was touched the bot will uprev for us.
# ...we'll use a simple intersection here as a heuristic...
- if set(ebuilds_9999) & set(affected_paths):
+ if set(ebuilds_9999) & affected_paths:
continue
if ebuilds:
- return HookFailure('Changelist probably needs a revbump of an ebuild\n'
- 'or a -r1.ebuild symlink if this is a new ebuild')
+ return HookFailure('Changelist probably needs a revbump of an ebuild, '
+ 'or a -r1.ebuild symlink if this is a new ebuild:\n'
+ '%s' % dir_path)
return None
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 30c2349..395126d 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -640,23 +640,41 @@
self.assertMessageRejected('o' * 200)
+def DiffEntry(src_file=None, dst_file=None, src_mode=None, dst_mode='100644',
+ status='M'):
+ """Helper to create a stub RawDiffEntry object"""
+ if src_mode is None:
+ if status == 'A':
+ src_mode = '000000'
+ elif status == 'M':
+ src_mode = dst_mode
+ elif status == 'D':
+ src_mode = dst_mode
+ dst_mode = '000000'
+
+ src_sha = dst_sha = 'abc'
+ if status == 'D':
+ dst_sha = '000000'
+ elif status == 'A':
+ src_sha = '000000'
+
+ return git.RawDiffEntry(src_mode=src_mode, dst_mode=dst_mode, src_sha=src_sha,
+ dst_sha=dst_sha, status=status, score=None,
+ src_file=src_file, dst_file=dst_file)
+
+
class HelpersTest(cros_test_lib.MockTestCase):
"""Various tests for utility functions."""
def _SetupGetAffectedFiles(self):
self.PatchObject(git, 'RawDiff', return_value=[
# A modified normal file.
- git.RawDiffEntry(src_mode='100644', dst_mode='100644', src_sha='abc',
- dst_sha='abc', status='M', score=None,
- src_file='buildbot/constants.py', dst_file=None),
+ DiffEntry(src_file='buildbot/constants.py', status='M'),
# A new symlink file.
- git.RawDiffEntry(src_mode='000000', dst_mode='120000', src_sha='abc',
- dst_sha='abc', status='A', score=None,
- src_file='scripts/cros_env_whitelist', dst_file=None),
+ DiffEntry(dst_file='scripts/cros_env_whitelist', dst_mode='120000',
+ status='A'),
# A deleted file.
- git.RawDiffEntry(src_mode='100644', dst_mode='000000', src_sha='abc',
- dst_sha='000000', status='D', score=None,
- src_file='scripts/sync_sonic.py', dst_file=None),
+ DiffEntry(src_file='scripts/sync_sonic.py', status='D'),
])
def testGetAffectedFilesNoDeletesNoRelative(self):
@@ -689,12 +707,91 @@
def testGetAffectedFilesDeletesRelative(self):
"""Verify _get_affected_files() works w/delete & relative."""
self._SetupGetAffectedFiles()
- path = os.getcwd()
files = pre_upload._get_affected_files('HEAD', include_deletes=True,
relative=True)
exp_files = ['buildbot/constants.py', 'scripts/sync_sonic.py']
self.assertEquals(files, exp_files)
+ def testGetAffectedFilesDetails(self):
+ """Verify _get_affected_files() works w/full_details."""
+ self._SetupGetAffectedFiles()
+ files = pre_upload._get_affected_files('HEAD', full_details=True,
+ relative=True)
+ self.assertEquals(files[0].src_file, 'buildbot/constants.py')
+
+
+class CheckForUprev(cros_test_lib.MockTempDirTestCase):
+ """Tests for _check_for_uprev."""
+
+ def setUp(self):
+ self.file_mock = self.PatchObject(git, 'RawDiff')
+
+ def _Files(self, files):
+ """Create |files| in the tempdir and return full paths to them."""
+ for obj in files:
+ if obj.status == 'D':
+ continue
+ if obj.dst_file is None:
+ f = obj.src_file
+ else:
+ f = obj.dst_file
+ osutils.Touch(os.path.join(self.tempdir, f), makedirs=True)
+ return files
+
+ 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)
+ 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)
+ self.assertTrue(isinstance(ret, errors.HookFailure))
+
+ def testWhitelistOverlay(self):
+ """Skip checks on whitelisted overlays."""
+ self.assertAccepted([DiffEntry(src_file='cat/pkg/pkg-0.ebuild')],
+ project='chromiumos/overlays/portage-stable')
+
+ def testWhitelistFiles(self):
+ """Skip checks on whitelisted files."""
+ files = ['ChangeLog', 'Manifest', 'metadata.xml']
+ self.assertAccepted([DiffEntry(src_file=os.path.join('c', 'p', x),
+ status='M')
+ for x in files])
+
+ def testRejectBasic(self):
+ """Reject ebuilds missing uprevs."""
+ self.assertRejected([DiffEntry(src_file='c/p/p-0.ebuild', status='M')])
+
+ def testNewPackage(self):
+ """Accept new ebuilds w/out uprevs."""
+ self.assertAccepted([DiffEntry(src_file='c/p/p-0.ebuild', status='A')])
+ self.assertAccepted([DiffEntry(src_file='c/p/p-0-r12.ebuild', status='A')])
+
+ def testModifiedFilesOnly(self):
+ """Reject ebuilds w/out uprevs and changes in files/."""
+ osutils.Touch(os.path.join(self.tempdir, 'cat/pkg/pkg-0.ebuild'),
+ makedirs=True)
+ self.assertRejected([DiffEntry(src_file='cat/pkg/files/f', status='A')])
+ self.assertRejected([DiffEntry(src_file='cat/pkg/files/g', status='M')])
+
+ def testFilesNoEbuilds(self):
+ """Ignore changes to paths w/out ebuilds."""
+ self.assertAccepted([DiffEntry(src_file='cat/pkg/files/f', status='A')])
+ self.assertAccepted([DiffEntry(src_file='cat/pkg/files/g', status='M')])
+
+ def testModifiedFilesWithUprev(self):
+ """Accept ebuilds w/uprevs and changes in files/."""
+ self.assertAccepted([DiffEntry(src_file='c/p/files/f', status='A'),
+ DiffEntry(src_file='c/p/p-0.ebuild', status='A')])
+ self.assertAccepted([
+ DiffEntry(src_file='c/p/files/f', status='M'),
+ DiffEntry(src_file='c/p/p-0-r1.ebuild', src_mode='120000',
+ dst_file='c/p/p-0-r2.ebuild', dst_mode='120000', status='R')])
+
if __name__ == '__main__':
cros_test_lib.main()
