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

[Greg Ward]

On 7/11/07, Roch Gadsdon <rochg at bakbone.co.uk> wrote:
> I have uploaded a patch we use to let BuildBot spot warnings in
> Compile and Test steps. Hopefully this sort of thing is useful to the
> BB community and might make it's way to the permanent code base.
>
> Any comments on problems or how to make this better would be
> appreciated.

I have general Python comments, nothing BuildBot-specific.

+try:
+    import cStringIO
+    StringIO = cStringIO.StringIO
+except ImportError:
+    from StringIO import StringIO

I think it's safe to assume that cStringIO is available everywhere
these days -- hasn't it been in the standard library since at least
1.5.2, which came out almost a decade ago?  And even if you can't
assume that, you can tighten that code up a smidge:

  try:
      from cStringIO import StringIO
  except ImportError:
      from StringIO import StringIO

+    def createSummary(self, log):
[...]
+        for line in StringIO(log.getText()).readlines():


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

?

+        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.)

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

?

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.

Greg