Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3130031:
Don't mask out errors in cros_mark_as_stable (Closed)
|
Created:
10 years, 4 months ago by sosa Modified:
9 years, 6 months ago Reviewers:
Chris Masone CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Base URL:
http://src.chromium.org/git/crosutils.git Visibility:
Public. |
DescriptionDon't mask out errors in cros_mark_as_stable
Change-Id: Ib6722b99008f8515aecc3f27896963291d197947
Patch Set 1 #Patch Set 2 : Try leftover #
Total comments: 1
Messages
Total messages: 8 (0 generated)
http://codereview.chromium.org/3130031/diff/2001/3001 File cros_mark_as_stable.py (right): http://codereview.chromium.org/3130031/diff/2001/3001#newcode144 cros_mark_as_stable.py:144: _RunCommand('git push') Ok...so now what happens if there's an error?
cbuildbot is the main client of this script (see src/scripts/cbuildbot.py). On an error, cbuildbot deletes it's entire source tree and starts from scratch like the incremental builders. This doesn't fix the git push issue yet but the pre-flight-queue builder will correctly go red if it fails to push
ok. LGTM On Wed, Aug 18, 2010 at 5:49 PM, <sosa@chromium.org> wrote: > cbuildbot is the main client of this script (see src/scripts/cbuildbot.py). > On > an error, cbuildbot deletes it's entire source tree and starts from scratch > like > the incremental builders. This doesn't fix the git push issue yet but the > pre-flight-queue builder will correctly go red if it fails to push > > > http://codereview.chromium.org/3130031/show >
sosa@chromium.org (sosa@chromium.org) wrote: > Reviewers: Chris Masone, > > Description: > Don't mask out errors in cros_mark_as_stable > > Change-Id: Ib6722b99008f8515aecc3f27896963291d197947 > > Please review this at http://codereview.chromium.org/3130031/show > > SVN Base: http://src.chromium.org/git/crosutils.git > > Affected files: > M cros_mark_as_stable.py > > > Index: cros_mark_as_stable.py > diff --git a/cros_mark_as_stable.py b/cros_mark_as_stable.py > index 75154f9e701bb0c5ffe78efe4ac2059e86a00db5..4cf93cbe47b0d598ea4f8722f4c63e6597cac6db > 100755 > --- a/cros_mark_as_stable.py > +++ b/cros_mark_as_stable.py > @@ -137,15 +137,11 @@ def _PushChange(): > _RunCommand('git remote update') > _RunCommand('git checkout -b %s %s' % ( > merge_branch_name, gflags.FLAGS.tracking_branch)) > - try: > - _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) > - _RunCommand('git commit -m "%s"' % description) > - # Ugh. There has got to be an easier way to push to a tracking branch > - _RunCommand('git config push.default tracking') > - _RunCommand('git push') > - finally: > - _RunCommand('git checkout %s' % _STABLE_BRANCH_NAME) > - _RunCommand('git branch -D %s' % merge_branch_name) > + _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) > + _RunCommand('git commit -m "%s"' % description) > + # Ugh. There has got to be an easier way to push to a tracking branch > + _RunCommand('git config push.default tracking') > + _RunCommand('git push') > > > def _RunCommand(command): > > I don't believe this fixes anything. The exception still gets raised with the old code. cbuildbot.py relies on the exit code which should be non-zero, with or without this patch. The one difference with this patch is that you stay in the merge_branch, a temporary branch that I'd rather hide the way git-cl does.
With the buildbot, you cannot expect any kind of clean exception handling except by doing the cleanup before you run the appropriate steps. When things go wrong (i.e. the master loses contact with a slave) ... the slave will forcefully reboot ... this means exception handlers aren't called and whatever state you are in will just be that state you are in when you restart. On a reboot ... if we are on a wrong branch, we should clobber the source directory (which is what this will do when it tries to mark as stable). FYI: If you try / finally an exception, it catches and exception completely and doesn't alter the return code and you get back a return code of 0 instead of 1. The pre-flight queue was broken yesterday because repo sync has been clobbered during the middle, and when the pfq restarted it didn't run git config for rw pushes (since it assumed it was incremental rather than a full checkout). After this point, git pushes were failing silently because your try / finally caught everything and cbuildbot just assumed that git push succeeds. Removing the try/finally fixes this issue and git push will propagate the error On Thu, Aug 19, 2010 at 10:23 AM, Mandeep Singh Baines <msb@chromium.org> wrote: > sosa@chromium.org (sosa@chromium.org) wrote: >> Reviewers: Chris Masone, >> >> Description: >> Don't mask out errors in cros_mark_as_stable >> >> Change-Id: Ib6722b99008f8515aecc3f27896963291d197947 >> >> Please review this at http://codereview.chromium.org/3130031/show >> >> SVN Base: http://src.chromium.org/git/crosutils.git >> >> Affected files: >> M cros_mark_as_stable.py >> >> >> Index: cros_mark_as_stable.py >> diff --git a/cros_mark_as_stable.py b/cros_mark_as_stable.py >> index 75154f9e701bb0c5ffe78efe4ac2059e86a00db5..4cf93cbe47b0d598ea4f8722f4c63e6597cac6db >> 100755 >> --- a/cros_mark_as_stable.py >> +++ b/cros_mark_as_stable.py >> @@ -137,15 +137,11 @@ def _PushChange(): >> _RunCommand('git remote update') >> _RunCommand('git checkout -b %s %s' % ( >> merge_branch_name, gflags.FLAGS.tracking_branch)) >> - try: >> - _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) >> - _RunCommand('git commit -m "%s"' % description) >> - # Ugh. There has got to be an easier way to push to a tracking branch >> - _RunCommand('git config push.default tracking') >> - _RunCommand('git push') >> - finally: >> - _RunCommand('git checkout %s' % _STABLE_BRANCH_NAME) >> - _RunCommand('git branch -D %s' % merge_branch_name) >> + _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) >> + _RunCommand('git commit -m "%s"' % description) >> + # Ugh. There has got to be an easier way to push to a tracking branch >> + _RunCommand('git config push.default tracking') >> + _RunCommand('git push') >> >> >> def _RunCommand(command): >> >> > > I don't believe this fixes anything. The exception still gets raised with > the old code. cbuildbot.py relies on the exit code which should be non-zero, > with or without this patch. The one difference with this patch is that > you stay in the merge_branch, a temporary branch that I'd rather hide > the way git-cl does. >
Actually about the try/finally documentation says I'm wrong ... but it doesn't make any sense why the exception wasn't being propagated after the finally. It may that the buildbots are running python 2.4 rather than 2.5 which seems to have made some changes in the behavior of try/finally On Thu, Aug 19, 2010 at 12:03 PM, Chris Sosa <sosa@chromium.org> wrote: > With the buildbot, you cannot expect any kind of clean exception > handling except by doing the cleanup before you run the appropriate > steps. When things go wrong (i.e. the master loses contact with a > slave) ... the slave will forcefully reboot ... this means exception > handlers aren't called and whatever state you are in will just be that > state you are in when you restart. On a reboot ... if we are on a > wrong branch, we should clobber the source directory (which is what > this will do when it tries to mark as stable). > > FYI: If you try / finally an exception, it catches and exception > completely and doesn't alter the return code and you get back a return > code of 0 instead of 1. The pre-flight queue was broken yesterday > because repo sync has been clobbered during the middle, and when the > pfq restarted it didn't run git config for rw pushes (since it assumed > it was incremental rather than a full checkout). After this point, > git pushes were failing silently because your try / finally caught > everything and cbuildbot just assumed that git push succeeds. > Removing the try/finally fixes this issue and git push will propagate > the error > > On Thu, Aug 19, 2010 at 10:23 AM, Mandeep Singh Baines <msb@chromium.org> wrote: >> sosa@chromium.org (sosa@chromium.org) wrote: >>> Reviewers: Chris Masone, >>> >>> Description: >>> Don't mask out errors in cros_mark_as_stable >>> >>> Change-Id: Ib6722b99008f8515aecc3f27896963291d197947 >>> >>> Please review this at http://codereview.chromium.org/3130031/show >>> >>> SVN Base: http://src.chromium.org/git/crosutils.git >>> >>> Affected files: >>> M cros_mark_as_stable.py >>> >>> >>> Index: cros_mark_as_stable.py >>> diff --git a/cros_mark_as_stable.py b/cros_mark_as_stable.py >>> index 75154f9e701bb0c5ffe78efe4ac2059e86a00db5..4cf93cbe47b0d598ea4f8722f4c63e6597cac6db >>> 100755 >>> --- a/cros_mark_as_stable.py >>> +++ b/cros_mark_as_stable.py >>> @@ -137,15 +137,11 @@ def _PushChange(): >>> _RunCommand('git remote update') >>> _RunCommand('git checkout -b %s %s' % ( >>> merge_branch_name, gflags.FLAGS.tracking_branch)) >>> - try: >>> - _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) >>> - _RunCommand('git commit -m "%s"' % description) >>> - # Ugh. There has got to be an easier way to push to a tracking branch >>> - _RunCommand('git config push.default tracking') >>> - _RunCommand('git push') >>> - finally: >>> - _RunCommand('git checkout %s' % _STABLE_BRANCH_NAME) >>> - _RunCommand('git branch -D %s' % merge_branch_name) >>> + _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) >>> + _RunCommand('git commit -m "%s"' % description) >>> + # Ugh. There has got to be an easier way to push to a tracking branch >>> + _RunCommand('git config push.default tracking') >>> + _RunCommand('git push') >>> >>> >>> def _RunCommand(command): >>> >>> >> >> I don't believe this fixes anything. The exception still gets raised with >> the old code. cbuildbot.py relies on the exit code which should be non-zero, >> with or without this patch. The one difference with this patch is that >> you stay in the merge_branch, a temporary branch that I'd rather hide >> the way git-cl does. >> >
Chris Sosa (sosa@chromium.org) wrote: > Actually about the try/finally documentation says I'm wrong ... but it > doesn't make any sense why the exception wasn't being propagated after > the finally. It may that the buildbots are running python 2.4 rather > than 2.5 which seems to have made some changes in the behavior of > try/finally > Ah. That explains it. The semantics changed in 2.5. Thanks for fixing the bug:) > On Thu, Aug 19, 2010 at 12:03 PM, Chris Sosa <sosa@chromium.org> wrote: > > With the buildbot, you cannot expect any kind of clean exception > > handling except by doing the cleanup before you run the appropriate > > steps. When things go wrong (i.e. the master loses contact with a > > slave) ... the slave will forcefully reboot ... this means exception > > handlers aren't called and whatever state you are in will just be that > > state you are in when you restart. On a reboot ... if we are on a > > wrong branch, we should clobber the source directory (which is what > > this will do when it tries to mark as stable). > > > > FYI: If you try / finally an exception, it catches and exception > > completely and doesn't alter the return code and you get back a return > > code of 0 instead of 1. The pre-flight queue was broken yesterday > > because repo sync has been clobbered during the middle, and when the > > pfq restarted it didn't run git config for rw pushes (since it assumed > > it was incremental rather than a full checkout). After this point, > > git pushes were failing silently because your try / finally caught > > everything and cbuildbot just assumed that git push succeeds. > > Removing the try/finally fixes this issue and git push will propagate > > the error > > > > On Thu, Aug 19, 2010 at 10:23 AM, Mandeep Singh Baines <msb@chromium.org> wrote: > >> sosa@chromium.org (sosa@chromium.org) wrote: > >>> Reviewers: Chris Masone, > >>> > >>> Description: > >>> Don't mask out errors in cros_mark_as_stable > >>> > >>> Change-Id: Ib6722b99008f8515aecc3f27896963291d197947 > >>> > >>> Please review this at http://codereview.chromium.org/3130031/show > >>> > >>> SVN Base: http://src.chromium.org/git/crosutils.git > >>> > >>> Affected files: > >>> M cros_mark_as_stable.py > >>> > >>> > >>> Index: cros_mark_as_stable.py > >>> diff --git a/cros_mark_as_stable.py b/cros_mark_as_stable.py > >>> index 75154f9e701bb0c5ffe78efe4ac2059e86a00db5..4cf93cbe47b0d598ea4f8722f4c63e6597cac6db > >>> 100755 > >>> --- a/cros_mark_as_stable.py > >>> +++ b/cros_mark_as_stable.py > >>> @@ -137,15 +137,11 @@ def _PushChange(): > >>> _RunCommand('git remote update') > >>> _RunCommand('git checkout -b %s %s' % ( > >>> merge_branch_name, gflags.FLAGS.tracking_branch)) > >>> - try: > >>> - _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) > >>> - _RunCommand('git commit -m "%s"' % description) > >>> - # Ugh. There has got to be an easier way to push to a tracking branch > >>> - _RunCommand('git config push.default tracking') > >>> - _RunCommand('git push') > >>> - finally: > >>> - _RunCommand('git checkout %s' % _STABLE_BRANCH_NAME) > >>> - _RunCommand('git branch -D %s' % merge_branch_name) > >>> + _RunCommand('git merge --squash %s' % _STABLE_BRANCH_NAME) > >>> + _RunCommand('git commit -m "%s"' % description) > >>> + # Ugh. There has got to be an easier way to push to a tracking branch > >>> + _RunCommand('git config push.default tracking') > >>> + _RunCommand('git push') > >>> > >>> > >>> def _RunCommand(command): > >>> > >>> > >> > >> I don't believe this fixes anything. The exception still gets raised with > >> the old code. cbuildbot.py relies on the exit code which should be non-zero, > >> with or without this patch. The one difference with this patch is that > >> you stay in the merge_branch, a temporary branch that I'd rather hide > >> the way git-cl does. > >> > > |
||||||||||||||||||||||||||||||||||||||||||||||||||
