Add more error checking to preflight queue.

Add more error checking to preflight queue.

What's new?
  - cros_mark_as_stable now exits with errors if directories are specified
    that don't exist.
  - cbuildbot.py always explicitly specifies overlay directories so
    cros_mark_as_stable can rely on them existing.
  - Package names and paths are now separated with colons instead of
    spaces, so as to allow for us using the same syntax with
    enter_chroot.sh as we use without the same script. (enter_chroot.sh
    mucks with command-lines that contain spaces or quotes.)
  - cbuildbot.py now ensures its build path is a absolute path. This ensures we don't kill the wrong processes, if, for instance, the buildpath is '../..'
  - All buildbots now explicitly specify what overlays they want to rev. Public buildbots only rev public ebuilds and private buildbots now only rev private ebuilds.

BUG=chromium-os:8647
TEST=Ran unit tests. Manually marked packages as stable. Ran cbuildbot.py test run.

Change-Id: I1df6d428973d91329c4f5159e2886889a3ebb7c7

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=795bd30

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=b49c6df

[sosa, 2010-11-03 23:49:06]

Can you fix the bug number ... also address my nits before committing.  LGTM

http://codereview.chromium.org/4442001/diff/3001/4001
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/3001/4001#newcode462
bin/cbuildbot.py:462: public_overlay = '%s/src/third_party/chromiumos-overlay' %
buildroot
May want to os.path.exists below.

http://codereview.chromium.org/4442001/diff/3001/4004
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/3001/4004#newcode24
cros_mark_as_stable.py:24: gflags.DEFINE_string('overlays', '',
colon separated

http://codereview.chromium.org/4442001/diff/3001/4004#newcode28
cros_mark_as_stable.py:28: 'Space separated list of packages to mark as
stable.',
colon-separated

http://codereview.chromium.org/4442001/diff/3001/4004#newcode246
cros_mark_as_stable.py:246: Die("Non-zero return code (%s) for command: %s" %
(retcode, command))
Can you throw an exception instead.

[diandersAtChromium, 2010-11-03 23:53:34]

LGTM aside from nits.  Note that I'm not familiar w/ code, so please don't
commit just based on my feedback.

http://codereview.chromium.org/4442001/diff/3001/4001
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/3001/4001#newcode461
bin/cbuildbot.py:461: """Return the list of overlays to use for a given
buildbot."""
nit: Should describe overlays arg in docstring.

http://codereview.chromium.org/4442001/diff/3001/4002
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/4442001/diff/3001/4002#newcode36
bin/cbuildbot_config.py:36: 'overlays': 'public',
This (and others) are a change in behavior.  I believe that the default was
'both' in the past.  Please mention that.

...I assume that there wasn't a good reason for including 'both' in the past?

http://codereview.chromium.org/4442001/diff/3001/4003
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/3001/4003#newcode168
bin/cbuildbot_unittest.py:168: 
Maybe add "assert ':' not in ''.join(overlays)" here and elsewhere?

[diandersAtChromium, 2010-11-03 23:57:56]

Bug number should be 8642.

...also, I think scottz was originally thinking of doing something extra like
validating that the same build wasn't happening over and over.  I'm not 100%
sure I understood, but please check with him.

[diandersAtChromium, 2010-11-03 23:58:24]

Darnit: I mean 8647

[davidjames, 2010-11-10 18:30:10]

PTAL

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot.py#newcode461
bin/cbuildbot.py:461: """Return the list of overlays to use for a given
buildbot."""
On 2010/11/03 23:53:34, diandersAtChromium wrote:
> nit: Should describe overlays arg in docstring.

Done.

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot.py#newcode462
bin/cbuildbot.py:462: public_overlay = '%s/src/third_party/chromiumos-overlay' %
buildroot
On 2010/11/03 23:49:06, sosa wrote:
> May want to os.path.exists below.

Done.

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot_config.py
File bin/cbuildbot_config.py (right):

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot_config.py#newc...
bin/cbuildbot_config.py:36: 'overlays': 'public',
On 2010/11/03 23:53:34, diandersAtChromium wrote:
> This (and others) are a change in behavior.  I believe that the default was
> 'both' in the past.  Please mention that.
> 
> ...I assume that there wasn't a good reason for including 'both' in the past?

Done.

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot_unittest.py
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/3001/bin/cbuildbot_unittest.py#ne...
bin/cbuildbot_unittest.py:168: 
On 2010/11/03 23:53:34, diandersAtChromium wrote:
> Maybe add "assert ':' not in ''.join(overlays)" here and elsewhere?

Done. I put the asserts in the cbuildbot code so that we could catch them before
they got passed to cros_mark_as_stable.

[scottz-goog, 2010-11-10 23:48:13]

While these checks are definitely worth putting in dianders is right, the core
of the bug is to provide a way for us to detect when we are spinning our wheels.


If preflight is broken in an open way we will just keep marking the same REVs as
valid for every build without pushing them out. I was hoping there was a
reasonable way to detect that case.

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode193
bin/cbuildbot.py:193: assert ':' not in package, 'Invalid package name: %s' %
package
Why is this being added? Have their been cases where this has caused a problem?

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode464
bin/cbuildbot.py:464: 
I know this file doesn't have proper docstrings anywhere but lets start please.

Arguments:
    overlays: .....

Returns:
     a list of overlays to use for a given buildbot

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode469
bin/cbuildbot.py:469: """
Align indent.

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode481
bin/cbuildbot.py:481: assert ':' not in dir, 'Overlay must not contain colons:
%s' % dir
Why again are we doing this asssertion? Can you please give me a little
explanation of this addition?

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot_unittest.py
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:169: overlays = [
'%s/src/third_party/chromiumos-overlay' % self._buildroot ]
os.path.join?

http://codereview.chromium.org/4442001/diff/23001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/23001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:244: def _SimpleRunCommand(command):
Update description to match new function behavior.

http://codereview.chromium.org/4442001/diff/23001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:250: assert retcode == 0, "Return code %s for command:
%s" % (retcode, command)
Assert are good for debugging but generally if you want to die here you should
be raising an exception so that callers could catch it if need be by name. This
isn't really any different than checking the return code and raising
AssertionError('Return code ....') which isn't really something I would suggest.

[davidjames, 2010-11-11 02:23:24]

> While these checks are definitely worth putting in dianders is right, the core
> of the bug is to provide a way for us to detect when we are spinning our
wheels.
> 
> If preflight is broken in an open way we will just keep marking the same REVs
as
> valid for every build without pushing them out. I was 
> hoping there was a reasonable way to detect that case.

I added checks for some of the causes of that case. For instance, if the
directories pushed to the preflight push step don't exist, we used to return
status code zero and not report an error message. Now we fail with an error
message so it's possible to detect the failure.

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode193
bin/cbuildbot.py:193: assert ':' not in package, 'Invalid package name: %s' %
package
On 2010/11/10 23:48:14, scottz-goog wrote:
> Why is this being added? Have their been cases where this has caused a
problem?

Nope. Dianders just suggested I add the check.

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode464
bin/cbuildbot.py:464: 
On 2010/11/10 23:48:14, scottz-goog wrote:
> I know this file doesn't have proper docstrings anywhere but lets start
please.
> 
> Arguments:
>     overlays: .....
> 
> Returns:
>      a list of overlays to use for a given buildbot

Done.

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode469
bin/cbuildbot.py:469: """
On 2010/11/10 23:48:14, scottz-goog wrote:
> Align indent.

Done.

http://codereview.chromium.org/4442001/diff/23001/bin/cbuildbot.py#newcode481
bin/cbuildbot.py:481: assert ':' not in dir, 'Overlay must not contain colons:
%s' % dir
On 2010/11/10 23:48:14, scottz-goog wrote:
> Why again are we doing this asssertion? Can you please give me a little
> explanation of this addition?

It's very unlikely that it'll contain colons. Just in case.

[davidjames, 2010-11-11 23:13:07]

PTAL

[sosa, 2010-11-12 19:18:45]

LGTM w/ nits

Ideally we could add some check now in cros_mark_as_stable that it should always
be run outside the chroot.

http://codereview.chromium.org/4442001/diff/36001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/36001/bin/cbuildbot.py#newcode197
bin/cbuildbot.py:197: RunCommand(['./cros_mark_as_stable', '--srcroot=..',
put on sep line and below

http://codereview.chromium.org/4442001/diff/36001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/36001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:7: """This module uprevs a given package's ebuild to the
next revision."""
Can you add a comment here that this should always be run outside of the chroot.

[diandersAtChromium, 2010-11-12 19:25:30]

I'm not really familiar with all of these tools/options, so I just did an hour's
worth of peephole code reviewing of the changes.  Give me a heads up if you need
me to do more than this.

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py#newcode462
bin/cbuildbot.py:462: def ResolveOverlays(buildroot, overlays):
Why is this public, anyway?  Could be _ResolveOverlays, no?

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py#newcode465
bin/cbuildbot.py:465: Args:
Need to add buildroot; make sure to mention that path must be absolute.

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py#newcode483
bin/cbuildbot.py:483: if not os.path.exists(dir):
os.path.isdir?

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:28: 'Colon-separated list of packages to mark as
stable.',
Any easy way to detect other places that might have called cros_mark_as_stable
before and that might be broken?  Build bots?  Tests?  Developers?  

There does seem to be something in crostools that calls cros_mark_as_stable,
though it doesn't pass either of these parameters.  Hmm, won't that cause a
warning?  Any bad behavior?

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:71: def _CleanStalePackages(board, package_array):
Downside of all the enter_chroot here: it will be much slower (entering/exiting
the chroot can take a lot of time, at least on my machine).

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:252: assert retcode == 0, "Return code %s for command:
%s" % (retcode, command)
Not convinced that assert should be used here unless it really is an unexpected
event.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:380: (ebuild_no_rev, _, rev_string) =
ebuild_path.rpartition('-')
It looks like you're sorta getting "lucky" that this isn't crashing when you've
got a "-9999.ebuild".  The path looks like this:
  net-misc/dhcpcd/dhcpcd-9999.ebuild

The fact that "net-misc" has a dash in it is saving you.  Ick.  Is it guaranteed
that we'll always have at least two dashes in the path?  If not, this needs to
be smarter.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:388: # Has no revision so we stripped the version number
instead.
Old case of having no revision number at all is no longer handled.  Was that on
purpose?

What would happen with something like "./sys-apps/parted/parted-1.9.0.ebuild"?

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:518: os.chdir(overlay)
Why is chdir here?  What actually needs it?  ...and why do you only chdir back
for certain conditions in the loop?  Comment please?

[sosa, 2010-11-12 19:28:10]

Actually, dianders brought up some really good points.  Can you please address
them

[sosa, 2010-11-12 19:38:09]

Dianders prompted me to look a little more carefully.  I've addressed some of
his comments and also think I may have found a bug.  That chdir at the bottom
before work_branch.Delete() is probably a bug.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:28: 'Colon-separated list of packages to mark as
stable.',
CBuildbot is the only caller as this manages the pfq.

On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Any easy way to detect other places that might have called cros_mark_as_stable
> before and that might be broken?  Build bots?  Tests?  Developers?  
> 
> There does seem to be something in crostools that calls cros_mark_as_stable,
> though it doesn't pass either of these parameters.  Hmm, won't that cause a
> warning?  Any bad behavior?

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:71: def _CleanStalePackages(board, package_array):
This is a really good point I didn't think about.  This is probably a good
enough reason not to move this into the chroot if we can avoid it.  There's
already a method in the common python lib to reinterpret a path in the chroot. 
THis might be a better solution to the problem that you had

On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Downside of all the enter_chroot here: it will be much slower
(entering/exiting
> the chroot can take a lot of time, at least on my machine).

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:252: assert retcode == 0, "Return code %s for command:
%s" % (retcode, command)
Throw an exception.

On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Not convinced that assert should be used here unless it really is an
unexpected
> event.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:388: # Has no revision so we stripped the version number
instead.
This handles the 9999 case for the above comment.

On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Old case of having no revision number at all is no longer handled.  Was that
on
> purpose?
> 
> What would happen with something like "./sys-apps/parted/parted-1.9.0.ebuild"?

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:518: os.chdir(overlay)
For local git commits.

On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Why is chdir here?  What actually needs it?  ...and why do you only chdir back
> for certain conditions in the loop?  Comment please?

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:548: os.chdir(cwd)
Why do you chdir back before work_branch.Delete???

[davidjames, 2010-11-12 21:18:05]

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py#newcode462
bin/cbuildbot.py:462: def ResolveOverlays(buildroot, overlays):
On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Why is this public, anyway?  Could be _ResolveOverlays, no?

Done.

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py#newcode465
bin/cbuildbot.py:465: Args:
On 2010/11/12 19:25:30, diandersAtChromium wrote:
> Need to add buildroot; make sure to mention that path must be absolute.

Done.

http://codereview.chromium.org/4442001/diff/39001/bin/cbuildbot.py#newcode483
bin/cbuildbot.py:483: if not os.path.exists(dir):
On 2010/11/12 19:25:30, diandersAtChromium wrote:
> os.path.isdir?

Done.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:71: def _CleanStalePackages(board, package_array):
On 2010/11/12 19:38:09, sosa wrote:
> This is a really good point I didn't think about.  This is probably a good
> enough reason not to move this into the chroot if we can avoid it.  There's
> already a method in the common python lib to reinterpret a path in the chroot.

> THis might be a better solution to the problem that you had
> 
> On 2010/11/12 19:25:30, diandersAtChromium wrote:
> > Downside of all the enter_chroot here: it will be much slower
> (entering/exiting
> > the chroot can take a lot of time, at least on my machine).
> 

Done.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:252: assert retcode == 0, "Return code %s for command:
%s" % (retcode, command)
On 2010/11/12 19:38:09, sosa wrote:
> Throw an exception.
> 
> On 2010/11/12 19:25:30, diandersAtChromium wrote:
> > Not convinced that assert should be used here unless it really is an
> unexpected
> > event.
> 

Done.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:380: (ebuild_no_rev, _, rev_string) =
ebuild_path.rpartition('-')
On 2010/11/12 19:25:30, diandersAtChromium wrote:
> It looks like you're sorta getting "lucky" that this isn't crashing when
you've
> got a "-9999.ebuild".  The path looks like this:
>   net-misc/dhcpcd/dhcpcd-9999.ebuild
> 
> The fact that "net-misc" has a dash in it is saving you.  Ick.  Is it
guaranteed
> that we'll always have at least two dashes in the path?  If not, this needs to
> be smarter.

Just tested this. Looks like it works, and does not in fact depend on a hyphen
in the package name.

>>> "net-misc/dhcpcd/dhcpcd-9999".rpartition('-')
('net-misc/dhcpcd/dhcpcd', '-', '9999')
>>> "foo/dhcpcd/dhcpcd-9999".rpartition('-')
('foo/dhcpcd/dhcpcd', '-', '9999')

That said it'd be good to move over to portage methods instead. But that can
wait until a subsequent commit.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:518: os.chdir(overlay)
On 2010/11/12 19:38:09, sosa wrote:
> For local git commits.
> 
> On 2010/11/12 19:25:30, diandersAtChromium wrote:
> > Why is chdir here?  What actually needs it?  ...and why do you only chdir
back
> > for certain conditions in the loop?  Comment please?
> 

Added a comment.

http://codereview.chromium.org/4442001/diff/39001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:548: os.chdir(cwd)
On 2010/11/12 19:38:09, sosa wrote:
> Why do you chdir back before work_branch.Delete???

Removed.

This was necessary for entering the chroot, but you're right, it breaks the
work_branch.Delete case. But I've followed your suggestions and took out the
chroot entering here so this is no longer necessary.

[sosa, 2010-11-12 21:28:40]

LGTM w/ nits, but ask Dianders if he wants to take another crack.

http://codereview.chromium.org/4442001/diff/61001/bin/cbuildbot_unittest.py
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/61001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:170: overlays = [
'%s/src/third_party/chromiumos-overlay' % self._buildroot ]
Save yourself a few lines and add to Setup ... self.overlays=blah

http://codereview.chromium.org/4442001/diff/61001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/61001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:499: if not os.path.exists(path):
I guess this should also be isdir

http://codereview.chromium.org/4442001/diff/61001/lib/cros_portage_versions.py
File lib/cros_portage_versions.py (right):

http://codereview.chromium.org/4442001/diff/61001/lib/cros_portage_versions.p...
lib/cros_portage_versions.py:1: # versions.py -- core Portage functionality
This is no longer needed.

[davidjames, 2010-11-12 21:37:50]

http://codereview.chromium.org/4442001/diff/61001/bin/cbuildbot_unittest.py
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/61001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:170: overlays = [
'%s/src/third_party/chromiumos-overlay' % self._buildroot ]
On 2010/11/12 21:28:40, sosa wrote:
> Save yourself a few lines and add to Setup ... self.overlays=blah

Done.

http://codereview.chromium.org/4442001/diff/61001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/61001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:499: if not os.path.exists(path):
On 2010/11/12 21:28:40, sosa wrote:
> I guess this should also be isdir

Done.

http://codereview.chromium.org/4442001/diff/61001/lib/cros_portage_versions.py
File lib/cros_portage_versions.py (right):

http://codereview.chromium.org/4442001/diff/61001/lib/cros_portage_versions.p...
lib/cros_portage_versions.py:1: # versions.py -- core Portage functionality
On 2010/11/12 21:28:40, sosa wrote:
> This is no longer needed.

Done.

[diandersAtChromium, 2010-11-12 22:09:35]

Mostly more nits, though I'm still confused about the "-9999" thing.

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:46: self._overlays = [
'%s/src/third_party/chromiumos-overlay' % self._buildroot ]
NIT: line too long

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:47: self._chroot_overlays = [
ReinterpretPathForChroot(self._overlays[0]) ]
NIT: [ ReinterpretPathForChroot(p) for p in self._overlays ]

...then if someone adds an overlay, they don't end up with a bug.

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:181: self._revision_file, self._test_board,
self._overlays)
NIT: > 80 characters

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:203: self._revision_file, self._test_board,
self._overlays)
NIT: > 80 characters

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:379: (ebuild_no_rev, _, rev_string) =
ebuild_path.rpartition('-')
Sorry!  I got confused with rsplit.  :(  Thanks for checking!

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:387: # Has no revision so we stripped the version number
instead.
I'm still confused how this would handle something like
'./sys-apps/parted/parted-1.9.0.ebuild'.  ...or does that never come up?  The
old code _did_ handle that case.

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:520: os.chdir(overlay)
Technically, it would probably still be a good idea to chdir back to the
original directory after the loop has finished.  If not, this function has an
undocumented side effect of leaving the cwd in an unpredictable location, which
is a terrible idea.

To be completely correct, the chdir back to the original dir should be in a
"finally" clause so that thrown exceptions don't leave the cwd in the wrong
state.

[davidjames, 2010-11-12 22:31:25]

Thanks for the review. Pushing. I'll send you some followup commits to address
the things I didn't get here.

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py
File bin/cbuildbot_unittest.py (right):

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:46: self._overlays = [
'%s/src/third_party/chromiumos-overlay' % self._buildroot ]
On 2010/11/12 22:09:35, diandersAtChromium wrote:
> NIT: line too long

Done.

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:47: self._chroot_overlays = [
ReinterpretPathForChroot(self._overlays[0]) ]
On 2010/11/12 22:09:35, diandersAtChromium wrote:
> NIT: [ ReinterpretPathForChroot(p) for p in self._overlays ]
> 
> ...then if someone adds an overlay, they don't end up with a bug.

Done.

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:181: self._revision_file, self._test_board,
self._overlays)
On 2010/11/12 22:09:35, diandersAtChromium wrote:
> NIT: > 80 characters

Done.

http://codereview.chromium.org/4442001/diff/67001/bin/cbuildbot_unittest.py#n...
bin/cbuildbot_unittest.py:203: self._revision_file, self._test_board,
self._overlays)
On 2010/11/12 22:09:35, diandersAtChromium wrote:
> NIT: > 80 characters

Done.

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:387: # Has no revision so we stripped the version number
instead.
On 2010/11/12 22:09:35, diandersAtChromium wrote:
> I'm still confused how this would handle something like
> './sys-apps/parted/parted-1.9.0.ebuild'.  ...or does that never come up?  The
> old code _did_ handle that case.

This looks like a real bug to me. But it's not new in my commit and we haven't
run into it yet.

Here's a reproduction case:
  1. Commit workon parted-0.0.1.ebuild and parted-9999.ebuild as workon ebuilds.
  2. cros_mark_as_stable creates parted-0.0.1.ebuild0.0.1-r1.ebuild (oops!)

I'm going to file a bug for this and fix it in a separate commit (because my
commit is already getting large and it's unrelated).

http://codereview.chromium.org/4442001/diff/67001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:520: os.chdir(overlay)
On 2010/11/12 22:09:35, diandersAtChromium wrote:
> Technically, it would probably still be a good idea to chdir back to the
> original directory after the loop has finished.  If not, this function has an
> undocumented side effect of leaving the cwd in an unpredictable location,
which
> is a terrible idea.
> 
> To be completely correct, the chdir back to the original dir should be in a
> "finally" clause so that thrown exceptions don't leave the cwd in the wrong
> state.

Agreed. Since this is unrelated, I'm going to correct this in a followup commit.