launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/empty-build-artifacts into lp:launchpad.

Commit message:
Handle empty artifacts in livefs builds.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1841878 in Launchpad itself: "livefs builds fail when they produce an empty artifact"
  https://bugs.launchpad.net/launchpad/+bug/1841878

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/empty-build-artifacts/+merge/371975

I also considered changing launchpad-buildd to avoid sending empty artifacts, but it seemed a bit arbitrary, and I think it makes sense for Launchpad to be more robust against this anyway.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/empty-build-artifacts into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/livefsupload.py'
--- lib/lp/archiveuploader/livefsupload.py	2018-05-06 08:52:34 +0000
+++ lib/lp/archiveuploader/livefsupload.py	2019-08-29 04:14:54 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Process a live filesystem upload."""
@@ -47,7 +47,7 @@
                     livefs_file, os.stat(livefs_path).st_size,
                     open(livefs_path, "rb"),
                     filenameToContentType(livefs_path),
-                    restricted=build.is_private)
+                    restricted=build.is_private, allow_zero_length=True)
                 build.addFile(libraryfile)
 
         # The master verifies the status to confirm successful upload.

=== modified file 'lib/lp/archiveuploader/tests/test_livefsupload.py'
--- lib/lp/archiveuploader/tests/test_livefsupload.py	2019-05-24 11:10:38 +0000
+++ lib/lp/archiveuploader/tests/test_livefsupload.py	2019-08-29 04:14:54 +0000
@@ -67,3 +67,21 @@
             "LiveFS upload failed\nGot: %s" % self.log.getLogBuffer())
         self.assertEqual(BuildStatus.FULLYBUILT, self.build.status)
         self.assertTrue(self.build.verifySuccessfulUpload())
+
+    def test_empty_file(self):
+        # The upload processor can cope with empty build artifacts.  (LiveFS
+        # is quite a general method and can include a variety of artifacts:
+        # in particular it often includes an additional log file which can
+        # be empty.)
+        self.assertFalse(self.build.verifySuccessfulUpload())
+        upload_dir = os.path.join(
+            self.incoming_folder, "test", str(self.build.id), "ubuntu")
+        write_file(os.path.join(upload_dir, "livecd.magic-proxy.log"), b"")
+        handler = UploadHandler.forProcessor(
+            self.uploadprocessor, self.incoming_folder, "test", self.build)
+        result = handler.processLiveFS(self.log)
+        self.assertEqual(
+            UploadStatusEnum.ACCEPTED, result,
+            "LiveFS upload failed\nGot: %s" % self.log.getLogBuffer())
+        self.assertEqual(BuildStatus.FULLYBUILT, self.build.status)
+        self.assertTrue(self.build.verifySuccessfulUpload())

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2019-06-19 09:50:07 +0000
+++ lib/lp/buildmaster/interactor.py	2019-08-29 04:14:54 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -62,16 +62,14 @@
         self.finished = finished
         if isinstance(file_to_write, (bytes, unicode)):
             self.filename = file_to_write
-            self.file = None
+            self.file = tempfile.NamedTemporaryFile(
+                mode="wb", prefix=os.path.basename(self.filename) + "_",
+                dir=os.path.dirname(self.filename), delete=False)
         else:
             self.filename = None
             self.file = file_to_write
 
     def dataReceived(self, data):
-        if self.file is None:
-            self.file = tempfile.NamedTemporaryFile(
-                mode="wb", prefix=os.path.basename(self.filename) + "_",
-                dir=os.path.dirname(self.filename), delete=False)
         try:
             self.file.write(data)
         except IOError:

=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2019-07-02 15:43:07 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2019-08-29 04:14:54 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Mock Build objects for tests soyuz buildd-system."""
@@ -316,18 +316,18 @@
             self.base_url, 'vmhost', config.builddmaster.socket_timeout,
             reactor=reactor, proxy=proxy, pool=pool)
 
-    def makeCacheFile(self, tachandler, filename):
+    def makeCacheFile(self, tachandler, filename, contents=b'something'):
         """Make a cache file available on the remote slave.
 
         :param tachandler: The TacTestSetup object used to start the remote
             slave.
         :param filename: The name of the file to create in the file cache
             area.
+        :param contents: Bytes to write to the file.
         """
         path = os.path.join(tachandler.root, 'filecache', filename)
-        fd = open(path, 'w')
-        fd.write('something')
-        fd.close()
+        with open(path, 'wb') as fd:
+            fd.write(contents)
         self.addCleanup(os.unlink, path)
 
     def triggerGoodBuild(self, slave, build_id=None):

=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py	2018-07-01 21:56:44 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py	2019-08-29 04:14:54 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test BuilderInteractor features."""
@@ -10,6 +10,7 @@
     'MockBuilderFactory',
     ]
 
+import hashlib
 import os
 import signal
 import tempfile
@@ -884,3 +885,17 @@
         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()
+        slave = self.slave_helper.getClientSlave()
+        temp_fd, temp_name = tempfile.mkstemp()
+        self.addCleanup(os.remove, temp_name)
+        empty_sha1 = hashlib.sha1(b'').hexdigest()
+        self.slave_helper.makeCacheFile(tachandler, empty_sha1, contents=b'')
+        yield slave.getFiles([(empty_sha1, temp_name)])
+        with open(temp_name) as f:
+            self.assertEqual(b'', f.read())
+        yield slave.pool.closeCachedConnections()