launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/latest-builds-failed-recipe-build into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #680733 broken recipe build gets stuck at top of "5 latest builds" list on recipe page, forever
  https://bugs.launchpad.net/bugs/680733

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/latest-builds-failed-recipe-build/+merge/48090

The query used to load the recipe build jobs was broken because it allowed failed builds to hang around at the top of the recent builds list and never expire. Details are in the bug 680733

== Implementation ==

Modify the getBuilds() query order by expression to order by greatest(date_finished, date_started). This allows expected ordering of builds which have started but failed to finish.

An extension to Storm was required - Storm does not provide a greatest() function. This was added into a new file lp.services.database.stormexpr.py. As a drive by, another similar Storm expression (CountDistinct) used in lp.code.model.recipebuild.py was also moved here.

== Tests ==

lp.code.browser.tests.test_sourcepackagerecipe.test_builds() - there were issues with this test. It was incorrectly trying to set a date_completed field on a recipe build job (should be date_finished) and in places was using set() to compare expected/actual build listings instead of lists (and thus the required ordering was not always being tested). Fixing these issues made the test as written fail (which is good) and then the test passed again when the fix was implemented.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py
  lib/lp/code/model/recipebuild.py
  lib/lp/code/model/sourcepackagerecipe.py
  lib/lp/services/database/stormexpr.py

./lib/lp/code/browser/tests/test_sourcepackagerecipe.py
     389: E501 line too long (80 characters)
     886: E501 line too long (85 characters)
     916: E501 line too long (85 characters)
     951: E501 line too long (85 characters)
     389: Line exceeds 78 characters.
     886: Line exceeds 78 characters.
     916: Line exceeds 78 characters.
     938: Line exceeds 78 characters.
     951: Line exceeds 78 characters.
./lib/lp/services/database/stormexpr.py
      21: E302 expected 2 blank lines, found 1
      28: E302 expected 2 blank lines, found 1


-- 
https://code.launchpad.net/~wallyworld/launchpad/latest-builds-failed-recipe-build/+merge/48090
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/latest-builds-failed-recipe-build into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-01-28 01:15:05 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-02-01 01:06:34 +0000
@@ -209,7 +209,11 @@
         """A list of interesting builds.
 
         All pending builds are shown, as well as 1-5 recent builds.
-        Recent builds are ordered by date completed.
+        Recent builds are ordered by date finished (if completed) or
+        date_started (if date finished is not set due to an error building or
+        other circumstance which resulted in the build not being completed).
+        This allows started but unfinished builds to show up in the view but
+        be discarded as more recent builds become available.
         """
         builds = list(self.context.getPendingBuilds())
         for build in self.context.getBuilds():

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-01-21 16:52:57 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-01 01:06:34 +0000
@@ -1007,21 +1007,23 @@
         build6 = self.makeBuildJob(recipe, date_gen.next())
         view = SourcePackageRecipeView(recipe, None)
         self.assertEqual(
-            set([build1, build2, build3, build4, build5, build6]),
-            set(view.builds))
+            [build1, build2, build3, build4, build5, build6],
+            view.builds)
 
         def set_status(build, status):
             naked_build = removeSecurityProxy(build)
             naked_build.status = status
             naked_build.date_started = naked_build.date_created
-            naked_build.date_completed = naked_build.date_created
+            if status == BuildStatus.FULLYBUILT:
+                naked_build.date_finished = (
+                    naked_build.date_created + timedelta(minutes=10))
         set_status(build1, BuildStatus.FULLYBUILT)
         set_status(build2, BuildStatus.FAILEDTOBUILD)
         # When there are 4+ pending builds, only the the most
         # recently-completed build is returned (i.e. build1, not build2)
         self.assertEqual(
-            set([build3, build4, build5, build6, build1]),
-            set(view.builds))
+            [build3, build4, build5, build6, build1],
+            view.builds)
         set_status(build3, BuildStatus.FULLYBUILT)
         set_status(build4, BuildStatus.FULLYBUILT)
         set_status(build5, BuildStatus.FULLYBUILT)

=== modified file 'lib/lp/code/model/recipebuild.py'
--- lib/lp/code/model/recipebuild.py	2010-12-14 05:29:37 +0000
+++ lib/lp/code/model/recipebuild.py	2011-02-01 01:06:34 +0000
@@ -18,9 +18,6 @@
 
 import pytz
 from storm.expr import (
-    compile,
-    EXPR,
-    Expr,
     Join,
     Max,
     Select)
@@ -39,6 +36,7 @@
 from lp.code.interfaces.recipebuild import IRecipeBuildRecordSet
 from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
+from lp.services.database.stormexpr import CountDistinct
 from lp.registry.model.person import Person
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.soyuz.model.archive import Archive
@@ -147,24 +145,6 @@
             result_set, _makeRecipeBuildRecord)
 
 
-# XXX: wallyworld 2010-11-26 bug=675377: storm's Count() implementation is
-# broken for distinct with > 1 column
-class CountDistinct(Expr):
-
-    __slots__ = ("columns")
-
-    def __init__(self, columns):
-        self.columns = columns
-
-
-@compile.when(CountDistinct)
-def compile_countdistinct(compile, countselect, state):
-    state.push("context", EXPR)
-    col = compile(countselect.columns)
-    state.pop()
-    return "count(distinct(%s))" % col
-
-
 class RecipeBuildRecordResultSet(DecoratedResultSet):
     """A ResultSet which can count() queries with group by."""
 

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-01-14 05:46:14 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-02-01 01:06:34 +0000
@@ -55,6 +55,7 @@
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.model.distroseries import DistroSeries
+from lp.services.database.stormexpr import Greatest
 from lp.soyuz.interfaces.archive import IArchiveSet
 from lp.soyuz.interfaces.component import IComponentSet
 
@@ -249,7 +250,9 @@
     def getBuilds(self):
         """See `ISourcePackageRecipe`."""
         where_clause = BuildFarmJob.status != BuildStatus.NEEDSBUILD
-        order_by = Desc(BuildFarmJob.date_finished), BuildFarmJob.id
+        order_by = Desc(Greatest(
+                BuildFarmJob.date_started,
+                BuildFarmJob.date_finished)), BuildFarmJob.id
         return self._getBuilds(where_clause, order_by)
 
     def getPendingBuilds(self):

=== added file 'lib/lp/services/database/stormexpr.py'
--- lib/lp/services/database/stormexpr.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/stormexpr.py	2011-02-01 01:06:34 +0000
@@ -0,0 +1,41 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+__all__ = [
+    'CountDistinct',
+    'Greatest',
+    ]
+
+from storm.expr import (
+    compile,
+    EXPR,
+    Expr,
+    NamedFunc,
+    )
+
+
+# XXX wallyworld 2011-01-31 bug=710466:
+# We need to use a Postgres greatest() function call but Storm doesn't
+# support that yet.
+class Greatest(NamedFunc):
+    __slots__ = ()
+    name = "GREATEST"
+
+
+# XXX: wallyworld 2010-11-26 bug=675377:
+# storm's Count() implementation is broken for distinct with > 1 column
+class CountDistinct(Expr):
+
+    __slots__ = ("columns")
+
+    def __init__(self, columns):
+        self.columns = columns
+
+
+@compile.when(CountDistinct)
+def compile_countdistinct(compile, countselect, state):
+    state.push("context", EXPR)
+    col = compile(countselect.columns)
+    state.pop()
+    return "count(distinct(%s))" % col