launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:buildmaster-refactor-getFile into launchpad:master.

Commit message:
Refactor BuilderSlave.getFile

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/385250

The inlineCallbacks style is easier to read and maintain here.

Accepting file objects made it more difficult to extend getFile to defer work to subprocesses, and this facility was only lightly used and easy to replace, so I've abolished it.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:buildmaster-refactor-getFile into launchpad:master.

diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index f81983c..7618da5 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -11,7 +11,9 @@ __all__ = [
 from collections import namedtuple
 import logging
 import os.path
+import sys
 import tempfile
+import traceback
 
 from six.moves.urllib.parse import urlparse
 import transaction
@@ -236,44 +238,38 @@ class BuilderSlave(object):
         """Get the URL for a file on the builder with a given SHA-1."""
         return urlappend(self._file_cache_url, sha1).encode('utf8')
 
-    def getFile(self, sha_sum, file_to_write, logger=None):
+    @defer.inlineCallbacks
+    def getFile(self, sha_sum, path_to_write, logger=None):
         """Fetch a file from the builder.
 
         :param sha_sum: The sha of the file (which is also its name on the
             builder)
-        :param file_to_write: A file name or file-like object to write
-            the file to
+        :param path_to_write: A file name to write the file to
         :param logger: An optional logger.
         :return: A Deferred that calls back when the download is done, or
             errback with the error string.
         """
         file_url = self.getURL(sha_sum)
-        d = Agent(self.reactor, pool=self.pool).request("GET", file_url)
-
-        def got_response(response):
+        try:
+            response = yield Agent(self.reactor, pool=self.pool).request(
+                "GET", file_url)
             finished = defer.Deferred()
-            response.deliverBody(FileWritingProtocol(finished, file_to_write))
-            return finished
-
-        def log_success(result):
-            logger.info("Grabbed %s" % file_url)
-            return result
-
-        def log_failure(failure):
-            logger.info("Failed to grab %s: %s\n%s" % (
-                file_url, failure.getErrorMessage(), failure.getTraceback()))
-            return failure
-
-        d.addCallback(got_response)
-        if logger is not None:
-            d.addCallbacks(log_success, log_failure)
-        return d
+            response.deliverBody(FileWritingProtocol(finished, path_to_write))
+            yield finished
+            if logger is not None:
+                logger.info("Grabbed %s" % file_url)
+        except Exception as e:
+            if logger is not None:
+                logger.info("Failed to grab %s: %s\n%s" % (
+                    file_url, e,
+                    " ".join(traceback.format_exception(*sys.exc_info()))))
+            raise
 
     def getFiles(self, files, logger=None):
         """Fetch many files from the builder.
 
         :param files: A sequence of pairs of the builder file name to
-            retrieve and the file name or file object to write the file to.
+            retrieve and the file name to write the file to.
         :param logger: An optional logger.
 
         :return: A DeferredList that calls back when the download is done.
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index 48a666c..20d7dd3 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -175,9 +175,9 @@ class BuildFarmJobBehaviourBase:
         :return: A Deferred that calls back with a librarian file alias.
         """
         out_file_fd, out_file_name = tempfile.mkstemp(suffix=".buildlog")
-        out_file = os.fdopen(out_file_fd, "r+")
+        os.close(out_file_fd)
 
-        def got_file(ignored, filename, out_file, out_file_name):
+        def got_file(ignored, filename, out_file_name):
             try:
                 # If the requested file is the 'buildlog' compress it
                 # using gzip before storing in Librarian.
@@ -189,8 +189,8 @@ class BuildFarmJobBehaviourBase:
                     copy_and_close(out_file, gz_file)
                     os.remove(out_file_name.replace('.gz', ''))
 
-                # Reopen the file, seek to its end position, count and seek
-                # to beginning, ready for adding to the Librarian.
+                # Open the file, seek to its end position, count and seek to
+                # beginning, ready for adding to the Librarian.
                 out_file = open(out_file_name)
                 out_file.seek(0, 2)
                 bytes_written = out_file.tell()
@@ -201,14 +201,13 @@ class BuildFarmJobBehaviourBase:
                     contentType=filenameToContentType(filename),
                     restricted=private)
             finally:
-                # Remove the temporary file.  getFile() closes the file
-                # object.
+                # Remove the temporary file.
                 os.remove(out_file_name)
 
             return library_file.id
 
-        d = self._slave.getFile(file_sha1, out_file)
-        d.addCallback(got_file, filename, out_file, out_file_name)
+        d = self._slave.getFile(file_sha1, out_file_name)
+        d.addCallback(got_file, filename, out_file_name)
         return d
 
     def getLogFileName(self):
diff --git a/lib/lp/buildmaster/tests/test_interactor.py b/lib/lp/buildmaster/tests/test_interactor.py
index b09b4dd..76b958c 100644
--- a/lib/lp/buildmaster/tests/test_interactor.py
+++ b/lib/lp/buildmaster/tests/test_interactor.py
@@ -875,22 +875,6 @@ class TestSlaveWithLibrarian(TestCaseWithFactory):
         return defer.DeferredList(dl).addCallback(finished_uploading)
 
     @defer.inlineCallbacks
-    def test_getFiles_with_file_objects(self):
-        # getFiles works with file-like objects as well as file names.
-        self.slave_helper.getServerSlave()
-        slave = self.slave_helper.getClientSlave()
-        temp_fd, temp_name = tempfile.mkstemp()
-        self.addCleanup(os.remove, temp_name)
-        lf = self.factory.makeLibraryFileAlias(
-            'content.txt', content='content')
-        self.layer.txn.commit()
-        yield slave.ensurepresent(lf.content.sha1, lf.http_url, "", "")
-        yield slave.getFiles([(lf.content.sha1, os.fdopen(temp_fd, "w"))])
-        with open(temp_name) as f:
-            self.assertEqual('content', f.read())
-        yield slave.pool.closeCachedConnections()
-
-    @defer.inlineCallbacks
     def test_getFiles_with_empty_file(self):
         # getFiles works with zero-length files.
         tachandler = self.slave_helper.getServerSlave()
diff --git a/lib/lp/translations/model/translationtemplatesbuildbehaviour.py b/lib/lp/translations/model/translationtemplatesbuildbehaviour.py
index 8a51315..861965e 100644
--- a/lib/lp/translations/model/translationtemplatesbuildbehaviour.py
+++ b/lib/lp/translations/model/translationtemplatesbuildbehaviour.py
@@ -86,9 +86,8 @@ class TranslationTemplatesBuildBehaviour(BuildFarmJobBehaviourBase):
             return defer.succeed(None)
 
         fd, fname = tempfile.mkstemp()
-        tarball_file = os.fdopen(fd, 'wb')
-        d = self._slave.getFile(slave_filename, tarball_file)
-        # getFile will close the file object.
+        os.close(fd)
+        d = self._slave.getFile(slave_filename, fname)
         return d.addCallback(lambda ignored: fname)
 
     def _uploadTarball(self, branch, tarball, logger):
diff --git a/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py b/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
index 525ff4a..a60e2ba 100644
--- a/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
+++ b/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
@@ -245,11 +245,12 @@ class TestTranslationTemplatesBuildBehaviour(
         queue_item.markAsBuilding(self.factory.makeBuilder())
         slave = behaviour._slave
 
-        def fake_getFile(sum, file):
+        def fake_getFile(sum, path):
             dummy_tar = os.path.join(
                 os.path.dirname(__file__), 'dummy_templates.tar.gz')
-            tar_file = open(dummy_tar)
-            copy_and_close(tar_file, file)
+            tar_file = open(dummy_tar, 'rb')
+            with open(path, 'wb') as f:
+                copy_and_close(tar_file, f)
             return defer.succeed(None)
 
         slave.getFile = fake_getFile