[Buildbot-devel] Patches index

[Brian Warner]

CodeCon is finally done and I can take a breather from Petmail. Thanks for
your patience.

> * cvs-up.patch			
> Does a cvs update instead of a full checkout each time, to save
> time and it avoid breaking our special cvs setup

to be precise, it fixes a bug that caused checkouts where updates were
requested and sufficient

[applied]

> * cvs-tag.patch
> Avoids sending -r HEAD if it's not needed. 

[applied]

It makes sense and it fits in well with some -D changes that are in the
works, but I'd like to know what prompted you to change it: is there a usage
model that gets broken when -r HEAD is passed on each command?

My concern is that somebody will do something foolish like check out a tree
on their slave machine by hand, with -r BRANCH, and then expect the
buildslave to always do an update (with no -r tag, to avoid un-selecting the
branch). This strikes me as fragile.

> * cvs-clobber.patch
> Don't do a clobber if clobber=0, just do an update each time instead

[not yet]

Let's talk about this one: I think it shouldn't be necessary. The only case
where it changes the slave's behavior is (clobber=False, copydir=something).
I'm writing up some docs to understand/explain the possibilities better, but
basically I think that (clobber=False, copydir=Something) should have the
same effective behavior as (clobber=False, copydir=None).

> * cvs-cmddir.patch 
> I needed this to pass all the test in test_slavecommand.py

[probably, but not yet]

This one doesn't sound unreasonable, but the test cases don't fail for me.
Could you add a test case that exposes the bug this patch fixes?

> * change-filter.patch 
> Implements an option "filter" in the configuration.
> It's a list of two sized tuples where the first item is a regexp (or
> None to match all) for the builders name, the second is a regexp (or
> None to match all) to the name of the checkin.
> So in our case, we have:
> c['filters'] = [('-plug$', '^gst-plugins'),
>                 ('-gst$', '^gstreamer')]
> 
> Which only rebuilds the plugin builds when a commit is done in
> gst-plugins and the same for gstreamer.

[probably, but not yet]

Sounds useful.. I need to think about it a bit more to make sure it's
sufficiently generalized.

> * event-fix.patch 
> Simple bugfix, it'll trigger an exception once in a while if it's not
> set

[not yet]

Hmm. I'd like to see a copy of those exceptions: they indicate a different
problem. Once the Event is .finish()'ed, nothing more is supposed to be added
to it, and self.entries=None is an indication that entries have been pushed
out to disk (using Swappable), so if someone is doing an .append on that None
then it means they're adding entries when the Event is supposed to be closed.

That said, the code is ugly and needs to be cleaned up (I think the
self.entries=None should probably be inside that if self.doSwap clause
anyway), so I will revisit that class and probably rewrite it a bit.

> * change-links.patch 
> Add an argument to the Change class, a list of links
> * change-html.patch 
> Adds a asHtml method to Change, it's turned on in our local build
> instead of the asText method (in the html status output), but I did not
> include a patch for it

[will apply]

I'll apply this, with some changes (to let the html-status code provide
headers and footers, and HTML-escape the body of the <pre> block). I need to
work on it a bit more before I commit it. Could you provide an example of
what is meant to be passed in the links= argument? The HTML-izing code looks
like it might get confused if it encounters two similar filenames in the same
Change, and it might be better to change the API to pass (file, link) pairs
instead of using separate list arguments for each.

> * compile-halt.patch 
> halt if compilations fail instead of just give a warning. Should be
> useful generally I believe.

[applied]

> * shellcommand-config.patch 
> This one I would prefer to be included, it cleans up the process
> launcher a little bit and minimizes our local changes.

[applied with changes]

I retained the "usePTY=0 on non-posix" code. I believe usePTY needs to be a
slave-local decision, not a master-side one (I think I'll turn it into a
mktap argument anyway, to make life easier for the not-quite-Posix-enough
Solaris buildslaves that fail when usePTY=1).

I'd like to know what your use case is for this. I'm working on replacing the
ugly split-on-spaces command string with a sensible argv list. We can make
useSH=1 something that the buildmaster can control, and then have the command
take a string (like it does now) instead of a list (like it will in the
future), but if you're doing stuff like command += ">out.txt &" or command =
"make foo | grep ERROR" then we need to make sure the new scheme can
accomplish whatever you're doing with the old one.

It may be easiest to have the master-side ShellCommand's 'command=' argument
take either a string (in which case it is passed to /bin/sh) or a list (in
which case it gets passed directly to execve).

Finally, I want to make sure we don't do anything to make the w32 buildslave
harder to get working, and I suspect /bin/sh is not an option there.

> * syncmail-rewrite.patch 
> Rewrote the syncmail parsing totally, since the last one seemed to be
> quite broken. 

[not yet]

I'm willing to believe that, and apply the patch, but the current test cases
pass with the old code and fail with the patch. Could you provide a test case
which shows the bug this fixes? It may be that we've got messages from
different versions of syncmail, in which case we need to fix the parser to
handle either kind.

> * slave-basedir.patch 
> Checks for $BASEDIR in the command and environment. We're using it like
> this:
> # GStreamer plugins
> configure = './autogen.sh --prefix=$BASEDIR/prefix'
> configureEnv = {'PKG_CONFIG_PATH': '$BASEDIR/prefix/lib/pkgconfig'}
> 
> * install-steps.patch 
> Adds two new steps, for Install and Uninstall

[not yet]
[not yet]

I need to look at these closer.. ran out of time for today.

thanks!
 -Brian