Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
274493002:
Only apply DEPS patches before gclient sync, apply all other patches afterwards (Closed)
|
Created:
6 years, 7 months ago by hinoka Modified:
6 years, 7 months ago CC:
chromium-reviews, kjellander-cc_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, stip+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Visibility:
Public. |
DescriptionOnly apply DEPS patches before gclient sync, apply all other patches afterwards
This should in theory allow for cross-repo patches.
BUG=370503
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=269490
Patch Set 1 #Patch Set 2 : Fixes #Patch Set 3 : Adds filename printing #
Total comments: 7
Patch Set 4 : Review fixes #
Total comments: 4
Patch Set 5 : Review fix #Patch Set 6 : Rebase #Messages
Total messages: 13 (0 generated)
Tested by:
python -u ../../../scripts/tools/runit.py ../../../scripts/slave/bot_update.py
--spec "solutions = [{'custom_vars': {'googlecode_url':
'svn://svn-mirror.golo.chromium.org/%s', 'nacl_trunk':
'svn://svn-mirror.golo.chromium.org/native_client/t
runk', 'sourceforge_url': 'svn://svn-mirror.golo.chromium.org/%(repo)s',
'webkit_trunk': 'svn://svn-mirror.golo.chromium.org/blink/trunk'}, 'deps_file':
'DEPS', 'managed': True, 'name': 'src', 'url':
'svn://svn-mirror.golo.chromium.org/chrome/trunk/src'}]" --root src --revision
268505 --p
atch_url
svn://svn.chromium.org/chrome-try/try/Matt_Menke.file.2014-05-06%2017.54.01.133000.diff@314861
--output_json /tmp/tmpaYNDxo.json --force
python -u ../../../scripts/tools/runit.py ../../../scripts/slave/bot_update.py
--spec "solutions = [{'custom_vars': {'googlecode_url':
'svn://svn-mirror.golo.chromium.org/%s', 'nacl_trunk':
'svn://svn-mirror.golo.chromium.org/native_client/trunk', 'sourceforge_url':
'svn://svn-mirror.golo.chromium.org/%(repo)s', 'webkit_trunk':
'svn://svn-mirror.golo.chromium.org/blink/trunk'}, 'deps_file': 'DEPS',
'managed': True, 'name': 'src', 'url':
'svn://svn-mirror.golo.chromium.org/chrome/trunk/src'}]" --root src --revision
HEAD --issue 253603004 --patchset 170001 --output_json /tmp/tmp3zAa3y.json
--force
[+iannucci review]
mostly lgtm % some nits, and obviously don't land until https://codereview.chromium.org/273543002/ does https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update.py File scripts/slave/bot_update.py (right): https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:877: def apply_issue_rietveld(issue, patchset, root, server, rev_map, revision, apply_rietveld_issue (to match apply_svn_patch) https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:877: def apply_issue_rietveld(issue, patchset, root, server, rev_map, revision, Also, take patch first (to match apply_svn_patch) https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:884: I wouldn't put empty lines in the middle of this. https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:891: # DEPRECATED? If it is, remove it. If it is still necessary, say why you're passing it.
seems fine to me, just some comments. https://codereview.chromium.org/274493002/diff/50001/scripts/slave/bot_update.py File scripts/slave/bot_update.py (right): https://codereview.chromium.org/274493002/diff/50001/scripts/slave/bot_update... scripts/slave/bot_update.py:235: for attempt in range(tries): hm... why not xrange? https://codereview.chromium.org/274493002/diff/50001/scripts/slave/bot_update... scripts/slave/bot_update.py:843: # Otherwise... I'm not really sure what to do with this. probably should abort loudly
https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update.py File scripts/slave/bot_update.py (right): https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:877: def apply_issue_rietveld(issue, patchset, root, server, rev_map, revision, On 2014/05/07 21:39:00, agable wrote: > Also, take patch first (to match apply_svn_patch) Patchset here is actually an integer, where as "patches" in apply_svn_patch is actually an object that contains the contents of the patch. Because patchsets on rietveld are uniquely referred to as "issue - patchset", it always makes more sense to put issue first. https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:884: On 2014/05/07 21:39:00, agable wrote: > I wouldn't put empty lines in the middle of this. Done. https://codereview.chromium.org/274493002/diff/30001/scripts/slave/bot_update... scripts/slave/bot_update.py:891: # DEPRECATED? On 2014/05/07 21:39:00, agable wrote: > If it is, remove it. If it is still necessary, say why you're passing it. Done. https://codereview.chromium.org/274493002/diff/50001/scripts/slave/bot_update.py File scripts/slave/bot_update.py (right): https://codereview.chromium.org/274493002/diff/50001/scripts/slave/bot_update... scripts/slave/bot_update.py:235: for attempt in range(tries): On 2014/05/08 22:17:01, iannucci wrote: > hm... why not xrange? "range" sounds cleaner (xrange isn't even a word!), and at O(3) the use of a generator is hardly relevant to performance. https://codereview.chromium.org/274493002/diff/50001/scripts/slave/bot_update... scripts/slave/bot_update.py:843: # Otherwise... I'm not really sure what to do with this. On 2014/05/08 22:17:01, iannucci wrote: > probably should abort loudly Done.
lgtm
The CQ bit was checked by hinoka@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hinoka@chromium.org/274493002/70001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for build/scripts/slave/bot_update.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file build/scripts/slave/bot_update.py
Hunk #1 succeeded at 264 (offset 3 lines).
Hunk #2 succeeded at 281 (offset 3 lines).
Hunk #3 succeeded at 319 (offset 3 lines).
Hunk #4 succeeded at 555 (offset -70 lines).
Hunk #5 succeeded at 632 (offset -70 lines).
Hunk #6 succeeded at 795 (offset -58 lines).
Hunk #7 succeeded at 912 (offset -58 lines).
Hunk #8 FAILED at 988.
Hunk #9 succeeded at 977 with fuzz 2 (offset -45 lines).
Hunk #10 FAILED at 1051.
2 out of 10 hunks FAILED -- saving rejects to file
build/scripts/slave/bot_update.py.rej
Patch: build/scripts/slave/bot_update.py
Index: scripts/slave/bot_update.py
diff --git build/scripts/slave/bot_update.py build/scripts/slave/bot_update.py
index
1df92f1b25a03ee57db1711f48a6da22a00cab65..1078af436217728b3d0421ecb3dddd2961e90680
100755
--- a/build/scripts/slave/bot_update.py
+++ b/build/scripts/slave/bot_update.py
@@ -261,11 +261,16 @@ class SVNRevisionNotFound(Exception):
pass
+class InvalidDiff(Exception):
+ pass
+
+
def call(*args, **kwargs):
"""Interactive subprocess call."""
kwargs['stdout'] = subprocess.PIPE
kwargs['stderr'] = subprocess.STDOUT
stdin_data = kwargs.pop('stdin_data', None)
+ tries = kwargs.pop('tries', RETRIES)
if stdin_data:
kwargs['stdin'] = subprocess.PIPE
out = cStringIO.StringIO()
@@ -273,7 +278,7 @@ def call(*args, **kwargs):
env = copy.copy(os.environ)
env.update(new_env)
kwargs['env'] = env
- for attempt in xrange(RETRIES):
+ for attempt in range(tries):
attempt_msg = ' (retry #%d)' % attempt if attempt else ''
if new_env:
print '===Injecting Environment Variables==='
@@ -311,7 +316,7 @@ def call(*args, **kwargs):
print
raise SubprocessFailed('%s failed with code %d in %s after %d attempts.' %
- (' '.join(args), code, os.getcwd(), RETRIES), code)
+ (' '.join(args), code, os.getcwd(), tries), code)
def git(*args, **kwargs):
@@ -620,12 +625,16 @@ def _last_commit_for_file(filename, repo_base):
def need_to_run_deps2git(repo_base, deps_file, deps_git_file):
"""Checks to see if we need to run deps2git.
- Returns True if there was a DEPS change after the last .DEPS.git update.
+ Returns True if there was a DEPS change after the last .DEPS.git update
+ or if DEPS has local modifications.
"""
print 'Checking if %s exists' % deps_git_file
if not path.isfile(deps_git_file):
# .DEPS.git doesn't exist but DEPS does? We probably want to generate one.
- print 'it exists!'
+ print 'it doesn\'t exist!'
+ return True
+
+ if git('diff', deps_file, cwd=repo_base).strip():
return True
last_known_deps_ref = _last_commit_for_file(deps_file, repo_base)
@@ -693,10 +702,10 @@ def ensure_deps2git(sln_dir, shallow):
repo_base = path.join(os.getcwd(), sln_dir)
deps_file = path.join(repo_base, 'DEPS')
deps_git_file = path.join(repo_base, '.DEPS.git')
- print 'Checking if %s is newer than %s' % (deps_file, deps_git_file)
if not path.isfile(deps_file):
return
+ print 'Checking if %s is newer than %s' % (deps_file, deps_git_file)
if not need_to_run_deps2git(repo_base, deps_file, deps_git_file):
return
@@ -844,24 +853,103 @@ def _download(url):
raise
-def apply_issue_svn(root, patch_url):
+def parse_diff(diff):
+ """Takes a unified diff and returns a list of diffed files and their diffs.
+
+ The return format is a list of pairs of:
+ (<filename>, <diff contents>)
+ <diff contents> is inclusive of the diff line.
+ """
+ result = []
+ current_diff = ''
+ current_header = None
+ for line in diff.splitlines():
+ # "diff" is for git style patches, and "Index: " is for SVN style patches.
+ if line.startswith('diff') or line.startswith('Index: '):
+ if current_header:
+ # If we are in a diff portion, then save the diff.
+ result.append((current_header, '%s\n' % current_diff))
+ git_header_match = re.match(r'diff (?:--git )?(\S+) (\S+)', line)
+ svn_header_match = re.match(r'Index: (.*)', line)
+
+ if git_header_match:
+ # First, see if its a git style header.
+ from_file = git_header_match.group(1)
+ to_file = git_header_match.group(2)
+ if from_file != to_file and from_file.startswith('a/'):
+ # Sometimes git prepends 'a/' and 'b/' in front of file paths.
+ from_file = from_file[2:]
+ current_header = from_file
+
+ elif svn_header_match:
+ # Otherwise, check if its an SVN style header.
+ current_header = svn_header_match.group(1)
+
+ else:
+ # Otherwise... I'm not really sure what to do with this.
+ raise InvalidDiff('Can\'t process header: %s\nFull diff:\n%s' %
+ (line, diff))
+
+ current_diff = ''
+ current_diff += '%s\n' % line
+ if current_header:
+ # We hit EOF, gotta save the last diff.
+ result.append((current_header, current_diff))
+ return result
+
+
+def get_svn_patch(patch_url):
+ """Fetch patch from patch_url, return list of (filename, diff)"""
patch_data = call('svn', 'cat', patch_url)
- call(PATCH_TOOL, '-p0', '--remove-empty-files', '--force', '--forward',
- stdin_data=patch_data, cwd=root)
+ return parse_diff(patch_data)
+
+
+def apply_svn_patch(patch_root, patches, whitelist=None, blacklist=None):
+ """Expects a list of (filename, diff), applies it on top of patch_root."""
+ if whitelist:
+ patches = [(name, diff) for name, diff in patches if name in whitelist]
+ elif blacklist:
+ patches = [(name, diff) for name, diff in patches if name not in blacklist]
+ diffs = [diff for _, diff in patches]
+ patch = ''.join(diffs)
+
+ if patch:
+ print '===Patching files==='
+ for filename, _ in patches:
+ print 'Patching %s' % filename
+ call(PATCH_TOOL, '-p0', '--remove-empty-files', '--force', '--forward',
+ stdin_data=patch, cwd=patch_root, tries=1)
-def apply_issue_rietveld(issue, patchset, root, server, rev_map, revision):
+def apply_rietveld_issue(issue, patchset, root, server, rev_map, revision,
+ whitelist=None, blacklist=None):
apply_issue_bin = ('apply_issue.bat' if sys.platform.startswith('win')
else 'apply_issue')
- call(apply_issue_bin,
- '--root_dir', root,
- '--issue', issue,
- '--patchset', patchset,
- '--no-auth',
- '--server', server,
- '--base_ref', revision,
- '--force',
- '--ignore_deps')
+ cmd = [apply_issue_bin,
+ # The patch will be applied on top of this directory.
+ '--root_dir', root,
+ # Tell apply_issue how to fetch the patch.
+ '--issue', issue,
+ '--patchset', patchset,
+ '--no-auth',
+ '--server', server,
+ # Always run apply_issue.py, otherwise it would see update.flag
+ # and then bail out.
+ '--force',
+ # Don't run gclient sync when it sees a DEPS change.
+ '--ignore_deps',
+ # Don't commit the patch or add it to the index.
+ '--no_commit',
+ ]
+ if whitelist:
+ for item in whitelist:
+ cmd.extend(['--whitelist', item])
+ elif blacklist:
+ for item in blacklist:
+ cmd.extend(['--blacklist', item])
+
+ # Only try once, since subsequent failures hide the real failure.
+ call(*cmd, tries=1)
def check_flag(flag_file):
@@ -882,14 +970,6 @@ def emit_flag(flag_file):
f.write('Success!')
-def get_real_git_hash(git_hash, dir_name):
- """Return git_hash or the parent of git_hash if its a patched hash."""
- log = git('log', '-1', '--format=%B', cwd=dir_name).strip()
- if 'committed patch' in log.lower():
- return git('log', '-1', '--format=%H', 'HEAD^', cwd=dir_name).strip()
- return git_hash
-
-
def parse_got_revision(gclient_output, got_revision_mapping, use_svn_revs):
"""Translate git gclient revision mapping to build properties.
@@ -908,7 +988,7 @@ def parse_got_revision(gclient_output, got_revision_mapping,
use_svn_revs):
else:
# Since we are using .DEPS.git, everything had better be git.
assert solution_output.get('scm') == 'git'
- git_revision = get_real_git_hash(solution_output['revision'], dir_name)
+ git_revision = solution_output['revision']
if use_svn_revs:
revision = get_svn_rev(git_revision, dir_name)
if not revision:
@@ -942,21 +1022,23 @@ def ensure_checkout(solutions, revision, first_sln,
target_os, target_os_only,
print 'Fetching Git checkout'
git_ref = git_checkout(solutions, revision, shallow)
- # If either patch_url or issue is passed in, then we need to apply a patch.
+ patches = None
if patch_url:
- # patch_url takes precidence since its only passed in on gcl try/git try.
- apply_issue_svn(root, patch_url)
- elif issue:
- apply_issue_rietveld(issue, patchset, root, rietveld_server,
- revision_mapping, git_ref)
+ patches = get_svn_patch(patch_url)
+
+ if root == first_sln:
+ # Only top level DEPS patching is supported right now.
+ if patches:
+ apply_svn_patch(root, patches, whitelist=['DEPS'])
+ elif issue:
+ apply_rietveld_issue(issue, patchset, root, rietveld_server,
+ revision_mapping, git_ref, whitelist=['DEPS'])
+
if buildspec_name:
buildspecs2git(root, buildspec_name)
- elif first_sln == root:
- # Run deps2git if there is a DEPS commit after the last .DEPS.git commit.
- # We only need to ensure deps2git if the root is not overridden, since
- # if the root is overridden, it means we are working with a sub repository
- # patch, which means its impossible for it to touch DEPS.
+ else:
+ # Run deps2git if there is a DEPS change after the last .DEPS.git commit.
ensu…
(message too large)
The CQ bit was checked by hinoka@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hinoka@chromium.org/274493002/90001
Message was sent while issue was closed.
Change committed as 269490 |
