Add ability to rev and build a chrome from cbuildbot

bin/cbuildbot.py

Index: bin/cbuildbot.py
diff --git a/bin/cbuildbot.py b/bin/cbuildbot.py
index e723cd64396b9d2156b12bf04375d01429e2cd89..3f71376aa1798ade9ca1c004335a725e90e5786b 100755
--- a/bin/cbuildbot.py
+++ b/bin/cbuildbot.py
@@ -206,6 +206,20 @@ def _UprevFromRevisionList(buildroot, tracking_branch, revision_list, board,
              cwd=cwd, enter_chroot=True)
 
 
+def _MarkChromeAsStable(buildroot, tracking_branch, chrome_rev):
+  """Returns the atom for the newly revved chrome ebuild."""

[scottz, 2010/11/23 02:44:40]

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' ? 

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

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:

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

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:

[sosa, 2010/11/24 21:18:02]

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:

+  cwd = os.path.join(buildroot, 'src', 'scripts')
+  portage_atom = RunCommand(['bin/cros_mark_chrome_as_stable',
+                             '--tracking_branch=%s' % tracking_branch,
+                             chrome_rev], cwd=cwd, redirect_stdout=True,
+                             enter_chroot=True).rstrip()
+  if not portage_atom:
+    Info('Found nothing to rev.')
+    return None
+  else:
+    return portage_atom
+
+
 def _UprevAllPackages(buildroot, tracking_branch, board, overlays):
   """Uprevs all packages that have been updated since last uprev."""
   cwd = os.path.join(buildroot, 'src', 'scripts')
@@ -305,6 +319,13 @@ def _Build(buildroot):
   RunCommand(['./build_packages'], cwd=cwd, enter_chroot=True)
 
 
+def _BuildChrome(buildroot, board, chrome_atom_to_build):
+  """Wrapper for emerge call to build Chrome."""
+  cwd = os.path.join(buildroot, 'src', 'scripts')
+  RunCommand(['emerge-%s =%s' % (board, chrome_atom_to_build)],

[scottz, 2010/11/23 02:44:40]

Why do we not want this to be individual list elements?

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

Done.

+             cwd=cwd, enter_chroot=True)
+
+
 def _EnableLocalAccount(buildroot):
   cwd = os.path.join(buildroot, 'src', 'scripts')
   # Set local account for test images.
@@ -386,15 +407,20 @@ def _UprevPackages(buildroot, tracking_branch, revisionfile, board, overlays):
   _UprevAllPackages(buildroot, tracking_branch, board, overlays)
 
 
-def _UprevPush(buildroot, tracking_branch, board, overlays):
+def _UprevPush(buildroot, tracking_branch, board, overlays, dryrun):

[scottz, 2010/11/23 02:44:40]

dryrun=False or should this always be passed?

docstring reminder.. :) 

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

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:

   """Pushes uprev changes to the main line."""
   cwd = os.path.join(buildroot, 'src', 'scripts')
-  RunCommand(['./cros_mark_as_stable', '--srcroot=..',
-              '--board=%s' % board,
-              '--overlays=%s' % ':'.join(overlays),
-              '--tracking_branch=%s' % tracking_branch,
-              '--push_options=--bypass-hooks -f', 'push'],
-             cwd=cwd)
+  cmd = ['./cros_mark_as_stable',
+         '--srcroot=%s' % os.path.join(buildroot, 'src'),
+         '--board=%s' % board,
+         '--overlays=%s' % ':'.join(overlays),
+         '--tracking_branch=%s' % tracking_branch
+        ]
+  if dryrun:
+    cmd.append('--dryrun')
+
+  cmd.append('push')

[scottz, 2010/11/23 02:44:40]

Is there a reason why push has to come after a potential dryrun? can we move
this just into the cmd list defined above?

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

I believe gflags will not correctly parse a flag after a arg

On 2010/11/23 02:44:40, scottz wrote:

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

Ah right I forgot we are using gflags in cros_mark_as_stable. 
On 2010/11/23 21:09:19, sosa wrote:

[sosa, 2010/11/24 21:18:02]

Yeah there's an issue to fix that :(

On 2010/11/24 04:21:53, scottz wrote:

+  RunCommand(cmd, cwd=cwd)
 
 
 def _ArchiveTestResults(buildroot, board, archive_dir, test_results_dir):
@@ -495,6 +521,9 @@ def main():
                     help='root directory where build occurs', default=".")
   parser.add_option('-n', '--buildnumber',
                     help='build number', type='int', default=0)
+  parser.add_option(
+      '-c', '--chrome_rev', default=None, type='string',

[scottz, 2010/11/23 02:44:40]

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.

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

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:

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

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:

+      help='Chrome_rev of type [tot|latest_release|sticky_release]')
   parser.add_option('-f', '--revisionfile',
                     help='file where new revisions are stored')
   parser.add_option('--clobber', action='store_true', dest='clobber',
@@ -503,6 +532,12 @@ def main():
   parser.add_option('--debug', action='store_true', dest='debug',
                     default=False,
                     help='Override some options to run as a developer.')
+  parser.add_option('--nosync', action='store_false', dest='sync',
+                    default=True,
+                    help='Don\'t sync before building.')

[scottz, 2010/11/23 02:44:40]

use double quotes instead of escaping the apostrophe.  "Don't" 

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

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:

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

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:

[sosa, 2010/11/24 21:18:02]

Done.

+  parser.add_option('--notests', action='store_false', dest='tests',
+                    default=True,
+                    help='Override values from buildconfig and run no tests.')
   parser.add_option('-t', '--tracking-branch', dest='tracking_branch',
                     default='cros/master', help='Run the buildbot on a branch')
   parser.add_option('-u', '--url', dest='url',
@@ -514,6 +549,7 @@ def main():
   buildroot = os.path.abspath(options.buildroot)
   revisionfile = options.revisionfile
   tracking_branch = options.tracking_branch
+  chrome_atom_to_build = None
 
   if len(args) >= 1:
     buildconfig = _GetConfig(args[-1])
@@ -527,10 +563,11 @@ def main():
 
   try:
     _PreFlightRinse(buildroot, buildconfig['board'], tracking_branch, overlays)
-    if options.clobber or not os.path.isdir(buildroot):
-      _FullCheckout(buildroot, tracking_branch, url=options.url)
-    else:
-      _IncrementalCheckout(buildroot)
+    if options.sync:
+      if options.clobber or not os.path.isdir(buildroot):
+        _FullCheckout(buildroot, tracking_branch, url=options.url)
+      else:
+        _IncrementalCheckout(buildroot)
 
     # Check that all overlays can be found.
     for path in overlays:
@@ -546,18 +583,28 @@ def main():
     if not os.path.isdir(boardpath):
       _SetupBoard(buildroot, board=buildconfig['board'])
 
-    if buildconfig['uprev']:
+    # Perform uprev.  If chrome_uprev is set, rev Chrome ebuilds.
+    if options.chrome_rev:
+      chrome_atom_to_build = _MarkChromeAsStable(buildroot, tracking_branch,
+                                                 options.chrome_rev)
+    elif buildconfig['uprev']:
       _UprevPackages(buildroot, tracking_branch, revisionfile,
                      buildconfig['board'], overlays)
 
     _EnableLocalAccount(buildroot)
-    _Build(buildroot)
-    if buildconfig['unittests']:
+    # Doesn't rebuild without acquiring more source.
+    if options.sync:
+      _Build(buildroot)
+
+    if chrome_atom_to_build:
+      _BuildChrome(buildroot, buildconfig['board'], chrome_atom_to_build)
+
+    if buildconfig['unittests'] and options.tests:
       _RunUnitTests(buildroot)
 
     _BuildImage(buildroot)
 
-    if buildconfig['smoke_bvt']:
+    if buildconfig['smoke_bvt'] and options.tests:
       _BuildVMImageForTesting(buildroot)
       test_results_dir = '/tmp/run_remote_tests.%s' % options.buildnumber
       try:
@@ -569,19 +616,18 @@ def main():
 
     if buildconfig['uprev']:
       # Don't push changes for developers.
-      if not options.debug:
-        if buildconfig['master']:
-          # Master bot needs to check if the other slaves completed.
-          if cbuildbot_comm.HaveSlavesCompleted(config):
-            _UprevPush(buildroot, tracking_branch, buildconfig['board'],
-                       overlays)
-          else:
-            Die('CBUILDBOT - One of the slaves has failed!!!')
-
+      if buildconfig['master']:
+        # Master bot needs to check if the other slaves completed.
+        if cbuildbot_comm.HaveSlavesCompleted(config):
+          _UprevPush(buildroot, tracking_branch, buildconfig['board'],
+                     overlays, options.debug)
         else:
-          # Publish my status to the master if its expecting it.
-          if buildconfig['important']:
-            cbuildbot_comm.PublishStatus(cbuildbot_comm.STATUS_BUILD_COMPLETE)
+          Die('CBUILDBOT - One of the slaves has failed!!!')
+
+      else:
+        # Publish my status to the master if its expecting it.
+        if buildconfig['important'] and not options.debug:
+          cbuildbot_comm.PublishStatus(cbuildbot_comm.STATUS_BUILD_COMPLETE)
 
   except:
     # Send failure to master bot.