Merge lp:~henninge/launchpad/bug-581746-serialization-error into lp:launchpad

[Henning Eggers]

Stop the builder from tripping over None not being serializable.

Tiny little fix that adds some real extra info to BuilderStatus.UNKNOWNBUILDER instead of None.

[Henning Eggers (henninge)]

Never mind the changelog entry. The next branch replaces that anyway ...

[Данило Шеган (danilo)]

How about adding allowNone to XMLRPC constructor as well, so we don't get errors when we possibly hit more bugs and try to serialize None elsewhere?

=== modified file 'lib/canonical/buildd/slave.py'
--- lib/canonical/buildd/slave.py 2010-04-07 18:57:10 +0000
+++ lib/canonical/buildd/slave.py 2010-05-19 17:17:44 +0000
@@ -546,7 +546,7 @@
     """XMLRPC build daemon slave management interface"""

     def __init__(self, config):
- xmlrpc.XMLRPC.__init__(self)
+ xmlrpc.XMLRPC.__init__(self, allowNone=True)
         # The V1.0 new-style protocol introduces string-style protocol
         # versions of the form 'MAJOR.MINOR', the protocol is '1.0' for now
         # implying the presence of /filecache/ /filecache/buildlog and

It would also be nice to test this.

[Данило Шеган (danilo)]

Note that buildd-manager xmlrpc implementation already passes allow_none=True (it uses a different library: xmlrpclib) so it will be able to handle it.

[Данило Шеган (danilo)]

Looks good, thanks.

review: Approve

=== modified file 'lib/canonical/buildd/debian/changelog'
--- lib/canonical/buildd/debian/changelog	2010-04-01 14:55:37 +0000
+++ lib/canonical/buildd/debian/changelog	2010-05-20 14:32:30 +0000
@@ -1,3 +1,9 @@
+launchpad-buildd (60.1) lucid-cat; urgency=low
+
+  * Improve output of build xmplrpc call.
+
+ -- Henning Eggers <[e-mail]>  Mon, 17 May 2010 15:58:48 +0200
+
 launchpad-buildd (60) lucid-cat; urgency=low
 
   * Depends: lsb-release, which is ubuntu-minimal, but not essential.

=== modified file 'lib/canonical/buildd/slave.py'
--- lib/canonical/buildd/slave.py	2010-04-07 18:57:10 +0000
+++ lib/canonical/buildd/slave.py	2010-05-20 14:32:30 +0000
@@ -546,7 +546,7 @@
     """XMLRPC build daemon slave management interface"""
 
     def __init__(self, config):
-        xmlrpc.XMLRPC.__init__(self)
+        xmlrpc.XMLRPC.__init__(self, allowNone=True)
         # The V1.0 new-style protocol introduces string-style protocol
         # versions of the form 'MAJOR.MINOR', the protocol is '1.0' for now
         # implying the presence of /filecache/ /filecache/buildlog and
@@ -651,7 +651,8 @@
         """
         # check requested builder
         if not builder in self._builders:
-            return (BuilderStatus.UNKNOWNBUILDER, None)
+            extra_info = "%s not in %r" % (builder, self._builders.keys())
+            return (BuilderStatus.UNKNOWNBUILDER, extra_info)
         # check requested chroot availability
         chroot_present, info = self.slave.ensurePresent(chrootsum)
         if not chroot_present:

=== modified file 'lib/canonical/buildd/tests/test_buildd_slave.py'
--- lib/canonical/buildd/tests/test_buildd_slave.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/buildd/tests/test_buildd_slave.py	2010-05-20 14:32:30 +0000
@@ -20,8 +20,10 @@
 import shutil
 import urllib2
 import unittest
+import xmlrpclib
 
-from canonical.buildd.tests.harness import BuilddTestCase
+from canonical.buildd.tests.harness import (
+    BuilddSlaveTestSetup, BuilddTestCase)
 
 
 def read_file(path):
@@ -170,5 +172,30 @@
         self.assertEqual(len(log_tail), 0)
 
 
+class XMLRPCBuildDSlaveTests(unittest.TestCase):
+
+    def setUp(self):
+        super(XMLRPCBuildDSlaveTests, self).setUp()
+        BuilddSlaveTestSetup().setUp()
+        self.server = xmlrpclib.Server('http://localhost:8221/rpc/')
+
+    def tearDown(self):
+        BuilddSlaveTestSetup().tearDown()
+        super(XMLRPCBuildDSlaveTests, self).tearDown()
+
+    def test_build_unknown_builder(self):
+        # If a bogus builder name is passed into build, it returns an
+        # appropriate error message and not just 'None'.
+        buildername = 'nonexistentbuilder'
+        status, info = self.server.build('foo', buildername, 'sha1', {}, {})
+
+        self.assertEqual('BuilderStatus.UNKNOWNBUILDER', status)
+        self.assertTrue(
+            info is not None, "UNKNOWNBUILDER returns 'None' info.")
+        self.assertTrue(
+            info.startswith("%s not in [" % buildername),
+            'UNKNOWNBUILDER info is "%s"' % info)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)