Add more error checking to preflight queue.

cros_mark_as_stable.py

Index: cros_mark_as_stable.py
diff --git a/cros_mark_as_stable.py b/cros_mark_as_stable.py
index 56a589c68214d2485ea7ec123a02e382a4ecfce0..e722ad3cd05781e0cbe36e6f8d5318bef7ab9db6 100755
--- a/cros_mark_as_stable.py
+++ b/cros_mark_as_stable.py
@@ -22,10 +22,10 @@ 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.')
@@ -73,19 +73,21 @@ def _CleanStalePackages(board, package_array):
     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:
@@ -245,7 +247,10 @@ 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 ========================
@@ -297,7 +302,7 @@ class _EBuild(object):
     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,
@@ -323,7 +328,7 @@ class _EBuild(object):
 
     # 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'
@@ -487,11 +492,16 @@ def main(argv):
   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: []
@@ -500,6 +510,7 @@ def main(argv):
   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)
@@ -534,6 +545,7 @@ def main(argv):
                   '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: