Merge lp:~rvb/launchpad/override-bug-826870 into lp:launchpad

[Raphaël Badin]

[r=julian-edwards][bug=826870] Fix UnknownOverridePolicy to put sources from debian into the right default component.

This branch fixes the UnknownOverridePolicy so that sources from debian are put into the right component by default ('contrib'=> 'multiverse', 'non-free'=> 'multiverse', all else => 'universe'). It also refactors the code in nascentupload.py (processUnknownFile) to use the same code from UnknownOverridePolicy.

= Tests =

./bin/test -vvc test_overrides test_getComponentOverride_default_name
./bin/test -vvc test_overrides test_getComponentOverride_default_component
./bin/test -vvc test_overrides test_getComponentOverride_return_component
./bin/test -vvc test_packagecopyjob test_copying_to_main_archive_debian_override_contrib
./bin/test -vvc test_packagecopyjob test_copying_to_main_archive_debian_override_nonfree

= QA =

Upload sources into debian components 'non-free' and 'contrib' and make sure that the default selected components for these source uploads are 'multiverse'.

[Julian Edwards (julian-edwards)]

That is a little bit of a quandary over the sources vs source. Since we can pass multiple sources at once then the choice is either to assume they are all the same component in their source, or pass multiple components. Since we generally only process one source at a time in PCJ I am fine with the single component here. If we ever do multiple then we'll have to batch overrides by source component.

So overall, it looks good - just a couple of things, I'd s/sources_component/source_component/g and move the DEBIAN_COMPONENT_OVERRIDE_MAP/DEFAULT_OVERRIDE_COMPONENT into the class, I don't think it's useful having it at the module level since we want to encourage people to use the new method.

168 + # getComponentOverride returns the default component namewhen an

Typo "namewhen"

Thanks for the change!

review: Approve

[Raphaël Badin (rvb)]

> That is a little bit of a quandary over the sources vs source. Since we can
> pass multiple sources at once then the choice is either to assume they are all
> the same component in their source, or pass multiple components. Since we
> generally only process one source at a time in PCJ I am fine with the single
> component here. If we ever do multiple then we'll have to batch overrides by
> source component.
>
> So overall, it looks good - just a couple of things, I'd
> s/sources_component/source_component/g and move the
> DEBIAN_COMPONENT_OVERRIDE_MAP/DEFAULT_OVERRIDE_COMPONENT into the class, I
> don't think it's useful having it at the module level since we want to
> encourage people to use the new method.

Done.

>
> 168 + # getComponentOverride returns the default component namewhen an
>
> Typo "namewhen"

Fixed.

Thanks for the review.

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2011-08-19 02:47:35 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2011-08-23 15:24:55 +0000
@@ -41,6 +41,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+from lp.soyuz.adapters.overrides import UnknownOverridePolicy
 from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
 from lp.soyuz.interfaces.queue import QueueInconsistentStateError
 
@@ -720,15 +721,7 @@
     def processUnknownFile(self, uploaded_file):
         """Apply a set of actions for newly-uploaded (unknown) files.
 
-        Newly-uploaded files have a default set of overrides to be applied.
-        This reduces the amount of work that archive admins have to do
-        since they override the majority of new uploads with the same
-        values.  The rules for overriding are: (See bug #120052)
-            'contrib' -> 'multiverse'
-            'non-free' -> 'multiverse'
-            everything else -> 'universe'
-        This mainly relates to Debian syncs, where the default component
-        is 'main' but should not be in main for Ubuntu.
+        Here we use the override policy defined in UnknownOverridePolicy.
 
         In the case of a PPA, files are not touched.  They are always
         overridden to 'main' at publishing time, though.
@@ -751,14 +744,10 @@
             # Don't override partner uploads.
             return
 
-        component_override_map = {
-            'contrib': 'multiverse',
-            'non-free': 'multiverse',
-            }
-
         # Apply the component override and default to universe.
-        uploaded_file.component_name = component_override_map.get(
-            uploaded_file.component_name, 'universe')
+        component_name_override = UnknownOverridePolicy.getComponentOverride(
+            uploaded_file.component_name)
+        uploaded_file.component_name = component_name_override
 
     def find_and_apply_overrides(self):
         """Look for ancestry and overrides information.

=== modified file 'lib/lp/soyuz/adapters/overrides.py'
--- lib/lp/soyuz/adapters/overrides.py	2011-07-20 08:50:10 +0000
+++ lib/lp/soyuz/adapters/overrides.py	2011-08-23 15:24:55 +0000
@@ -139,13 +139,15 @@
     keep the same component and section as their ancestor publications.
     """
 
-    def calculateSourceOverrides(archive, distroseries, pocket, sources):
+    def calculateSourceOverrides(archive, distroseries, pocket, sources,
+                                 source_component=None):
         """Calculate source overrides.
 
         :param archive: The target `IArchive`.
         :param distroseries: The target `IDistroSeries`.
         :param pocket: The target `PackagePublishingPocket`.
         :param sources: A tuple of `ISourcePackageName`s.
+        :param source_component: The sources' `IComponent` (optional).
 
         :return: A list of `ISourceOverride`
         """
@@ -171,7 +173,7 @@
     implements(IOverridePolicy)
 
     def calculateSourceOverrides(self, archive, distroseries, pocket,
-                                 sources):
+                                 sources, source_component=None):
         raise NotImplementedError()
 
     def calculateBinaryOverrides(self, archive, distroseries, pocket,
@@ -188,7 +190,8 @@
     for the latest published binary publication.
     """
 
-    def calculateSourceOverrides(self, archive, distroseries, pocket, spns):
+    def calculateSourceOverrides(self, archive, distroseries, pocket, spns,
+                                 source_component=None):
         # Avoid circular imports.
         from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
         from lp.soyuz.model.publishing import SourcePackagePublishingHistory
@@ -276,12 +279,43 @@
 
     Override policy that assumes everything passed in doesn't exist, so
     returns the defaults.
+
+    Newly-uploaded files have a default set of overrides to be applied.
+    This reduces the amount of work that archive admins have to do
+    since they override the majority of new uploads with the same
+    values.  The rules for overriding are: (See bug #120052)
+        'contrib' -> 'multiverse'
+        'non-free' -> 'multiverse'
+        everything else -> 'universe'
+    This mainly relates to Debian syncs, where the default component
+    is 'main' but should not be in main for Ubuntu.
     """
 
+    DEBIAN_COMPONENT_OVERRIDE_MAP = {
+        'contrib': 'multiverse',
+        'non-free': 'multiverse',
+        }
+
+    DEFAULT_OVERRIDE_COMPONENT = 'universe'
+
+    @classmethod
+    def getComponentOverride(cls, component=None, return_component=False):
+        # component can be a Component object or a component name.
+        if isinstance(component, Component):
+            component = component.name
+        override_component_name = cls.DEBIAN_COMPONENT_OVERRIDE_MAP.get(
+            component, cls.DEFAULT_OVERRIDE_COMPONENT)
+        if return_component:
+            return getUtility(IComponentSet)[override_component_name]
+        else:
+            return override_component_name
+
     def calculateSourceOverrides(self, archive, distroseries, pocket,
-                                 sources):
-        default_component = archive.default_component or getUtility(
-            IComponentSet)['universe']
+                                 sources, source_component=None):
+        default_component = (
+            archive.default_component or
+            UnknownOverridePolicy.getComponentOverride(
+                source_component, return_component=True))
         return [
             SourceOverride(source, default_component, None)
             for source in sources]
@@ -304,15 +338,16 @@
     """
 
     def calculateSourceOverrides(self, archive, distroseries, pocket,
-                                 sources):
+                                 sources, source_component=None):
         total = set(sources)
         overrides = FromExistingOverridePolicy.calculateSourceOverrides(
-            self, archive, distroseries, pocket, sources)
+            self, archive, distroseries, pocket, sources, source_component)
         existing = set(override.source_package_name for override in overrides)
         missing = total.difference(existing)
         if missing:
             unknown = UnknownOverridePolicy.calculateSourceOverrides(
-                self, archive, distroseries, pocket, missing)
+                self, archive, distroseries, pocket, missing,
+                source_component)
             overrides.extend(unknown)
         return overrides
 

=== modified file 'lib/lp/soyuz/adapters/tests/test_overrides.py'
--- lib/lp/soyuz/adapters/tests/test_overrides.py	2011-07-20 08:49:48 +0000
+++ lib/lp/soyuz/adapters/tests/test_overrides.py	2011-08-23 15:24:55 +0000
@@ -149,6 +149,31 @@
                 distroseries.main_archive, distroseries, pocket, bpns)
         self.assertThat(recorder, HasQueryCount(Equals(4)))
 
+    def test_getComponentOverride_default_name(self):
+        # getComponentOverride returns the default component name when an
+        # unknown component name is passed.
+        component_name = UnknownOverridePolicy.getComponentOverride('no-name')
+
+        self.assertEqual('universe', component_name)
+
+    def test_getComponentOverride_default_component(self):
+        # getComponentOverride also accepts a component object (as
+        # opposed to a component's name).
+        component = getUtility(IComponentSet)['universe']
+        component_name = UnknownOverridePolicy.getComponentOverride(component)
+
+        self.assertEqual('universe', component_name)
+
+    def test_getComponentOverride_return_component(self):
+        # Passing return_component=True to getComponentOverride makes it
+        # return the Component object (as opposed to the component's
+        # name).
+        universe_component = getUtility(IComponentSet)['universe']
+        component = UnknownOverridePolicy.getComponentOverride(
+            universe_component, return_component=True)
+
+        self.assertEqual(universe_component, component)
+
     def test_unknown_sources(self):
         # If the unknown policy is used, it does no checks, just returns the
         # defaults.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-08-16 15:12:42 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-08-23 15:24:55 +0000
@@ -402,7 +402,7 @@
 
         return SourceOverride(source_package_name, component, section)
 
-    def _checkPolicies(self, source_name):
+    def _checkPolicies(self, source_name, source_component=None):
         # This helper will only return if it's safe to carry on with the
         # copy, otherwise it raises SuspendJobException to tell the job
         # runner to suspend the job.
@@ -418,7 +418,7 @@
             # metadata.
             defaults = UnknownOverridePolicy().calculateSourceOverrides(
                 self.target_archive, self.target_distroseries,
-                self.target_pocket, [source_name])
+                self.target_pocket, [source_name], source_component)
             self.addSourceOverride(defaults[0])
 
             approve_new = copy_policy.autoApproveNew(
@@ -502,7 +502,8 @@
         pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
             [self.context.id]).any()
         if pu is None:
-            self._checkPolicies(source_name)
+            self._checkPolicies(
+                source_name, source_package.sourcepackagerelease.component)
 
         # The package is free to go right in, so just copy it now.
         override = self.getSourceOverride()

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-08-18 20:07:33 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-08-23 15:24:55 +0000
@@ -705,7 +705,6 @@
         self.assertEqual(None, existing_sources.any())
 
         # Now, run the copy job.
-
         source = getUtility(IPlainPackageCopyJobSource)
         requester = self.factory.makePerson()
         job = source.create(
@@ -731,6 +730,67 @@
         # UnknownOverridePolicy policy.
         self.assertEqual('universe', pcj.metadata['component_override'])
 
+    def createCopyJob(self, sourcename, component, section, version):
+        # Helper method to create a package copy job for a package with
+        # the given sourcename, component, section and version.
+        publisher = SoyuzTestPublisher()
+        publisher.prepareBreezyAutotest()
+        distroseries = publisher.breezy_autotest
+
+        target_archive = self.factory.makeArchive(
+            distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
+        source_archive = self.factory.makeArchive()
+
+        # Publish a package in the source archive with some overridable
+        # properties set to known values.
+        publisher.getPubSource(
+            distroseries=distroseries, sourcename=sourcename,
+            component=component, section=section,
+            version=version, status=PackagePublishingStatus.PUBLISHED,
+            archive=source_archive)
+
+        # Now, run the copy job.
+        source = getUtility(IPlainPackageCopyJobSource)
+        requester = self.factory.makePerson()
+        job = source.create(
+            package_name=sourcename,
+            package_version=version,
+            source_archive=source_archive,
+            target_archive=target_archive,
+            target_distroseries=distroseries,
+            target_pocket=PackagePublishingPocket.RELEASE,
+            include_binaries=False,
+            requester=requester)
+
+        # Run the job so it gains a PackageUpload.
+        self.assertRaises(SuspendJobException, self.runJob, job)
+        pcj = removeSecurityProxy(job).context
+        return pcj
+
+    def test_copying_to_main_archive_debian_override_contrib(self):
+        # The job uses the overrides to map debian components to
+        # the right components.
+        # 'contrib' gets mapped to 'multiverse'.
+
+        # Create debian component.
+        self.factory.makeComponent('contrib')
+        # Create a copy job for a package in 'contrib'.
+        pcj = self.createCopyJob('package', 'contrib', 'web', '2.8.1')
+
+        self.assertEqual('multiverse', pcj.metadata['component_override'])
+
+    def test_copying_to_main_archive_debian_override_nonfree(self):
+        # The job uses the overrides to map debian components to
+        # the right components.
+        # 'nonfree' gets mapped to 'multiverse'.
+
+        # Create debian component.
+        self.factory.makeComponent('non-free')
+        # Create a copy job for a package in 'non-free'.
+        pcj = self.createCopyJob('package', 'non-free', 'web', '2.8.1')
+
+        self.assertEqual('multiverse', pcj.metadata['component_override'])
+
     def test_copying_to_main_archive_unapproved(self):
         # Uploading to a series that is in a state that precludes auto
         # approval will cause the job to suspend and a packageupload

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2011-08-15 03:41:35 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2011-08-23 15:24:55 +0000
@@ -886,7 +886,7 @@
             self.assertEquals(copy.component.name, 'universe')
 
     def test_overrideFromAncestry_fallback_to_source_component(self):
-        # overrideFromancestry on the lack of ancestry, falls back to the
+        # overrideFromAncestry on the lack of ancestry, falls back to the
         # component the source was originally uploaded to.
         source = self.makeSource()