Code review comment for lp://staging/~jtv/launchpad/bug-500110

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 500110 =

This code is what it takes to get a new IBuildFarmJobBehavior implementation tested. Actually the diff is quite diverse, so I'll go through it step by step.

The diff starts with lib/lp/buildmaster/doc/buildfarmjob.txt:

BuildQueue.specific_job retrieves a build farm job based on two elements: the specific class of build farm job that it wants (implementing IBuildFarmJob, of course), and the Job it's attached to. It takes those two elements straight into an ORM query. Unfortunately the TranslationTemplatesBuildJob has no database story of its own, and so can't be queried in this way.

To get around this, I had BuildQueue delegate this query to the specific build farm job class. BuildFarmJob, the base class for these classes, now contains the generic implementation that was previously in BuildQueue.specific_job; TranslationTemplatesBuildJob overrides it for its specific case.

The new method is called getByJob. It's a class method, not a regular method, and is declared in a new interface ISpecificBuildFarmJobClass. The new doctest verifies that BranchJob and its descendants provide this interface.

lib/lp/buildmaster/interfaces/buildfarmjob.py contains the new interface. It's very simple, but named to allow future extension. This can be useful if we find ourselves tempted to put methods in IBuildFarmJobBehavior that really aren't related to build behaviour.

lib/lp/buildmaster/model/buildfarmjob.py implements the new method for all build-farm job classes except the Translations one. This is one of the rare cases where the "cls" argument to class methods is actually useful: it is the specific build farm job class that the method needs to look up, not the one it is defined in.

lib/lp/code/model/branchjob.py exports a few extra classes. Without this, implementing BranchJob descendants outside of this class results in warnings about import policy violations.

lib/lp/soyuz/interfaces/buildqueue.py has two separate utility changes:
 1. Some job_id parameters are renamed buildqueue_id to avoid the impression that they might be job ids. They are really buildqueue ids; I haven't changed them apart from renaming them.
 2. A new method getByJob is similar to the one in BuildFarmJob, but looks up a BuildQueue instead.

lib/lp/soyuz/model/buildpackagejob.py: since BuildFarmJob now contains an essential bit of functionality for finding specific jobs, BuildPackageJob does now need to inherit from it to benefit from the generic implementation.

lib/lp/soyuz/model/buildqueue.py reflects the changes in the matching interface file.

lib/lp/soyuz/tests/test_buildqueue.py has grown a test for the BuildQueueSet utility.

lib/lp/translations/configure.zcml shows a simplification: now that the BuildFarmJob hierarchy has a getByJob, there is no longer any reason for ITranslationTemplatesBuildJob. Instead, TranslationTemplatesBuildJob implements IBranchJob and IBuildFarmJob directly.

lib/lp/translations/doc/translationtemplatesbuildbehavior.txt is where it all comes together. It started out as a copy of the initial part of lib/lp/soyuz/doc/buildd-dispatching.txt. I left out some irrelevant parts, generalized a bit, and removed some reliance on pre-existing test data.

Where buildd-dispatching.txt calls findAndStartJob, this new test calls _dispatchBuildCandidate. Not ideal: it's a private method. But necessary because findAndStartJob involves candidate selection, whose generalisation is not complete yet. An XXX marks the spot.

lib/lp/translations/interfaces/translationtemplatesbuildjob.py gets rid of ITranslationTemplatesBuildJob, as mentioned in the ZCML chapter. Also, getForJob is no longer needed in the utility since we can now get the TranslationTemplatesBuild from its class method getByJob.

lib/lp/translations/model/translationtemplatesbuildbehavior.py has changed less than one might think. Most changes are adaptation to API changes that jml implemented in the mean time:
 - A behavior classes now knows its BuildQueue record. No need to look it up.
 - Builder.cacheFileOnSlave has become cacheFile on the build-slave class.
 - Failures in talking to the slave are now handled in the caller. No more "except:"
 - _findTranslationTemplatesBuildJob is now obviously no longer needed.
 - Behavior classes must now implement logStartBuild.

lib/lp/translations/model/translationtemplatesbuildjob.py has changes following from the interface changes: ITranslationTemplatesBuildJob is gone, and getForJob is superseded by getByJob.

lib/lp/translations/tests/test_translationtemplatesbuildjob.py also tests for the interface changes. _findTranslationTemplatesBuildJob is gone, so no longer tested either.

That's it. To test,
{{{
./bin/test -vv -t translationtemplatesbuild
}}}

No lint.

Jeroen

« Back to merge proposal