Code review comment for lp://staging/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-1
- 567922-binarypackagebuild-new-table-1
- Merge into db-devel
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Nelson (michael.nelson) wrote : | # |
1 | === modified file 'lib/lp/buildmaster/doc/buildfarmjob.txt' |
2 | --- lib/lp/buildmaster/doc/buildfarmjob.txt 2010-04-28 10:59:12 +0000 |
3 | +++ lib/lp/buildmaster/doc/buildfarmjob.txt 2010-05-04 13:42:25 +0000 |
4 | @@ -29,17 +29,6 @@ |
5 | |
6 | For more details, please refer to lp.buildmaster.tests.test_buildfarmjob. |
7 | |
8 | -BuildFarmJobDerived |
9 | -=================== |
10 | - |
11 | -BuildFarmJobDerived exists to encapsulate the functionality required |
12 | -by classes delegating IBuildFarmJob. |
13 | - |
14 | - >>> from lp.buildmaster.interfaces.buildfarmjob import ( |
15 | - ... IBuildFarmJobDerived) |
16 | - >>> verifyObject(IBuildFarmJobDerived, BuildFarmJobDerived()) |
17 | - True |
18 | - |
19 | This class also includes the getByJob() class method used to find the |
20 | instance of a specific build-farm job implementation associated with a |
21 | given Job instance which must be called in the context of a concrete |
22 | |
23 | === modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py' |
24 | --- lib/lp/buildmaster/interfaces/buildfarmjob.py 2010-04-30 16:11:03 +0000 |
25 | +++ lib/lp/buildmaster/interfaces/buildfarmjob.py 2010-05-04 13:42:25 +0000 |
26 | @@ -11,7 +11,6 @@ |
27 | 'IBuildFarmJob', |
28 | 'IBuildFarmJobOld', |
29 | 'IBuildFarmJobSource', |
30 | - 'IBuildFarmJobDerived', |
31 | 'BuildFarmJobType', |
32 | ] |
33 | |
34 | @@ -106,6 +105,71 @@ |
35 | def jobAborted(): |
36 | """'Job aborted' life cycle event, handle as appropriate.""" |
37 | |
38 | + def addCandidateSelectionCriteria(processor, virtualized): |
39 | + """Provide a sub-query to refine the candidate job selection. |
40 | + |
41 | + Return a sub-query to narrow down the list of candidate jobs. |
42 | + The sub-query will become part of an "outer query" and is free to |
43 | + refer to the `BuildQueue` and `Job` tables already utilized in the |
44 | + latter. |
45 | + |
46 | + Example (please see the `BuildPackageJob` implementation for a |
47 | + complete example): |
48 | + |
49 | + SELECT TRUE |
50 | + FROM Archive, Build, BuildPackageJob, DistroArchSeries |
51 | + WHERE |
52 | + BuildPackageJob.job = Job.id AND |
53 | + .. |
54 | + |
55 | + :param processor: the type of processor that the candidate jobs are |
56 | + expected to run on. |
57 | + :param virtualized: whether the candidate jobs are expected to run on |
58 | + the `processor` natively or inside a virtual machine. |
59 | + :return: a string containing a sub-query that narrows down the list of |
60 | + candidate jobs. |
61 | + """ |
62 | + |
63 | + def postprocessCandidate(job, logger): |
64 | + """True if the candidate job is fine and should be dispatched |
65 | + to a builder, False otherwise. |
66 | + |
67 | + :param job: The `BuildQueue` instance to be scrutinized. |
68 | + :param logger: The logger to use. |
69 | + |
70 | + :return: True if the candidate job should be dispatched |
71 | + to a builder, False otherwise. |
72 | + """ |
73 | + |
74 | + def getByJob(job): |
75 | + """Get the specific `IBuildFarmJob` for the given `Job`. |
76 | + |
77 | + Invoked on the specific `IBuildFarmJob`-implementing class that |
78 | + has an entry associated with `job`. |
79 | + """ |
80 | + |
81 | + def generateSlaveBuildCookie(): |
82 | + """Produce a cookie for the slave as a token of the job it's doing. |
83 | + |
84 | + The cookie need not be unique, but should be hard for a |
85 | + compromised slave to guess. |
86 | + |
87 | + :return: a hard-to-guess ASCII string that can be reproduced |
88 | + accurately based on this job's properties. |
89 | + """ |
90 | + |
91 | + def makeJob(): |
92 | + """Create the specific job relating this with an lp.services.job. |
93 | + |
94 | + XXX 2010-04-26 michael.nelson bug=567922 |
95 | + Once all *Build classes are using BuildFarmJob we can lose the |
96 | + 'specific_job' attributes and simply have a reference to the |
97 | + services job directly on the BuildFarmJob. |
98 | + """ |
99 | + |
100 | + def cleanUp(): |
101 | + """Job's finished. Delete its supporting data.""" |
102 | + |
103 | |
104 | class IBuildFarmJob(IBuildFarmJobOld): |
105 | """Operations that jobs for the build farm must implement.""" |
106 | @@ -179,78 +243,6 @@ |
107 | |
108 | title = exported(TextLine(title=_("Title"), required=False)) |
109 | |
110 | - def makeJob(): |
111 | - """Create the specific job relating this with an lp.services.job. |
112 | - |
113 | - XXX 2010-04-26 michael.nelson bug=567922 |
114 | - Once all *Build classes are using BuildFarmJob we can lose the |
115 | - 'specific_job' attributes and simply have a reference to the |
116 | - services job directly on the BuildFarmJob. |
117 | - """ |
118 | - |
119 | - |
120 | -class IBuildFarmJobDerived(Interface): |
121 | - """Common functionality required by classes delegating IBuildFarmJob. |
122 | - |
123 | - An implementation of this class must setup the necessary delegation. |
124 | - """ |
125 | - |
126 | - def getByJob(job): |
127 | - """Get the specific `IBuildFarmJob` for the given `Job`. |
128 | - |
129 | - Invoked on the specific `IBuildFarmJob`-implementing class that |
130 | - has an entry associated with `job`. |
131 | - """ |
132 | - |
133 | - def addCandidateSelectionCriteria(processor, virtualized): |
134 | - """Provide a sub-query to refine the candidate job selection. |
135 | - |
136 | - Return a sub-query to narrow down the list of candidate jobs. |
137 | - The sub-query will become part of an "outer query" and is free to |
138 | - refer to the `BuildQueue` and `Job` tables already utilized in the |
139 | - latter. |
140 | - |
141 | - Example (please see the `BuildPackageJob` implementation for a |
142 | - complete example): |
143 | - |
144 | - SELECT TRUE |
145 | - FROM Archive, Build, BuildPackageJob, DistroArchSeries |
146 | - WHERE |
147 | - BuildPackageJob.job = Job.id AND |
148 | - .. |
149 | - |
150 | - :param processor: the type of processor that the candidate jobs are |
151 | - expected to run on. |
152 | - :param virtualized: whether the candidate jobs are expected to run on |
153 | - the `processor` natively or inside a virtual machine. |
154 | - :return: a string containing a sub-query that narrows down the list of |
155 | - candidate jobs. |
156 | - """ |
157 | - |
158 | - def postprocessCandidate(job, logger): |
159 | - """True if the candidate job is fine and should be dispatched |
160 | - to a builder, False otherwise. |
161 | - |
162 | - :param job: The `BuildQueue` instance to be scrutinized. |
163 | - :param logger: The logger to use. |
164 | - |
165 | - :return: True if the candidate job should be dispatched |
166 | - to a builder, False otherwise. |
167 | - """ |
168 | - |
169 | - def generateSlaveBuildCookie(): |
170 | - """Produce a cookie for the slave as a token of the job it's doing. |
171 | - |
172 | - The cookie need not be unique, but should be hard for a |
173 | - compromised slave to guess. |
174 | - |
175 | - :return: a hard-to-guess ASCII string that can be reproduced |
176 | - accurately based on this job's properties. |
177 | - """ |
178 | - |
179 | - def cleanUp(): |
180 | - """Job's finished. Delete its supporting data.""" |
181 | - |
182 | |
183 | class IBuildFarmJobSource(Interface): |
184 | """A utility of BuildFarmJob used to create _things_.""" |
185 | |
186 | === modified file 'lib/lp/buildmaster/model/buildfarmjob.py' |
187 | --- lib/lp/buildmaster/model/buildfarmjob.py 2010-04-30 16:12:05 +0000 |
188 | +++ lib/lp/buildmaster/model/buildfarmjob.py 2010-05-04 13:42:25 +0000 |
189 | @@ -14,7 +14,6 @@ |
190 | |
191 | from lazr.delegates import delegates |
192 | |
193 | -import hashlib |
194 | import pytz |
195 | |
196 | from storm.locals import Bool, DateTime, Int, Reference, Storm |
197 | @@ -32,8 +31,8 @@ |
198 | |
199 | from lp.buildmaster.interfaces.buildbase import BuildStatus |
200 | from lp.buildmaster.interfaces.buildfarmjob import ( |
201 | - BuildFarmJobType, IBuildFarmJob, IBuildFarmJobDerived, |
202 | - IBuildFarmJobOld, IBuildFarmJobSource) |
203 | + BuildFarmJobType, IBuildFarmJob, IBuildFarmJobOld, |
204 | + IBuildFarmJobSource) |
205 | from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet |
206 | |
207 | |
208 | @@ -63,6 +62,10 @@ |
209 | """See `IBuildFarmJobOld`.""" |
210 | raise NotImplementedError |
211 | |
212 | + def getByJob(self): |
213 | + """See `IBuildFarmJobOld`.""" |
214 | + raise NotImplementedError |
215 | + |
216 | def jobStarted(self): |
217 | """See `IBuildFarmJobOld`.""" |
218 | pass |
219 | @@ -75,8 +78,27 @@ |
220 | """See `IBuildFarmJobOld`.""" |
221 | pass |
222 | |
223 | + @staticmethod |
224 | + def addCandidateSelectionCriteria(processor, virtualized): |
225 | + """See `IBuildFarmJobOld`.""" |
226 | + return ('') |
227 | + |
228 | + @staticmethod |
229 | + def postprocessCandidate(job, logger): |
230 | + """See `IBuildFarmJobOld`.""" |
231 | + return True |
232 | + |
233 | + def cleanUp(self): |
234 | + """See `IBuildFarmJob`.""" |
235 | + Store.of(self).remove(self) |
236 | + |
237 | + def generateSlaveBuildCookie(self): |
238 | + """See `IBuildFarmJobOld`.""" |
239 | + raise NotImplementedError |
240 | + |
241 | |
242 | class BuildFarmJobOldDerived: |
243 | + """Setup the delegation and provide some common implementation.""" |
244 | delegates(IBuildFarmJobOld, context='build_farm_job') |
245 | |
246 | def __init__(self, *args, **kwargs): |
247 | @@ -102,22 +124,12 @@ |
248 | |
249 | @classmethod |
250 | def getByJob(cls, job): |
251 | - """See `IBuildFarmJobDerived`.""" |
252 | + """See `IBuildFarmJobOld`.""" |
253 | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) |
254 | return store.find(cls, cls.job == job).one() |
255 | |
256 | - @staticmethod |
257 | - def addCandidateSelectionCriteria(processor, virtualized): |
258 | - """See `IBuildFarmJobDerived`.""" |
259 | - return ('') |
260 | - |
261 | - @staticmethod |
262 | - def postprocessCandidate(job, logger): |
263 | - """See `IBuildFarmJobDerived`.""" |
264 | - return True |
265 | - |
266 | def generateSlaveBuildCookie(self): |
267 | - """See `IBuildFarmJobDerived`.""" |
268 | + """See `IBuildFarmJobOld`.""" |
269 | buildqueue = getUtility(IBuildQueueSet).getByJob(self.job) |
270 | |
271 | if buildqueue.processor is None: |
272 | @@ -136,10 +148,6 @@ |
273 | |
274 | return hashlib.sha1(contents).hexdigest() |
275 | |
276 | - def cleanUp(self): |
277 | - """See `IBuildFarmJob`.""" |
278 | - Store.of(self).remove(self) |
279 | - |
280 | |
281 | class BuildFarmJob(BuildFarmJobOld, Storm): |
282 | """A base implementation for `IBuildFarmJob` classes.""" |
283 | @@ -258,8 +266,16 @@ |
284 | """ |
285 | return None |
286 | |
287 | + def cleanUp(self): |
288 | + """See `IBuildFarmJobOld`. |
289 | + |
290 | + XXX 2010-05-04 michael.nelson bug=570939 |
291 | + This can be removed once IBuildFarmJobOld is no longer used |
292 | + and services jobs are linked directly to IBuildFarmJob. |
293 | + """ |
294 | + pass |
295 | + |
296 | |
297 | class BuildFarmJobDerived: |
298 | - """See `IBuildFarmJobDerived`.""" |
299 | - implements(IBuildFarmJobDerived) |
300 | + implements(IBuildFarmJob) |
301 | delegates(IBuildFarmJob, context='build_farm_job') |
302 | |
303 | === modified file 'lib/lp/buildmaster/model/packagebuild.py' |
304 | --- lib/lp/buildmaster/model/packagebuild.py 2010-05-04 12:07:35 +0000 |
305 | +++ lib/lp/buildmaster/model/packagebuild.py 2010-05-04 13:47:11 +0000 |
306 | @@ -160,17 +160,13 @@ |
307 | |
308 | |
309 | class PackageBuildDerived: |
310 | + """Setup the delegation for package build. |
311 | + |
312 | + This class also provides some common implementation for handling |
313 | + build status. |
314 | + """ |
315 | delegates(IPackageBuild, context="package_build") |
316 | |
317 | - |
318 | -class PackageBuildStatusHandlerMixin: |
319 | - """A mixin to provide the common implementation of handleStatus. |
320 | - |
321 | - This can be used by classes based on PackageBuild to provide |
322 | - the implementation of handleStatus, ensuring that the methods |
323 | - called during handleStatus are called on the top-level object |
324 | - first. |
325 | - """ |
326 | def handleStatus(self, status, librarian, slave_status): |
327 | """See `IPackageBuild`.""" |
328 | return BuildBase.handleStatus(self, status, librarian, slave_status) |
329 | |
330 | === modified file 'lib/lp/code/configure.zcml' |
331 | --- lib/lp/code/configure.zcml 2010-04-30 15:05:53 +0000 |
332 | +++ lib/lp/code/configure.zcml 2010-05-04 13:42:25 +0000 |
333 | @@ -1024,7 +1024,6 @@ |
334 | <class |
335 | class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob"> |
336 | <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/> |
337 | - <allow interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJobDerived"/> |
338 | </class> |
339 | |
340 | <securedutility |
341 | |
342 | === modified file 'lib/lp/soyuz/configure.zcml' |
343 | --- lib/lp/soyuz/configure.zcml 2010-04-20 14:30:00 +0000 |
344 | +++ lib/lp/soyuz/configure.zcml 2010-05-04 13:42:25 +0000 |
345 | @@ -848,8 +848,6 @@ |
346 | class="lp.soyuz.model.buildpackagejob.BuildPackageJob"> |
347 | <allow |
348 | interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/> |
349 | - <allow |
350 | - interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJobDerived"/> |
351 | </class> |
352 | <!-- |
353 | The registration below is used to discover all build farm job classes |
354 | |
355 | === modified file 'lib/lp/soyuz/tests/test_buildpackagejob.py' |
356 | --- lib/lp/soyuz/tests/test_buildpackagejob.py 2010-04-28 13:57:13 +0000 |
357 | +++ lib/lp/soyuz/tests/test_buildpackagejob.py 2010-05-04 13:42:25 +0000 |
358 | @@ -13,7 +13,6 @@ |
359 | |
360 | from lp.buildmaster.interfaces.buildbase import BuildStatus |
361 | from lp.buildmaster.interfaces.builder import IBuilderSet |
362 | -from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobDerived |
363 | from lp.soyuz.interfaces.archive import ArchivePurpose |
364 | from lp.soyuz.interfaces.buildfarmbuildjob import IBuildFarmBuildJob |
365 | from lp.soyuz.interfaces.buildpackagejob import IBuildPackageJob |
366 | @@ -235,6 +234,3 @@ |
367 | build_farm_job = bq.specific_job |
368 | self.assertProvides(build_farm_job, IBuildPackageJob) |
369 | self.assertProvides(build_farm_job, IBuildFarmBuildJob) |
370 | - self.assertProvides(build_farm_job, IBuildFarmJobDerived) |
371 | - |
372 | - |
373 | |
374 | === modified file 'lib/lp/translations/configure.zcml' |
375 | --- lib/lp/translations/configure.zcml 2010-04-22 07:33:12 +0000 |
376 | +++ lib/lp/translations/configure.zcml 2010-05-04 13:42:25 +0000 |
377 | @@ -578,9 +578,7 @@ |
378 | <allow |
379 | interface="lp.code.interfaces.branchjob.IBranchJob"/> |
380 | <allow |
381 | - interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/> |
382 | - <allow |
383 | - interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJobDerived"/> |
384 | + interface="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJobOld"/> |
385 | </class> |
386 | <securedutility |
387 | component="lp.translations.model.translationtemplatesbuildjob.TranslationTemplatesBuildJob" |
388 | |
389 | === modified file 'lib/lp/translations/model/translationtemplatesbuildjob.py' |
390 | --- lib/lp/translations/model/translationtemplatesbuildjob.py 2010-04-30 16:11:03 +0000 |
391 | +++ lib/lp/translations/model/translationtemplatesbuildjob.py 2010-05-04 13:42:25 +0000 |
392 | @@ -147,7 +147,7 @@ |
393 | |
394 | @classmethod |
395 | def getByJob(cls, job): |
396 | - """See `IBuildFarmJobDerived`. |
397 | + """See `IBuildFarmJob`. |
398 | |
399 | Overridden here to search via a BranchJob, rather than a Job. |
400 | """ |
401 | |
402 | === modified file 'lib/lp/translations/tests/test_translationtemplatesbuildjob.py' |
403 | --- lib/lp/translations/tests/test_translationtemplatesbuildjob.py 2010-04-30 16:11:03 +0000 |
404 | +++ lib/lp/translations/tests/test_translationtemplatesbuildjob.py 2010-05-04 13:42:25 +0000 |
405 | @@ -19,7 +19,7 @@ |
406 | from lp.testing import TestCaseWithFactory |
407 | |
408 | from lp.buildmaster.interfaces.buildfarmjob import ( |
409 | - IBuildFarmJobOld, IBuildFarmJobDerived) |
410 | + IBuildFarmJobOld) |
411 | from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet |
412 | from lp.buildmaster.model.buildqueue import BuildQueue |
413 | from lp.code.interfaces.branch import IBranchSet |
414 | @@ -54,9 +54,8 @@ |
415 | |
416 | def test_new_TranslationTemplatesBuildJob(self): |
417 | # TranslationTemplateBuildJob implements IBuildFarmJobOld, |
418 | - # IBuildFarmJobDerived, and IBranchJob. |
419 | + # and IBranchJob. |
420 | verifyObject(IBranchJob, self.specific_job) |
421 | - verifyObject(IBuildFarmJobDerived, self.specific_job) |
422 | verifyObject(IBuildFarmJobOld, self.specific_job) |
423 | |
424 | # Each of these jobs knows the branch it will operate on. |
On Tue, May 4, 2010 at 2:14 PM, Michael Nelson rived
<email address hidden> wrote:
...
> I've attached the incremental of the above, but if it's ok with you,
> I'll also do a second incremental to get rid of IBuildFarmJobDe
> for the same reason you've outlined above.
>
See attached for the removal of the IBuildFarmJobDe rived interface as ived/PackageJob Derived classes directly.
above. I also realised while doing this that I don't need the separate
mixin, and have instead provided the shared implementation on the
BuildFarmJobDer
tested with: agerecipebuild -t test_translatio ntemplatesbuild job
bin/test -vv -t test_buildfarmjob -t test_packagebuild -t
test_sourcepack
-Michael