Merge lp:~michael.nelson/launchpad/429318-copy-archives-timeout into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= Summary =

This branch aims to fix bug 429318 by replacing the code used for
ArchiveView.num_pkgs_building with storm-code, so that (1) the unused
prefetch data is not included in the result, and (2) the builds do not
need to be iterated.

== Proposed fix ==

See the description on bug 429318.

== Pre-implementation notes ==

I'm not sure if selecting the SourcePackageReleases is the right thing -
 I'm only using them to count etc. I could also use
SourcePackagePublishingHistory - but (1) it would be an extra join, and
(2) there are already methods on IArchive for this (that do not enable
me to do what I need here.)

== Implementation details ==

I've had to use removeSecurityProxy to be able to get to the difference
method of ResultSet - this is because the interface at
zope.storm.interfaces.IResultSet does not include difference, or
_find_spec (which is accessed by the difference method).

I'll create a bug for this and XXX it.

== Tests ==

bin/test -vv -t archive-views.txt -t TestGetSourcePackageReleases

== Demo and Q/A ==

No UI change.

To Q/A:
https://edge.launchpad.net/ubuntu/+archive/test-rebuild-20090909/+index

(currently times out).

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/interfaces/archive.py
  lib/lp/soyuz/model/archive.py
  lib/lp/soyuz/browser/archive.py

== Pylint notices ==

lib/lp/soyuz/interfaces/archive.py
    38: [F0401] Unable to import 'lazr.enum' (No module named enum)
    51: [F0401] Unable to import 'lazr.restful.declarations' (No module
named restful)
    57: [F0401] Unable to import 'lazr.restful.fields' (No module named
restful)

lib/lp/soyuz/model/archive.py
    14: [F0401] Unable to import 'lazr.lifecycle.event' (No module named
lifecycle)

--
Michael

[Julian Edwards (julian-edwards)]

 review needs-fixing

Thanks for this fix Michael. I have the usual comments in-line.

On Wednesday 16 September 2009 16:55:13 Michael Nelson wrote:
> I'm not sure if selecting the SourcePackageReleases is the right thing -
> I'm only using them to count etc. I could also use
> SourcePackagePublishingHistory - but (1) it would be an extra join, and
> (2) there are already methods on IArchive for this (that do not enable
> me to do what I need here.)

I think it's fine - in fact the old way of doing it was pretty inefficient
even without the prejoin problem.

> I've had to use removeSecurityProxy to be able to get to the difference
> method of ResultSet - this is because the interface at
> zope.storm.interfaces.IResultSet does not include difference, or
> _find_spec (which is accessed by the difference method).
>
> I'll create a bug for this and XXX it.

Ok, thanks.

>+ def getSourcePackageReleases(buildstatus=None):

Can you make this build_status please.

>+ """Return the releases for this archive.
>+
>+ :param buildstatus: If specified, only the distinct releases with
>+ builds in the specified build status will be returned.
>+ :return: A `ResultSet` of distinct `SourcePackageReleases` for this
>+ archive.
>+ """
>

>+ def getSourcePackageReleases(self, buildstatus=None):

And here of course.

>+ """See `IArchive`."""
>+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)

Is using the main store deliberate? I would have thought
 store = Store.of(self)
should suffice?

>+
>+ extra_exprs = []
>+ if buildstatus is not None:
>+ extra_exprs.append(Build.buildstate == buildstatus)
>+
>+ result_set = store.find(SourcePackageRelease,

PEP-8 - line feed after the open paren. please.

>+ Build.sourcepackagereleaseID == SourcePackageRelease.id,
>+ Build.archive == self,
>+ *extra_exprs)
>+
>+ result_set.config(distinct=True).order_by(SourcePackageRelease.id)
>+ return result_set
>
> class ArchiveSet:
> implements(IArchiveSet)
>
>=== modified file 'lib/lp/soyuz/tests/test_archive.py'
>--- lib/lp/soyuz/tests/test_archive.py 2009-07-27 14:45:01 +0000
>+++ lib/lp/soyuz/tests/test_archive.py 2009-09-16 15:22:15 +0000
>@@ -14,6 +14,7 @@
> from lp.registry.interfaces.distribution import IDistributionSet
> from lp.soyuz.interfaces.archive import IArchiveSet
> from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
>+from lp.soyuz.interfaces.build import BuildStatus
> from lp.soyuz.interfaces.publishing import PackagePublishingStatus
> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> from lp.testing import TestCaseWithFactory
>@@ -243,5 +244,54 @@
> [u'1.0', u'0.5'], versions,
> "The latest version was not first.")
>
>+
>+class TestGetSourcePackageReleases(TestCaseWithFactory):
>+
>+ layer = LaunchpadZopelessLayer
>+
>+ def setUp(self):
>+ super(TestGetSourcePackageReleases, self).setUp()
>+ self.publisher = SoyuzTestPublisher()
>+ self.publisher.prepareBreezyAutotest()
>+
>+ # Create an archive wit...

review: Needs Fixing

[Michael Nelson (michael.nelson)]

Julian Edwards wrote:
> Review: Needs Fixing
> review needs-fixing
>
> Thanks for this fix Michael. I have the usual comments in-line.
>
> On Wednesday 16 September 2009 16:55:13 Michael Nelson wrote:
>> I'm not sure if selecting the SourcePackageReleases is the right thing -
>> I'm only using them to count etc. I could also use
>> SourcePackagePublishingHistory - but (1) it would be an extra join, and
>> (2) there are already methods on IArchive for this (that do not enable
>> me to do what I need here.)
>
> I think it's fine - in fact the old way of doing it was pretty inefficient
> even without the prejoin problem.

Yes - I was only thinking of PPAs when I wrote it. So I'm all for the
new version, I just wasn't sure if selecting SourcePackageReleases was
the best way.

>
>> I've had to use removeSecurityProxy to be able to get to the difference
>> method of ResultSet - this is because the interface at
>> zope.storm.interfaces.IResultSet does not include difference, or
>> _find_spec (which is accessed by the difference method).
>>
>> I'll create a bug for this and XXX it.
>
> Ok, thanks.
>

Added XXX with bug 431203.

>> + def getSourcePackageReleases(buildstatus=None):
>
> Can you make this build_status please.
>

Done.

>> + """Return the releases for this archive.
>> +
>> + :param buildstatus: If specified, only the distinct releases with
>> + builds in the specified build status will be returned.
>> + :return: A `ResultSet` of distinct `SourcePackageReleases` for this
>> + archive.
>> + """
>>
>
>> + def getSourcePackageReleases(self, buildstatus=None):
>
> And here of course.

Yep.

>
>> + """See `IArchive`."""
>> + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
>
> Is using the main store deliberate? I would have thought
> store = Store.of(self)
> should suffice?

Done.

>
>> +
>> + extra_exprs = []
>> + if buildstatus is not None:
>> + extra_exprs.append(Build.buildstate == buildstatus)
>> +
>> + result_set = store.find(SourcePackageRelease,
>
> PEP-8 - line feed after the open paren. please.

Gar - sorry.

>
>> + Build.sourcepackagereleaseID == SourcePackageRelease.id,
>> + Build.archive == self,
>> + *extra_exprs)
>> +
>> + result_set.config(distinct=True).order_by(SourcePackageRelease.id)
>> + return result_set
>>
>> class ArchiveSet:
>> implements(IArchiveSet)
>>
>> === modified file 'lib/lp/soyuz/tests/test_archive.py'
>> --- lib/lp/soyuz/tests/test_archive.py 2009-07-27 14:45:01 +0000
>> +++ lib/lp/soyuz/tests/test_archive.py 2009-09-16 15:22:15 +0000
>> @@ -14,6 +14,7 @@
>> from lp.registry.interfaces.distribution import IDistributionSet
>> from lp.soyuz.interfaces.archive import IArchiveSet
>> from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
>> +from lp.soyuz.interfaces.build import BuildStatus
>> from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>> from lp.testing import TestCaseWithFactory
>> @@ -243,5 +244,54 @@
>> ...

[Julian Edwards (julian-edwards)]

 merge approve
 review approve

On Thursday 17 September 2009 08:18:08 Michael Nelson wrote:
> Yes - I was only thinking of PPAs when I wrote it. So I'm all for the
> new version, I just wasn't sure if selecting SourcePackageReleases was
> the best way.

Well I was thinking of the previous use of Python sets rather than distinct
SQL clauses :)

> Yes, I wonder whether it would even be worthwhile splitting the content
> class up using mixins to match the documentation. Also think it would be
> great to improve the documentation by moving lots of the unit tests
> there to actual python unittests, leaving the documentation much more
> readable. But I think that would take planning and time?

Yes, it's certainly something for a rainy day.

For me, doctests should contain basic instructions on how to use the content
class and its methods. Testing corner cases belong in a unit test, but I
don't think your new test is that.

Our problem is that Soyuz depends heavily on setting up a lot of data, so
doctests tend to be full of calls to the STP and factory, which muddies the
story, so I can see why you made it a unit test.

Ho hum.

> For the moment, I've added a brief section in archive.txt with a
> reference to the unit test. See what you think.

It's fine, thanks.

Land it!

review: Approve

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2009-09-11 17:55:23 +0000
+++ lib/lp/soyuz/browser/archive.py	2009-09-16 15:38:07 +0000
@@ -33,6 +33,7 @@
 from zope.component import getUtility
 from zope.formlib import form
 from zope.interface import implements, Interface
+from zope.security.proxy import removeSecurityProxy
 from zope.schema import Choice, List, TextLine
 from zope.schema.interfaces import IContextSourceBinder
 from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm
@@ -850,23 +851,24 @@
     def num_pkgs_building(self):
         """Return the number of building/waiting to build packages."""
 
-        building = getUtility(IBuildSet).getBuildsForArchive(
-            self.context, status=BuildStatus.BUILDING)
-        needs_build = getUtility(IBuildSet).getBuildsForArchive(
-            self.context, status=BuildStatus.NEEDSBUILD)
-
-        # Create a set of all source package releases that have builds
-        # waiting to build, as well as a set of those with building builds.
-        needs_build_set = set(
-            build.sourcepackagerelease for build in needs_build)
-
-        building_set = set(
-            build.sourcepackagerelease for build in building)
+        sprs_building = self.context.getSourcePackageReleases(
+            buildstatus = BuildStatus.BUILDING)
+        sprs_waiting = self.context.getSourcePackageReleases(
+            buildstatus = BuildStatus.NEEDSBUILD)
+
+        pkgs_building_count = sprs_building.count()
 
         # A package is not counted as waiting if it already has at least
         # one build building.
-        pkgs_building_count = len(building_set)
-        pkgs_waiting_count = len(needs_build_set.difference(building_set))
+        # Note: because neither the 'difference' method or the '_find_spec'
+        # property are exposed via storm.zope.interfaces.IResultSet,
+        # we need to remove the proxy for both results to use
+        # the difference method.
+        naked_sprs_waiting = removeSecurityProxy(sprs_waiting)
+        naked_sprs_building = removeSecurityProxy(sprs_building)
+
+        pkgs_waiting_count = naked_sprs_waiting.difference(
+            naked_sprs_building).count()
 
         # The total is just used for conditionals in the template.
         return {

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2009-09-02 21:28:11 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2009-09-16 15:22:15 +0000
@@ -739,6 +739,14 @@
         :return: True if the person is allowed to upload the source package.
         """
 
+    def getSourcePackageReleases(buildstatus=None):
+        """Return the releases for this archive.
+
+        :param buildstatus: If specified, only the distinct releases with
+            builds in the specified build status will be returned.
+        :return: A `ResultSet` of distinct `SourcePackageReleases` for this
+            archive.
+        """
 
 class IArchiveView(IHasBuildRecords):
     """Archive interface for operations restricted by view privilege."""

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2009-09-10 12:59:56 +0000
+++ lib/lp/soyuz/model/archive.py	2009-09-16 15:22:15 +0000
@@ -1186,6 +1186,21 @@
 
         return subscription
 
+    def getSourcePackageReleases(self, buildstatus=None):
+        """See `IArchive`."""
+        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+
+        extra_exprs = []
+        if buildstatus is not None:
+            extra_exprs.append(Build.buildstate == buildstatus)
+
+        result_set = store.find(SourcePackageRelease, 
+            Build.sourcepackagereleaseID == SourcePackageRelease.id,
+            Build.archive == self,
+            *extra_exprs)
+
+        result_set.config(distinct=True).order_by(SourcePackageRelease.id)
+        return result_set
 
 class ArchiveSet:
     implements(IArchiveSet)

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2009-07-27 14:45:01 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2009-09-16 15:22:15 +0000
@@ -14,6 +14,7 @@
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.soyuz.interfaces.archive import IArchiveSet
 from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
+from lp.soyuz.interfaces.build import BuildStatus
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import TestCaseWithFactory
@@ -243,5 +244,54 @@
             [u'1.0', u'0.5'], versions,
             "The latest version was not first.")
 
+
+class TestGetSourcePackageReleases(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestGetSourcePackageReleases, self).setUp()
+        self.publisher = SoyuzTestPublisher()
+        self.publisher.prepareBreezyAutotest()
+
+        # Create an archive with some published binaries.
+        self.archive = self.factory.makeArchive()
+        binaries_foo = self.publisher.getPubBinaries(
+            archive=self.archive, binaryname="foo-bin")
+        binaries_bar = self.publisher.getPubBinaries(
+            archive=self.archive, binaryname="bar-bin")
+
+        # Collect the builds for reference.
+        self.builds_foo = [
+            binary.binarypackagerelease.build for binary in binaries_foo]
+        self.builds_bar = [
+            binary.binarypackagerelease.build for binary in binaries_bar]
+
+        # Collect the source package releases for reference.
+        self.sourcepackagereleases = [
+            self.builds_foo[0].sourcepackagerelease,
+            self.builds_bar[0].sourcepackagerelease,
+            ]
+
+    def test_getSourcePackageReleases_with_no_params(self):
+        # With no params all source package releases are returned.
+        sprs = self.archive.getSourcePackageReleases()
+
+        self.assertContentEqual(self.sourcepackagereleases, sprs)
+
+    def test_getSourcePackageReleases_with_buildstatus(self):
+        # Results are filtered by the specified buildstatus.
+
+        # Set the builds for one of the sprs to needs build.
+        for build in self.builds_foo:
+            build.buildstate = BuildStatus.NEEDSBUILD
+
+        result = self.archive.getSourcePackageReleases(
+            buildstatus=BuildStatus.NEEDSBUILD)
+
+        self.failUnlessEqual(1, result.count())
+        self.failUnlessEqual(
+            self.sourcepackagereleases[0], result[0])
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)