launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/better-status into lp:launchpad-buildd.

Commit message:
Add a status_dict XML-RPC method for better extensibility, including reporting the python-lpbuildd version.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #680514 in launchpad-buildd: "No way to discover version of launchpad-buildd installed on builders"
  https://bugs.launchpad.net/launchpad-buildd/+bug/680514

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/better-status/+merge/189675

Report the python-lpbuildd version over XML-RPC, so that Launchpad can be extended later to retrieve and report it.

Rather than mangling the awkward XML-RPC status method further, I added a new status_dict method.  This is very duplicative right now, but it is the first step of a plan which will eventually remove the duplication: roll out status_dict, change the master to use it, make status be a synonym for status_dict, change the master to use status again but with dict semantics, and finally remove the status_dict name.  (We can skip the aliasing step if it's felt to be unnecessary, but I'd prefer to end up on the status name again, and there's no particular rush for all of this.)
-- 
https://code.launchpad.net/~cjwatson/launchpad-buildd/better-status/+merge/189675
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/better-status into lp:launchpad-buildd.

=== modified file 'debian/changelog'
--- debian/changelog	2013-10-07 11:49:40 +0000
+++ debian/changelog	2013-10-07 17:51:53 +0000
@@ -7,6 +7,8 @@
   [ Colin Watson ]
   * Fail the builder immediately if $HOME/.sbuildrc is corrupt
     (LP: #1235287).
+  * Add a status_dict XML-RPC method for better extensibility, including
+    reporting the python-lpbuildd version (LP: #680514).
 
  -- William Grant <william.grant@xxxxxxxxxxxxx>  Fri, 04 Oct 2013 11:03:51 +1000
 

=== modified file 'debian/control'
--- debian/control	2013-07-29 23:48:58 +0000
+++ debian/control	2013-10-07 17:51:53 +0000
@@ -23,7 +23,7 @@
 Package: python-lpbuildd
 Section: python
 Architecture: all
-Depends: python, python-twisted-core, python-twisted-web, ${misc:Depends}
+Depends: python, python-twisted-core, python-twisted-web, python-apt, ${misc:Depends}
 Conflicts: launchpad-buildd (<< 88)
 Description: Python libraries for a Launchpad buildd slave
  This contains the Python libraries that control the Launchpad buildd slave.

=== modified file 'lpbuildd/slave.py'
--- lpbuildd/slave.py	2013-09-24 09:41:10 +0000
+++ lpbuildd/slave.py	2013-10-07 17:51:53 +0000
@@ -13,13 +13,20 @@
 import os
 import re
 import urllib2
+import warnings
 import xmlrpclib
 
+import apt
 from twisted.internet import protocol
 from twisted.internet import reactor as default_reactor
 from twisted.internet import process
 from twisted.web import xmlrpc
 
+# XXX cjwatson 2013-10-07: Remove when all builders are on Ubuntu 10.04 LTS
+# or newer.
+warnings.filterwarnings(
+    "ignore", "apt API not stable yet", category=FutureWarning)
+
 devnull = open("/dev/null", "r")
 
 
@@ -623,6 +630,16 @@
         self.protocolversion = '1.0'
         self.slave = BuildDSlave(config)
         self._builders = {}
+        cache = apt.Cache()
+        try:
+            try:
+                self._version = cache["python-lpbuildd"].installed.version
+            except AttributeError:
+                # XXX cjwatson 2013-10-07: Remove when all builders are on
+                # Ubuntu 10.04 LTS or newer.
+                self._version = cache["python-lpbuildd"].installedVersion
+        except KeyError:
+            self._version = None
         print "Initialized"
 
     def registerBuilder(self, builderclass, buildertag):
@@ -643,6 +660,8 @@
         Depending on the builder status we return differing amounts of
         data. We do however always return the builder status as the first
         value.
+
+        This method is deprecated in favour of `xmlrpc_status_dict`.
         """
         status = self.slave.builderstatus
         statusname = status.split('.')[-1]
@@ -651,6 +670,23 @@
             raise ValueError("Unknown status '%s'" % status)
         return (status, ) + func()
 
+    def xmlrpc_status_dict(self):
+        """Return the status of the build daemon, as a dictionary.
+
+        Depending on the builder status we return differing amounts of data,
+        but this always includes the builder status itself.
+        """
+        status = self.slave.builderstatus
+        statusname = status.split('.')[-1]
+        func = getattr(self, "status_dict_" + statusname, None)
+        if func is None:
+            raise ValueError("Unknown status '%s'" % status)
+        ret = {"status": status}
+        if self._version is not None:
+            ret["version"] = self._version
+        ret.update(func())
+        return ret
+
     def status_IDLE(self):
         """Handler for xmlrpc_status IDLE.
 
@@ -660,14 +696,26 @@
         # keep the result code sane
         return ('', )
 
+    def status_dict_IDLE(self):
+        """Handler for xmlrpc_status_dict IDLE."""
+        return {}
+
     def status_BUILDING(self):
         """Handler for xmlrpc_status BUILDING.
 
-        Returns the build id and up to one kilobyte of log tail
+        Returns the build id and up to one kilobyte of log tail.
         """
         tail = self.slave.getLogTail()
         return (self.buildid, xmlrpclib.Binary(tail))
 
+    def status_dict_BUILDING(self):
+        """Handler for xmlrpc_status_dict BUILDING.
+
+        Returns the build id and up to one kilobyte of log tail.
+        """
+        tail = self.slave.getLogTail()
+        return {"buildid": self.buildid, "logtail": xmlrpclib.Binary(tail)}
+
     def status_WAITING(self):
         """Handler for xmlrpc_status WAITING.
 
@@ -681,15 +729,38 @@
                     self.slave.waitingfiles, self.slave.builddependencies)
         return (self.slave.buildstatus, self.buildid)
 
+    def status_dict_WAITING(self):
+        """Handler for xmlrpc_status_dict WAITING.
+
+        Returns the build id and the set of files waiting to be returned
+        unless the builder failed in which case we return the buildstatus
+        and the build id but no file set.
+        """
+        ret = {"buildstatus": self.slave.buildstatus, "buildid": self.buildid}
+        if self.slave.buildstatus in (BuildStatus.OK, BuildStatus.PACKAGEFAIL,
+                                      BuildStatus.DEPFAIL):
+            ret["waitingfiles"] = self.slave.waitingfiles
+            ret["builddependencies"] = self.slave.builddependencies
+        return ret
+
     def status_ABORTING(self):
         """Handler for xmlrpc_status ABORTING.
 
-        This state means the builder performing the ABORT command and is
-        not able to do anything else than answer its status, returns the
+        This state means the builder is performing the ABORT command and is
+        not able to do anything else than answer its status, so returns the
         build id only.
         """
         return (self.buildid, )
 
+    def status_dict_ABORTING(self):
+        """Handler for xmlrpc_status_dict ABORTING.
+
+        This state means the builder is performing the ABORT command and is
+        not able to do anything else than answer its status, so returns the
+        build id only.
+        """
+        return {"buildid": self.buildid}
+
     def xmlrpc_ensurepresent(self, sha1sum, url, username, password):
         """Attempt to ensure the given file is present."""
         return self.slave.ensurePresent(sha1sum, url, username, password)

=== modified file 'lpbuildd/tests/harness.py'
--- lpbuildd/tests/harness.py	2013-07-25 17:26:10 +0000
+++ lpbuildd/tests/harness.py	2013-10-07 17:51:53 +0000
@@ -95,6 +95,9 @@
     >>> s.status()
     ['BuilderStatus.IDLE', '']
 
+    >>> s.status_dict()["status"]
+    'BuilderStatus.IDLE'
+
     >>> fixture.tearDown()
     """
     def setUpRoot(self):