ShellCommand: 'env' doesn't work in 0.7.6 (because remote_kwargs not stored in factory)

[gward]

If you use the new 0.7.6 style of passing a whole BuildStep object to factory.addStep(), many attributes of ShellCommand do not work, e.g. 'env'. Example:

  factory.addStep(ShellCommand(command=[...], description="...", env={...})

In this case, the environment variables specified in 'env' are not actually used when the BuildStep runs. This is because 'env' (and many other of the kwargs passed to ShellCommand()) is not stored in the step's factory tuple, so is not included in future clones of this ShellCommand object.

I have hacked around this by subclassing BuildFactory and adding this to MyBuildFactory.addStep():

    if isinstance(step, shell.ShellCommand):
        step.addFactoryArguments(**step.remote_kwargs)

I *suspect* this patch will fix it, but have not tested it:

--- buildbot/steps/shell.py.orig        2007-09-30 22:48:39.000000000 -0400
+++ buildbot/steps/shell.py     2007-10-24 16:45:32.285303148 -0400
@@ -111,7 +111,8 @@
         self.addFactoryArguments(workdir=workdir,
                                  description=description,
                                  descriptionDone=descriptionDone,
-                                 command=command)
+                                 command=command,
+                                 **kwargs)

         # everything left over goes to the RemoteShellCommand
         kwargs['workdir'] = workdir # including a copy of 'workdir'

[gward]

I have tested the patch I posted above, and it works fine.

(However, I have doubts about this whole "build step factory" idea. Wouldn't it be simpler just to require every BuildStep? to implement a copy() method? At least then we'd find BuildSteps? that have not been updated for 0.7.6 quickly and clearly, because calling copy() on the object passed to factory.addStep() would crash hard.)

[warner]

I think copy() would suffer the same problem that addFactoryArguments() has: subclasses of ShellCommand? that failed to override their copy() method would silently drop those extra arguments, resulting in hard-to-find problems (not unlike this hard-to-find problem). New classes that don't inherit from ShellCommand? would be easy to catch, but subclasses wouldn't be.

I seem to remember thinking that the addFactoryArguments() approach would result in the least extra code in the subclasses (and thus improve the chances that people would remember to add it), but I no longer remember my exact reasons. I certainly agree that this factory thing is clunky.

I think the kwargs addition is the correct one. I'm running tests on it now, and will probably push this change in tomorrow.

[warner]

This should be fixed by the same commit [b060cf9b5d56366b6e505e81722004885f40cec6] that fixed #150. I did some brief manual testing on it, and test_steps.py has a test case that should cover it.