433874 - mozilla-central tinderbox logs don't show commands that are being executed

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[(not currently active) Ted Mielczarek]

This kind of sucks. For one, it makes it really hard to determine some things, like if we built a PGO build. It also means I can't search the log for 'buildsymbols' to find the breakpad symbol store build.

Comment 1

[[e-mail] (:bhearsum)]

Yeah, this sucks. I'll see what I can do with this.

Comment 2

[[e-mail] (:bhearsum)]

Here's a run with some dumping enabled:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1212765839.1212766063.32611.gz&fulltext=1

Unless there's objections I'll clean-up and post this patch.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: make tinderboxmailnotifier print the same log headers that buildbot does

This makes the log output very very similar to that on the Waterfall. Mostly, this is adding a distinct header to each BuildStep, and s/log.getText()/log.getTextWithHeaders()/

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 324058 [details] [diff] [review]
make tinderboxmailnotifier print the same log headers that buildbot does

+                if not re.compile(".*[^\s].*").match(shortText):

If you're going to use re.compile, then do it once and save the result in a var, otherwise just use re.match directly.

Looks like you're compressing just the log sections and not the entire log, so r- for that. Should be able to build the entire log and then compress once at the end.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: [checked in] again

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 324078 [details] [diff] [review]
[checked in] again

+                tinderboxLogs += "======== BuildStep started ========\n"
+                tinderboxLogs += shortText
+                tinderboxLogs += "=== Output ===\n"
+                for log in logs:
+                    tinderboxLogs += log.getTextWithHeaders()
+
+                tinderboxLogs += "=== Output ended ===\n"
+                tinderboxLogs += "======== BuildStep ended ========\n"

Personally I'd do this as either a triple-quoted string with interpolation for the two variable bits, or as one big ''.join(list of strings). That's my stylistic preference, anyway. I'll leave issues of style up to your choice here, though.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 324078 [details] [diff] [review]
[checked in] again

Checking in buildbot/status/tinderbox.py;
/cvsroot/mozilla/tools/buildbot/buildbot/status/tinderbox.py,v  <--  tinderbox.py
new revision: 1.10; previous revision: 1.9
done

Comment 8

[[e-mail] (:bhearsum)]

Needs a Buildbot master restart to pick this up, I'll do that when there is some idle time.

Comment 9

[[e-mail] (:bhearsum)]

Okay, done.

Comment 10

[[e-mail] (:bhearsum)]

This caused bug 438002, backing it out until a suitable workaround is found.

Comment 11

[[e-mail] (:bhearsum)]

Attachment: [checked in] ignore invalid TinderboxPrint statements

I'm pretty sure this is the best fix for this. We could require that TinderboxPrint's be done in a BuildStep subclass (and therefore not end up in the summary), but I think that's silly.

This should fix bug 438002.

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 324301 [details] [diff] [review]
[checked in] ignore invalid TinderboxPrint statements

Any chance you can get to this soon?

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 324301 [details] [diff] [review]
[checked in] ignore invalid TinderboxPrint statements

+                if re.match(".+TinderboxPrint.*", shortText):

I don't think you really need a regex here, you're just looking for a literal string. Why not shortText.find("TinderboxPrint") != -1 ?

Otherwise looks good.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to comment #13)
> (From update of attachment 324301 [details] [diff] [review])
> +                if re.match(".+TinderboxPrint.*", shortText):
> 
> I don't think you really need a regex here, you're just looking for a literal
> string. Why not shortText.find("TinderboxPrint") != -1 ?
> 
> Otherwise looks good.
> 

Yeah, you're right.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 324301 [details] [diff] [review] [details])
> > +                if re.match(".+TinderboxPrint.*", shortText):
> > 
> > I don't think you really need a regex here, you're just looking for a literal
> > string. Why not shortText.find("TinderboxPrint") != -1 ?
> > 
> > Otherwise looks good.
> > 
> 
> Yeah, you're right.
> 

Hm, on second thought, I'm pretty sure I need a regex here. I'm only looking for TinderboxPrints that *aren't* at the start of the line, which lets us ignore them in the BuildStep headers.

Does that make sense?

Comment 16

[(not currently active) Ted Mielczarek]

You could use find and test != 0 and != -1, or just use the regex if you're happy with that. It's not a big deal.

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 324301 [details] [diff] [review]
[checked in] ignore invalid TinderboxPrint statements

Checking in tinderbox.py;
/cvsroot/mozilla/tools/buildbot/buildbot/status/tinderbox.py,v  <--  tinderbox.py
new revision: 1.12; previous revision: 1.11
done

Comment 18

[[e-mail] (:bhearsum)]

Working properly this time.