launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/ddeb-override-lockstep into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #747558 in Launchpad itself: "PPAs should create backtracable packages"
  https://bugs.launchpad.net/launchpad/+bug/747558

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/ddeb-override-lockstep/+merge/79799

= Summary =
At upload time, override DDEB files so they are the same as their DEB counterparts. This ensures that domination will work.

== Pre-implementation notes ==
See comments on bug 747558

== Implementation details ==
Add a new method to the upload processor, "_overrideDDEBSs", that ensures that
any DDEBs in the upload are overridden to have the same
component/section/priority as their counterpart DEBS. This method is then called
from process().

There are new unit tests for both of these changes.  There's also some lint
fixed.

== Tests ==
bin/test -cvv test_nascentupload TestOverrideDDEBs
bin/test -cvv test_test_uploadprocessor test_ddeb_upload_overrides

== Demo and Q/A ==
Upload some packages that create DDEBs in a PPA and check their overrides.


= Launchpad lint =

Ignore this bogus lint.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archiveuploader/nascentupload.py
  lib/lp/archiveuploader/tests/test_nascentupload.py
  lib/lp/archiveuploader/tests/data/suite/debug_1.0-1/debug_1.0-1_i386.changes
  lib/lp/archiveuploader/tests/test_uploadprocessor.py

./lib/lp/archiveuploader/tests/data/suite/debug_1.0-1/debug_1.0-1_i386.changes
      11: Line has trailing whitespace.
      13: Line has trailing whitespace.
      17: Line has trailing whitespace.
      20: Line has trailing whitespace.
      21: Line exceeds 78 characters.
      22: Line exceeds 78 characters.
      23: Line has trailing whitespace.
      25: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/ddeb-override-lockstep/+merge/79799
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/ddeb-override-lockstep into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2011-09-29 04:12:59 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2011-10-19 11:20:31 +0000
@@ -178,6 +178,7 @@
             # before doing component verifications because the component
             # actually comes from overrides for packages that are not NEW.
             self.find_and_apply_overrides()
+            self._overrideDDEBSs()
 
         # Override archive location if necessary.
         self.overrideArchive()
@@ -365,9 +366,22 @@
                 "Orphaned debug packages: %s" % ', '.join(
                     '%s %s (%s)' % d for d in unmatched_ddebs))
 
+    def _overrideDDEBSs(self):
+        """Make sure that any DDEBs in the upload have the same overrides
+        as their counterpart DEBs.  This method needs to be called *after*
+        _matchDDEBS.
+
+        This is required so that domination can supersede both files in
+        lockstep.
+        """
+        for uploaded_file in self.changes.files:
+            if isinstance(uploaded_file, DdebBinaryUploadFile):
+                self._overrideBinaryFile(uploaded_file,
+                                         uploaded_file.deb_file)
     #
     # Helpers for warnings and rejections
     #
+
     def run_and_check_error(self, callable):
         """Run the given callable and process errors and warnings.
 
@@ -697,7 +711,9 @@
             override.binarypackagerelease.title,
             override.distroarchseries.architecturetag,
             override.pocket.name))
+        self._overrideBinaryFile(uploaded_file, override)
 
+    def _overrideBinaryFile(self, uploaded_file, override):
         uploaded_file.component_name = override.component.name
         uploaded_file.section_name = override.section.name
         # Both, changesfiles and nascentuploadfile local maps, reffer to

=== modified file 'lib/lp/archiveuploader/tests/data/suite/debug_1.0-1/debug_1.0-1_i386.changes'
--- lib/lp/archiveuploader/tests/data/suite/debug_1.0-1/debug_1.0-1_i386.changes	2009-10-11 05:25:10 +0000
+++ lib/lp/archiveuploader/tests/data/suite/debug_1.0-1/debug_1.0-1_i386.changes	2011-10-19 11:20:31 +0000
@@ -22,4 +22,4 @@
  9c0013badb1bb78fa1ccbf1508398cbf51904f7242d8931f4994c1aa18519b63 668 debug-bin-dbgsym_1.0-1_i386.ddeb
 Files: 
  48b4eed5980c754cc74a06ffc4372f64 654 devel optional debug-bin_1.0-1_i386.deb
- ac5c44025ffc026485b22722e0f13804 668 devel optional debug-bin-dbgsym_1.0-1_i386.ddeb
+ ac5c44025ffc026485b22722e0f13804 668 devel extra debug-bin-dbgsym_1.0-1_i386.ddeb

=== modified file 'lib/lp/archiveuploader/tests/test_nascentupload.py'
--- lib/lp/archiveuploader/tests/test_nascentupload.py	2010-12-22 01:08:48 +0000
+++ lib/lp/archiveuploader/tests/test_nascentupload.py	2011-10-19 11:20:31 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from testtools import TestCase
+from testtools.matchers import MatchesStructure
 
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.archiveuploader.changesfile import determine_file_class_and_name
@@ -32,13 +33,15 @@
         self.changes = FakeChangesFile()
         self.upload = NascentUpload(self.changes, None, DevNullLogger())
 
-    def addFile(self, filename):
+    def addFile(self, filename, comp_and_section='main/devel',
+                priority='extra'):
         """Add a file of the right type to the upload."""
         package, cls = determine_file_class_and_name(filename)
         file = cls(
-            filename, None, 100, 'devel', 'extra', package, '666',
+            filename, None, 100, comp_and_section, priority, package, '666',
             self.changes, None, self.upload.logger)
         self.changes.files.append(file)
+        return file
 
     def assertMatchDDEBErrors(self, error_list):
         self.assertEquals(
@@ -83,3 +86,20 @@
         self.addFile('libblah-dbgsym_1.0_amd64.ddeb')
         self.assertMatchDDEBErrors(
             ['Orphaned debug packages: libblah-dbgsym 666 (amd64)'])
+
+
+class TestOverrideDDEBs(TestMatchDDEBs):
+
+    def test_DDEBsGetOverrideFromDEBs(self):
+        # Test the basic case ensuring that DDEB files always match the
+        # DEB's overrides.
+        deb = self.addFile("foo_1.0_i386.deb", "main/devel", "extra")
+        ddeb = self.addFile(
+            "foo-dbgsym_1.0_i386.ddeb", "universe/web",  "low")
+        self.assertMatchDDEBErrors([])
+        self.upload._overrideDDEBSs()
+
+        self.assertThat(
+            ddeb,
+            MatchesStructure.fromExample(
+                deb, "component_name", "section_name", "priority_name"))

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-09-29 06:59:03 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-10-19 11:20:31 +0000
@@ -32,8 +32,15 @@
 from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.app.errors import NotFoundError
+from lp.archiveuploader.nascentupload import NascentUpload
+from lp.archiveuploader.nascentuploadfile import DdebBinaryUploadFile
+from lp.archiveuploader.tests import (
+    datadir,
+    getPolicy,
+    )
 from lp.archiveuploader.uploadpolicy import (
     AbstractUploadPolicy,
+    ArchiveUploadType,
     findPolicyByName,
     IArchiveUploadPolicy,
     )
@@ -57,7 +64,10 @@
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.features.testing import FeatureFixture
-from lp.services.log.logger import BufferLogger
+from lp.services.log.logger import (
+    BufferLogger,
+    DevNullLogger,
+    )
 from lp.services.mail import stub
 from lp.soyuz.enums import (
     ArchivePermissionType,
@@ -174,7 +184,7 @@
         if builds is None:
             builds = self.options.builds
 
-        def getPolicy(distro, build):
+        def getUploadPolicy(distro, build):
             self.options.distro = distro.name
             policy = findPolicyByName(self.options.context)
             if builds:
@@ -186,8 +196,8 @@
 
         upload_processor = UploadProcessor(
             self.options.base_fsroot, self.options.dryrun,
-            self.options.nomails, builds, self.options.keep, getPolicy, txn,
-            self.log)
+            self.options.nomails, builds, self.options.keep, getUploadPolicy,
+            txn, self.log)
         self.switchToUploader()
         return upload_processor
 
@@ -1228,7 +1238,7 @@
         # Housekeeping so the next test won't fail.
         shutil.rmtree(upload_dir)
 
-    def disabled_per_bug_825486_testPartnerUploadToNonReleaseOrProposedPocket(self):
+    def disabled_testPartnerUploadToNonReleaseOrProposedPocket(self):
         # XXX: bug 825486 robertcollins 2011-08-13 this test is broken.
         """Test partner upload pockets.
 
@@ -1927,6 +1937,29 @@
         self.assertEqual(len(stub.test_emails), 0)
         self.assertNoNewOops(last_oops)
 
+    def test_ddeb_upload_overrides(self):
+        # DDEBs should always be overridden to the same values as their
+        # counterpart DEB's.
+        policy = getPolicy(
+            name="sync", distro="ubuntu", distroseries="hoary")
+        policy.accepted_type = ArchiveUploadType.BINARY_ONLY
+        uploader = NascentUpload.from_changesfile_path(
+            datadir("suite/debug_1.0-1/debug_1.0-1_i386.changes"),
+            policy, DevNullLogger())
+        uploader.process()
+
+        # The package data on disk that we just uploaded has a different
+        # priority setting between the deb and the ddeb. We can now assert
+        # that the process() method above overrode the ddeb.
+        for uploaded_file in uploader.changes.files:
+            if isinstance(uploaded_file, DdebBinaryUploadFile):
+                ddeb = uploaded_file
+            else:
+                deb = uploaded_file
+
+        self.assertEqual("optional", ddeb.priority_name)
+        self.assertEqual(deb.priority_name, ddeb.priority_name)
+
 
 class TestBuildUploadProcessor(TestUploadProcessorBase):
     """Test that processing build uploads works."""