Builder should be able to take a 'env' argument that applies to all BuildSteps

yeah, that sounds reasonable. I frequently do this by setting up the environment of the buildslave, but I can see where you'd want to do it per-Builder instead. Let's make env= an argument to the Builder __init__ that we should build for ticket #10 .

Sounds good.

+1

builder-wide environment

This first patch is as-of-yet untested but I'm pretty sure it'll work. ShellCommands? automatically pull in the Build.slaveEnvironment dictionary.

I'll do some testing today/tomorrow and see how it goes.

same as before, fix stupid error (setup.get(env?) -> setup.get('env'))

I tested this one and it works as intended and passes unit tests.

Looks good .. needs tests, though, and it would be *great* to get rid of:

    def setupEnvironment(self, cmd):
        # XXX is this used? documented? replaced by properties?
        # merge in anything from Build.slaveEnvironment . Earlier steps
        # (perhaps ones which compile libraries or sub-projects that need to
        # be referenced by later steps) can add keys to
        # self.build.slaveEnvironment to affect later steps.

You're absolutely right about the tests.

Let me do some thinking and tinkering about setupEnvironment() before I comment on that.

OK, I'm not clear on what you want to get rid of? The function, or the 'XXX' comment..?

I'd like to resolve the concerns in the comment -- the function can stay, if it's still in use.

It's part of my "leave it cleaner than when you came" patch policy :)

Okay, it seems clear that my patch depends on ShellCommand?.setupEnvironment(). After looking at it closer I realized that in its current form, the builder-wide environment will override anything in the BuildStep? environment. I would consider this incorrect. However, if a BuildStep? were to set something in slaveEnvironment it would make sense for it to override the BuildStep? environment (IMHO). Therefore, I propose that a BuildStep? setting something in slaveEnvironment be superceded by BuildProperties?, as suggested by the documentation. I'm pretty sure you can use WithProperties? with env vars, so I don't see any reason to support that operation.

If that makes sense, I can make the following changes to my patch: * Add tests * Make the BuildStep?-environment override the Builder environment * Add documentation that describes the Builder environment, and makes clear exactly how BuildStep? environment setting works.

sounds good!

again, with tests + clarified ShellCommand?.setupEnviornment

Oops, I forgot to write docs for this. I'll add them shortly.

docs

Is this going to get landed for 0.7.8?

some thoughts:

• I suspect somebody will eventually complain that WithProperties aren't accepted in the builder-wide settings like they are in the per-step settings.. you might want to merge in the builder-wide dict first, *then* do the properties.render() call
• if you do that, the docs should be updated to mention this feature
• the docs should probably have an example of what you might want to use this for. They should also mention the possibility of setting environment variables in the buildslave's startup environment, since variables like that are sometimes slave-specific

Otherwise, it looks good to me.

bhearsum, what do you think of warner's suggestions? We can break those out into separate tickets, or fix them here.