Merge lp:~jelmer/launchpad/bug499115 into lp:launchpad/db-devel

[Jelmer Vernooij]

[Jelmer Vernooij (jelmer)]

This trivial change prevents the buildd master from falling over if a container directory was created but no log file uploaded. (bug 499115)

[Henning Eggers (henninge)]

Thank you for adding this simple fix. I still think this corner case should be tested in a unit test. To make testing possible, I'd suggest that you split out the lines under "# Retrieve log file content" into its own method "get_logfile_content" and add tests for it to test/test_bulidbase.py.

review: Needs Fixing (code)

[Jelmer Vernooij (jelmer)]

Superseded by https://code.edge.launchpad.net/~jelmer/launchpad/bug499115/+merge/20465

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-02-16 02:27:24 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-03-03 02:43:34 +0000
@@ -3,6 +3,8 @@
 
 # pylint: disable-msg=E0211,E0213
 
+from __future__ import with_statement
+
 """Common build base classes."""
 
 __metaclass__ = type
@@ -32,6 +34,9 @@
 from lp.soyuz.model.buildqueue import BuildQueue
 
 
+UPLOAD_LOG_FILENAME = 'uploader.log'
+
+
 class BuildBase:
     """A mixin class providing functionality for farm jobs that build a
     package.
@@ -96,6 +101,7 @@
             return None
         return self._getProxiedFileURL(self.buildlog)
 
+<<<<<<< TREE
     @property
     def upload_log_url(self):
         """See `IBuildBase`."""
@@ -103,6 +109,27 @@
             return None
         return self._getProxiedFileURL(self.upload_log)
 
+=======
+    def getUploadLogContent(self, root, leaf):
+        """Retrieve the upload log contents.
+
+        :param root: Root directory for the uploads
+        :param leaf: Leaf for this particular upload
+        :return: Contents of log file or message saying no log file was found.
+        """
+        # Retrieve log file content.
+        possible_locations = (
+            'failed', 'failed-to-move', 'rejected', 'accepted')
+        for location_dir in possible_locations:
+            log_filepath = os.path.join(root, location_dir, leaf,
+                UPLOAD_LOG_FILENAME)
+            if os.path.exists(log_filepath):
+                with open(log_filepath, 'r') as uploader_log_file:
+                    return uploader_log_file.read()
+        else:
+            return 'Could not find upload log file'
+
+>>>>>>> MERGE-SOURCE
     def handleStatus(self, status, librarian, slave_status):
         """See `IBuildBase`."""
         logger = logging.getLogger()
@@ -161,7 +188,7 @@
             out_file = open(out_file_name, "wb")
             copy_and_close(slave_file, out_file)
 
-        uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
+        uploader_logfilename = os.path.join(upload_dir, UPLOAD_LOG_FILENAME)
         uploader_command = self.getUploaderCommand(
             upload_leaf, uploader_logfilename)
         logger.debug("Saving uploader log at '%s'" % uploader_logfilename)
@@ -239,23 +266,8 @@
             not self.verifySuccessfulUpload()):
             logger.debug("Build %s upload failed." % self.id)
             self.buildstate = BuildStatus.FAILEDTOUPLOAD
-            # Retrieve log file content.
-            possible_locations = (
-                'failed', 'failed-to-move', 'rejected', 'accepted')
-            for location_dir in possible_locations:
-                upload_final_location = os.path.join(
-                    root, location_dir, upload_leaf)
-                if os.path.exists(upload_final_location):
-                    log_filepath = os.path.join(
-                        upload_final_location, 'uploader.log')
-                    uploader_log_file = open(log_filepath)
-                    try:
-                        uploader_log_content = uploader_log_file.read()
-                    finally:
-                        uploader_log_file.close()
-                    break
-            else:
-                uploader_log_content = 'Could not find upload log file'
+            uploader_log_content = self.getUploadLogContent(root,
+                upload_leaf)
             # Store the upload_log_contents in librarian so it can be
             # accessed by anyone with permission to see the build.
             self.storeUploadLog(uploader_log_content)

=== modified file 'lib/lp/buildmaster/tests/test_buildbase.py'
--- lib/lp/buildmaster/tests/test_buildbase.py	2010-02-03 16:21:57 +0000
+++ lib/lp/buildmaster/tests/test_buildbase.py	2010-03-03 02:43:34 +0000
@@ -1,6 +1,8 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 """Tests for `IBuildBase`."""
 
 __metaclass__ = type
@@ -45,6 +47,33 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_getUploadLogContent_nolog(self):
+        """If there is no log file there, a string explaining that is returned.
+        """
+        self.useTempDir()
+        build_base = BuildBase()
+        self.assertEquals('Could not find upload log file', 
+            build_base.getUploadLogContent(os.getcwd(), "myleaf"))
+
+    def test_getUploadLogContent_only_dir(self):
+        """If there is a directory but no log file, expect the error string,
+        not an exception."""
+        self.useTempDir()
+        os.makedirs("accepted/myleaf")
+        build_base = BuildBase()
+        self.assertEquals('Could not find upload log file', 
+            build_base.getUploadLogContent(os.getcwd(), "myleaf"))
+
+    def test_getUploadLogContent_readsfile(self):
+        """If there is a log file, return its contents."""
+        self.useTempDir()
+        os.makedirs("accepted/myleaf")
+        with open('accepted/myleaf/uploader.log', 'w') as f:
+            f.write('foo')
+        build_base = BuildBase()
+        self.assertEquals('foo',
+            build_base.getUploadLogContent(os.getcwd(), "myleaf"))
+
     def test_getUploaderCommand(self):
         build_base = BuildBase()
         upload_leaf = self.factory.getUniqueString('upload-leaf')