Merge lp:~cjwatson/launchpad/copy-ddtp into lp:launchpad

[Colin Watson]

Teach CustomUploadsCopier to copy ddtp-tarball files.

== 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.

[Benji York (benji)]

In ddtp_tarball.py ( line 23 of the diff) there is this line:

    _, component, _ = tarfile_path.split("_")

Since you are interested in underscores in the file name and not
the entire path, os.path.basename(tarfile_path).split("_") would be
better.

It would also be good to have some tests with underscores in the path
that show that the method handles them correctly.

Using the ValueError as an way of determining that the destructuring
assignment failed seems a bit indirect as far as input validation goes.

Why is getSeriesKey a classmethod when "cls" isn't used? Maybe a
staticmethod is more appropriate.

How about this for getSeriesKey?

@staticmethod
def getSeriesKey(tarfile_path):
    fn = os.path.basename(tarfile_path)
    if fn.count('_') != 2:
        # The file name does not match the pattern, so no series key
        # can be extracted.
        return None
    return fn.split('_')[1]

[Colin Watson (cjwatson)]

On Mon, Jun 25, 2012 at 06:25:23PM -0000, Benji York wrote:
> In ddtp_tarball.py ( line 23 of the diff) there is this line:
>
> _, component, _ = tarfile_path.split("_")
>
> Since you are interested in underscores in the file name and not
> the entire path, os.path.basename(tarfile_path).split("_") would be
> better.

Yes, indeed. Much of this is changing anyway as a result of feedback on
https://code.launchpad.net/~cjwatson/launchpad/custom-uefi/+merge/111626;
as a result I think I'm going to put this branch on temporary hold until
that one's ready to land, rather than doing much the same work in two
branches. (For instance recent work on that branch means that
getSeriesKey has to be a classmethod, although a helper method it calls
can probably become a staticmethod.)

> Using the ValueError as an way of determining that the destructuring
> assignment failed seems a bit indirect as far as input validation goes.
[...]
> @staticmethod
> def getSeriesKey(tarfile_path):
> fn = os.path.basename(tarfile_path)
> if fn.count('_') != 2:
> # The file name does not match the pattern, so no series key
> # can be extracted.
> return None
> return fn.split('_')[1]

I don't much like the look-before-you-leap pattern here, TBH; it seems
easy for it to get out of sync as code mutates over time, especially
since "fn.count('_') != 2" and "fn.split('_')[1]" are spelled just
differently enough to make it easy to forget to change one of them.

[Colin Watson (cjwatson)]

I think this is ready for review again now, since custom-uefi has landed so I was able to rephrase this a little in terms of that.

[Benji York (benji)]

I still think the look-before-you-leap approach is better here, but there's no reason to hold this branch up on that.

review: Approve (code)

[Colin Watson (cjwatson)]

I'm putting together a compromise that's a bit more explicit about the input validation without duplicating checking and parsing logic in quite the same way.

=== 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-10 11:27:22 +0000
@@ -49,8 +49,11 @@
 
     @staticmethod
     def parsePath(tarfile_path):
-        name, component, version = os.path.basename(tarfile_path).split("_")
-        return name, component, version
+        tarfile_base = os.path.basename(tarfile_path)
+        bits = tarfile_base.split("_")
+        if len(bits) != 3:
+            raise ValueError("%s is not NAME_COMPONENT_VERSION" % tarfile_base)
+        return tuple(bits)
 
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         _, component, self.version = self.parsePath(tarfile_path)
@@ -58,6 +61,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/debian_installer.py'
--- lib/lp/archivepublisher/debian_installer.py	2012-06-28 16:32:27 +0000
+++ lib/lp/archivepublisher/debian_installer.py	2012-07-10 11:27:22 +0000
@@ -42,8 +42,11 @@
 
     @staticmethod
     def parsePath(tarfile_path):
-        base, version, arch = os.path.basename(tarfile_path).split("_")
-        return base, version, arch.split(".")[0]
+        tarfile_base = os.path.basename(tarfile_path)
+        bits = tarfile_base.split("_")
+        if len(bits) != 3:
+            raise ValueError("%s is not BASE_VERSION_ARCH" % tarfile_base)
+        return bits[0], bits[1], bits[2].split(".")[0]
 
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         _, self.version, self.arch = self.parsePath(tarfile_path)
 
=== modified file 'lib/lp/archivepublisher/dist_upgrader.py'
--- lib/lp/archivepublisher/dist_upgrader.py	2012-07-03 17:09:00 +0000
+++ lib/lp/archivepublisher/dist_upgrader.py	2012-07-10 11:27:22 +0000
@@ -59,8 +59,11 @@
 
     @staticmethod
     def parsePath(tarfile_path):
-        name, version, arch = os.path.basename(tarfile_path).split("_")
-        return name, version, arch.split(".")[0]
+        tarfile_base = os.path.basename(tarfile_path)
+        bits = tarfile_base.split("_")
+        if len(bits) != 3:
+            raise ValueError("%s is not NAME_VERSION_ARCH" % tarfile_base)
+        return bits[0], bits[1], bits[2].split(".")[0]
 
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         _, self.version, self.arch = self.parsePath(tarfile_path)
 
=== 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-10 11:27:22 +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/archivepublisher/uefi.py'
--- lib/lp/archivepublisher/uefi.py	2012-06-28 16:32:27 +0000
+++ lib/lp/archivepublisher/uefi.py	2012-07-10 11:27:22 +0000
@@ -69,8 +69,11 @@
 
     @staticmethod
     def parsePath(tarfile_path):
-        loader_type, version, arch = os.path.basename(tarfile_path).split("_")
-        return loader_type, version, arch.split(".")[0]
+        tarfile_base = os.path.basename(tarfile_path)
+        bits = tarfile_base.split("_")
+        if len(bits) != 3:
+            raise ValueError("%s is not TYPE_VERSION_ARCH" % tarfile_base)
+        return bits[0], bits[1], bits[2].split(".")[0]
 
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         if pubconf.uefiroot is None:
 
=== 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-10 11:27:22 +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-10 11:27:22 +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.