Generalise IBuilder

[Michael Nelson]

As per:

https://dev.launchpad.net/Soyuz/Specs/BuilddGeneralisation

"Refactor the builder.py file / IBuilder class so that it's generic. Move the package build specific code to IPackageBuilder (non-model class)."

Although I wonder whether IBinaryPackageBuilder would be better, so that the branch->source package builder can be ISourcePackageBuilder?

[Michael Nelson (michael.nelson)]

Here's the pre-implementation suggestion that I'm starting now:

---------- Forwarded message ----------
From: Michael Nelson <email address hidden>
Date: Mon, Nov 23, 2009 at 7:56 PM

OK, I had a bit of time to look at this today. Just in terms of a pre-implementation discussion, here's what I'm planning to do tomorrow - so if you see any flaws or improvements in my plan or assumptions, please let me know.

The specialized builders need to both (a) augment existing functionality (eg. startBuild()), and (b) provide some additional different functionality (eg. checkCanBuildForDistroArchSeries()).

My assumption is that all call-sites will know whether they want a specific type of builder, or be happy to deal with a set of general builders (ie. a view listing all builders).

The approach that I'm planning to take tomorrow is to (note bigjools, this is different to the initial example I showed you earlier):

1) Create IPackageBuilder (or IBinaryPackageBuilder) as an extension of the IBuilder interface.
2) Create a PackageBuilder (or BinaryPackageBuilder) content-class that uses composition + delegation to provide the IBuilder general functionality while adding custom functionality or augmenting existing functionality of IBuilder (using lazr.delegates).
3) Provide adapters for IBuilder->IPackageBuilder etc., so that call-sites can grab a builder and adapt it as required.
4) Where appropriate, provide hooks in any general Builder methods that will need additional behaviour so that customisations will generally (but not necessarily) follow a standard api.

Also, when a builder is building it will have a know job type and so can potentially adapt itself (via the job/build queue entry). Not sure if this will be necessary.

This means that the general IBuilder interface/class won't need to know about the specializations (or be updated when new specialized builders are added).

[Michael Nelson (michael.nelson)]

Here's an example diff of the above approach (BinaryPackageBuilder has-an IBuilder which to which it delegates):

http://pastebin.ubuntu.com/327583/

BinaryPackageBuilder isn't *an* IBuilder.
--------------------------------------------------

BinaryPackageBuilder implements the IBuilder interface (by delegation) which means that most of the time, it can be treated just like an IBuilder (eg. bin_builder.status or bin_builder.id all work as expected), but Storm won't be tricked... build.builder = bin_builder needs to be replaced with either bin_builder.id or bin_builder.builder.

I was already aware that call-sites would need to know what type of builder they wanted to deal with, but this means that call-sites sometimes need to know to differentiate between the BinaryPackageBuilder and the general builder that it encapsulates.

Using simple inheritance would avoid this (ie. BinaryPackageBuilder inherits Builder).

Another option that I've been thinking about while coding the above: what we're really trying to extract out is the *behaviour* of the builder. If we could define a general IBuildBehaviour which is implemented by BinaryPackageBuildBehaviour and co., then we could have the general Builder delegating IBuildBehaviour to the custom behaviour (ie. Builder has-a BuildBehaviour). The advantage here is that both call-sites *and* the general builder do not need to know about the behaviour details. For example, when the buildmaster dispatches a job, the job itself can return an appropriate build job behaviour for itself. This should ensure that extending and maintaining the different build behaviours follows good OO principles (Encapsulate what varies, favor composition over inheritance, program to interfaces - not implementation, classes should be open for extension but closed for modification, etc.)

But to do this, we need to be able to define the standard build behaviour interface (verifyBuildJob(), startBuildJob(), etc.)

[Michael Nelson (michael.nelson)]

I've tried the thought that I mentioned above - extracting the different behaviors into separate classes and then delegating to those classes from Builder and it works really well. It means that the call-sites don't need to know about the different types of builder behaviors (which means the current soyuz code that uses Builder shouldn't need changing - outside of refactoring cleanups), and neither does Builder need to know about the different behaviors.

A diff showing the initial change is here - without any modifications to current tests:

http://pastebin.ubuntu.com/329392/

Running `bin/test -vvt doc/buildd-dispatching.txt` still passes fine.

So unless anyone sees any problems with this approach (I'll be asking BjornT for his thoughts - there are also a few comments in the diff requiring attention), I'll write tests for the new functionality above and then continue pulling out the rest of the soyuz specific stuff from IBuilder into the BinaryPackageBuildBehavior class.

The branch is available at:

lp:~michael.nelson/launchpad/487009-db-generalise-ibuilder-1b

[Michael Nelson (michael.nelson)]

<BjornT_> noodles775: it seems a bit odd to adapt a job type. adapting an actual job seems more natural
<noodles775> BjornT_: yes - that would be much better (I'd headed down that path as I initially had a factory there).
<noodles775> BjornT_: although, that would then require adding a factory that knows about the different types, wouldn't it?
<noodles775> That was what I was trying to avoid too (having multiple places that need to know about the different types)
<BjornT_> noodles775: no. you can mark the job as providing the right interface, depending on which job type it is
<noodles775> BjornT_: ah, great!
<BjornT_> noodles775: you can see an example in BugTask._init()
* noodles775 looks
<bigjools-afk> noodles775: same sort of thing happens in Archive.__init__
<noodles775> Right.

[Michael Nelson (michael.nelson)]

Hrm, I just played with the above suggestion - but afaics I can only do that by introducing knowledge of the different build types and behaviours into the job class (well, BuildQueue item - it's the one with the job_type attribute).

Currently, all the knowledge of types and behaviours is restricted to the lp.buildmaster.interfaces.buildfarmjob and the zcml configuration, so to create a new job type, for example, you would:

1. Create a new db item on the BuildFarmJobType enum in buildfarmjob.py
2. Add the marker interface for the job type, also in buildfarmjob.py
3. Add your custom build behaviour in your own code, and then
4. Update the zcml configuration so that your new type can be adapted to your new behaviour.

On the other hand, if I update so that the adaption is on BuildQueue (using the _init() to set the marker interface depending on the job_type), then (2) above would be replaced with:

2b. Update BuildQueue._init() to set the correct marker interface for your new job type, where the _init() method needs to know about all BuildFarmJobType's and their related behaviours.

I personally like the current implementation better (knowledge of different types/behaviours only required in the one place) but will switch it if you guys think it should definitely otherwise. Let me know.

[Michael Nelson (michael.nelson)]

OK, I've realised that BjornT was probably referring to the actual IBuildPackageJob (ie. the soyuz-specific implementation of IBuildFarmJob) as the thing that should be adapted to the behavior - and that would make more sense. Sorry for the noise.

[Diogo Matsubara (matsubara)]

Fixed in devel r10003 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/10003>