Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904663002:
bot_update: call gclient sync after checking out a gerrit ref (Closed)
|
Created:
4 years, 8 months ago by Paweł Hajdan Jr. Modified:
4 years, 8 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptionbot_update: call gclient sync after checking out a gerrit ref
BUG=chromium:602906
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300109
Patch Set 1 #
Total comments: 5
Messages
Total messages: 20 (7 generated)
phajdan.jr@chromium.org changed reviewers: + hinoka@chromium.org, tandrii@chromium.org
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904663002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904663002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1366: apply_issue_key_file, blacklist=already_patched) so, is this normally calling gclient sync automatically? i frankly don't understand why the difference between gerrit and rietveld :(
https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1366: apply_issue_key_file, blacklist=already_patched) On 2016/04/20 at 11:41:19, tandrii(chromium) wrote: > so, is this normally calling gclient sync automatically? My understanding is that it isn't. However, there's additional patching logic on original lines 1314-1328 that is not being performed for gerrit. > i frankly don't understand why the difference between gerrit and rietveld :( With gerrit, we're not really patching anything. We're checking out the ref (which is more like overwriting than patching). That means a gerrit ref can only apply to one repo in my understanding. This is different with unified text diffs, which might patch multiple repos, and are applied on top of existing sources. My reasoning is, eventually we'd only support gerrit refs (possible a set of refs for each repo), which should simplify this code. Does that make sense?
lgtm
yyanagisawa@chromium.org changed reviewers: + yyanagisawa@chromium.org
I may misunderstand the code but let me warn the possible issue I noticed. I expect the concern is solved before the submission. https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1369: gclient_sync(with_branch_heads, shallow) For our use case, this might revert the changes by git fetch. I think second gclient sync should sync to FETCH_HEAD not branch head, and because of that reason, apply_gerrit_ref might be the good place to run second gclient sync. I suggest to add ii. step below to apply_gerrit_ref. i. git fetch to apply patch ii. if git diff --name-only has DEPS, then gclient sync to FETCH_HEAD iii. git reset --soft if without gerrit_reset. FYI, my workaround implementation is like: 1. bot_update with no_gerrit_reset to make got_revision have FETCH_HEAD revision. 2. gclient sync to got_revision property. https://goto.google.com/gmviv I am afraid that this change might also break my workaround because got_revision might be overridden by HEAD.
https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1369: gclient_sync(with_branch_heads, shallow) On 2016/04/21 at 02:47:33, Yoshisato Yanagisawa wrote: > For our use case, this might revert the changes by git fetch. I think second gclient sync should sync to FETCH_HEAD not branch head, and because of that reason, apply_gerrit_ref might be the good place to run second gclient sync. I suggest to add ii. step below to apply_gerrit_ref. > i. git fetch to apply patch > ii. if git diff --name-only has DEPS, then gclient sync to FETCH_HEAD > iii. git reset --soft if without gerrit_reset. > > FYI, my workaround implementation is like: > 1. bot_update with no_gerrit_reset to make got_revision have FETCH_HEAD revision. > 2. gclient sync to got_revision property. > https://goto.google.com/gmviv > I am afraid that this change might also break my workaround because got_revision might be overridden by HEAD. gclient sync shouldn't touch main solution when it's unmanaged, and I was testing with goma where it was unmanaged Does that make sense to you? Are we good to land? If not, what specific changes do you suggest? We can always make a specific test if you want, and in the worst case revert the CL.
hinoka@google.com changed reviewers: + hinoka@google.com
Yeah gclient sync is unmanaged so it doesn't touch the main solution my only concern is that I think the first gclient sync is actually unnecessary, and running it twice would be slow.
lgtm https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/1904663002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:1369: gclient_sync(with_branch_heads, shallow) Yes, I seems to misunderstand the behavior. I thought --revision option is also applied to client revision to "gclient sync", but it seems not. I also followed the same step this + Ryan's suggestion bot_update do (i.e. run gclient sync only after apply_gerrit_ref), and confirmed it works.
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904663002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904663002/1
Message was sent while issue was closed.
Description was changed from ========== bot_update: call gclient sync after checking out a gerrit ref BUG=chromium:602906 ========== to ========== bot_update: call gclient sync after checking out a gerrit ref BUG=chromium:602906 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300109 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300109
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1918903003/ by tandrii@chromium.org. The reason for reverting is: I believe this is mostly likely to have broken Angle, but this is speculative. http://crbug.com/606150. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
