← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~stevenk/launchpad/no-dead-custom-uploads into lp:launchpad

 

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-dead-custom-uploads into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1173128 in Launchpad itself: "Custom uploads get carried over to the next series, even for dead architectures"
  https://bugs.launchpad.net/launchpad/+bug/1173128

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-dead-custom-uploads/+merge/176995

When we copy the custom uploads from a previous series, we copy all of them, even those that are not interesting to us -- for example, if the architecture is no longer supported in the new series. Now, we check the architecture and will not copy custom uploads for architectures that are not in the target series.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-dead-custom-uploads/+merge/176995
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-dead-custom-uploads into lp:launchpad.
=== modified file 'lib/lp/archivepublisher/customupload.py'
--- lib/lp/archivepublisher/customupload.py	2012-11-09 12:42:32 +0000
+++ lib/lp/archivepublisher/customupload.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Infrastructure for handling custom uploads.
@@ -122,6 +122,10 @@
         """
         raise NotImplementedError
 
+    def setComponents(tarfile_path):
+        """Set self.version and self.arch (if applicable)."""
+        raise NotImplementedError
+
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         """Set self.targetdir based on parameters.
 

=== modified file 'lib/lp/archivepublisher/ddtp_tarball.py'
--- lib/lp/archivepublisher/ddtp_tarball.py	2012-11-09 12:42:32 +0000
+++ lib/lp/archivepublisher/ddtp_tarball.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of translated packages descriptions (ddtp) tarballs.
@@ -55,11 +55,14 @@
             raise ValueError("%s is not NAME_COMPONENT_VERSION" % tarfile_base)
         return tuple(bits)
 
+    def setComponents(self, tarfile_path):
+        _, self.component, self.version = self.parsePath(tarfile_path)
+        self.arch = None
+
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
-        _, component, self.version = self.parsePath(tarfile_path)
-        self.arch = None
+        self.setComponents(tarfile_path)
         self.targetdir = os.path.join(
-            pubconf.archiveroot, 'dists', distroseries, component)
+            pubconf.archiveroot, 'dists', distroseries, self.component)
 
     @classmethod
     def getSeriesKey(cls, tarfile_path):

=== modified file 'lib/lp/archivepublisher/debian_installer.py'
--- lib/lp/archivepublisher/debian_installer.py	2012-11-09 12:42:32 +0000
+++ lib/lp/archivepublisher/debian_installer.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of debian installer tarballs."""
@@ -48,8 +48,11 @@
             raise ValueError("%s is not BASE_VERSION_ARCH" % tarfile_base)
         return bits[0], bits[1], bits[2].split(".")[0]
 
+    def setComponents(self, tarfile_path):
+        _, self.version, self.arch = self.parsePath(tarfile_path)
+
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
-        _, self.version, self.arch = self.parsePath(tarfile_path)
+        self.setComponents(tarfile_path)
         self.targetdir = os.path.join(
             pubconf.archiveroot, 'dists', distroseries, 'main',
             'installer-%s' % self.arch)

=== modified file 'lib/lp/archivepublisher/dist_upgrader.py'
--- lib/lp/archivepublisher/dist_upgrader.py	2012-11-09 12:42:32 +0000
+++ lib/lp/archivepublisher/dist_upgrader.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of dist-upgrader tarballs."""
@@ -65,8 +65,11 @@
             raise ValueError("%s is not NAME_VERSION_ARCH" % tarfile_base)
         return bits[0], bits[1], bits[2].split(".")[0]
 
+    def setComponents(self, tarfile_path):
+        _, self.version, self.arch = self.parsePath(tarfile_path)
+
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
-        _, self.version, self.arch = self.parsePath(tarfile_path)
+        self.setComponents(tarfile_path)
         self.targetdir = os.path.join(
             pubconf.archiveroot, 'dists', distroseries, 'main',
             'dist-upgrader-%s' % self.arch)

=== modified file 'lib/lp/archivepublisher/uefi.py'
--- lib/lp/archivepublisher/uefi.py	2012-11-09 12:54:08 +0000
+++ lib/lp/archivepublisher/uefi.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of UEFI boot loader images.
@@ -58,6 +58,10 @@
             raise ValueError("%s is not TYPE_VERSION_ARCH" % tarfile_base)
         return bits[0], bits[1], bits[2].split(".")[0]
 
+    def setComponents(self, tarfile_path):
+        self.loader_type, self.version, self.arch = self.parsePath(
+            tarfile_path)
+
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         if pubconf.uefiroot is None:
             if self.logger is not None:
@@ -78,10 +82,10 @@
                         "UEFI certificate %s not readable" % self.cert)
                 self.cert = None
 
-        loader_type, self.version, self.arch = self.parsePath(tarfile_path)
+        self.setComponents(tarfile_path)
         self.targetdir = os.path.join(
             pubconf.archiveroot, "dists", distroseries, "main", "uefi",
-            "%s-%s" % (loader_type, self.arch))
+            "%s-%s" % (self.loader_type, self.arch))
 
     @classmethod
     def getSeriesKey(cls, tarfile_path):

=== modified file 'lib/lp/soyuz/scripts/custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-10-19 11:11:40 +0000
+++ lib/lp/soyuz/scripts/custom_uploads_copier.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Copy latest custom uploads into a distribution release series.
@@ -116,6 +116,18 @@
         existing_upload = target_uploads.get(self.getKey(upload))
         return existing_upload is not None and existing_upload.id >= upload.id
 
+    def isForValidDAS(self, upload):
+        """Is `upload` for a valid DAS for the target series?
+
+        :param upload: A `PackageUploadCustom` from the source series.
+        """
+        concrete = self.copyable_types[upload.customformat]()
+        concrete.setComponents(upload.libraryfilealias.filename)
+        if concrete.arch is None:
+            return True
+        return concrete.arch in [
+            das.architecturetag for das in self.target_series.architectures]
+
     def copyUpload(self, original_upload):
         """Copy `original_upload` into `self.target_series`."""
         if self.target_archive is None:
@@ -142,5 +154,6 @@
         source_uploads = self.getLatestUploads(
             source_series, source_pocket=source_pocket)
         for upload in source_uploads.itervalues():
-            if not self.isObsolete(upload, target_uploads):
+            if (not self.isObsolete(upload, target_uploads) and
+                self.isForValidDAS(upload)):
                 self.copyUpload(upload)

=== modified file 'lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-10-19 11:27:32 +0000
+++ lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2013-07-25 16:50:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test copying of custom package uploads for a new `DistroSeries`."""
@@ -133,10 +133,12 @@
         # CustomUploadsCopier copies custom uploads from one series to
         # another.
         current_series = self.factory.makeDistroSeries()
-        original_upload = self.makeUpload(current_series)
+        original_upload = self.makeUpload(current_series, arch='alpha')
         new_series = self.factory.makeDistroSeries(
             distribution=current_series.distribution,
             previous_series=current_series)
+        self.factory.makeDistroArchSeries(
+            distroseries=new_series, architecturetag='alpha')
 
         CustomUploadsCopier(new_series).copy(current_series)
 
@@ -325,6 +327,30 @@
             copier.isObsolete(
                 source_upload, copier.getLatestUploads(target_series)))
 
+    def test_isForValidDAS_returns_False_with_dead_arch(self):
+        source_series = self.factory.makeDistroSeries()
+        source_upload = self.makeUpload(source_series, arch='alpha')
+        target_series = self.factory.makeDistroSeries()
+        copier = CustomUploadsCopier(target_series)
+        self.assertFalse(copier.isForValidDAS(source_upload))
+
+    def test_isForValidDAS_returns_True(self):
+        source_series = self.factory.makeDistroSeries()
+        source_upload = self.makeUpload(source_series, arch='alpha')
+        target_series = self.factory.makeDistroSeries()
+        self.factory.makeDistroArchSeries(
+            distroseries=target_series, architecturetag='alpha')
+        copier = CustomUploadsCopier(target_series)
+        self.assertTrue(copier.isForValidDAS(source_upload))
+
+    def test_isForValidDAS_returns_True_for_DDTP(self):
+        source_series = self.factory.makeDistroSeries()
+        source_upload = self.makeUpload(
+            source_series, custom_type=PackageUploadCustomFormat.DDTP_TARBALL)
+        target_series = self.factory.makeDistroSeries()
+        copier = CustomUploadsCopier(target_series)
+        self.assertTrue(copier.isForValidDAS(source_upload))
+
     def test_copyUpload_creates_upload(self):
         # copyUpload creates a new upload that's very similar to the
         # original, but for the target series.