Add ability to rev and build a chrome from cbuildbot

bin/cbuildbot.py

 #!/usr/bin/python
 
 # Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """CBuildbot is wrapper around the build process used by the pre-flight queue"""
 
 import errno
 import heapq
@@ skipping 188 matching lines @@
   cwd = os.path.join(buildroot, 'src', 'scripts')
   RunCommand(['./cros_mark_as_stable',
               '--board=%s' % board,
               '--tracking_branch=%s' % tracking_branch,
               '--overlays=%s' % ':'.join(chroot_overlays),
               '--packages=%s' % ':'.join(packages),
               'commit'],
              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')
   chroot_overlays = [ReinterpretPathForChroot(path) for path in overlays]
   RunCommand(['./cros_mark_as_stable', '--all',
               '--board=%s' % board,
               '--overlays=%s' % ':'.join(chroot_overlays),
               '--tracking_branch=%s' % tracking_branch, 'commit'],
               cwd=cwd, enter_chroot=True)
 
@@ skipping 79 matching lines @@
   RunCommand(['./setup_board', '--fast', '--default', '--board=%s' % board],
              cwd=cwd, enter_chroot=True)
 
 
 def _Build(buildroot):
   """Wrapper around build_packages."""
   cwd = os.path.join(buildroot, 'src', 'scripts')
   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.
   RunCommand(['./enable_localaccount.sh',
              'chronos'],
              print_cmd=False, cwd=cwd)
 
 
 def _BuildImage(buildroot):
   _WipeOldOutput(buildroot)
@@ skipping 61 matching lines @@
   #  print >> sys.stderr, 'CBUILDBOT Revision list found %s' % revisions
   #  revision_list = _ParseRevisionString(revisions,
   #      _CreateRepoDictionary(buildroot, board))
   #  _UprevFromRevisionList(buildroot, tracking_branch, revision_list, board,
   #                         overlays)
   #else:
   Info('CBUILDBOT Revving all')
   _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):
   """Archives the test results into the www dir for later use.
 
   Takes the results from the test_results_dir and dumps them into the archive
   dir specified.  This also archives the last qemu image.
 
   board:  Board to find the qemu image.
   archive_dir:  Path from ARCHIVE_BASE to store image.
@@ skipping 80 matching lines @@
 
 
 def main():
   # Parse options
   usage = "usage: %prog [options] cbuildbot_config"
   parser = optparse.OptionParser(usage=usage)
   parser.add_option('-r', '--buildroot',
                     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',
                     default=False,
                     help='Clobbers an old checkout before syncing')
   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',
                     default='http://git.chromium.org/git/manifest',
                     help='Run the buildbot on internal manifest')
 
   (options, args) = parser.parse_args()
 
   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])
   else:
     Warning('Missing configuration description')
     parser.print_usage()
     sys.exit(1)
 
   # Calculate list of overlay directories.
   overlays = _ResolveOverlays(buildroot, buildconfig['overlays'])
 
   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:
       assert ':' not in path, 'Overlay must not contain colons: %s' % path
       if not os.path.isdir(path):
         Die('Missing overlay: %s' % path)
 
     chroot_path = os.path.join(buildroot, 'chroot')
     if not os.path.isdir(chroot_path):
       _MakeChroot(buildroot)
 
     boardpath = os.path.join(chroot_path, 'build', buildconfig['board'])
     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:
         _RunSmokeSuite(buildroot, test_results_dir)
       finally:
         _ArchiveTestResults(buildroot, buildconfig['board'],
                             archive_dir=options.buildnumber,
                             test_results_dir=test_results_dir)
 
     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:
+          Die('CBUILDBOT - One of the slaves has failed!!!')
 
-        else:
-          # Publish my status to the master if its expecting it.
-          if buildconfig['important']:
-            cbuildbot_comm.PublishStatus(cbuildbot_comm.STATUS_BUILD_COMPLETE)
+      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.
     if not buildconfig['master'] and buildconfig['important']:
       cbuildbot_comm.PublishStatus(cbuildbot_comm.STATUS_BUILD_FAILED)
 
     raise
 
 
 if __name__ == '__main__':
     main()