launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/duplicate-build-files into lp:launchpad.

Commit message:
Properly fetch builds that return multiple files with identical content.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/duplicate-build-files/+merge/219104

== Summary ==

Some livefs build types return multiple files with identical content, such as ubuntu-touch builds due to this code in livecd-rootfs:

                        # drop this following line once cdimage can handle -boot-*.img
                        cp "${PREFIX}.boot-i386+${subarch}.img" "${PREFIX}.bootimg-${subarch}"

After fixing launchpad-buildd to not crash (https://code.launchpad.net/~cjwatson/launchpad-buildd/clean-duplicate-files/+merge/219102), the buildmaster then only collects one of each set of duplicates.

== Proposed fix ==

Make BuilderSlave.getFiles take a list of (builder_file, local_file) pairs rather than a dictionary mapping builder_file to local_file.

== Tests ==

bin/test -vvct TestSlaveWithLibrarian
-- 
https://code.launchpad.net/~cjwatson/launchpad/duplicate-build-files/+merge/219104
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/duplicate-build-files into lp:launchpad.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2014-02-03 14:43:08 +0000
+++ lib/lp/buildmaster/interactor.py	2014-05-10 18:59:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -138,18 +138,17 @@
         # change it.
         return downloadPage(file_url, file_to_write, followRedirect=0)
 
-    def getFiles(self, filemap):
+    def getFiles(self, files):
         """Fetch many files from the builder.
 
-        :param filemap: A Dictionary containing key values of the builder
-            file name to retrieve, which maps to a value containing the
-            file name or file object to write the file to.
+        :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.
 
         :return: A DeferredList that calls back when the download is done.
         """
         dl = defer.gatherResults([
-            self.getFile(builder_file, filemap[builder_file])
-            for builder_file in filemap])
+            self.getFile(builder_file, local_file)
+            for builder_file, local_file in files])
         return dl
 
     def resume(self, clock=None):

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2014-04-25 11:32:12 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2014-05-10 18:59:30 +0000
@@ -203,7 +203,7 @@
         os.makedirs(upload_path)
 
         successful_copy_from_slave = True
-        filenames_to_download = {}
+        filenames_to_download = []
         for filename in filemap:
             logger.info("Grabbing file: %s" % filename)
             out_file_name = os.path.join(upload_path, filename)
@@ -216,7 +216,7 @@
                     "A slave tried to upload the file '%s' "
                     "for the build %d." % (filename, build.id))
                 break
-            filenames_to_download[filemap[filename]] = out_file_name
+            filenames_to_download.append((filemap[filename], out_file_name))
         yield self._slave.getFiles(filenames_to_download)
 
         status = (

=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2014-02-03 14:43:08 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2014-05-10 18:59:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 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."""
@@ -125,10 +125,10 @@
         return self.sendFileToSlave(
             libraryfilealias.content.sha1, libraryfilealias.http_url)
 
-    def getFiles(self, filemap):
+    def getFiles(self, files):
         dl = defer.gatherResults([
-            self.getFile(builder_file, filemap[builder_file])
-            for builder_file in filemap])
+            self.getFile(builder_file, local_file)
+            for builder_file, local_file in files])
         return dl
 
 

=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py	2014-02-03 14:43:08 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py	2014-05-10 18:59:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test BuilderInteractor features."""
@@ -693,8 +693,7 @@
         self.layer.txn.commit()
         self.slave_helper.getServerSlave()
         slave = self.slave_helper.getClientSlave()
-        d = slave.ensurepresent(
-            lf.content.sha1, lf.http_url, "", "")
+        d = slave.ensurepresent(lf.content.sha1, lf.http_url, "", "")
         d.addCallback(self.assertEqual, [True, 'Download'])
         return d
 
@@ -710,8 +709,7 @@
             self.slave_helper.BASE_URL, lf.content.sha1)
         self.slave_helper.getServerSlave()
         slave = self.slave_helper.getClientSlave()
-        d = slave.ensurepresent(
-            lf.content.sha1, lf.http_url, "", "")
+        d = slave.ensurepresent(lf.content.sha1, lf.http_url, "", "")
 
         def check_file(ignored):
             d = getPage(expected_url.encode('utf8'))
@@ -727,20 +725,18 @@
         contents = ["content1", "content2", "content3"]
         self.slave_helper.getServerSlave()
         slave = self.slave_helper.getClientSlave()
-        filemap = {}
+        files = []
         content_map = {}
 
         def got_files(ignored):
             # Called back when getFiles finishes.  Make sure all the
             # content is as expected.
-            for sha1 in filemap:
-                local_file = filemap[sha1]
-                file = open(local_file)
-                self.assertEqual(content_map[sha1], file.read())
-                file.close()
+            for sha1, local_file in files:
+                with open(local_file) as f:
+                    self.assertEqual(content_map[sha1], f.read())
 
         def finished_uploading(ignored):
-            d = slave.getFiles(filemap)
+            d = slave.getFiles(files)
             return d.addCallback(got_files)
 
         # Set up some files on the builder and store details in
@@ -750,8 +746,12 @@
             filename = content + '.txt'
             lf = self.factory.makeLibraryFileAlias(filename, content=content)
             content_map[lf.content.sha1] = content
-            fd, filemap[lf.content.sha1] = tempfile.mkstemp()
-            self.addCleanup(os.remove, filemap[lf.content.sha1])
+            files.append((lf.content.sha1, tempfile.mkstemp()[1]))
+            self.addCleanup(os.remove, files[-1][1])
+            # Add the same file contents again with a different name, to
+            # ensure that we can tolerate duplication.
+            files.append((lf.content.sha1, tempfile.mkstemp()[1]))
+            self.addCleanup(os.remove, files[-1][1])
             self.layer.txn.commit()
             d = slave.ensurepresent(lf.content.sha1, lf.http_url, "", "")
             dl.append(d)