Merge lp:~stevenk/launchpad/queue-recipes-better into lp:launchpad
- queue-recipes-better
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robert Collins | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12298 | ||||
Proposed branch: | lp:~stevenk/launchpad/queue-recipes-better | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
316 lines (+128/-43) 4 files modified
lib/lp/code/model/sourcepackagerecipe.py (+30/-13) lib/lp/code/model/sourcepackagerecipebuild.py (+7/-4) lib/lp/code/model/tests/test_sourcepackagerecipe.py (+16/-0) lib/lp/code/model/tests/test_sourcepackagerecipebuild.py (+75/-26) |
||||
To merge this branch: | bzr merge lp:~stevenk/launchpad/queue-recipes-better | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code* | Approve | |
Robert Collins (community) | Approve | ||
Review via email: mp+47358@code.launchpad.net |
Commit message
[r=lifeless,
Description of the change
Change findStaleDailyB
Steve Kowalik (stevenk) wrote : | # |
The point of this branch is to foster work to getting the request_
Robert Collins (lifeless) wrote : | # |
On Tue, Jan 25, 2011 at 6:43 PM, Steve Kowalik <email address hidden> wrote:
> The point of this branch is to foster work to getting the request_
That implies that we'll be considering builds which are not
interesting to build, which is inefficient. I don't think this is a
useful step towards where you want to get. Better to only iterate
recipes which are interesting to build on a given run - which won't
need any of the changes of this patch, and will be sufficient in and
of itself.
Seperately, remember than changing something takes time - a 24 hour
interval isn't 24 hours, its really 24 hours - the time we take to do
stuff - otherwise folk will get built one day, not the next, and so
on.
-Rob
William Grant (wgrant) wrote : | # |
Thanks for this change, Steve. However, findStaleDailyB
A few other largely stylistic comments:
- requestBuild should use Archive.
- The join is unreadable; could you please fix up the indentation?
- Why use a RightJoin instead of a LeftJoin? There is not a single other RightJoin callsite in the tree.
- There is some trailing whitespace in findStaleDailyB
Robert Collins (lifeless) wrote : | # |
Mentor review.
I think this is fine to land with a few small tweaks.
William: meta on the review - we try not to escalate the work done in a branch except to avoid regressions; in particular the defects about series and archive are already present in the tree; they are neutral as far as this branch is concerned. I agree that they are defects, but not new to the branch and so deferrable. When possible suggest actual fixes with diffs/something to copy and paste in. Lastly some of the prose could be improved - comments in particular can bitrot so its worth making sure they really are clear.
As for the right join, thats because this is appropriate; we have a really weird one-object,
Steven: I agree about the join indentation. How about this:
+ joins = RightJoin(
+ Join(
+ Join(SourcePack
+ PackageBuild.id == SourcePackageRe
+ BuildFarmJob,
+ And(BuildFarmJob.id == PackageBuild.
+ BuildFarmJob.
+ SourcePackageRe
+ SourcePackageRe
200 + # makeDailyBuilds() won't create a build if the recipe isn't stale.
->
200 + # only stale builds cause makeDailyBuilds() to make new builds
William Grant (wgrant) wrote : | # |
The defects are new, since requesting a single build did not previously unset is_stale.
Robert Collins (lifeless) wrote : | # |
On Mon, Jan 31, 2011 at 4:13 PM, William Grant <email address hidden> wrote:
> The defects are new, since requesting a single build did not previously unset is_stale.
Good catch, we do need to fix them - sorry that I misunderstood.
William Grant (wgrant) wrote : | # |
I'd personally prefer it to be indented like http://
Preview Diff
1 | === modified file 'lib/lp/code/model/sourcepackagerecipe.py' |
2 | --- lib/lp/code/model/sourcepackagerecipe.py 2011-01-14 05:46:14 +0000 |
3 | +++ lib/lp/code/model/sourcepackagerecipe.py 2011-02-01 01:40:41 +0000 |
4 | @@ -11,7 +11,17 @@ |
5 | 'SourcePackageRecipe', |
6 | ] |
7 | |
8 | +from datetime import ( |
9 | + datetime, |
10 | + timedelta, |
11 | + ) |
12 | from lazr.delegates import delegates |
13 | +from pytz import utc |
14 | +from storm.expr import ( |
15 | + And, |
16 | + Join, |
17 | + RightJoin, |
18 | + ) |
19 | from storm.locals import ( |
20 | Bool, |
21 | Desc, |
22 | @@ -56,7 +66,6 @@ |
23 | from lp.registry.interfaces.distroseries import IDistroSeriesSet |
24 | from lp.registry.model.distroseries import DistroSeries |
25 | from lp.soyuz.interfaces.archive import IArchiveSet |
26 | -from lp.soyuz.interfaces.component import IComponentSet |
27 | |
28 | |
29 | def get_buildable_distroseries_set(user): |
30 | @@ -178,8 +187,22 @@ |
31 | |
32 | @classmethod |
33 | def findStaleDailyBuilds(cls): |
34 | - store = IStore(cls) |
35 | - return store.find(cls, cls.is_stale == True, cls.build_daily == True) |
36 | + one_day_ago = datetime.now(utc) - timedelta(hours=23, minutes=50) |
37 | + joins = RightJoin( |
38 | + Join( |
39 | + Join(SourcePackageRecipeBuild, PackageBuild, |
40 | + PackageBuild.id == |
41 | + SourcePackageRecipeBuild.package_build_id), |
42 | + BuildFarmJob, |
43 | + And(BuildFarmJob.id == PackageBuild.build_farm_job_id, |
44 | + BuildFarmJob.date_created > one_day_ago)), |
45 | + SourcePackageRecipe, |
46 | + And(SourcePackageRecipeBuild.recipe == SourcePackageRecipe.id, |
47 | + SourcePackageRecipe.daily_build_archive_id == |
48 | + PackageBuild.archive_id)) |
49 | + return IStore(cls).using(joins).find( |
50 | + cls, cls.is_stale == True, cls.build_daily == True, |
51 | + BuildFarmJob.date_created == None).config(distinct=True) |
52 | |
53 | @staticmethod |
54 | def exists(owner, name): |
55 | @@ -216,14 +239,14 @@ |
56 | raise ValueError('Source package recipe builds disabled.') |
57 | if not archive.is_ppa: |
58 | raise NonPPABuildRequest |
59 | - component = getUtility(IComponentSet)["multiverse"] |
60 | |
61 | buildable_distros = get_buildable_distroseries_set(archive.owner) |
62 | if distroseries not in buildable_distros: |
63 | raise BuildNotAllowedForDistro(self, distroseries) |
64 | |
65 | reject_reason = archive.checkUpload( |
66 | - requester, self.distroseries, None, component, pocket) |
67 | + requester, self.distroseries, None, archive.default_component, |
68 | + pocket) |
69 | if reject_reason is not None: |
70 | raise reject_reason |
71 | if self.isOverQuota(requester, distroseries): |
72 | @@ -271,14 +294,8 @@ |
73 | |
74 | def getLastBuild(self): |
75 | """See `ISourcePackageRecipeBuild`.""" |
76 | - store = Store.of(self) |
77 | - result = store.find( |
78 | - (SourcePackageRecipeBuild), |
79 | - SourcePackageRecipeBuild.recipe == self, |
80 | - SourcePackageRecipeBuild.package_build_id == PackageBuild.id, |
81 | - PackageBuild.build_farm_job_id == BuildFarmJob.id) |
82 | - result.order_by(Desc(BuildFarmJob.date_finished)) |
83 | - return result.first() |
84 | + return self._getBuilds( |
85 | + True, Desc(BuildFarmJob.date_finished)).first() |
86 | |
87 | def getMedianBuildDuration(self): |
88 | """Return the median duration of builds of this recipe.""" |
89 | |
90 | === modified file 'lib/lp/code/model/sourcepackagerecipebuild.py' |
91 | --- lib/lp/code/model/sourcepackagerecipebuild.py 2011-01-20 22:01:29 +0000 |
92 | +++ lib/lp/code/model/sourcepackagerecipebuild.py 2011-02-01 01:40:41 +0000 |
93 | @@ -10,7 +10,10 @@ |
94 | 'SourcePackageRecipeBuild', |
95 | ] |
96 | |
97 | -import datetime |
98 | +from datetime import ( |
99 | + datetime, |
100 | + timedelta, |
101 | + ) |
102 | import logging |
103 | import sys |
104 | |
105 | @@ -259,9 +262,9 @@ |
106 | def getRecentBuilds(cls, requester, recipe, distroseries, _now=None): |
107 | from lp.buildmaster.model.buildfarmjob import BuildFarmJob |
108 | if _now is None: |
109 | - _now = datetime.datetime.now(utc) |
110 | + _now = datetime.now(utc) |
111 | store = IMasterStore(SourcePackageRecipeBuild) |
112 | - old_threshold = _now - datetime.timedelta(days=1) |
113 | + old_threshold = _now - timedelta(days=1) |
114 | return store.find(cls, cls.distroseries_id == distroseries.id, |
115 | cls.requester_id == requester.id, cls.recipe_id == recipe.id, |
116 | BuildFarmJob.date_created > old_threshold, |
117 | @@ -282,7 +285,7 @@ |
118 | median = self.recipe.getMedianBuildDuration() |
119 | if median is not None: |
120 | return median |
121 | - return datetime.timedelta(minutes=10) |
122 | + return timedelta(minutes=10) |
123 | |
124 | def verifySuccessfulUpload(self): |
125 | return self.source_package_release is not None |
126 | |
127 | === modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py' |
128 | --- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2011-01-13 03:53:53 +0000 |
129 | +++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2011-02-01 01:40:41 +0000 |
130 | @@ -561,6 +561,22 @@ |
131 | self.assertContentEqual([stale_daily], |
132 | SourcePackageRecipe.findStaleDailyBuilds()) |
133 | |
134 | + def test_findStaleDailyBuildsDistinct(self): |
135 | + # If a recipe has 2 builds due to 2 distroseries, it only returns |
136 | + # one recipe. |
137 | + recipe = self.factory.makeSourcePackageRecipe( |
138 | + build_daily=True, is_stale=True) |
139 | + hoary = self.factory.makeSourcePackageRecipeDistroseries("hoary") |
140 | + recipe.distroseries.add(hoary) |
141 | + for series in recipe.distroseries: |
142 | + build = recipe.requestBuild( |
143 | + recipe.daily_build_archive, recipe.owner, |
144 | + series, PackagePublishingPocket.RELEASE) |
145 | + removeSecurityProxy(build).date_created = ( |
146 | + datetime.now(UTC) - timedelta(hours=24, seconds=1)) |
147 | + stale_recipes = SourcePackageRecipe.findStaleDailyBuilds() |
148 | + self.assertEqual([recipe], list(stale_recipes)) |
149 | + |
150 | def test_getMedianBuildDuration(self): |
151 | |
152 | def set_duration(build, minutes): |
153 | |
154 | === modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py' |
155 | --- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py 2011-01-20 22:01:29 +0000 |
156 | +++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py 2011-02-01 01:40:41 +0000 |
157 | @@ -5,9 +5,12 @@ |
158 | |
159 | __metaclass__ = type |
160 | |
161 | -import datetime |
162 | +from datetime import ( |
163 | + datetime, |
164 | + timedelta, |
165 | + ) |
166 | +from pytz import utc |
167 | import re |
168 | -import unittest |
169 | |
170 | from storm.locals import Store |
171 | import transaction |
172 | @@ -177,16 +180,13 @@ |
173 | # If there are no successful builds, estimate 10 minutes. |
174 | spb = self.makeSourcePackageRecipeBuild() |
175 | cur_date = self.factory.getUniqueDate() |
176 | - self.assertEqual( |
177 | - datetime.timedelta(minutes=10), spb.estimateDuration()) |
178 | + self.assertEqual(timedelta(minutes=10), spb.estimateDuration()) |
179 | for minutes in [20, 5, 1]: |
180 | build = removeSecurityProxy( |
181 | self.factory.makeSourcePackageRecipeBuild(recipe=spb.recipe)) |
182 | build.date_started = cur_date |
183 | - build.date_finished = ( |
184 | - cur_date + datetime.timedelta(minutes=minutes)) |
185 | - self.assertEqual( |
186 | - datetime.timedelta(minutes=5), spb.estimateDuration()) |
187 | + build.date_finished = cur_date + timedelta(minutes=minutes) |
188 | + self.assertEqual(timedelta(minutes=5), spb.estimateDuration()) |
189 | |
190 | def test_getFileByName(self): |
191 | """getFileByName returns the logs when requested by name.""" |
192 | @@ -269,20 +269,74 @@ |
193 | SourcePackageRecipeBuild.makeDailyBuilds()[0] |
194 | self.assertFalse(recipe.is_stale) |
195 | |
196 | - def test_makeDailyBuilds_skips_pending(self): |
197 | - """When creating daily builds, skip ones that are already pending.""" |
198 | + def test_makeDailyBuilds_skips_if_built_in_last_24_hours(self): |
199 | + # We won't create a build during makeDailyBuilds() if the recipe |
200 | + # has been built in the last 24 hours. |
201 | recipe = self.factory.makeSourcePackageRecipe( |
202 | build_daily=True, is_stale=True) |
203 | - first_distroseries = list(recipe.distroseries)[0] |
204 | recipe.requestBuild( |
205 | - recipe.daily_build_archive, recipe.owner, first_distroseries, |
206 | + recipe.daily_build_archive, recipe.owner, |
207 | + list(recipe.distroseries)[0], PackagePublishingPocket.RELEASE) |
208 | + daily_builds = SourcePackageRecipeBuild.makeDailyBuilds() |
209 | + self.assertEqual([], daily_builds) |
210 | + |
211 | + def test_makeDailyBuilds_skips_non_stale_builds(self): |
212 | + # If the recipe isn't stale, makeDailyBuilds() won't create a build. |
213 | + self.factory.makeSourcePackageRecipe( |
214 | + build_daily=True, is_stale=False) |
215 | + daily_builds = SourcePackageRecipeBuild.makeDailyBuilds() |
216 | + self.assertEqual([], daily_builds) |
217 | + |
218 | + def test_makeDailyBuilds_with_an_older_build(self): |
219 | + # If a previous build is more than 24 hours old, and the recipe is |
220 | + # stale, we'll fire another off. |
221 | + recipe = self.factory.makeSourcePackageRecipe( |
222 | + build_daily=True, is_stale=True) |
223 | + build = recipe.requestBuild( |
224 | + recipe.daily_build_archive, recipe.owner, |
225 | + list(recipe.distroseries)[0], PackagePublishingPocket.RELEASE) |
226 | + nb = removeSecurityProxy(build) |
227 | + nb.date_created = datetime.now(utc) - timedelta(hours=24, seconds=1) |
228 | + # The build also needs to be completed |
229 | + nb.status = BuildStatus.FULLYBUILT |
230 | + daily_builds = SourcePackageRecipeBuild.makeDailyBuilds() |
231 | + self.assertEquals(1, len(daily_builds)) |
232 | + actual_title = [b.title for b in daily_builds] |
233 | + self.assertEquals([build.title], actual_title) |
234 | + |
235 | + def test_makeDailyBuilds_with_an_older_and_newer_build(self): |
236 | + # If a recipe has been built twice, and the most recent build is |
237 | + # within 24 hours, makeDailyBuilds() won't create a build. |
238 | + recipe = self.factory.makeSourcePackageRecipe( |
239 | + build_daily=True, is_stale=True) |
240 | + for timediff in (timedelta(hours=24, seconds=1), timedelta(hours=8)): |
241 | + build = recipe.requestBuild( |
242 | + recipe.daily_build_archive, recipe.owner, |
243 | + list(recipe.distroseries)[0], |
244 | + PackagePublishingPocket.RELEASE) |
245 | + nb = removeSecurityProxy(build) |
246 | + nb.date_created = datetime.now(utc) - timediff |
247 | + # The build also needs to be completed |
248 | + nb.status = BuildStatus.FULLYBUILT |
249 | + daily_builds = SourcePackageRecipeBuild.makeDailyBuilds() |
250 | + self.assertEquals([], list(daily_builds)) |
251 | + |
252 | + def test_makeDailyBuilds_with_new_build_different_archive(self): |
253 | + # If a recipe has been built into an archive that isn't the |
254 | + # daily_build_archive, we will create a build. |
255 | + recipe = self.factory.makeSourcePackageRecipe( |
256 | + build_daily=True, is_stale=True) |
257 | + archive = self.factory.makeArchive(owner=recipe.owner) |
258 | + build = recipe.requestBuild( |
259 | + archive, recipe.owner, list(recipe.distroseries)[0], |
260 | PackagePublishingPocket.RELEASE) |
261 | - second_distroseries = \ |
262 | - self.factory.makeSourcePackageRecipeDistroseries("hoary") |
263 | - recipe.distroseries.add(second_distroseries) |
264 | - builds = SourcePackageRecipeBuild.makeDailyBuilds() |
265 | - self.assertEqual( |
266 | - [second_distroseries], [build.distroseries for build in builds]) |
267 | + nb = removeSecurityProxy(build) |
268 | + nb.date_created = datetime.now(utc) - timedelta(hours=8) |
269 | + # The build also needs to be completed |
270 | + nb.status = BuildStatus.FULLYBUILT |
271 | + daily_builds = SourcePackageRecipeBuild.makeDailyBuilds() |
272 | + actual_title = [b.title for b in daily_builds] |
273 | + self.assertEquals([build.title], actual_title) |
274 | |
275 | def test_getRecentBuilds(self): |
276 | """Recent builds match the same person, series and receipe. |
277 | @@ -306,12 +360,12 @@ |
278 | return SourcePackageRecipeBuild.getRecentBuilds( |
279 | requester, recipe, series, _now=now) |
280 | self.assertContentEqual([], get_recent()) |
281 | - yesterday = now - datetime.timedelta(days=1) |
282 | + yesterday = now - timedelta(days=1) |
283 | recent_build = self.factory.makeSourcePackageRecipeBuild( |
284 | recipe=recipe, distroseries=series, requester=requester, |
285 | date_created=yesterday) |
286 | self.assertContentEqual([], get_recent()) |
287 | - a_second = datetime.timedelta(seconds=1) |
288 | + a_second = timedelta(seconds=1) |
289 | removeSecurityProxy(recent_build).date_created += a_second |
290 | self.assertContentEqual([recent_build], get_recent()) |
291 | |
292 | @@ -363,7 +417,6 @@ |
293 | # getSpecificJob returns the SourcePackageRecipeBuild |
294 | sprb = self.makeSourcePackageRecipeBuild() |
295 | Store.of(sprb).flush() |
296 | - build = sprb.build_farm_job |
297 | job = sprb.build_farm_job.getSpecificJob() |
298 | self.assertEqual(sprb, job) |
299 | |
300 | @@ -443,7 +496,7 @@ |
301 | build = self.factory.makeSourcePackageRecipeBuild( |
302 | distroseries=distroseries, |
303 | status=BuildStatus.FULLYBUILT, |
304 | - duration=datetime.timedelta(minutes=5)) |
305 | + duration=timedelta(minutes=5)) |
306 | build.queueBuild(build) |
307 | return build |
308 | |
309 | @@ -456,7 +509,3 @@ |
310 | class TestHandleStatusForSPRBuild( |
311 | MakeSPRecipeBuildMixin, TestHandleStatusMixin, TrialTestCase): |
312 | """IPackageBuild.handleStatus works with SPRecipe builds.""" |
313 | - |
314 | - |
315 | -def test_suite(): |
316 | - return unittest.TestLoader().loadTestsFromName(__name__) |
So, whats this intended to achieve? As I read it it will result in builds building once every two days. That seems undesirable and confusing.