Merge lp:~wallyworld/launchpad/package-defaults-192076 into lp:launchpad

[Ian Booth]

Correctly set the overridden component for binary publications.

== Implementation ==

I added a small extra block of code to the NascentUpload find_and_apply_overrides() method - if there is no ancestory for binary uploads, see if there is a current corresponding source publication and use the component from that if available.

== Tests ==

I added some new tests to test_uploadprocessor, since this test module seemed to be the main place where the end-end operation of the nascent upload and processing functionality was tested.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archiveuploader/nascentupload.py
  lib/lp/archiveuploader/nascentuploadfile.py
  lib/lp/archiveuploader/tests/test_uploadprocessor.py

[Steve Kowalik (stevenk)]

I think I'd rather see this an Override Policy, like SetToExistingOverridePolicy or so. Instantiate it with the uploaded_file, and allow the policy to work everything out.

review: Needs Fixing (code)

[Ian Booth (wallyworld)]

I'd like not to have to do that here - the idea was simply to tweak the
existing implementation. There's already been a previous attempt to
re-implement the override stuff as an adaptor but that stuff is not
used everywhere yet. That level of refactoring is best done in a
separate branch. I just wanted to fix the critical here. The
refactoring is not really critical. I think William agrees with the
approach.

On Wed 31 Oct 2012 16:22:20 EST, Steve Kowalik wrote:
> Review: Needs Fixing code
>
> I think I'd rather see this an Override Policy, like SetToExistingOverridePolicy or so. Instantiate it with the uploaded_file, and allow the policy to work everything out.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2012-10-24 09:43:58 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2012-10-31 23:46:22 +0000
@@ -723,10 +723,11 @@
         # That's why we need this conversion here.
         uploaded_file.priority_name = override.priority.name.lower()
 
-    def processUnknownFile(self, uploaded_file):
+    def processUnknownFile(self, uploaded_file, override=None):
         """Apply a set of actions for newly-uploaded (unknown) files.
 
-        Here we use the override policy defined in UnknownOverridePolicy.
+        Here we use the override, if specified, or simply default to the policy
+        defined in UnknownOverridePolicy.
 
         In the case of a PPA, files are not touched.  They are always
         overridden to 'main' at publishing time, though.
@@ -749,7 +750,10 @@
             # Don't override partner uploads.
             return
 
-        # Apply the component override and default to universe.
+        # Use the specified override, or delegate to UnknownOverridePolicy.
+        if override:
+            uploaded_file.component_name = override.component.name
+            return
         component_name_override = UnknownOverridePolicy.getComponentOverride(
             uploaded_file.component_name)
         uploaded_file.component_name = component_name_override
@@ -759,6 +763,9 @@
 
         Anything not yet in the DB gets tagged as 'new' and won't count
         towards the permission check.
+
+        XXX: wallyworld 2012-11-01 bug=1073755: This work should be done using
+        override polices defined in lp.soyuz.adapters.overrides.py
         """
         self.logger.debug("Finding and applying overrides.")
 
@@ -819,7 +826,16 @@
                 else:
                     self.logger.debug(
                         "%s: (binary) NEW" % (uploaded_file.package))
-                    self.processUnknownFile(uploaded_file)
+                    # Check the current source publication's component.
+                    # If there is a corresponding source publication, we will
+                    # use the component from that, otherwise default mappings
+                    # are used.
+                    spph = None
+                    try:
+                        spph = uploaded_file.findCurrentSourcePublication()
+                    except UploadError:
+                        pass
+                    self.processUnknownFile(uploaded_file, spph)
 
     #
     # Actually processing accepted or rejected uploads -- and mailing people

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2012-09-24 01:05:18 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2012-10-31 23:46:22 +0000
@@ -794,16 +794,13 @@
     #
     #   Database relationship methods
     #
-    def findSourcePackageRelease(self):
-        """Return the respective ISourcePackageRelease for this binary upload.
-
-        It inspect publication in the targeted DistroSeries.
-
-        It raises UploadError if the source was not found.
-
-        Verifications on the designed source are delayed because for
-        mixed_uploads (source + binary) we do not have the source stored
-        in DB yet (see verifySourcepackagerelease).
+    def findCurrentSourcePublication(self):
+        """Return the respective ISourcePackagePublishingHistory for this
+        binary upload.
+
+        It inspects publication in the targeted DistroSeries.
+
+        It raises UploadError if the spph was not found.
         """
         assert self.source_name is not None
         assert self.source_version is not None
@@ -814,12 +811,26 @@
         # Workaround storm bug in EmptyResultSet.
         spphs = list(spphs[:1])
         try:
-            return spphs[0].sourcepackagerelease
+            return spphs[0]
         except IndexError:
             raise UploadError(
-                "Unable to find source package %s/%s in %s" % (
+                "Unable to find source publication %s/%s in %s" % (
                 self.source_name, self.source_version, distroseries.name))
 
+    def findSourcePackageRelease(self):
+        """Return the respective ISourcePackageRelease for this binary upload.
+
+        It inspect publication in the targeted DistroSeries.
+
+        It raises UploadError if the source was not found.
+
+        Verifications on the designed source are delayed because for
+        mixed_uploads (source + binary) we do not have the source stored
+        in DB yet (see verifySourcepackagerelease).
+        """
+        spph = self.findCurrentSourcePublication()
+        return spph.sourcepackagerelease
+
     def verifySourcePackageRelease(self, sourcepackagerelease):
         """Check if the given ISourcePackageRelease matches the context."""
         assert 'source' in self.changes.architectures, (

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-10-24 11:03:29 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-10-31 23:46:22 +0000
@@ -198,7 +198,8 @@
         self.switchToUploader()
         return upload_processor
 
-    def publishPackage(self, packagename, version, source=True, archive=None):
+    def publishPackage(self, packagename, version, source=True, archive=None,
+                       component_override=None):
         """Publish a single package that is currently NEW in the queue."""
         self.switchToAdmin()
 
@@ -215,6 +216,8 @@
             pubrec = queue_item.sources[0].publish(self.log)
         else:
             pubrec = queue_item.builds[0].publish(self.log)
+        if component_override:
+            pubrec.component = component_override
         self.switchToUploader()
         return pubrec
 
@@ -1325,7 +1328,7 @@
     # The following three tests check this.
     def checkComponentOverride(self, upload_dir_name,
                                expected_component_name):
-        """Helper function to check overridden component names.
+        """Helper function to check overridden source component names.
 
         Upload a 'bar' package from upload_dir_name, then
         inspect the package 'bar' in the NEW queue and ensure its
@@ -1370,6 +1373,48 @@
         """
         self.checkComponentOverride("bar_1.0-1", "universe")
 
+    def checkBinaryComponentOverride(self, component_override=None,
+                                     expected_component_name='universe'):
+        """Helper function to check overridden binary component names.
+
+        Upload a 'bar' package from upload_dir_name, publish it, and then
+        override the publication's component. When the binary release is
+        published, it's component correspond to the source publication's
+        component.
+
+        The original component comes from the source package contained
+        in upload_dir_name.
+        """
+        uploadprocessor = self.setupBreezyAndGetUploadProcessor()
+        # Upload 'bar-1.0-1' source and binary to ubuntu/breezy.
+        upload_dir = self.queueUpload("bar_1.0-1")
+        self.processUpload(uploadprocessor, upload_dir)
+        bar_source_pub = self.publishPackage(
+            'bar', '1.0-1', component_override=component_override)
+        [bar_original_build] = bar_source_pub.createMissingBuilds()
+
+        self.options.context = 'buildd'
+        upload_dir = self.queueUpload("bar_1.0-1_binary")
+        build_uploadprocessor = self.getUploadProcessor(
+            self.layer.txn, builds=True)
+        self.processUpload(
+            build_uploadprocessor, upload_dir, build=bar_original_build)
+        [bar_binary_pub] = self.publishPackage("bar", "1.0-1", source=False)
+        self.assertEqual(
+            bar_binary_pub.component.name, expected_component_name)
+        self.assertEqual(
+            bar_binary_pub.binarypackagerelease.component.name,
+            expected_component_name)
+
+    def testBinaryPackageDefaultComponent(self):
+        """The default component is universe."""
+        self.checkBinaryComponentOverride(None, "universe")
+
+    def testBinaryPackageSourcePublicationOverride(self):
+        """The source publication's component is used."""
+        restricted = getUtility(IComponentSet)["restricted"]
+        self.checkBinaryComponentOverride(restricted, "restricted")
+
     def testOopsCreation(self):
         """Test that an unhandled exception generates an OOPS."""
         self.options.builds = False