Add ability to rev and build a chrome from cbuildbot

This CL adds the ability to Rev and Build Chrome using cros_mark_chrome_as_stable.

Other things:
Removing commit|clean|push command from cros_mark_chrome because it's unnecessary ... I'm just gonna use the same stabilizing branch as the other stable marker, so I can use the same clean / push from the other script.  This makes this change fit in much more nicely to cbuildbot.

Other changes:
Add ability for cros_mark_chrome_as_stable to communicate to calling script through stdout.
Make STABLE_BRANCH in cros_mark_as_stable public for above reason.
Removed push_options from cros_mark and accompanying change to cbuildbot as it isn't used anymore.
Made it easier to debug with cbuildbot (I ran cbuildbot A LOT of times to test this change so I felt the pain)...this includes:
  - only build when syncing
  - allow one to explicitly disable tests
  - use dryrun is options.debug is set


Change-Id: I413a2e81a99cdde2e4d9139561cd518245b9b346

BUG=chromium-os:8693
TEST=Ran cbuildbot with --notest --debug --nosync x86-pre-flight and
saw it go through correctly and dryrun push.  Ran cbuildbot with same and --chrome_rev=tot.  Ran with syncing and running tests too.

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

[sosa, 2010-11-22 21:31:23]

Ping

[scottz, 2010-11-23 02:44:39]

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode210
bin/cbuildbot.py:210: """Returns the atom for the newly revved chrome ebuild."""
I know you are going to love me for this.

Time to start adding proper Docstrings with args and retruns and raises Oh MY.
Please do so on any new function or function you modify. Future devs will thank
you :)

Also, what is the 'atom' ?

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode325
bin/cbuildbot.py:325: RunCommand(['emerge-%s =%s' % (board,
chrome_atom_to_build)],
Why do we not want this to be individual list elements?

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode410
bin/cbuildbot.py:410: def _UprevPush(buildroot, tracking_branch, board,
overlays, dryrun):
dryrun=False or should this always be passed?

docstring reminder.. :)

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode422
bin/cbuildbot.py:422: cmd.append('push')
Is there a reason why push has to come after a potential dryrun? can we move
this just into the cmd list defined above?

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode525
bin/cbuildbot.py:525: '-c', '--chrome_rev', default=None, type='string',
please align as the others are.
add a dest=chrome_rev, I know it isn't required but we put it everywhere else. I
see the above two don't have it either, they should.

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode537
bin/cbuildbot.py:537: help='Don\'t sync before building.')
use double quotes instead of escaping the apostrophe.  "Don't"

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
File bin/cros_mark_chrome_as_stable.py (right):

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:267: usage = '%s OPTIONS [%s]' % ((__file__),
'|'.join(CHROME_REV))
__file__ withut the parens?

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:269: parser.add_option('-s', '--srcroot',
default=('%s/trunk/src' %
os.path.join(os.environ['HOME'], ....)

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:274: (options, argv) = parser.parse_args(argv)
I know this isn't your code but this is bad. We are taking sys.argv as passed in
below and then passing it to parser which isn't necessary and then redefining
argv to be something different.

Can we just change this to parser.parse_args()? Rename argv to args in the tuple
and not pass sys.argv in to main at all? 

I got to this from line 281 so it sort of relates to your change :)

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:313: # Explicit print to communicate to
caller.
perhaps I do not understand all that is being done here but are we explicitly
printing this so the caller can scrape this? If so maybe adding a bit more about
this or to the file in general about how if prints are added it could break
things?  We may want to print something like PORTAGE_VERSION_ATOM=XXXXX 

then whoever is scraping it can key in on that but i need more information about
what is going on here.

[sosa, 2010-11-23 21:09:19]

PTAL

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode210
bin/cbuildbot.py:210: """Returns the atom for the newly revved chrome ebuild."""
I don't agree.  According to PEP-8 and our own style guides, private functions
do not need to be fully docstring'd.  After all, this function is just a wrapper
to a python module that IS documented.  That should be sufficient.  The same
goes for other places.

It's a portage atom.  See man emerge - updating doc

On 2010/11/23 02:44:40, scottz wrote:
> I know you are going to love me for this.
> 
> Time to start adding proper Docstrings with args and retruns and raises Oh MY.
> Please do so on any new function or function you modify. Future devs will
thank
> you :)
> 
> Also, what is the 'atom' ?

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode325
bin/cbuildbot.py:325: RunCommand(['emerge-%s =%s' % (board,
chrome_atom_to_build)],
On 2010/11/23 02:44:40, scottz wrote:
> Why do we not want this to be individual list elements?

Done.

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode410
bin/cbuildbot.py:410: def _UprevPush(buildroot, tracking_branch, board,
overlays, dryrun):
Prefer not have a default as it is a private function and is only used in this
module.

Docstring - see above

On 2010/11/23 02:44:40, scottz wrote:
> dryrun=False or should this always be passed?
> 
> docstring reminder.. :)

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode422
bin/cbuildbot.py:422: cmd.append('push')
I believe gflags will not correctly parse a flag after a arg

On 2010/11/23 02:44:40, scottz wrote:
> Is there a reason why push has to come after a potential dryrun? can we move
> this just into the cmd list defined above?

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode525
bin/cbuildbot.py:525: '-c', '--chrome_rev', default=None, type='string',
Ok.  I don't particuraly like adding unnecessary things.  I may at some point
just remove all the unnecessary dest's.

On 2010/11/23 02:44:40, scottz wrote:
> please align as the others are.
> add a dest=chrome_rev, I know it isn't required but we put it everywhere else.
I
> see the above two don't have it either, they should.

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode537
bin/cbuildbot.py:537: help='Don\'t sync before building.')
I believe this breaks style guidelines and will encourage others to use ""'s
when they shouldn't

On 2010/11/23 02:44:40, scottz wrote:
> use double quotes instead of escaping the apostrophe.  "Don't"

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
File bin/cros_mark_chrome_as_stable.py (right):

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:267: usage = '%s OPTIONS [%s]' % ((__file__),
'|'.join(CHROME_REV))
On 2010/11/23 02:44:40, scottz wrote:
> __file__ withut the parens? 

Done.

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:269: parser.add_option('-s', '--srcroot',
default=('%s/trunk/src' %
On 2010/11/23 02:44:40, scottz wrote:
> os.path.join(os.environ['HOME'], ....)

Done.

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:274: (options, argv) = parser.parse_args(argv)
On 2010/11/23 02:44:40, scottz wrote:
> I know this isn't your code but this is bad. We are taking sys.argv as passed
in
> below and then passing it to parser which isn't necessary and then redefining
> argv to be something different.
> 
> Can we just change this to parser.parse_args()? Rename argv to args in the
tuple
> and not pass sys.argv in to main at all? 
> 
> I got to this from line 281 so it sort of relates to your change :) 

Done.

http://codereview.chromium.org/5154008/diff/1003/bin/cros_mark_chrome_as_stab...
bin/cros_mark_chrome_as_stable.py:313: # Explicit print to communicate to
caller.
Good idea.  Even though cbuildbot is the only expected caller it does make sense
to make it as general as possible.

Sorry this was committed in a previous commit and is new.  I broke this into 4
commits to make it easier to review.


On 2010/11/23 02:44:40, scottz wrote:
> perhaps I do not understand all that is being done here but are we explicitly
> printing this so the caller can scrape this? If so maybe adding a bit more
about
> this or to the file in general about how if prints are added it could break
> things?  We may want to print something like PORTAGE_VERSION_ATOM=XXXXX 
> 
> then whoever is scraping it can key in on that but i need more information
about
> what is going on here.

[scottz, 2010-11-24 04:21:53]

As for the code LGTM. 

I wouldn't mind continuing the style guide discussion. If you want to move it
into email that is fine with me.

-Scott

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode210
bin/cbuildbot.py:210: """Returns the atom for the newly revved chrome ebuild."""
That is entirely up to you, I like to error on the side of more documentation
but as you stated there is nothing that enforces that. 

On 2010/11/23 21:09:19, sosa wrote:
> I don't agree.  According to PEP-8 and our own style guides, private functions
> do not need to be fully docstring'd.  After all, this function is just a
wrapper
> to a python module that IS documented.  That should be sufficient.  The same
> goes for other places.
> 
> It's a portage atom.  See man emerge - updating doc
> 
> On 2010/11/23 02:44:40, scottz wrote:
> > I know you are going to love me for this.
> > 
> > Time to start adding proper Docstrings with args and retruns and raises Oh
MY.
> > Please do so on any new function or function you modify. Future devs will
> thank
> > you :)
> > 
> > Also, what is the 'atom' ? 
>

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode422
bin/cbuildbot.py:422: cmd.append('push')
Ah right I forgot we are using gflags in cros_mark_as_stable. 
On 2010/11/23 21:09:19, sosa wrote:
> I believe gflags will not correctly parse a flag after a arg
> 
> On 2010/11/23 02:44:40, scottz wrote:
> > Is there a reason why push has to come after a potential dryrun? can we move
> > this just into the cmd list defined above?
>

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode525
bin/cbuildbot.py:525: '-c', '--chrome_rev', default=None, type='string',
I am fine with them either all being removed or added everywhere, it is more the
consistency I am after. 

On 2010/11/23 21:09:19, sosa wrote:
> Ok.  I don't particuraly like adding unnecessary things.  I may at some point
> just remove all the unnecessary dest's.
> 
> On 2010/11/23 02:44:40, scottz wrote:
> > please align as the others are.
> > add a dest=chrome_rev, I know it isn't required but we put it everywhere
else.
> I
> > see the above two don't have it either, they should.
>

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode537
bin/cbuildbot.py:537: help='Don\'t sync before building.')
Where do you see this breaking the style guide? The rule i've seen followed is:
"Unless internal quote characters would mess things up, the general rule is
  that single quotes should be used for short strings, double quotes for
  triple-quoted multi-line strings and docstrings.  E.g."

Which is taken from Barry's guide referenced in PEP8. 


On 2010/11/23 21:09:19, sosa wrote:
> I believe this breaks style guidelines and will encourage others to use ""'s
> when they shouldn't
> 
> On 2010/11/23 02:44:40, scottz wrote:
> > use double quotes instead of escaping the apostrophe.  "Don't" 
>

[sosa, 2010-11-24 21:18:01]

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode210
bin/cbuildbot.py:210: """Returns the atom for the newly revved chrome ebuild."""
I generally agree but there are a few special cases in this scenario.

1)  Ideally I would want to import cros_mark_* modules directly into this code. 
However, the requirement of having both run inside the chroot and this one makes
it impossible (for now).  

2)  I've put a good amount of time documenting those modules and since these are
simple wrappers for other documented python modules, I would rather have devs go
to the caller ... otherwise everytime I change the interface for the simple
wrapper I'll have to change the documentation in two places.  This isn't that
bad, except that every dev would have to do this, and I've already seen places
where this isn't kept up so we just have partially stale documentation i.e. it
is often you have a change to cros_mark_* but not a corresponding change to
cbuildbot (even though if you gave complete documentation you would want to). 
We would catch this easily if this was all one package ... as it is really
intended to be ... but see 1 (chroot's).  If you don't feel the documentation in
the other modules is sufficient, that is a different case and I'd be more than
happy to add more documentation there.



On 2010/11/24 04:21:53, scottz wrote:
> That is entirely up to you, I like to error on the side of more documentation
> but as you stated there is nothing that enforces that. 
> 
> On 2010/11/23 21:09:19, sosa wrote:
> > I don't agree.  According to PEP-8 and our own style guides, private
functions
> > do not need to be fully docstring'd.  After all, this function is just a
> wrapper
> > to a python module that IS documented.  That should be sufficient.  The same
> > goes for other places.
> > 
> > It's a portage atom.  See man emerge - updating doc
> > 
> > On 2010/11/23 02:44:40, scottz wrote:
> > > I know you are going to love me for this.
> > > 
> > > Time to start adding proper Docstrings with args and retruns and raises Oh
> MY.
> > > Please do so on any new function or function you modify. Future devs will
> > thank
> > > you :)
> > > 
> > > Also, what is the 'atom' ? 
> > 
>

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode422
bin/cbuildbot.py:422: cmd.append('push')
Yeah there's an issue to fix that :(

On 2010/11/24 04:21:53, scottz wrote:
> Ah right I forgot we are using gflags in cros_mark_as_stable. 
> On 2010/11/23 21:09:19, sosa wrote:
> > I believe gflags will not correctly parse a flag after a arg
> > 
> > On 2010/11/23 02:44:40, scottz wrote:
> > > Is there a reason why push has to come after a potential dryrun? can we
move
> > > this just into the cmd list defined above?
> > 
>

http://codereview.chromium.org/5154008/diff/1003/bin/cbuildbot.py#newcode537
bin/cbuildbot.py:537: help='Don\'t sync before building.')
On 2010/11/24 04:21:53, scottz wrote:
> Where do you see this breaking the style guide? The rule i've seen followed
is:
> "Unless internal quote characters would mess things up, the general rule is
>   that single quotes should be used for short strings, double quotes for
>   triple-quoted multi-line strings and docstrings.  E.g."
> 
> Which is taken from Barry's guide referenced in PEP8. 
> 
> 
> On 2010/11/23 21:09:19, sosa wrote:
> > I believe this breaks style guidelines and will encourage others to use ""'s
> > when they shouldn't
> > 
> > On 2010/11/23 02:44:40, scottz wrote:
> > > use double quotes instead of escaping the apostrophe.  "Don't" 
> > 
> 

Done.