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.',

[diandersAtChromium, 2010/11/12 19:25:30]

Any easy way to detect other places that might have called cros_mark_as_stable
before and that might be broken?  Build bots?  Tests?  Developers?  

There does seem to be something in crostools that calls cros_mark_as_stable,
though it doesn't pass either of these parameters.  Hmm, won't that cause a
warning?  Any bad behavior?

[sosa, 2010/11/12 19:38:09]

CBuildbot is the only caller as this manages the pfq.

On 2010/11/12 19:25:30, diandersAtChromium wrote:

                      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 22 matching lines @@
 
 # ======================= Global Helper Functions ========================
 
 
 def _Print(message):
   """Verbose print function."""
   if gflags.FLAGS.verbose:
     Info(message)
 
 
 def _CleanStalePackages(board, package_array):

[diandersAtChromium, 2010/11/12 19:25:30]

Downside of all the enter_chroot here: it will be much slower (entering/exiting
the chroot can take a lot of time, at least on my machine).

[sosa, 2010/11/12 19:38:09]

This is a really good point I didn't think about.  This is probably a good
enough reason not to move this into the chroot if we can avoid it.  There's
already a method in the common python lib to reinterpret a path in the chroot. 
THis might be a better solution to the problem that you had

On 2010/11/12 19:25:30, diandersAtChromium wrote:

[davidjames, 2010/11/12 21:18:05]

Done.

     """Cleans up stale package info from a previous build."""
     Info('Cleaning up stale packages %s.' % package_array)
     unmerge_board_cmd = ['emerge-%s' % board, '--unmerge']
     unmerge_board_cmd.extend(package_array)
-    RunCommand(unmerge_board_cmd)
+    RunCommand(unmerge_board_cmd, enter_chroot=True)
 
     unmerge_host_cmd = ['sudo', 'emerge', '--unmerge']
     unmerge_host_cmd.extend(package_array)
-    RunCommand(unmerge_host_cmd)
+    RunCommand(unmerge_host_cmd, enter_chroot=True)
 
-    RunCommand(['eclean-%s' % board, '-d', 'packages'], redirect_stderr=True)
-    RunCommand(['sudo', 'eclean', '-d', 'packages'], redirect_stderr=True)
+    RunCommand(['eclean-%s' % board, '-d', 'packages'], redirect_stderr=True,
+               enter_chroot=True)
+    RunCommand(['sudo', 'eclean', '-d', 'packages'], redirect_stderr=True,
+               enter_chroot=True)
 
 
 def _BestEBuild(ebuilds):
   """Returns the newest EBuild from a list of EBuild objects."""
-  from portage.versions import vercmp
+  from cros_portage_versions import vercmp
   winner = ebuilds[0]
   for ebuild in ebuilds[1:]:
     if vercmp(winner.version, ebuild.version) < 0:
       winner = ebuild
   return winner
 
 
 def _FindUprevCandidates(files):
   """Return a list of uprev candidates from specified list of files.
 
@@ skipping 139 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()
+  assert retcode == 0, "Return code %s for command: %s" % (retcode, command)

[diandersAtChromium, 2010/11/12 19:25:30]

Not convinced that assert should be used here unless it really is an unexpected
event.

[sosa, 2010/11/12 19:38:09]

Throw an exception.

On 2010/11/12 19:25:30, diandersAtChromium wrote:

[davidjames, 2010/11/12 21:18:05]

Done.

+  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 31 matching lines @@
 
 class _EBuild(object):
   """Wrapper class for an ebuild."""
 
   def __init__(self, path):
     """Initializes all data about an ebuild.
 
     Uses equery to find the ebuild path and sets data about an ebuild for
     easy reference.
     """
-    from portage.versions import pkgsplit
+    from cros_portage_versions import pkgsplit
     self.ebuild_path = path
     (self.ebuild_path_no_revision,
      self.ebuild_path_no_version,
      self.current_revision) = self._ParseEBuildPath(self.ebuild_path)
     _, self.category, pkgpath, filename = path.rsplit('/', 3)
     filename_no_suffix = os.path.join(filename.replace('.ebuild', ''))
     self.pkgname, version_no_rev, rev = pkgsplit(filename_no_suffix)
     self.version = '%s-%s' % (version_no_rev, rev)
     self.package = '%s/%s' % (self.category, self.pkgname)
     self.is_workon = False
     self.is_stable = False
 
     for line in fileinput.input(path):
       if line.startswith('inherit ') and 'cros-workon' in line:
         self.is_workon = True
       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'
@@ skipping 28 matching lines @@
 
   @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 19:25:30]

It looks like you're sorta getting "lucky" that this isn't crashing when you've
got a "-9999.ebuild".  The path looks like this:
  net-misc/dhcpcd/dhcpcd-9999.ebuild

The fact that "net-misc" has a dash in it is saving you.  Ick.  Is it guaranteed
that we'll always have at least two dashes in the path?  If not, this needs to
be smarter.

[davidjames, 2010/11/12 21:18:05]

Just tested this. Looks like it works, and does not in fact depend on a hyphen
in the package name.
('net-misc/dhcpcd/dhcpcd', '-', '9999')
('foo/dhcpcd/dhcpcd', '-', '9999')

That said it'd be good to move over to portage methods instead. But that can
wait until a subsequent commit.

 
     # 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 19:25:30]

Old case of having no revision number at all is no longer handled.  Was that on
purpose?

What would happen with something like "./sys-apps/parted/parted-1.9.0.ebuild"?

[sosa, 2010/11/12 19:38:09]

This handles the 9999 case for the above comment.

On 2010/11/12 19:25:30, diandersAtChromium wrote:

       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.exists(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)
 
+  cwd = os.getcwd()
   for overlay, ebuilds in overlays.items():
     if not os.path.exists(overlay):
       Warning("Skipping %s" % overlay)
       continue
     os.chdir(overlay)

[diandersAtChromium, 2010/11/12 19:25:30]

Why is chdir here?  What actually needs it?  ...and why do you only chdir back
for certain conditions in the loop?  Comment please?

[sosa, 2010/11/12 19:38:09]

For local git commits.

On 2010/11/12 19:25:30, diandersAtChromium wrote:

[davidjames, 2010/11/12 21:18:05]

Added a comment.

 
     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)
 
       # Contains the array of packages we actually revved.
       revved_packages = []
       for ebuild in ebuilds:
         try:
           _Print('Working on %s' % ebuild.package)
           worker = EBuildStableMarker(ebuild)
           commit_id = ebuild.GetCommitId()
           if worker.RevEBuild(commit_id):
             message = _GIT_COMMIT_MESSAGE % (ebuild.package, commit_id)
             worker.CommitChange(message)
             revved_packages.append(ebuild.package)
 
         except (OSError, IOError):
           Warning('Cannot rev %s\n' % ebuild.package,
                   'Note you will have to go into %s '
                   'and reset the git repo yourself.' % overlay)
           raise
 
+      os.chdir(cwd)

[sosa, 2010/11/12 19:38:09]

Why do you chdir back before work_branch.Delete???

[davidjames, 2010/11/12 21:18:05]

Removed.

This was necessary for entering the chroot, but you're right, it breaks the
work_branch.Delete case. But I've followed your suggestions and took out the
chroot entering here so this is no longer necessary.

       if revved_packages:
         _CleanStalePackages(gflags.FLAGS.board, revved_packages)
       else:
         work_branch.Delete()
 
 
 if __name__ == '__main__':
   main(sys.argv)