Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2328313002:
Skip apply_gerrit rebase when there is a rebase failure (Closed)
|
Created:
4 years, 3 months ago by rmistry Modified:
4 years, 3 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionSkip apply_gerrit rebase when there is a rebase failure.
Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/builds/4995
This causes the checkout to remain in the "rebase in progress" state.
BUG=chromium:645955
BUG=skia:5749
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e67337bb45dccf79ae8699b9625fa6
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/ea9e93f28c8d432445acc8208a6b885ffc5a803c
Patch Set 1 : Initial upload #
Total comments: 2
Patch Set 2 : Use abort #Patch Set 3 : Put in except not finally block #
Total comments: 3
Patch Set 4 : Reraise #Messages
Total messages: 44 (23 generated)
Description was changed from ========== Skip apply_gerrit rebase if there is a failure Not doing so causes a failure in the ... BUG= ========== to ========== Skip apply_gerrit rebase if there is a failure Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to be in the "rebase in progress" state: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... BUG=chromium:645955 BUG=skia:5749 ==========
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Skip apply_gerrit rebase if there is a failure Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to be in the "rebase in progress" state: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... BUG=chromium:645955 BUG=skia:5749 ========== to ========== Skip apply_gerrit rebase if there is a failure Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 ==========
Description was changed from ========== Skip apply_gerrit rebase if there is a failure Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 ========== to ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rmistry@google.com changed reviewers: + tandrii@chromium.org
rmistry@google.com changed reviewers: + andybons@chromium.org
andybons@chromium.org changed reviewers: + agable@chromium.org
This looks good to me but I’d like to have Aaron take a look since Andrii is OOO for a couple days.
https://codereview.chromium.org/2328313002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2328313002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:611: git('rebase', '--skip', cwd=root) I would use --abort instead of --skip; in this case they have the same result (getting the repo out of the "rebase in progress" state), but --abort is clearer to the reader that we're abandoning the rebase operation entirely rather than trying to keep rebasing everything except for the failed file(s).
https://codereview.chromium.org/2328313002/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2328313002/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:611: git('rebase', '--skip', cwd=root) On 2016/09/12 16:25:11, agable wrote: > I would use --abort instead of --skip; in this case they have the same result > (getting the repo out of the "rebase in progress" state), but --abort is clearer > to the reader that we're abandoning the rebase operation entirely rather than > trying to keep rebasing everything except for the failed file(s). Makes sense. Done.
lgtm
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 ========== to ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e673... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e673...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2331103002/ by rmistry@google.com. The reason for reverting is: The "git rebase --abort" should only happen when there is a failure during rebase not when it succeeds..
Message was sent while issue was closed.
On 2016/09/12 at 18:45:28, rmistry wrote: > A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2331103002/ by rmistry@google.com. > > The reason for reverting is: The "git rebase --abort" should only happen when there is a failure during rebase not when it succeeds.. Ah damn, of course. This should just be in the except block.
Message was sent while issue was closed.
Description was changed from ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e673... ========== to ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e673... ==========
On 2016/09/12 18:56:29, agable wrote: > On 2016/09/12 at 18:45:28, rmistry wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2331103002/ by mailto:rmistry@google.com. > > > > The reason for reverting is: The "git rebase --abort" should only happen when > there is a failure during rebase not when it succeeds.. > > Ah damn, of course. This should just be in the except block. Reopened the CL and put fix in Patchset3. PTAL.
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2328313002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2328313002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:609: except SubprocessFailed: No, because this will catch the exception and then prevent us from hitting the outer "except SubprocessFailed" on line 626. Either re-raise (messy), or find a way to put this in that exception handler.
(P.S., we generally prefer to reland by opening a new CL. That way the commit message doesn't confusingly contain multiple "Review-URL" and "Committed:" lines.)
https://codereview.chromium.org/2328313002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2328313002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:609: except SubprocessFailed: On 2016/09/12 21:00:16, agable wrote: > No, because this will catch the exception and then prevent us from hitting the > outer "except SubprocessFailed" on line 626. Either re-raise (messy), or find a > way to put this in that exception handler. Ah true. I have no idea how to put this in the other exception handler, reraising will definitely work though.
On 2016/09/12 21:01:09, agable wrote: > (P.S., we generally prefer to reland by opening a new CL. That way the commit > message doesn't confusingly contain multiple "Review-URL" and "Committed:" > lines.) Yeah, there are 2 camps for that and multiple discussions on chromium-dev which is why I wrote up https://docs.google.com/document/d/1lMewWeARYME5S8TwlEkhfEwMioaHxWsEm_UZONt7Q... a while back. Good thing that AFAIK there is no way to reuse a Gerrit CL.
https://codereview.chromium.org/2328313002/diff/40001/recipe_modules/bot_upda... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2328313002/diff/40001/recipe_modules/bot_upda... recipe_modules/bot_update/resources/bot_update.py:609: except SubprocessFailed: On 2016/09/12 21:06:31, rmistry wrote: > On 2016/09/12 21:00:16, agable wrote: > > No, because this will catch the exception and then prevent us from hitting the > > outer "except SubprocessFailed" on line 626. Either re-raise (messy), or find > a > > way to put this in that exception handler. > > Ah true. I have no idea how to put this in the other exception handler, > reraising will definitely work though. Put in a raise.
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/09/12 21:38:34, agable wrote: > lgtm Thanks for the review. I'll submit this early tomorrow and test it.
perfect, thanks. LGTM
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e673... ========== to ========== Skip apply_gerrit rebase when there is a rebase failure. Not doing so causes a failure in the 'git checkout $base_rev' step as visible here: https://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/... This causes the checkout to remain in the "rebase in progress" state. BUG=chromium:645955 BUG=skia:5749 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/9736fe8938e673... Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/ea9e93f28c8d43... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/ea9e93f28c8d43... |
