[Buildbot-devel] Patch to detect warnings in Test and Compile steps

[Roch Gadsdon]

Thanks very much for the helpful comments Greg. It's great to have  
some feedback.

>
> I think it's safe to assume that cStringIO is available everywhere
> these days
[...]
> ...having said all that, this seems like a perverse way to abuse
> StringIO.  Isn't this an expensive, roundabout, and slow way to say
>
>     for line in log.getText().split("\n"):
>

Thanks. I will change to this approach.

>
> +        linkregexp = re.compile('.*: *warning.*')
> [...]
> +            if line.find("warning:") != -1 or None !=  
> linkregexp.match(line):
>
> Why do you need both a substring check and a regex test?  And why is
> that variable called "linkregexp" -- what do warnings have to do with
> links?  And what links, anyways?  (Or is it linking ... in which case,
> this smells like it's specific to C/C++ programming.)
>

OK, I can switch to just using one regexp. The code grew from a gcc  
compile check (the sub-string check) to also include a C language  
linking check (the regexp) but one regexp is better.

The point about being C/C++ specific is interesting. We are currently  
using BuildBot for C code. I keyed off the TODO in the Compile code  
saying "grep for the characteristic GCC warning/error lines...". That  
seemed to imply that catching some common errors was acceptable as a  
default even if Compile wouldn't always be used with gcc or with C code.

However, a couple of possibly better approaches that come to mind:

a) Provide derivations of Compile and Test that are oriented towards  
C/C++ code

b) Allow for a set of regexp patterns to be specified for Compile and  
Test steps. All of these pattterns are checked when looking for  
warnings. We could provide some default patterns that catch common  
cases for C/C++/whatever, but the user then has the freedom to  
override the patterns when they setup the step in their config file.  
Possibly introduces a class between ShellCommand and Compile/Test to  
share the functionality.

Any opinions from anyone on the best way to go?

> Also, you're compiling that regex way more often than you need to.
> How about this:
>
>     _warning_regex = re.compile("\bwarning\b")
>
>     def createSummary(self, log):
>     [...]
>         if self._warning_regex.search(line):
>

Thanks. That's better.

>
> Finally, you have two copies of createSummary() and evaluateCommand()
> in your patch -- presumably methods of two different classes?
> Regardless, they are identical.  I can't speak for BuildBot, but copy
> and paste programming is unacceptable in any project that I have a say
> in.  I think you need to refactor a bit.
>

I don't like cut and paste and this looks wrong in hindsight :( Too  
eager to contribute back something that was OK internally but for not  
for the ongoing project. I'll share the functionality.


I'll gather any feedback on the points here and then post a revised  
patch.

Thanks again,

-- Roch Gadsdon