launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/copy-ddtp into lp:launchpad.

Requested reviews:
  Benji York (benji)
Related bugs:
  Bug #827941 in Launchpad itself: "Copy ddtp-translations uploads to new distroseries"
  https://bugs.launchpad.net/launchpad/+bug/827941

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/copy-ddtp/+merge/111688

== Summary ==

Bug 827941 reports that there's another custom upload type, namely ddtp-tarball, that needs to be accounted for in CustomUploadsCopier so that it gets copied to new distroseries.  (Bug 672314 has an example of a concrete problem that I think has this as its root cause.)

== Proposed fix ==

At one point this would have been fiddly, but following https://code.launchpad.net/~cjwatson/launchpad/custom-upload-parsing/+merge/107656 it's pretty easy: implement DdtpTarballUpload.getSeriesKey and add an item to CustomUploadsCopier.copyable_types.  Job done.

== LOC Rationale ==

+46.  However, this is part of an arc as follows:

  https://code.launchpad.net/~cjwatson/launchpad/pcj-reupload/+merge/111124 (+180)
  https://code.launchpad.net/~cjwatson/launchpad/copy-custom-uploads/+merge/111653 (+66)
  this branch (+46)
  a probable future branch to fix P-a-s handling in direct copies (should be <100)
  removal of unembargo-package, delayed copies, ubuntu-security celebrity, etc. (at least -600)

Even fairly conservatively, this arc should end up well in the black.

== Tests ==

bin/test -vvct test_ddtp_tarball -t test_custom_uploads_copier

== Demo and Q/A ==

It would be best to wait until after https://code.launchpad.net/~cjwatson/launchpad/copy-custom-uploads/+merge/111653 has landed.  Once that's done, the same QA procedure as in that MP should work, except that the package to upload is whatever Michael uses to upload translations and the paths are dists/precise-updates/*/i18n/.

I won't be able to land and QA this until the issue I spotted in https://bugs.launchpad.net/launchpad/+bug/827941/comments/3 is fixed, because the current way that DDTP tarballs are uploaded is really not amenable to the package copier, owing to not having an SPPH.  However, I believe this code will be fine once the uploads are fixed to be less evil (and we might even be able to delete a bit of code from archiveuploader as a result) so I might as well get this in for review anyway.
-- 
https://code.launchpad.net/~cjwatson/launchpad/copy-ddtp/+merge/111688
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/archivepublisher/ddtp_tarball.py'
--- lib/lp/archivepublisher/ddtp_tarball.py	2012-06-28 16:32:27 +0000
+++ lib/lp/archivepublisher/ddtp_tarball.py	2012-07-03 17:47:20 +0000
@@ -58,6 +58,13 @@
         self.targetdir = os.path.join(
             pubconf.archiveroot, 'dists', distroseries, component)
 
+    @classmethod
+    def getSeriesKey(cls, tarfile_path):
+        try:
+            return cls.parsePath(tarfile_path)[1]
+        except ValueError:
+            return None
+
     def checkForConflicts(self):
         # We just overwrite older files, so no conflicts are possible.
         pass

=== modified file 'lib/lp/archivepublisher/tests/test_ddtp_tarball.py'
--- lib/lp/archivepublisher/tests/test_ddtp_tarball.py	2012-06-28 16:32:27 +0000
+++ lib/lp/archivepublisher/tests/test_ddtp_tarball.py	2012-07-03 17:47:20 +0000
@@ -9,7 +9,10 @@
 
 import os
 
-from lp.archivepublisher.ddtp_tarball import process_ddtp_tarball
+from lp.archivepublisher.ddtp_tarball import (
+    DdtpTarballUpload,
+    process_ddtp_tarball,
+    )
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
 from lp.testing import TestCase
 
@@ -107,3 +110,31 @@
             self.assertEqual("", ca_file.read())
         self.assertEqual(1, os.stat(bn).st_nlink)
         self.assertEqual(1, os.stat(ca).st_nlink)
+
+    def test_parsePath_handles_underscore_in_directory(self):
+        # parsePath is not misled by an underscore in the directory name.
+        self.assertEqual(
+            # XXX cjwatson 2012-07-03: .tar.gz is not stripped off the end
+            # of the version due to something of an ambiguity in the design;
+            # how should translations_main_1.0.1.tar.gz be parsed?  In
+            # practice this doesn't matter because DdtpTarballUpload never
+            # uses the version for anything.
+            ("translations", "main", "1.tar.gz"),
+            DdtpTarballUpload.parsePath(
+                "/dir_with_underscores/translations_main_1.tar.gz"))
+
+    def test_getSeriesKey_extracts_component(self):
+        # getSeriesKey extracts the component from an upload's filename.
+        self.openArchive("20060728")
+        self.assertEqual("main", DdtpTarballUpload.getSeriesKey(self.path))
+
+    def test_getSeriesKey_returns_None_on_mismatch(self):
+        # getSeriesKey returns None if the filename does not match the
+        # expected pattern.
+        self.assertIsNone(DdtpTarballUpload.getSeriesKey("argh_1.0.jpg"))
+
+    def test_getSeriesKey_refuses_names_with_wrong_number_of_fields(self):
+        # getSeriesKey requires exactly three fields.
+        self.assertIsNone(DdtpTarballUpload.getSeriesKey("package_1.0.tar.gz"))
+        self.assertIsNone(DdtpTarballUpload.getSeriesKey(
+            "one_two_three_four_5.tar.gz"))

=== modified file 'lib/lp/soyuz/scripts/custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-07-03 17:09:00 +0000
+++ lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-07-03 17:47:20 +0000
@@ -16,6 +16,7 @@
 
 from zope.component import getUtility
 
+from lp.archivepublisher.ddtp_tarball import DdtpTarballUpload
 from lp.archivepublisher.debian_installer import DebianInstallerUpload
 from lp.archivepublisher.dist_upgrader import DistUpgraderUpload
 from lp.archivepublisher.uefi import UefiUpload
@@ -41,6 +42,7 @@
     copyable_types = {
         PackageUploadCustomFormat.DEBIAN_INSTALLER: DebianInstallerUpload,
         PackageUploadCustomFormat.DIST_UPGRADER: DistUpgraderUpload,
+        PackageUploadCustomFormat.DDTP_TARBALL: DdtpTarballUpload,
         PackageUploadCustomFormat.UEFI: UefiUpload,
         }
 
@@ -70,9 +72,6 @@
 
     def getKey(self, upload):
         """Get an indexing key for `upload`."""
-        # XXX JeroenVermeulen 2011-08-17, bug=827941: For ddtp
-        # translations tarballs, we'll have to include the component
-        # name as well.
         custom_format = upload.customformat
         series_key = self.extractSeriesKey(
             self.copyable_types[custom_format],

=== modified file 'lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-06-22 17:26:53 +0000
+++ lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-07-03 17:47:20 +0000
@@ -111,16 +111,22 @@
 
     def makeUpload(self, distroseries=None, pocket=None,
                    custom_type=PackageUploadCustomFormat.DEBIAN_INSTALLER,
-                   version=None, arch=None):
+                   version=None, arch=None, component=None):
         """Create a `PackageUploadCustom`."""
         if distroseries is None:
             distroseries = self.factory.makeDistroSeries()
         package_name = self.factory.getUniqueString("package")
         if version is None:
             version = self.makeVersion()
-        if arch is None:
-            arch = self.factory.getUniqueString()
-        filename = "%s.tar.gz" % '_'.join([package_name, version, arch])
+        if custom_type == PackageUploadCustomFormat.DDTP_TARBALL:
+            if component is None:
+                component = self.factory.getUniqueString()
+            filename = "%s.tar.gz" % "_".join(
+                [package_name, component, version])
+        else:
+            if arch is None:
+                arch = self.factory.getUniqueString()
+            filename = "%s.tar.gz" % "_".join([package_name, version, arch])
         package_upload = self.factory.makeCustomPackageUpload(
             distroseries=distroseries, pocket=pocket, custom_type=custom_type,
             filename=filename)
@@ -230,9 +236,6 @@
     def test_getKey_includes_format_and_architecture(self):
         # The key returned by getKey consists of custom upload type,
         # and architecture.
-        # XXX JeroenVermeulen 2011-08-17, bug=827941: To support
-        # ddtp-translations uploads, this will have to include the
-        # component name as well.
         source_series = self.factory.makeDistroSeries()
         upload = self.makeUpload(
             source_series, custom_type=PackageUploadCustomFormat.DIST_UPGRADER,
@@ -244,6 +247,20 @@
             )
         self.assertEqual(expected_key, copier.getKey(upload))
 
+    def test_getKey_ddtp_includes_format_and_component(self):
+        # The key returned by getKey for a ddtp-tarball upload consists of
+        # custom upload type, and component.
+        source_series = self.factory.makeDistroSeries()
+        upload = self.makeUpload(
+            source_series, custom_type=PackageUploadCustomFormat.DDTP_TARBALL,
+            component='restricted')
+        copier = CustomUploadsCopier(FakeDistroSeries())
+        expected_key = (
+            PackageUploadCustomFormat.DDTP_TARBALL,
+            'restricted',
+            )
+        self.assertEqual(expected_key, copier.getKey(upload))
+
     def test_getLatestUploads_indexes_uploads_by_key(self):
         # getLatestUploads returns a dict of uploads, indexed by keys
         # returned by getKey.