[Buildbot-devel] BuildFactory.addStep()

[Brian Warner]

Stefan Seefeld <seefeld at sympatico.ca> writes:

> I notice that the new recommended way to create steps is to use
> BuildFactory.addStep(type, **params). I think a much more use-case is
>
> step = some_step_factory()
> ..
> builder.addStep(step)
>
> In other words, I think it would be more 'ergonomic' to have users
> create steps as free-standing objects, and then assign them to builders.

I agree.. passing steps around as (type,**args) tuples is a drag.

> For example, I'm not sure why I couldn't use a syntax as simple as
>
> checkout = step.SVN(baseurl='...', mode='update')
> configure_1 = step.Configure(command=[...])
> configure_2 = step.Configure(command=[...])
>
> to create individual steps, before setting up builders as
>
> builder_1 = BuildFactory(checkout, configure_1, compile, test)
> builder_2 = BuildFactory(checkout, configure_2, compile, test)

FWIW, BuildFactory actually takes a 'steps' argument, which is a list of
(type,**args) tuples. Not as concise, but perhaps useful.

The reason that buildbot isn't currently using the syntax you describe is
precisly what you've identified: the BuildFactory is given a list of step
*factories*, rather than step instances themselves. This is because each
Build needs to get its own copy of each BuildStep.

BuildSteps are allowed to store state inside their own instance variables.
Also, there are a number of instances that are set up when the BuildStep is
created, to keep track of the Build that the step is part of, and the status
object where the step should be announcing its changes. Because of these, the
buildbot.steps.shell.Compile instance that build#1 uses must be different
than the instance that build#2 uses.

I'd love to change this. I'm not quite sure what would work well. If we
assume that we want to use the syntax you described:

> checkout = step.SVN(baseurl='...', mode='update')

and then either f.addStep(checkout) or f = BuildFactory([checkout,...])) ,
then we need to be able to clone those BuildSteps when it comes time to
actually use them. (instantiating the BuildSteps when the config file is read
is a near-term goal anyways, since it gives BuildStep.__init__ a chance to
complain about its arguments during the reconfig, where log messages are
displayed to the user).

One (ugly) possibility is to have step.SVN be just a marker class, not unlike
the way we handle SlaveLocks. That's gross because it requires two classes
for each BuildStep: one as a marker, a second as the real step.

Implementing BuildStep.clone() might be the cleanest approach. I think it's
also a good idea to move the 'build' argument from BuildStep.__init__ out to
a separate setBuild() method, which would make it a lot easier to construct
those instances in the config file. Then the remaining big issue is the
transition process: can we arrange for existing config files (which pass
classes into addStep) to keep working? Probably yes, we just change addStep
to detect that it's being given a class instead of an instance and
instantiate it then and there. Allowing old custom BuildStep classes to keep
working is trickier, but we can declare that all such custom classes must
implement clone() to be useable with the latest version, and without clone()
they'll at least fail in a nice and obvious way.

I'll see what I can accomplish with this for the next release. The
compatibility stuff might be a bit tricky, so I'll probably do the work on a
separate branch and solicit comments on it before merging to trunk.

thanks for the suggestion!
 -Brian