William Grant has proposed merging 
lp:~wgrant/launchpad/bug-701383-ppa-component-override into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #701383 PPA component override should not be hardcoded everywhere
  https://bugs.launchpad.net/bugs/701383

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-701383-ppa-component-override/+merge/46221

PPAs only support the 'main' component, with other components being overridden 
whenever publications are created. This restriction is not well encapsulated, 
with various parts of Soyuz and Code hardcoding 'main' for PPAs.

This branch moves most of the logic into Archive. Archive.default_component now 
specifies the target override component for single-component archives, and 
Archive.getComponentsForSeries specifies the permitted set of components. Both 
are cached, so they can be called cheaply. Callsites no longer have to hardcode 
PPA-based checks, instead just checking the archive's permitted and default 
components.

I think I've replaced every use of 'main' outside archive.py and tests. 
However, some instances of 'universe' and 'multiverse' remain due to more 
complex component mapping rules that apply to the primary archive :(


Implementation details
----------------------

Archive.getComponentsForSeries needed to be quick, so I refactored it to wrap 
both Archive.default_component and DistroSeries.components, both of which are 
cachedproperties. DistroSeries.components is now a list instead of a ResultSet, 
because ResultSets are too lazy for caching to be effective.

I've added a new ArchivePurpose tuple, FULL_COMPONENT_SUPPORT, enumerating 
those purposes that have the full set of archive components (ie. not partner 
archives or PPAs).

A side-effect of this change is that all partner publications will be 
overridden to 'partner'. However, all publications to partner are meant to be 
in 'partner' anyway, and the lack of this override has caused problems in past, 
so this is a positive thing.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-701383-ppa-component-override/+merge/46221
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~wgrant/launchpad/bug-701383-ppa-component-override into lp:launchpad.
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2011-01-10 03:22:42 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2011-01-14 03:33:01 +0000
@@ -62,7 +62,6 @@
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.job.model.job import Job
-from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
 from lp.soyuz.model.buildfarmbuildjob import BuildFarmBuildJob
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
@@ -94,10 +93,11 @@
 
     @property
     def current_component(self):
-        # Since recipes only build into PPAs, they should always build
-        # in main. This will need to change once other archives are
-        # supported.
-        return getUtility(IComponentSet)['main']
+        # Only PPAs currently have a sane default component at the
+        # moment, but we only support recipes for PPAs.
+        component = self.archive.default_component
+        assert component is not None
+        return component
 
     distroseries_id = Int(name='distroseries', allow_none=True)
     distroseries = Reference(distroseries_id, 'DistroSeries.id')

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-12-21 00:26:41 +0000
+++ lib/lp/registry/model/distroseries.py	2011-01-14 03:33:01 +0000
@@ -270,18 +270,18 @@
             """ % self.id,
             clauseTables=["ComponentSelection"])
 
-    @property
+    @cachedproperty
     def components(self):
         """See `IDistroSeries`."""
         # XXX julian 2007-06-25
         # This is filtering out the partner component for now, until
         # the second stage of the partner repo arrives in 1.1.8.
-        return Component.select("""
+        return list(Component.select("""
             ComponentSelection.distroseries = %s AND
             Component.id = ComponentSelection.component AND
             Component.name != 'partner'
             """ % self.id,
-            clauseTables=["ComponentSelection"])
+            clauseTables=["ComponentSelection"]))
 
     @property
     def answers_usage(self):
@@ -1540,7 +1540,7 @@
             latest_releases = series.getCurrentSourceReleases(
                 packagenames)
             for spph in spphs:
-                latest_release = latest_releases.get(spph.meta_sourcepackage, None)
+                latest_release = latest_releases.get(spph.meta_sourcepackage)
                 if latest_release is not None and apt_pkg.VersionCompare(
                     latest_release.version, spph.source_package_version) > 0:
                     version = latest_release

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2011-01-03 22:15:46 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2011-01-14 03:33:01 +0000
@@ -21,6 +21,7 @@
     'CannotUploadToPPA',
     'CannotUploadToPocket',
     'DistroSeriesNotFound',
+    'FULL_COMPONENT_SUPPORT',
     'IArchive',
     'IArchiveAppend',
     'IArchiveEdit',
@@ -334,6 +335,10 @@
     debug_archive = Attribute(
         "The archive into which debug binaries should be uploaded.")
 
+    default_component = Attribute(
+        "The default component for this archive. Publications without a "
+        "valid component will be assigned this one.")
+
     archive_url = Attribute("External archive URL.")
 
     is_ppa = Attribute("True if this archive is a PPA.")
@@ -1670,4 +1675,10 @@
     ArchivePurpose.COPY,
     )
 
+FULL_COMPONENT_SUPPORT = (
+    ArchivePurpose.PRIMARY,
+    ArchivePurpose.DEBUG,
+    ArchivePurpose.COPY,
+    )
+
 # Circular dependency issues fixed in _schema_circular_imports.py

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-01-03 22:15:46 +0000
+++ lib/lp/soyuz/model/archive.py	2011-01-14 03:33:01 +0000
@@ -89,7 +89,10 @@
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.job.interfaces.job import JobStatus
-from lp.services.propertycache import get_property_cache
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.soyuz.adapters.archivedependencies import expand_dependencies
 from lp.soyuz.adapters.packagelocation import PackageLocation
 from lp.soyuz.enums import (
@@ -112,6 +115,7 @@
     CannotUploadToPPA,
     default_name_by_purpose,
     DistroSeriesNotFound,
+    FULL_COMPONENT_SUPPORT,
     IArchive,
     IArchiveSet,
     IDistributionArchive,
@@ -415,6 +419,16 @@
         else:
             return self
 
+    @cachedproperty
+    def default_component(self):
+        """See `IArchive`."""
+        if self.is_partner:
+            return getUtility(IComponentSet)['partner']
+        elif self.is_ppa:
+            return getUtility(IComponentSet)['main']
+        else:
+            return None
+
     @property
     def archive_url(self):
         """See `IArchive`."""
@@ -841,12 +855,13 @@
         return permission
 
     def getComponentsForSeries(self, distroseries):
-        if self.is_partner:
-            return [getUtility(IComponentSet)['partner']]
-        elif self.is_ppa:
-            return [getUtility(IComponentSet)['main']]
-        else:
+        """See `IArchive`."""
+        # DistroSeries.components and Archive.default_component are
+        # cachedproperties, so this is fairly cheap.
+        if self.purpose in FULL_COMPONENT_SUPPORT:
             return distroseries.components
+        else:
+            return [self.default_component]
 
     def updateArchiveCache(self):
         """See `IArchive`."""
@@ -948,9 +963,10 @@
                 raise ArchiveDependencyError(
                     "Non-primary archives only support the RELEASE pocket.")
             if (component is not None and
-                component.id is not getUtility(IComponentSet)['main'].id):
+                component != dependency.default_component):
                 raise ArchiveDependencyError(
-                    "Non-primary archives only support the 'main' component.")
+                    "Non-primary archives only support the '%s' component." %
+                    dependency.default_component.name)
 
         return ArchiveDependency(
             archive=self, dependency=dependency, pocket=pocket,
@@ -1086,7 +1102,7 @@
                 # interface will no longer require them because we can
                 # then relax the database constraint on
                 # ArchivePermission.
-                component_or_package = getUtility(IComponentSet)['main']
+                component_or_package = self.default_component
 
         # Flatly refuse uploads to copy archives, at least for now.
         if self.is_copy:
@@ -1234,8 +1250,10 @@
             else:
                 name = None
 
-            if name is None or name != 'main':
-                raise InvalidComponent("Component for PPAs should be 'main'")
+            if name != self.default_component.name:
+                raise InvalidComponent(
+                    "Component for PPAs should be '%s'" %
+                    self.default_component.name)
 
         permission_set = getUtility(IArchivePermissionSet)
         return permission_set.newComponentUploader(

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-12-16 20:01:09 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-01-14 03:33:01 +0000
@@ -83,7 +83,6 @@
     BuildSetStatus,
     IBinaryPackageBuildSet,
     )
-from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.publishing import (
     active_publishing_status,
     IBinaryPackageFilePublishing,
@@ -120,6 +119,21 @@
         'pool', poolify(source_name, component_name))
 
 
+def maybe_override_component(archive, distroseries, component):
+    """Override the component to fit in the archive, if possible.
+
+    If the archive has a default component, and it forbids use of the
+    requested component in the requested series, use the default.
+
+    If there is no default, just return the given component.
+    """
+    permitted_components = archive.getComponentsForSeries(distroseries)
+    if (component not in permitted_components and
+        archive.default_component is not None):
+        return archive.default_component
+    return component
+
+
 class FilePublishingBase:
     """Base class to publish files in the archive."""
 
@@ -817,7 +831,6 @@
         assert self.component in (
             self.archive.getComponentsForSeries(self.distroseries))
 
-
     def _proxied_urls(self, files, parent):
         """Run the files passed through `ProxiedLibraryFileAlias`."""
         return [
@@ -1244,19 +1257,10 @@
 
     def copyBinariesTo(self, binaries, distroseries, pocket, archive):
         """See `IPublishingSet`."""
-
-        # If the target archive is a ppa then we will need to override
-        # the component for each copy - so lookup the main component
-        # here once.
-        override_component = None
-        if archive.is_ppa:
-            override_component = getUtility(IComponentSet)['main']
-
         secure_copies = []
 
         for binary in binaries:
             binarypackagerelease = binary.binarypackagerelease
-            target_component = override_component or binary.component
 
             # XXX 2010-09-28 Julian bug=649859
             # This piece of code duplicates the logic in
@@ -1293,16 +1297,10 @@
                     distroarchseries=distroarchseries)
 
                 if binary_in_destination.count() == 0:
-                    pub = BinaryPackagePublishingHistory(
-                        archive=archive,
-                        binarypackagerelease=binarypackagerelease,
-                        distroarchseries=distroarchseries,
-                        component=target_component,
-                        section=binary.section,
-                        priority=binary.priority,
-                        status=PackagePublishingStatus.PENDING,
-                        datecreated=UTC_NOW,
-                        pocket=pocket)
+                    pub = self.newBinaryPublication(
+                        archive, binarypackagerelease, distroarchseries,
+                        binary.component, binary.section, binary.priority,
+                        pocket)
                     secure_copies.append(pub)
 
         return secure_copies
@@ -1311,15 +1309,12 @@
                              distroarchseries, component, section, priority,
                              pocket):
         """See `IPublishingSet`."""
-        if archive.is_ppa:
-            # PPA component must always be 'main', so we override it
-            # here.
-            component = getUtility(IComponentSet)['main']
         pub = BinaryPackagePublishingHistory(
             archive=archive,
             binarypackagerelease=binarypackagerelease,
             distroarchseries=distroarchseries,
-            component=component,
+            component=maybe_override_component(
+                archive, distroarchseries.distroseries, component),
             section=section,
             priority=priority,
             status=PackagePublishingStatus.PENDING,
@@ -1332,16 +1327,13 @@
                              distroseries, component, section, pocket,
                              ancestor=None):
         """See `IPublishingSet`."""
-        if archive.is_ppa:
-            # PPA component must always be 'main', so we override it
-            # here.
-            component = getUtility(IComponentSet)['main']
         pub = SourcePackagePublishingHistory(
             distroseries=distroseries,
             pocket=pocket,
             archive=archive,
             sourcepackagerelease=sourcepackagerelease,
-            component=component,
+            component=maybe_override_component(
+                archive, distroseries, component),
             section=section,
             status=PackagePublishingStatus.PENDING,
             datecreated=UTC_NOW,

=== modified file 'lib/lp/soyuz/scripts/initialise_distroseries.py'
--- lib/lp/soyuz/scripts/initialise_distroseries.py	2010-12-21 00:26:41 +0000
+++ lib/lp/soyuz/scripts/initialise_distroseries.py	2011-01-14 03:33:01 +0000
@@ -121,7 +121,7 @@
             raise InitialisationError(error)
         if self.distroseries.architectures.count():
             raise InitialisationError(error)
-        if self.distroseries.components.count():
+        if len(self.distroseries.components):
             raise InitialisationError(error)
         if self.distroseries.sections.count():
             raise InitialisationError(error)

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2010-12-13 06:44:16 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-01-14 03:33:01 +0000
@@ -27,6 +27,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.propertycache import clear_property_cache
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -1474,10 +1475,11 @@
         # The primary archive uses the series' defined components.
         archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
         self.assertEquals(
-            0, archive.getComponentsForSeries(self.series).count())
+            0, len(archive.getComponentsForSeries(self.series)))
 
         ComponentSelection(distroseries=self.series, component=self.comp1)
         ComponentSelection(distroseries=self.series, component=self.comp2)
+        clear_property_cache(self.series)
 
         self.assertEquals(
             set((self.comp1, self.comp2)),
@@ -1501,6 +1503,26 @@
             [main_comp], list(archive.getComponentsForSeries(self.series)))
 
 
+class TestDefaultComponent(TestCaseWithFactory):
+    """Tests for Archive.default_component."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_default_component_for_other_archives(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        self.assertIs(None, archive.default_component)
+
+    def test_default_component_for_partner(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PARTNER)
+        self.assertEquals(
+            getUtility(IComponentSet)['partner'], archive.default_component)
+
+    def test_default_component_for_ppas(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        self.assertEquals(
+            getUtility(IComponentSet)['main'], archive.default_component)
+
+
 class TestGetPockets(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Reply via email to