simplify BuildStep construction in master.cfg

[warner]

Stefan Seefeld pointed out that BuildFactory.addStep() could be easier. At the moment it takes (type, **params), because the actual instantiation of the BuildSteps must be delayed until the Build actually starts. The reason for this is twofold. The first is that BuildStep.__init__() takes a 'build' argument, which obviously is different for each build. The second is that BuildSteps are allowed to keep state in their internal attributes, and are likely to get confused if the same instance is re-used from one build to the next.

Implementing BuildStep.clone() might be the cleanest approach. The calls to addStep() (and the 'steps' arg to BuildFactory) in master.cfg would take actual BuildStep instances, which would live for a long time. The only method that would get called on these instances is .clone(), which would be required to return a suitable duplicate of the instance. Each build would get a separate copy, and .start() would only be called on those duplicates. The duplicates would also be where setBuild(), setStepStatus(), and setProgress() get called.

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.

[warner]

I'm working on the clone() approach. I'm also cleaning up the way ShellCommands? are constructed in the process: there is a lot of weirdness in there that I think I can get rid of.

The approach I'm taking is for __init__ to always capture a copy of all arguments and then delegate to the "real" init method. This way, everybody uses the same clone() method, something like:

    def __init__(self, **kwargs):
        self.kwargs = kwargs
        self.init(**kwargs)
    def clone(self):
        return self.__class__(**self.kwargs.copy)

The downside is that subclassers must remember to override init() (and upcall) instead of overriding the usual __init__. Doing the latter by mistake would cause self.kwargs to not contain all of the arguments, messing up clone(). I can think of a couple of ways to check for this (having clone() compare the __dict__ of both instances, or having __init__ assert that __dict__ is empty), but of course I'd prefer an approach that removes the possibility of the mistake in the first place. Maybe a metaclass to change the way __init__ works, so that all arguments are stashed before calling the real __init__.

[jknight]

Wait, why is this a good idea? The way it is right now seems pretty darn simple: BuildSteps? are created for each build, and I can stash whatever I please in them. Trying to make the instance actually be a instance factory, sometimes, and can be re-instantiated into the actual instance just seems *more* complicated to me, not less.

One way that might be simpler, though, is to have a function call which returns a list of actual buildstep instances, and provide this function to the BuildFactory?. That function gets called to return a list of steps each time a build starts. I guess that might break reconfig though.

[warner]

Good point. I spent a long time actually implementing this approach, but it's been sitting in a branch for two months because I don't have enough confidence that I did it correctly, and I'm inclined to drop the effort altogether.

There are some more subtle problems involved: the whole don't-update-unless-necessary approach that we take to config changes (which is the cause of #35 and probably involved in #22) means that it is probably safer to refer to factories (or their informational equivalents) rather than actual instances.

Maybe, once a reconfig has taken place, we could run through all the steps and ask them to give us a factory+args that could be used to re-create them, then stash those. The comparison of whether a BuildFactory? had changed or not would compare the factory+args, rather than the instances that were used to generate them. The only benefit of that approach would be for the config file syntax to use actual instances, though.

[warner]

I implemented this latter approach: once we've parsed the config file, we ask all the BuildStep? instances that it creates for their factory information (class and args necessary to re-create the step), and stash those. All comparison takes place between that factory information.

This makes the config file a bit easier to read, and didn't involve sweeping changes to the internals. I'm happy with the compromise.