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

[Roch Gadsdon]

On 2 Aug 2007, at 02:28, Greg Ward wrote:

> On 8/1/07, Roch Gadsdon <rochg at bakbone.co.uk> wrote:
>> Hopefully better late than never, I have posted a revised patch that
>> I think takes on board the discussion from last time. There is a new
>> intermediate class between ShellCommand and Compile/Test that allows
>> for a regular expression to be used to specify what counts as a
>> warning. By default this is set to "warning[: ]", but the user can
>> override this in their config file when adding a step.
>
> I still think the right regex to use is
>
>   \bwarning\b
>

> .  Any sane compiler output should match that.  And any build command
> that emits text that matches that regex is worth checking out, even if
> it's not a compiler.
>

OK, let's run with \bwarning'b

BTW, in C/C++ land some of the linkers manage to spit out warnings  
that don't include "warning". The lines are recognisable but would  
need another clause in the regex e.g. "^ld: ". We include those in  
our regex here. I was toying with adding those to the default on the  
basis that it's easy to override if it ever false hits and for some  
folks that would be a nice thing for BB to do by default. I've left  
it out for now though. Might be more something to mention as an  
option in the documentation.

> Also, prefixing the regex with ".*" and using match() makes life
> harder for people providing their own regex pattern.  Use search()
> when checking if a line matches the regex, drop the leading ".*", and
> life will be easier.  If someone really truly only wants to match
> lines that start with "warning", they can use "^warning".
>

This may be just me, but I think I prefer the idea of writing a regex  
to match the line rather than a sub-section of the line. As either  
approach can be turned into either a sub-string match or a full line  
match are you passionate on preferring search()?

> Finally, I really don't think WarningCountingShellCommand is a good
> idea.  What if someone else has provided a different subclass of
> ShellCommand that I also need to use?  The strict OO design would be
> to implement a WarningCounter class, and have every ShellCommand carry
> an instance of WarningCounter around.  The default instance does
> nothing; you just have to make it really easy to provide an instance
> that actually scans for /\bwarning\b/.
>
> A practical Pythonic design might be simpler than this, e.g. just add
> a small checkWarning() method to ShellCommand which does nothing if no
> warning pattern was supplied.  It all depends on 1) how complex
> checkWarning() is and 2) how likely someone is to want to drop in a
> completely different implementation without having to subclass
> ShellCommand.  Strict OO design is a bit higher overhead, but it
> really has a lot going for it.  (Or maybe I've been hanging around
> with Java programmers at work too long. ;-)
>

When writing the code this way round, my thinking was that I may be  
doing the wrong thing by placing the warning detection code in  
ShellCommand as it wasn't applicable to all shell commands. I know  
you could switch it off by having a default that did nothing but I  
thought that may be starting to clutter up ShellCommand with code  
that didn't belong. (In my case, I definitely haven't spent enough  
time hanging around Java programmers ;-)

That said, I'd love to get the patch into the state where it has the  
best chance of entering the BB code, so I'm very happy to be guided.  
D'you still think it needs to switch to a checkWarning() method in  
ShellCommand?

Lastly, to try and get this finished off I would propose:

- Make the default warning .*\bwarning\b.*
- Leaving the regex matching code using match()
- Going with what you're happy with on the structure of the code i.e.  
WarningCountingShellCommand or altenatives

Would you feel happy with that as a final version?

Thanks again for the help,

-- Roch