CrOS: Update Chromite recipe module.

scripts/slave/recipe_modules/chromite/api.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import cgi
 import re
 
 from recipe_engine import recipe_api
 
 
 class ChromiteApi(recipe_api.RecipeApi):
   chromite_url = 'https://chromium.googlesource.com/chromiumos/chromite.git'
   manifest_url = 'https://chromium.googlesource.com/chromiumos/manifest.git'
   repo_url = 'https://chromium.googlesource.com/external/repo.git'
   chromite_subpath = 'chromite'
 
   # The number of Gitiles attempts to make before giving up.
   _GITILES_ATTEMPTS = 10
 
   _MANIFEST_CMD_RE = re.compile(r'Automatic:\s+Start\s+([^\s]+)\s+([^\s]+)')
   _BUILD_ID_RE = re.compile(r'CrOS-Build-Id: (.+)')
 
+  def get_config_defaults(self):
+    build_number = self.m.properties.get('buildnumber')
+    if build_number is not None:
+        # On a developer system, it's been noted that when the build number is
+        # zero, it's passed as an empty string in the properties JSON blob.

[luqui, 2015/07/31 19:58:15]

The purpose of this comment is unclear.  So this line is a bug workaround?  File
a crbug and link it if so.

[dnj (Google), 2015/08/01 01:31:34]

Filed https://code.google.com/p/chromium/issues/detail?id=516091

Rather than link, I'm going to just fix this bug:
https://codereview.chromium.org/1254003005

+        build_number = int(build_number or 0)
+
+    return {
+        'CBB_CONFIG': self.m.properties.get('cbb_config'),
+        'CBB_BRANCH': self.m.properties.get('cbb_branch'),
+        'CBB_BUILD_NUMBER': build_number,
+        'CBB_DEBUG': self.m.properties.get('cbb_debug') is not None,
+        'CBB_CLOBBER': 'clobber' in self.m.properties,
+    }
+
   def check_repository(self, repo_type_key, value):
     """Scans through registered repositories for a specified value.
 
     Args:
       repo_type_key (str): The key in the 'repositories' config to scan through.
       value (str): The value to scan for.
     Returns (bool): True if the value was found.
     """
     def remove_tail(v, tail):
       if v.endswith(tail):
@@ skipping 81 matching lines @@
       # Build ID?
       match = self._BUILD_ID_RE.match(line)
       if match:
         self.c.cbb.build_id = match.group(1)
         loaded.append('Build ID: %s' % (self.c.cbb.build_id,))
         continue
     if loaded:
       loaded.insert(0, '')
       result.presentation.step_text += '<br/>'.join(loaded)
 
+  @property
   def default_chromite_path(self):
     """Returns: (Path) The default Chromite checkout path."""
     return self.m.path['slave_build'].join(self.chromite_subpath)
 
   def gclient_config(self):
     """Generate a 'gclient' configuration to check out Chromite.
 
     Return: (config) A 'gclient' recipe module configuration.
     """
     cfg = self.m.gclient.make_config()
     soln = cfg.solutions.add()
     soln.name = 'chromite'
     soln.url = self.chromite_url
     # Set the revision using 'bot_update' remote branch:revision notation.
     # Omitting the revision uses HEAD.
     soln.revision = '%s:' % (self.c.chromite_branch,)
     return cfg
 
   def checkout(self, manifest_url=None, repo_url=None):
     manifest_url = manifest_url or self.manifest_url
     repo_url = repo_url or self.repo_url
 
     self.m.repo.init(manifest_url, '--repo-url', repo_url)
     self.m.repo.sync()
 
+  @property
+  def old_chromite_layout(self):

[luqui, 2015/07/31 19:58:15]

How about using_old_chromite_layout.  This naming makes me think that it returns
the layout (which I know is nonsense, still confused me though)

[dnj (Google), 2015/08/01 01:31:34]

Done.

+    """Returns (bool): True if we're using old Chromite checkout layout.
+    """
+    return self.c.chromite_branch in self.c.old_chromite_branches
+
   def cbuildbot(self, name, config, args=None, chromite_path=None, **kwargs):
     """Runs the cbuildbot command defined by the arguments.
 
     Args:
       name: (str) The name of the command step.
       config: (str) The name of the 'cbuildbot' configuration to invoke.
       args: (list) If not None, addition arguments to pass to 'cbuildbot'.
       chromite_path: (str) The path to the Chromite checkout; if None, the
-          'default_chromite_path()' will be used.
+          'default_chromite_path' will be used.
 
     Returns: (Step) The step that was run.
     """
-    chromite_path = chromite_path or self.default_chromite_path()
+    chromite_path = chromite_path or self.default_chromite_path
     args = (args or [])[:]
     args.append(config)
 
-    cmd = [self.m.path.join(chromite_path, 'bin', 'cbuildbot')] + args
-
-    # TODO(petermayo): Wrap this nested annotation in a stabilizing wrapper.
+    bindir = ('bin' if not self.old_chromite_layout else 'buildbot')

[luqui, 2015/07/31 19:58:15]

This is convoluted to read.  I see what you're going for though, 'bin' is like
the usual one, 'buildbot' is the strange off-case.  Maybe

   bindir = 'bin'
   if self.old_chromite_layout:
       bindir = 'buildbot'

[dnj (Google), 2015/08/01 01:31:34]

Agreed this is more readable. Stylistically I like this better too, so Done!

+    cmd = [self.m.path.join(chromite_path, bindir, 'cbuildbot')] + args
     return self.m.step(name, cmd, allow_subannotations=True, **kwargs)
 
   def cros_sdk(self, name, cmd, args=None, environ=None, chromite_path=None,
                  **kwargs):
     """Return a step to run a command inside the cros_sdk."""
-    chromite_path = chromite_path or self.default_chromite_path()
+    chromite_path = chromite_path or self.default_chromite_path
 
     chroot_cmd = self.m.path.join(chromite_path, 'bin', 'cros_sdk')
 
     arg_list = (args or [])[:]
     for t in sorted((environ or {}).items()):
       arg_list.append('%s=%s' % t)
     arg_list.append('--')
     arg_list.extend(cmd)
 
     self.m.python(name, chroot_cmd, arg_list, **kwargs)
@@ skipping 25 matching lines @@
     master_config = config_map.get('master_config')
     assert master_config, (
         "No 'master_config' configuration for '%s'" % (master,))
     self.set_config(master_config)
 
     # Apply any variant configurations.
     if variant:
       for config_name in config_map.get('variants', {}).get(variant, ()):
         self.apply_config(config_name)
 
-    # If a Chromite branch is supplied, use it to override the default Chromite
-    # checkout revision.
-    if properties.get('cbb_branch'):
-      self.c.chromite_branch = properties['cbb_branch']
-
-    # Set the build number if one is defined in the properties.
-    if self.m.properties.get('buildnumber') is not None:
-      # On a developer system, it's been noted that when the build number is
-      # zero, it's passed as an empty string in the properties JSON blob.
-      self.c.cbb.build_number = int(self.m.properties['buildnumber'] or 0)
-
-    # Run a debug build if instructed.
-    if properties.get('cbb_debug'):
-      self.c.cbb.debug = True
-
-    # If a clobber build was requested, set this builder to clobber.
-    if 'clobber' in properties:
-      self.c.cbb.clobber = True
 
     # If a config repo was specified, use it.
     if 'config_repo' in properties:
       self.c.cbb.config_repo = self.m.properties['config_repo']
 
-  def run_cbuildbot(self, config, tryjob=False):
+  def run_cbuildbot(self, tryjob=False):
     """Runs a 'cbuildbot' checkout-and-build workflow.
 
     This workflow uses the registered configuration dictionary to make master-
     and builder-specific changes to the standard workflow.
 
     The specific workflow paths that are taken are also influenced by several
     build properties.
 
     TODO(dnj): When CrOS migrates away from BuildBot, replace property
         inferences with command-line parameters.
 
     This workflow:
     - Checks out the specified 'cbuildbot' repository.
     - Pulls information based on the configured change's repository/revision
       to pass to 'cbuildbot'.
     - Executes the 'cbuildbot' command.
 
     Args:
-      config (str): The name of the 'cbuildbot' configuration target to build.
       tryjob (bool): If True, load a tryjob description from the source
           repository and augment the cbuildbot command-line with it.
     Returns: (Step) the 'cbuildbot' execution step.
     """
 
     # Assert correct configuration.
-    assert config, 'An empty configuration was specified.'
     assert self.c.cbb.builddir, 'A build directory name must be specified.'
 
     # Load properties from the commit being processed. This requires both a
     # repository and revision to proceed.
     repository = self.m.properties.get('repository')
     revision = self.m.properties.get('revision')
     tryjob_args = []
     if repository and revision:
       if tryjob:
         assert self.check_repository('tryjob', repository), (
@@ skipping 42 matching lines @@
     # Add tryjob args, if there are any.
     cbb_args.extend(tryjob_args)
 
     # Checkout Chromite.
     self.m.bot_update.ensure_checkout(
         gclient_config=self.gclient_config(),
         update_presentation=False,
         force=True)
 
     # Run cbuildbot.
-    return self.cbuildbot(str('cbuildbot [%s]' % (config,)),
-                          config,
+    return self.cbuildbot(str('cbuildbot [%s]' % (self.c.cbb.config,)),
+                          self.c.cbb.config,
                           args=cbb_args,
                           chromite_path=self.m.path['checkout'],
                           cwd=self.m.path['slave_root'])