Add more error checking to preflight queue.

cros_mark_as_stable.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.
 
 """This module uprevs a given package's ebuild to the next revision."""
 
 
 import fileinput
 import gflags
 import os
 import re
 import shutil
 import subprocess
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), 'lib'))
 from cros_build_lib import Info, RunCommand, Warning, Die
 
 
 gflags.DEFINE_string('board', '',
                      'Board for which the package belongs.', short_name='b')
 gflags.DEFINE_string('overlays', '',
-                     'Space separated list of overlays to modify.',
+                     'Colon-separated list of overlays to modify.',
                      short_name='o')
 gflags.DEFINE_string('packages', '',
-                     'Space separated list of packages to mark as stable.',
+                     'Colon-separated list of packages to mark as stable.',
                      short_name='p')
 gflags.DEFINE_string('push_options', '',
                      'Options to use with git-cl push using push command.')
 gflags.DEFINE_string('srcroot', '%s/trunk/src' % os.environ['HOME'],
                      'Path to root src directory.',
                      short_name='r')
 gflags.DEFINE_string('tracking_branch', 'cros/master',
                      'Used with commit to specify branch to track against.',
                      short_name='t')
 gflags.DEFINE_boolean('all', False,
@@ skipping 199 matching lines @@
   _SimpleRunCommand('git commit -m "%s"' % description)
   # Ugh. There has got to be an easier way to push to a tracking branch
   _SimpleRunCommand('git config push.default tracking')
   _SimpleRunCommand('git push')
 
 
 def _SimpleRunCommand(command):
   """Runs a shell command and returns stdout back to caller."""
   _Print('  + %s' % command)
   proc_handle = subprocess.Popen(command, stdout=subprocess.PIPE, shell=True)
-  return proc_handle.communicate()[0]
+  stdout = proc_handle.communicate()[0]
+  retcode = proc_handle.wait()
+  if retcode != 0:
+    raise subprocess.CalledProcessError(retcode, command, output=stdout)
+  return stdout
 
 
 # ======================= End Global Helper Functions ========================
 
 
 class _GitBranch(object):
   """Wrapper class for a git branch."""
 
   def __init__(self, branch_name):
     """Sets up variables but does not create the branch."""
@@ skipping 57 matching lines @@
       elif (line.startswith('KEYWORDS=') and '~' not in line and
             ('amd64' in line or 'x86' in line or 'arm' in line)):
         self.is_stable = True
     fileinput.close()
 
   def GetCommitId(self):
     """Get the commit id for this ebuild."""
 
     # Grab and evaluate CROS_WORKON variables from this ebuild.
     unstable_ebuild = '%s-9999.ebuild' % self.ebuild_path_no_version
-    cmd = ('CROS_WORKON_LOCALNAME="%s" CROS_WORKON_PROJECT="%s" '
+    cmd = ('export CROS_WORKON_LOCALNAME="%s" CROS_WORKON_PROJECT="%s"; '
            'eval $(grep -E "^CROS_WORKON" %s) && '
            'echo $CROS_WORKON_PROJECT '
            '$CROS_WORKON_LOCALNAME/$CROS_WORKON_SUBDIR'
            % (self.pkgname, self.pkgname, unstable_ebuild))
     project, subdir = _SimpleRunCommand(cmd).split()
 
     # Calculate srcdir.
     srcroot = gflags.FLAGS.srcroot
     if self.category == 'chromeos-base':
       dir = 'platform'
     else:
       dir = 'third_party'
     srcdir = os.path.join(srcroot, dir, subdir)
 
     # TODO(anush): This hack is only necessary because the kernel ebuild has
     # 'if' statements, so we can't grab the CROS_WORKON_LOCALNAME properly.
     # We should clean up the kernel ebuild and remove this hack.
-    if not os.path.exists(srcdir) and subdir == 'kernel/':
+    if not os.path.isdir(srcdir) and subdir == 'kernel/':
       srcdir = os.path.join(srcroot, 'third_party/kernel/files')
 
-    if not os.path.exists(srcdir):
+    if not os.path.isdir(srcdir):
       Die('Cannot find commit id for %s' % self.ebuild_path)
 
     # Verify that we're grabbing the commit id from the right project name.
     # NOTE: chromeos-kernel has the wrong project name, so it fails this
     # check.
     # TODO(davidjames): Fix the project name in the chromeos-kernel ebuild.
     cmd = 'cd %s && git config --get remote.cros.projectname' % srcdir
     actual_project = _SimpleRunCommand(cmd).rstrip()
     if project not in (actual_project, 'chromeos-kernel'):
       Die('Project name mismatch for %s (%s != %s)' % (unstable_ebuild, project,
           actual_project))
 
     # Get commit id.
     output = _SimpleRunCommand('cd %s && git rev-parse HEAD' % srcdir)
     if not output:
       Die('Missing commit id for %s' % self.ebuild_path)
     return output.rstrip()
 
   @classmethod
   def _ParseEBuildPath(cls, ebuild_path):
     """Static method that parses the path of an ebuild
 
     Returns a tuple containing the (ebuild path without the revision
     string, without the version string, and the current revision number for
     the ebuild).
     """
      # Get the ebuild name without the revision string.
     (ebuild_no_rev, _, rev_string) = ebuild_path.rpartition('-')

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

Sorry!  I got confused with rsplit.  :(  Thanks for checking!

 
     # Verify the revision string starts with the revision character.
     if rev_string.startswith('r'):
       # Get the ebuild name without the revision and version strings.
       ebuild_no_version = ebuild_no_rev.rpartition('-')[0]
       rev_string = rev_string[1:].rpartition('.ebuild')[0]
     else:
       # Has no revision so we stripped the version number instead.

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

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.

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

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).

       ebuild_no_version = ebuild_no_rev
       ebuild_no_rev = ebuild_path.rpartition('9999.ebuild')[0] + '0.0.1'
       rev_string = '0'
     revision = int(rev_string)
     return (ebuild_no_rev, ebuild_no_version, revision)
 
 
 class EBuildStableMarker(object):
   """Class that revs the ebuild and commits locally or pushes the change."""
 
@@ skipping 86 matching lines @@
 def main(argv):
   try:
     argv = gflags.FLAGS(argv)
     if len(argv) != 2:
       _PrintUsageAndDie('Must specify a valid command')
     else:
       command = argv[1]
   except gflags.FlagsError, e :
     _PrintUsageAndDie(str(e))
 
-  package_list = gflags.FLAGS.packages.split()
+  package_list = gflags.FLAGS.packages.split(':')
   _CheckSaneArguments(package_list, command)
   if gflags.FLAGS.overlays:
-    overlays = dict((path, []) for path in gflags.FLAGS.overlays.split())
+    overlays = {}
+    for path in gflags.FLAGS.overlays.split(':'):
+      if not os.path.isdir(path):
+        Die('Cannot find overlay: %s' % path)
+      overlays[path] = []
   else:
+    Warning('Missing --overlays argument')
     overlays = {
       '%s/private-overlays/chromeos-overlay' % gflags.FLAGS.srcroot: [],
       '%s/third_party/chromiumos-overlay' % gflags.FLAGS.srcroot: []
     }
 
   if command == 'commit':
     _BuildEBuildDictionary(overlays, gflags.FLAGS.all, package_list)
 
   for overlay, ebuilds in overlays.items():
-    if not os.path.exists(overlay):
+    if not os.path.isdir(overlay):
       Warning("Skipping %s" % overlay)
       continue
+
+    # TODO(davidjames): Currently, all code that interacts with git depends on
+    # the cwd being set to the overlay directory. We should instead pass in
+    # this parameter so that we don't need to modify the cwd globally.
     os.chdir(overlay)

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

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]

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

 
     if command == 'clean':
       _Clean()
     elif command == 'push':
       _PushChange()
     elif command == 'commit' and ebuilds:
       work_branch = _GitBranch(_STABLE_BRANCH_NAME)
       work_branch.CreateBranch()
       if not work_branch.Exists():
         Die('Unable to create stabilizing branch in %s' % overlay)
@@ skipping 17 matching lines @@
           raise
 
       if revved_packages:
         _CleanStalePackages(gflags.FLAGS.board, revved_packages)
       else:
         work_branch.Delete()
 
 
 if __name__ == '__main__':
   main(sys.argv)