pre-upload: Read .presubmitignore files.
Make pre-upload.py look for .presubmitignore files
containing lists of relative-path wildcards, one per line,
matching files that should be excluded from common presubmit
checks.
BUG=chromium:217286
TEST=manual: created a .presubmitignore file and uploaded a
change to a powerd pref without getting nagged about a
missing license
Change-Id: I3648e04ed840abd39d8a7fcc53fc7c65d27128ee
Reviewed-on: https://chromium-review.googlesource.com/249882
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index 6ee09dd..4a077cd 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -11,6 +11,7 @@
from __future__ import print_function
import ConfigParser
+import fnmatch
import functools
import json
import os
@@ -51,7 +52,7 @@
COMMON_EXCLUDED_PATHS = [
- # avoid doing source file checks for kernel
+ # Avoid doing source file checks for kernel.
r"/src/third_party/kernel/",
r"/src/third_party/kernel-next/",
r"/src/third_party/ktop/",
@@ -59,26 +60,32 @@
r".*\bexperimental[\\\/].*",
r".*\b[A-Z0-9_]{2,}$",
r".*[\\\/]debian[\\\/]rules$",
- # for ebuild trees, ignore any caches and manifest data
+
+ # For ebuild trees, ignore any caches and manifest data.
r".*/Manifest$",
r".*/metadata/[^/]*cache[^/]*/[^/]+/[^/]+$",
- # ignore profiles data (like overlay-tegra2/profiles)
+ # Ignore profiles data (like overlay-tegra2/profiles).
r"(^|.*/)overlay-.*/profiles/.*",
r"^profiles/.*$",
- # ignore minified js and jquery
+ # Ignore minified js and jquery.
r".*\.min\.js",
r".*jquery.*\.js",
# Ignore license files as the content is often taken verbatim.
- r'.*/licenses/.*',
+ r".*/licenses/.*",
]
_CONFIG_FILE = 'PRESUBMIT.cfg'
+# File containing wildcards, one per line, matching files that should be
+# excluded from presubmit checks. Lines beginning with '#' are ignored.
+_IGNORE_FILE = '.presubmitignore'
+
+
# Exceptions
@@ -240,9 +247,71 @@
return new_lines
+def _get_ignore_wildcards(directory, cache):
+ """Get wildcards listed in a directory's _IGNORE_FILE.
+
+ Args:
+ directory: A string containing a directory path.
+ cache: A dictionary (opaque to caller) caching previously-read wildcards.
+
+ Returns:
+ A list of wildcards from _IGNORE_FILE or an empty list if _IGNORE_FILE
+ wasn't present.
+ """
+ # In the cache, keys are directories and values are lists of wildcards from
+ # _IGNORE_FILE within those directories (and empty if no file was present).
+ if directory not in cache:
+ wildcards = []
+ dotfile_path = os.path.join(directory, _IGNORE_FILE)
+ if os.path.exists(dotfile_path):
+ # TODO(derat): Consider using _get_file_content() to get the file as of
+ # this commit instead of the on-disk version. This may have a noticeable
+ # performance impact, as each call to _get_file_content() runs git.
+ with open(dotfile_path, 'r') as dotfile:
+ for line in dotfile.readlines():
+ line = line.strip()
+ if line.startswith('#'):
+ continue
+ if line.endswith('/'):
+ line += '*'
+ wildcards.append(line)
+ cache[directory] = wildcards
+
+ return cache[directory]
+
+
+def _path_is_ignored(path, cache):
+ """Check whether a path is ignored by _IGNORE_FILE.
+
+ Args:
+ path: A string containing a path.
+ cache: A dictionary (opaque to caller) caching previously-read wildcards.
+
+ Returns:
+ True if a file named _IGNORE_FILE in one of the passed-in path's parent
+ directories contains a wildcard matching the path.
+ """
+ # Skip ignore files.
+ if os.path.basename(path) == _IGNORE_FILE:
+ return True
+
+ path = os.path.abspath(path)
+ base = os.getcwd()
+
+ prefix = os.path.dirname(path)
+ while prefix.startswith(base):
+ rel_path = path[len(prefix) + 1:]
+ for wildcard in _get_ignore_wildcards(prefix, cache):
+ if fnmatch.fnmatch(rel_path, wildcard):
+ return True
+ prefix = os.path.dirname(prefix)
+
+ return False
+
+
def _get_affected_files(commit, include_deletes=False, relative=False,
include_symlinks=False, include_adds=True,
- full_details=False):
+ full_details=False, use_ignore_files=True):
"""Returns list of file paths that were modified/added, excluding symlinks.
Args:
@@ -252,6 +321,7 @@
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.
+ use_ignore_files: Whether we ignore files matched by _IGNORE_FILE files.
Returns:
A list of modified/added (and perhaps deleted) files
@@ -276,6 +346,11 @@
if not include_adds:
files = [x for x in files if x.status != 'A']
+ if use_ignore_files:
+ cache = {}
+ is_ignored = lambda x: _path_is_ignored(x.dst_file or x.src_file, cache)
+ files = [x for x in files if not is_ignored(x)]
+
if full_details:
# Caller wants the raw objects to parse status/etc... themselves.
return files
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 686ea08..c073a01 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -804,10 +804,13 @@
src_file=src_file, dst_file=dst_file)
-class HelpersTest(cros_test_lib.MockTestCase):
+class HelpersTest(cros_test_lib.MockTempDirTestCase):
"""Various tests for utility functions."""
- def _SetupGetAffectedFiles(self):
+ def setUp(self):
+ self.orig_cwd = os.getcwd()
+ os.chdir(self.tempdir)
+
self.PatchObject(git, 'RawDiff', return_value=[
# A modified normal file.
DiffEntry(src_file='buildbot/constants.py', status='M'),
@@ -818,9 +821,18 @@
DiffEntry(src_file='scripts/sync_sonic.py', status='D'),
])
+ def tearDown(self):
+ os.chdir(self.orig_cwd)
+
+ def _WritePresubmitIgnoreFile(self, subdir, data):
+ """Writes to a .presubmitignore file in the passed-in subdirectory."""
+ directory = os.path.join(self.tempdir, subdir)
+ if not os.path.exists(directory):
+ os.makedirs(directory)
+ osutils.WriteFile(os.path.join(directory, pre_upload._IGNORE_FILE), data)
+
def testGetAffectedFilesNoDeletesNoRelative(self):
"""Verify _get_affected_files() works w/no delete & not relative."""
- self._SetupGetAffectedFiles()
path = os.getcwd()
files = pre_upload._get_affected_files('HEAD', include_deletes=False,
relative=False)
@@ -829,7 +841,6 @@
def testGetAffectedFilesDeletesNoRelative(self):
"""Verify _get_affected_files() works w/delete & not relative."""
- self._SetupGetAffectedFiles()
path = os.getcwd()
files = pre_upload._get_affected_files('HEAD', include_deletes=True,
relative=False)
@@ -839,7 +850,6 @@
def testGetAffectedFilesNoDeletesRelative(self):
"""Verify _get_affected_files() works w/no delete & relative."""
- self._SetupGetAffectedFiles()
files = pre_upload._get_affected_files('HEAD', include_deletes=False,
relative=True)
exp_files = ['buildbot/constants.py']
@@ -847,7 +857,6 @@
def testGetAffectedFilesDeletesRelative(self):
"""Verify _get_affected_files() works w/delete & relative."""
- self._SetupGetAffectedFiles()
files = pre_upload._get_affected_files('HEAD', include_deletes=True,
relative=True)
exp_files = ['buildbot/constants.py', 'scripts/sync_sonic.py']
@@ -855,11 +864,44 @@
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')
+ def testGetAffectedFilesPresubmitIgnoreDirectory(self):
+ """Verify .presubmitignore can be used to exclude a directory."""
+ self._WritePresubmitIgnoreFile('.', 'buildbot/')
+ self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True), [])
+
+ def testGetAffectedFilesPresubmitIgnoreDirectoryWildcard(self):
+ """Verify .presubmitignore can be used with a directory wildcard."""
+ self._WritePresubmitIgnoreFile('.', '*/constants.py')
+ self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True), [])
+
+ def testGetAffectedFilesPresubmitIgnoreWithinDirectory(self):
+ """Verify .presubmitignore can be placed in a subdirectory."""
+ self._WritePresubmitIgnoreFile('buildbot', '*.py')
+ self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True), [])
+
+ def testGetAffectedFilesPresubmitIgnoreDoesntMatch(self):
+ """Verify .presubmitignore has no effect when it doesn't match a file."""
+ self._WritePresubmitIgnoreFile('buildbot', '*.txt')
+ self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True),
+ ['buildbot/constants.py'])
+
+ def testGetAffectedFilesPresubmitIgnoreAddedFile(self):
+ """Verify .presubmitignore matches added files."""
+ self._WritePresubmitIgnoreFile('.', 'buildbot/\nscripts/')
+ self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True,
+ include_symlinks=True),
+ [])
+
+ def testGetAffectedFilesPresubmitIgnoreSkipIgnoreFile(self):
+ """Verify .presubmitignore files are automatically skipped."""
+ self.PatchObject(git, 'RawDiff', return_value=[
+ DiffEntry(src_file='.presubmitignore', status='M')
+ ])
+ self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True), [])
class CheckForUprev(cros_test_lib.MockTempDirTestCase):
"""Tests for _check_for_uprev."""
