Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#100 closed enhancement (fixed)

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

Reported by: bhearsum Owned by: bhearsum
Priority: minor Milestone: 0.8.+
Version: Keywords:
Cc: dustin, dwlocks

Description

I think it's a fairly common use case that all of a Builder's BuildSteps? require the same environment variables. I propose that the Builder 'setup' should be able to take in a dict of environment variables and pass it along to all BuildSteps? that require/support it (ie. it wouldn't pass it to something like FileDownload?).

I'd be happy to code this up myself assuming it is wanted.

Attachments (5)

builderWideEnvironnment.diff (1.5 KB) - added by bhearsum 13 years ago.
builder-wide environment
builderWideEnvironnment-v2.diff (1.5 KB) - added by bhearsum 13 years ago.
same as before, fix stupid error (setup.get(env?) -> setup.get('env'))
builderWideEnvironnment-with-tests.diff (6.6 KB) - added by bhearsum 13 years ago.
again, with tests + clarified ShellCommand?.setupEnviornment
builderEnv-docs.diff (1.1 KB) - added by bhearsum 13 years ago.
docs
builderEnv-full.diff (8.0 KB) - added by bhearsum 12 years ago.
builder env, better docs, supports properties

Download all attachments as: .zip

Change History (25)

comment:1 Changed 13 years ago by warner

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 .

comment:2 Changed 13 years ago by bhearsum

Sounds good.

comment:3 Changed 13 years ago by dustin

  • Cc dustin dwlocks added

+1

Changed 13 years ago by bhearsum

builder-wide environment

comment:4 Changed 13 years ago by bhearsum

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.

comment:5 Changed 13 years ago by bhearsum

  • Status changed from new to assigned

Changed 13 years ago by bhearsum

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

comment:6 Changed 13 years ago by bhearsum

  • Milestone changed from undecided to 0.7.8

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

comment:7 Changed 13 years ago by dustin

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.

comment:8 Changed 13 years ago by bhearsum

You're absolutely right about the tests.

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

comment:9 Changed 13 years ago by bhearsum

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

comment:10 Changed 13 years ago by dustin

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 :)

comment:11 Changed 13 years ago by bhearsum

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.

comment:12 Changed 13 years ago by dustin

sounds good!

Changed 13 years ago by bhearsum

again, with tests + clarified ShellCommand?.setupEnviornment

comment:13 Changed 13 years ago by bhearsum

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

Changed 13 years ago by bhearsum

docs

comment:14 Changed 12 years ago by bhearsum

Is this going to get landed for 0.7.8?

comment:15 Changed 12 years ago by warner

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.

comment:16 Changed 12 years ago by dustin

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

comment:17 Changed 12 years ago by bhearsum

(4 months later)

Yeah, that makes total sense. I'll work that into this patch.

Changed 12 years ago by bhearsum

builder env, better docs, supports properties

comment:18 Changed 12 years ago by bhearsum

  • Keywords review added

Alright, this patch is updated to support properties in both Builder and ShellCommand? environments. I've also added to the docs as per Brian's comments.

comment:19 Changed 12 years ago by dustin

  • Keywords review removed
  • Resolution set to fixed
  • Status changed from assigned to closed

comment:20 Changed 12 years ago by dustin

  • Milestone changed from 0.8.0 to 0.7.+
Note: See TracTickets for help on using tickets.