Modify devserver and gmerge to be friendlier

gmerge

-#!/bin/bash
+#!/usr/bin/env python

[sosa, 2011/01/14 23:03:54]

Never seen this style before ... 

[rochberg, 2011/01/15 06:16:48]

This is so we can run even if python is in /usr/local/bin

 
-# Copyright (c) 2009-2010 The Chromium OS Authors. All rights reserved.
+# Copyright (c) 2011 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.

[sosa, 2011/01/14 23:03:54]

Maybe add a module docstring.

[rochberg, 2011/01/15 06:16:48]

Done.

 
-# Prints the arguments and dies.
-print_and_die() {
-  echo $*
-  exit 1
-}
+import optparse
+import os
+import shutil
+import subprocess
+import sys
+import urllib
+import urllib2
 
-# Set up PORTAGE variables to be rooted in /usr/local/portage.
-setup_portage_vars() {
-  export PORTDIR=/usr/local/portage
-  export PKGDIR=/usr/local/portage
-  export DISTDIR=/usr/local/portage/distfiles
-  export PORTAGE_BINHOST="${DEVKIT_URL}/static/pkgroot/${BOARD_NAME}/packages"
-  export PORTAGE_TMPDIR=/tmp
-  export CONFIG_PROTECT="-*"
-  export FEATURES="-sandbox"
-  # Accept keywords only for stable ebuilds by default.
-  [ -z "$ACCEPT_KEYWORDS" ] && ACCEPT_KEYWORDS="arm x86 ~arm ~x86"
-  export ACCEPT_KEYWORDS
-  export ROOT=/
-}
 
-# Prompts the user to change the root to /usr/local.
-change_portage_root() {
-  read -p "Do you wish to install this program into /usr/local? (Y/n) " input
-  if [[ ${input} = [Nn]* ]]; then
-    echo "Better safe than sorry."
-   exit 1
-  fi
-  export ROOT=/usr/local
-}
+PARSER = optparse.OptionParser()

[sosa, 2011/01/14 23:03:54]

Why did you choose to make this global rather than have this in your main?

[sosa, 2011/01/14 23:03:54]

usage?

[rochberg, 2011/01/15 06:16:48]

Done.

[rochberg, 2011/01/15 06:16:48]

Done.

+PARSER.add_option('--accept_stable',
+                  action='store_true', dest='accept_stable', default=False,
+                  help=('Build even if a cros_workon package is not '
+                        'using the live package'))
+PARSER.add_option('-n', '--no_devserver',
+                  action='store_false', dest='call_devserver', default=True,
+                  help='Do not actually ask the server to build')
+PARSER.add_option('--use',
+                  dest='use', default=None,
+                  help='USE flags to pass to emerge on the server')
 
-set -e
 
-# Get variables for the devserver from the lsb-release file.
-DEVKIT_URL=$(grep ^CHROMEOS_DEVSERVER /etc/lsb-release | cut -d = -f 2-)
-BOARD_NAME=$(grep ^CHROMEOS_RELEASE_BOARD /etc/lsb-release | cut -d = -f 2-)
-[ -z "${BOARD_NAME}" ] && print_and_die "No board in /etc/lsb-release"
-[ -z "${DEVKIT_URL}" ] && print_and_die "No dev server url in /etc/lsb-release"
+class GMerger(object):
+  """emerges a package from the devserver.
 
-setup_portage_vars
+  NB:  Must be instantiated using with, e.g.:
+    with GMerger(open('/etc/lsb-release').readlines()) as merger:
+  in order to remount /tmp as executable.
+  """
 
-# Determine if we should send a build command to the devserver.
-BUILD=1
-if [ x$1 == x-n ]; then
-  BUILD=0
-  shift
-fi
+  def __init__(self, lsb_release_lines):
+    self.lsb_release = self.ParseLsbRelease(lsb_release_lines)
+    try:
+      self.devkit_url = self.lsb_release['CHROMEOS_DEVSERVER']
+      self.board_name = self.lsb_release['CHROMEOS_RELEASE_BOARD']
+    except KeyError, e:
+      sys.exit('Could not find /etc/lsb_release value: ' + e)
 
-# Package name is the last argument.
-# TODO(sosa) - Support multiple packages.
-PACKAGE_NAME=${!#}
+  def RemountOrChangeRoot(self, environ):

[sosa, 2011/01/14 23:03:54]

Please add at least one line doc strings to each method.

[rochberg, 2011/01/15 06:16:48]

Done.

+    rc = subprocess.call(['mount', '-o', 'remount,rw', '/'])
+    if rc == 0:
+      return
+    answer = raw_input(
+        'Could not mount / as writable.  Install into /usr/local? (Y/n)')
+    if answer[0] not in 'Yy':
+      sys.exit('Better safe than sorry.')
+    environ['ROOT'] = '/usr/local'
 
-# If no package name is provided skip to emerge options.
-[[ ${PACKAGE_NAME} == -* ]] && BUILD=0
+  def CleanPortageCache(self):
+    shutil.rmtree(os.path.join(os.environ['PKGDIR'], 'packages'),
+                  ignore_errors=True)

[sosa, 2011/01/14 23:03:54]

This is somewhat functionally different than the other.  Not sure if it matters
though.  Why did you choose to ignore_errors?

[rochberg, 2011/01/15 06:16:48]

First of all, the ignore_errors bit was due to some confusion and clearly wrong.

But I don't think the line in the original is correct either:

# Delete the local binary package cache.
rm -rf "${PKGDIR}/packages"

There is no such file on my machine.  There are a bunch of directories of tbz2's
and a file Packages (note capitalization) that contains metadata.  

Furthermore, it seems like --getbinpkgonly obviates the need for this, though my
understanding of this is incomplete. 

I'm tentatively removing the whole thing because I don't think it did anything,
but I would like a second opinion.

 
-mount -o remount,rw / || change_portage_root
+  def ParseLsbRelease(self, lsb_release_lines):
+    """Convert a list of KEY=VALUE lines to a dictionary."""
+    partitioned_lines = [line.rstrip().partition('=')
+                         for line in lsb_release_lines]
+    return dict([(fields[0], fields[2]) for fields in partitioned_lines])
 
-# Re-mount /tmp as exec.
-mount -o remount,exec /tmp
-trap "mount -o remount,noexec /tmp" EXIT
+  def SetupPortageEnvironment(self, environ):
+    environ.update({
+        'PORTDIR': '/usr/local/portage',
+        'PKGDIR': '/usr/local/portage',
+        'DISTDIR': '/usr/local/portage/distfiles',
+        'PORTAGE_BINHOST': '%s/static/pkgroot/%s/packages' % (
+            self.devkit_url, self.board_name),
+        'PORTAGE_TMPDIR': '/tmp',
+        'CONFIG_PROTECT': '-*',
+        'FEATURES': '-sandbox',
+        'ACCEPT_KEYWORDS': 'arm x86 ~arm ~x86',
+        'ROOT': '/',
+        })
 
-# Delete the local binary package cache.
-rm -rf "${PKGDIR}/packages"
+  def GeneratePackageRequest(self, package_name):
+    post_data = {'board': self.board_name,
+                 'pkg': package_name,
+                 'use': FLAGS.use,
+                 'accept_stable': FLAGS.accept_stable,
+                }
+    post_data = dict([(key, value) for (key, value) in post_data.iteritems()
+                      if value])
+    return urllib.urlencode(post_data)
 
-if [ ${BUILD} == 1 ]; then
-  echo "Building ${PACKAGE_NAME}"
-  ESCAPED_PACKAGE=$(python -c \
-    "import urllib; print urllib.quote('''${PACKAGE_NAME}''')")
-  ESCAPED_BOARD=$(python -c \
-    "import urllib; print urllib.quote('''${BOARD_NAME}''')")
+  def RequestPackageBuild(self, package_name):
+    try:
+      result = urllib2.urlopen(self.devkit_url + '/build',
+                               data=self. GeneratePackageRequest(package_name))
+      print result.read()

[sosa, 2011/01/14 23:03:54]

result.close()?

[rochberg, 2011/01/15 06:16:48]

Done.

 
-  wget $DEVKIT_URL/build \
-    --post-data="pkg=${ESCAPED_PACKAGE}&board=${ESCAPED_BOARD}"
-fi
+    except urllib2.HTTPError, e:
+      # The exception includes the content, which is the error mesage
+      print e.read()
+      sys.exit(1)

[sosa, 2011/01/14 23:03:54]

sys.exit(e.read())?

[rochberg, 2011/01/15 06:16:48]

Done.

 
-echo "Emerging ${PACKAGE_NAME}"
-emerge --getbinpkgonly --usepkgonly "$@"
+
+def main():
+  global FLAGS
+  (FLAGS, remaining_arguments) = PARSER.parse_args()
+  if len(remaining_arguments) != 1:

[sosa, 2011/01/14 23:03:54]

So this removes the ability to to do gmerge -n -h?

[rochberg, 2011/01/15 06:16:48]

gmerge --help works, but gmerge -n -h as help for emerge does not work.   If
there's a use case where we want to pass nontrivial extra arguments to emerge,
I'm happy to change, but it seems like those seeking help for emerge can run
emerge -h.

+    sys.exit('Usage: gmerge package_name')

[sosa, 2011/01/14 23:03:54]

It makes it a little easier to read to have a line after conditional blocks.

[rochberg, 2011/01/15 06:16:48]

Done.

+  package_name = remaining_arguments[0]
+
+  try:
+    subprocess.check_call(['mount', '-o', 'remount,exec', '/tmp'])
+    merger = GMerger(open('/etc/lsb-release').readlines())

[sosa, 2011/01/14 23:03:54]

Why not just pass the path?

[rochberg, 2011/01/15 06:16:48]

Easier to test.  

+    merger.SetupPortageEnvironment(os.environ)
+    merger.RemountOrChangeRoot(os.environ)
+    merger.CleanPortageCache()
+    if FLAGS.call_devserver:
+      merger.RequestPackageBuild(package_name)
+    else:
+      print 'Not requesting fresh build on server---installing whatever we find'
+
+    print 'Emerging ', package_name
+    subprocess.call(['emerge', '--getbinpkgonly', '--usepkgonly', package_name])

[sosa, 2011/01/14 23:03:54]

check_call?

[rochberg, 2011/01/15 06:16:48]

Done.

+  finally:
+    subprocess.call(['mount', '-o', 'remount,noexec', '/tmp'])
+
+
+if __name__ == '__main__':
+  main()