launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/weaken-xz-dpkg-requirement into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #905322 in Launchpad itself: "Lower required dpkg version for xz-compressed packages"
  https://bugs.launchpad.net/launchpad/+bug/905322

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/weaken-xz-dpkg-requirement/+merge/86056

== Summary ==

Bug 905322 reports that the required dpkg pre-dependency in NascentUploadFile for .debs with xz-compressed data members is slightly too strict.  Apparently some Debian uploads are using a suffixed ~ to permit installing with backports of that dpkg version, which is a common pattern for (build-)dependencies on packaging infrastructure.  We should weaken this restriction in Launchpad rather than changing packages.

== Proposed fix ==

The code fix is a trivial search-and-replace.

== Pre-implementation notes ==

None.

== Implementation details ==

While the code fix is trivial, this function wasn't well unit-tested, only really integration-tested by a .deb in lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_xz_binary/bar_1.0-1_i386.deb, and that's annoying to modify.  I decided to extend lp.archiveuploader.tests.test_nascentuploadfile.DebBinaryUploadFileTests to unit-test this change instead.  NascentUploadFile could do with many more unit tests in the future, which I haven't attempted right now, but hopefully the helper method I introduced will help somebody working on that.

== Tests ==

bin/test -vvct archiveuploader

== Demo and Q/A ==

Upload the source package referenced in the bug to dogfood and let it build.  It should upload successfully rather than failing.

(We can retry the builds on production after this lands.)

== lint ==

I cleaned up some pre-existing lint.  There's now none here.
-- 
https://code.launchpad.net/~cjwatson/launchpad/weaken-xz-dpkg-requirement/+merge/86056
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/weaken-xz-dpkg-requirement into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2011-08-28 07:29:11 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2011-12-16 15:15:34 +0000
@@ -713,8 +713,8 @@
                 "data.tar.bz2, data.tar.lzma or data.tar.xz." %
                 (self.filename, data_tar))
 
-        # xz-compressed debs must pre-depend on dpkg >= 1.15.6.
-        XZ_REQUIRED_DPKG_VER = '1.15.6'
+        # xz-compressed debs must pre-depend on dpkg >= 1.15.6~.
+        XZ_REQUIRED_DPKG_VER = '1.15.6~'
         if data_tar == "data.tar.xz":
             parsed_deps = []
             try:

=== modified file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
--- lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2010-12-20 06:46:09 +0000
+++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2011-12-16 15:15:34 +0000
@@ -7,6 +7,7 @@
 
 import hashlib
 import os
+import subprocess
 
 from debian.deb822 import (
     Changes,
@@ -276,10 +277,32 @@
                 "protocols",
             }
 
+    def createDeb(self, filename, data_format):
+        """Return the contents of a dummy .deb file."""
+        tempdir = self.makeTemporaryDirectory()
+        members = [
+            "debian-binary",
+            "control.tar.gz",
+            "data.tar.%s" % data_format,
+            ]
+        for member in members:
+            with open(os.path.join(tempdir, member), "w") as f:
+                pass
+        retcode = subprocess.call(
+            ["ar", "rc", filename] + members, cwd=tempdir)
+        self.assertEqual(0, retcode)
+        with open(os.path.join(tempdir, filename)) as f:
+            return f.read()
+
     def createDebBinaryUploadFile(self, filename, component_and_section,
-                                  priority_name, package, version, changes):
+                                  priority_name, package, version, changes,
+                                  data_format=None):
         """Create a DebBinaryUploadFile."""
-        (path, digest, size) = self.writeUploadFile(filename, "DUMMY DATA")
+        if data_format:
+            data = self.createDeb(filename, data_format)
+        else:
+            data = "DUMMY DATA"
+        (path, digest, size) = self.writeUploadFile(filename, data)
         return DebBinaryUploadFile(
             path, digest, size, component_and_section, priority_name, package,
             version, changes, self.policy, self.logger)
@@ -302,6 +325,42 @@
         self.assertEquals("0.42", uploadfile.source_version)
         self.assertEquals("0.42", uploadfile.control_version)
 
+    def test_verifyFormat_xz_good_predep(self):
+        # verifyFormat accepts xz-compressed .debs with a sufficient dpkg
+        # pre-dependency.
+        uploadfile = self.createDebBinaryUploadFile(
+            "foo_0.42_i386.deb", "main/python", "unknown", "mypkg", "0.42",
+            None, data_format="xz")
+        control = self.getBaseControl()
+        control["Pre-Depends"] = "dpkg (>= 1.15.6~)"
+        uploadfile.parseControl(control)
+        self.assertEqual([], list(uploadfile.verifyFormat()))
+
+    def test_verifyFormat_xz_bad_predep(self):
+        # verifyFormat rejects xz-compressed .debs with an insufficient dpkg
+        # pre-dependency.
+        uploadfile = self.createDebBinaryUploadFile(
+            "foo_0.42_i386.deb", "main/python", "unknown", "mypkg", "0.42",
+            None, data_format="xz")
+        control = self.getBaseControl()
+        control["Pre-Depends"] = "dpkg (>= 1.15.5)"
+        uploadfile.parseControl(control)
+        errors = list(uploadfile.verifyFormat())
+        self.assertEqual(1, len(errors))
+        self.assertIsInstance(errors[0], UploadError)
+
+    def test_verifyFormat_xz_no_predep(self):
+        # verifyFormat rejects xz-compressed .debs with no dpkg
+        # pre-dependency.
+        uploadfile = self.createDebBinaryUploadFile(
+            "foo_0.42_i386.deb", "main/python", "unknown", "mypkg", "0.42",
+            None, data_format="xz")
+        control = self.getBaseControl()
+        uploadfile.parseControl(control)
+        errors = list(uploadfile.verifyFormat())
+        self.assertEqual(1, len(errors))
+        self.assertIsInstance(errors[0], UploadError)
+
     def test_storeInDatabase(self):
         # storeInDatabase creates a BinaryPackageRelease.
         uploadfile = self.createDebBinaryUploadFile(
@@ -395,7 +454,7 @@
         # findSourcePackageRelease finds the matching SourcePackageRelease.
         das = self.factory.makeDistroArchSeries(
             distroseries=self.policy.distroseries, architecturetag="i386")
-        build = self.factory.makeBinaryPackageBuild(
+        self.factory.makeBinaryPackageBuild(
             distroarchseries=das,
             archive=self.policy.archive)
         uploadfile = self.createDebBinaryUploadFile(
@@ -416,7 +475,7 @@
         # SourcePackageRelease.
         das = self.factory.makeDistroArchSeries(
             distroseries=self.policy.distroseries, architecturetag="i386")
-        build = self.factory.makeBinaryPackageBuild(
+        self.factory.makeBinaryPackageBuild(
             distroarchseries=das,
             archive=self.policy.archive)
         uploadfile = self.createDebBinaryUploadFile(
@@ -433,14 +492,14 @@
         # package releases.
         das = self.factory.makeDistroArchSeries(
             distroseries=self.policy.distroseries, architecturetag="i386")
-        build = self.factory.makeBinaryPackageBuild(
+        self.factory.makeBinaryPackageBuild(
             distroarchseries=das,
             archive=self.policy.archive)
         uploadfile = self.createDebBinaryUploadFile(
             "foo_0.42_i386.deb", "main/python", "unknown", "mypkg", "0.42",
             None)
         spn = self.factory.makeSourcePackageName("foo")
-        spph1 = self.factory.makeSourcePackagePublishingHistory(
+        self.factory.makeSourcePackagePublishingHistory(
             sourcepackagename=spn,
             distroseries=self.policy.distroseries,
             version="0.42", archive=self.policy.archive,