375714 - implement tinderbox log parser

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Robert Helmer [:rhelmer]]

Need to be able to parse tinderbox logs, here are the primary reasons:

* better error detection in the Build/Repack->Verify() step
* figure out where Tinderbox uploaded the files to, to automate the push-to-candidates dir step
* determine the build ID, needed for config file generation (bug 371325)

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: base class method to get build ID from tinderbox log

This is in Step.pm because that's where the other log checking method CheckLog() is. This is untested, just want to get feedback on the general idea before I hook this up.

Comment 2

[J. Paul Reed [:preed]]

Comment on attachment 260364 [details] [diff] [review]
base class method to get build ID from tinderbox log

Couple of things:

$log is going to be pretty big (megabytes), so might want to be careful with stuffing the entire thing into memory.

Also, you could save time parsing by stopping parsing once you find a build id.

I'm not so sure this shoudl go into Bootstrap::Step.

There are other parsing routines we'll need, so maybe a MozBuild::TinderLogParse module is more appropriate.

Can you include a sample (in the comments) of the string you're trying to capture, too?

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: tinderbox log parser

Yeah good call on just making this it's own class. Attached is  a proposed TinderLogParser.pm, usage would look something like:

    my $logParser = TinderLogParser->new(
        log => $buildLog,
    );
    $buildID = $logParser->GetBuildID();

Presumably we'll want to add methods like GetPushDir() (to get the directory tbox pushed to) and an error scanner as well.

This is untested, just trying to get the design right before we go through a lengthy testing round.

Comment 4

[J. Paul Reed [:preed]]

Comment on attachment 260493 [details] [diff] [review]
tinderbox log parser

>##
># TinderLogParse - A tinderbox log parser
>##
>
>sub new {

I think you need a package statement somewhere here.

>    my $proto = shift;
>    my $class = ref($proto) || $proto;
>    my %args = @_;
>
>    my $logFile = $args{'logFile'};
>    if (! defined($logFile)) {
>        die("logFile is a required argument");

I'd put an "ASSERT: TinderLogParse::new: logFile is..."


>    if (not defined($log)) {
>        die("No log file specified");
>    }

In normal usage, would this ever be reached?

>        die("BuildID not found in log file $log");

Should this always die?

>sub GetLogFile() {

I'd call this "GetLogfileName.

Also, no prototype.

Other than that, this is pretty close; would this go in MozUtil::TinderLogParse.pm?

Comment 5

[Robert Helmer [:rhelmer]]

Good stuff, couple responses:

(In reply to comment #4)
> (From update of attachment 260493 [details] [diff] [review])
> In normal usage, would this ever be reached?
> 
> >        die("BuildID not found in log file $log");
> Should this always die?


The caller can handle it however they like, so I think so.

 
> Other than that, this is pretty close; would this go in
> MozUtil::TinderLogParse.pm?


Right, sorry about the missing package name :O

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: address preed's comments

I know you +'d the last one, just wanted to make sure this is what you mean about the "no proto". 

I'd like to go through the other new() methods and do this too, as a separate patch (since it's extraneous)

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: removed proto and die

Going to land this one as per comments above and conversation w/ preed.

Comment 8

[Robert Helmer [:rhelmer]]

Landed:

RCS file: /cvsroot/mozilla/tools/release/MozBuild/TinderLogParse.pm,v
done
Checking in MozBuild/TinderLogParse.pm;
/cvsroot/mozilla/tools/release/MozBuild/TinderLogParse.pm,v  <--  TinderLogParse.pm
initial revision: 1.1
done

Comment 9

[Robert Helmer [:rhelmer]]

I'm sure we'll want to add more methods later, but this I think this is enough for one bug.