Bug 44081 – Add standalone script that filters the output of build-webkit to be more human-readable

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Paul Knight]

The output of the build-webkit script can be a bit difficult to read. It would be nice if there was some way to make it easer to scan for build errors and remove some of the often unnecessarily verbose output.

[Paul Knight]

Created attachment 64532 [details]
Work in progress patch that adds a filter-build-webkit script

[Paul Knight]

Created attachment 64533 [details]
Example of output from filter-build-webkit script

Filtering SnowLeopard Intel buildbot output, http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/15626/steps/compile-webkit/logs/stdio/text

[Mark Rowe (bdash)]

I’m ok with hiding parts of the output so long as they’re available behind a disclosure triangle or similar.  The “unnecessarily versbose” output is vital for tracking down certain types of build issues.

[Zoltan Horvath]

Actually, I like our output... It's easy to solve build issues, because you see what was the command, you can just copy-paste and try it. In my opinion, this filtering can be an extra option e.g. --simple-build-output or --filtered-build-output to build-webkit instead of making a new file for this.

[Paul Knight]

The advantage of having it as a separate step is it can be used to filter the build output after the fact, for example, from a buildbot or console log. Of course, that doesn't mean build-webkit couldn't have a flag to enable filtering.

Mark, I like the idea of a disclosure triangle.

[Matthew Delaney]

*** Bug 67513 has been marked as a duplicate of this bug. ***

[Matthew Delaney]

Created attachment 106200 [details]
Patch

[Matthew Delaney]

This patch is the latest version of what Paul had a year ago. I find it incredibly useful for building for myself on the command line. 

I think there are at least 2 good reason to have it be its own file:
1) It provides a nice way to filter old build logs
2) Build scripts other than build-webkit can benefit from it.

I've filed https://bugs.webkit.org/show_bug.cgi?id=67523 as a followup for getting an option for build-webkit to use this script.

[Eric Seidel (no email)]

Comment on attachment 106200 [details]
Patch

Looks sane enough to me.  Adam Roben or Dan Bates are better perl reviewers than I.

[Eric Seidel (no email)]

Adam Roben, Dan Bates, and Darin Adler all speak perl much better than I.

[Zoltan Horvath]

Comment on attachment 106200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106200&action=review

> Tools/Scripts/filter-build-webkit:1
> +#! /usr/bin/env perl -w

Please remove extra space, and env!
#!/usr/bin/perl -w

> Tools/Scripts/filter-build-webkit:118
> +if (!$getOptionsResult || $showHelp) {

$showHelp is undefined if you haven't given -h/--help parameter, so please use defined($showHelp)

Now, if you execute the standalone script without input and you don't give parameter neither it will wait for standard input in line 130 since the script won't call usageAndExit(). ($getOptionsResult will be always true since GetOptions hasn't got problem with parameter processing and $showHelp is undefined => it will be interpreted as false)

I think it would be nice to diplay the help and exit in this case.


For Qt-port this script paints the build output as red (even for simple make) and paints WebKit is now built... green. I know you did it for mac port for first, but in the future it will be good to support other ports also.

By the way, with the first two change that I suggested the patch looks okay to me.

[Zoltan Horvath]

(In reply to comment #11)

> I think it would be nice to diplay the help and exit in this case.

For example use  -t STDIN:
if ( -t STDIN || defined($showHelp) || !$getOptionsResult)

[Matthew Delaney]

Created attachment 106446 [details]
Patch

[Daniel Bates]

Comment on attachment 106446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106446&action=review

I wish there was a better way to do the filtering. The blacklist approach that this script implements seems fragile. This change looks good. I have some suggestions.

> Tools/Scripts/filter-build-webkit:14
> +# 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of

Apple Computer, Inc. => Apple, Inc.

This license appears to be outdated both in its reference to Apple Computer, Inc. as well as the presence of this third clause. The preferred license block for new files is <http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE>. That being said, you would know best as to which license block Apple uses.

> Tools/Scripts/filter-build-webkit:51
> +    ANSI_BLUE => "\033[34m",
> +    ANSI_GREEN => "\033[32m",
> +    ANSI_RED => "\033[31m",
> +    ANSI_PLAIN => "\033[0m",

Can we use Term::ANSIColor instead of hardcoding the ANSI escape sequences?

One example of using this module and outputting colored output can be seen in the script run-api-tests. In particular, see possiblyColored() <http://trac.webkit.org/browser/trunk/Tools/Scripts/run-api-tests?rev=93240#L294> and one example call site is <http://trac.webkit.org/browser/trunk/Tools/Scripts/run-api-tests?rev=93240#L186>.

Also, see <http://search.cpan.org/~rra/Term-ANSIColor-3.01/ANSIColor.pm> for more details on Term::ANSIColor.

> Tools/Scripts/filter-build-webkit:69
> +    HTML_HEADER =><<HTMLHEADER
> +<html>
> +    <head>
> +        <title>Build Log</title>
> +        <style>
> +            body { font-family: Monaco, monospace; font-size: 10px; color: #666; line-height: 1.5em; }
> +            h2 { margin: 1.5em 0 0 0; font-size: 1.0em; font-weight: bold; color: blue; }
> +            p { margin: 0; padding-left: 1.5em; border-left: 3px solid #fff; }
> +            p.alert { border-left-color: red; color: red; margin: 1.5em 0 0 0; }
> +            p.alert + p { margin: 1.5em 0 0 0; }
> +            p.alert + p.alert { margin: 0; }
> +            p.success { color: green; }
> +        </style>
> +    </head>
> +    <body>
> +HTMLHEADER
> +    ,   

For your consideration, I suggest moving the comma (line 69) after the "<<HTMLHEADER" so as to remove the need for placing the comma on a line of its own:

HTML_HEADER =><<HTMLHEADER,
<html>
        <head>
....
        <body>
HTML_HEADER

> Tools/Scripts/filter-build-webkit:75
> +    HTML_FOOTER =><<HTMLFOOTER
> +    </body>
> +</html>
> +HTMLFOOTER
> +    ,

Similarly, you can move the comma (line 75) to the end of line 71.

> Tools/Scripts/filter-build-webkit:93
> +Usage: @{[ basename($0) ]} [options] [buildlog]

From my understanding square brackets indicate something that is optional. For the first usage option, it seems that having at least one build log is necessary to get anything meaningful out of this script. Moreover, this script can process multiple build logs files because because you used the diamond operator (<>): <http://docstore.mik.ua/orelly/perl/learn/ch06_02.htm>. I suggest conveying this information in the usage message like:

Usage: @{[ basename($0) ]} [options] buildlog1 buildlog2 ...

> Tools/Scripts/filter-build-webkit:97
> +  -o|--output   Path for output (default is standard out)

"(default is standard out)" is inconsistent with the formatting you've used below for default values. In particular, the format used below is: "default: ...". I suggest using the "default:" notation to be consistent with other WebKit scripts, including build-webkit and run-javascriptcore-tests.

> Tools/Scripts/filter-build-webkit:98
> +  -f|--format   Output format (default)

You don't seem to mention that the default output format is text. I take it that you meant to write "... Output format (default: $outputFormat)"

> Tools/Scripts/filter-build-webkit:100
> +                  html: Standalone html document

Nit: Standalone html document => Standalone HTML document

(since HTML is an abbreviation)

> Tools/Scripts/filter-build-webkit:101
> +  --[no-]color  ANSI color output for text (default: auto)

Instead of hardcoding "auto" here I suggest somehow incorporating the value of $colorMode into this line so as to ensure that this usage message is in sync with the default behavior.

Also, it seems extraneous to explicitly have a state for COLORMODE_AUTO. It seems sufficient to just have $colorMode be a boolean value that is initialized to the value of -t STDOUT instead of COLORMODE_AUTO.

> Tools/Scripts/filter-build-webkit:118
> +if ( -t STDIN || defined($showHelp) || !$getOptionsResult) {

An undefined value evaluates to false. So we can substitute $showHelp for defined($showHelp) in this statement.

> Tools/Scripts/filter-build-webkit:124
> +open(OUTPUT_HANDLE, ">$outputPath");

We should die with a error if we can't open the file specified by $outputPath.

> Tools/Scripts/filter-build-webkit:125
> +open(LOG_HANDLE, ">$unfilteredOutputPath") if $logUnfilteredOutput;

Similarly, we should die with an error if we can't open the file specified by $unfilteredOutputPath.

Additionally, the name of the handle (LOG_HANDLE) isn't consistent with the name of the value $unfilteredOutputPath. Maybe name the handle UNFILTERED_OUTPUT? or UNFILTERED_OUTPUT_HANDLE?

> Tools/Scripts/filter-build-webkit:129
> +my $buildFinished = 0;

It's unnecessary to explicitly initialize a variable to 0 when it will be used as a boolean since an undefined value evaluates to false. I would write this line as:

my $buildFinished;

> Tools/Scripts/filter-build-webkit:133
> +    chomp($line);

Do we need to concern ourselves with CRLF line feeds? What are the line endings output under Cygwin when building a Visual Studio project with VCExpress or Visual Studio?

> Tools/Scripts/filter-build-webkit:138
> +    }
> +    elsif ($line =~ /^===/) {

We tend to follow the WebKit Code Style Guidelines for Perl code.  So, the elsif statement should be on the same line as the closing brace.

> Tools/Scripts/filter-build-webkit:141
> +    }
> +    elsif ($line =~ /Checking Dependencies|Check dependencies/) {

Ditto.

> Tools/Scripts/filter-build-webkit:144
> +    }
> +    elsif ($line =~ /\*\* BUILD SUCCEEDED \*\*/) {

Ditto.

> Tools/Scripts/filter-build-webkit:148
> +        (my $command, my $path) = ($1, basename($2));

I would write this as:

my ($command, $path) = ($1, basename($2));

> Tools/Scripts/filter-build-webkit:164
> +    elsif ($line =~ /^\s*$/) {} # ignore
> +    elsif ($line =~ /^Build settings from command line:/) {} #ignore
> +    elsif ($line =~ /make: Nothing to be done for `all'\./) {} # ignore
> +    elsif ($line =~ /^JavaScriptCore\/create_hash_table/) {} # ignore
> +    elsif ($line =~ /JavaScriptCore.framework\/PrivateHeaders\/create_hash_table/) {} # ignore
> +    elsif ($line =~ /^JavaScriptCore\/pcre\/dftables/) {} # ignore
> +    elsif ($line =~ /^Creating hashtable for /) {} # ignore
> +    elsif ($line =~ /^Wrote output to /) {} #ignore
> +    elsif ($line =~ /^(touch|perl|cat|rm -f|bison|flex|python|\/usr\/bin\/g\+\+|gperf|echo|sed|if \[ \-f|WebCore\/generate-export-file) /) {} # ignore
> +    elsif ($line =~ /^UNDOCUMENTED: /) {} # ignore
> +    elsif ($line =~ /libtool.*has no symbols/) {} # ignore
> +    elsif ($line =~ /^# Lower case all the values, as CSS values are case-insensitive$/) {} # ignore
> +    elsif ($line =~ /^if sort /) {} # ignore
> +    elsif ($line =~ /^    /) {} # ignore

I would write these as:

next if $line =~ /^\s*$/;
next if $line =~ /^Build settings from command line:/;
next if $line =~ /make: Nothing to be done for `all'\./;
...
next if $line =~ /^if sort /;
next if $line =~ /^    /;

Also, it would be nice if there was an inline comment to explain the regular expression, especially for some of the less obvious ones like /^    /. I take it that /^    / matches the indent used in the setenv and build settings lines that xcodebuild writes out?

> Tools/Scripts/filter-build-webkit:198
> +    my ($opt,$value) = @_;

Nit: Missing a space between $opt and $value.

Also, I sugest renaming $opt to $option.

> Tools/Scripts/filter-build-webkit:205
> +    my ($opt,$value) = @_;

Ditto.

> Tools/Scripts/filter-build-webkit:207
> +    unless ($value eq "html" or $value eq "text") {

Using "unless"/negating expressions  tends to be more difficult to read that pushing the negation through. I suggest we push the negation through and write this as:

if ($value ne "html"  && $value ne "text") {
    die "The $option option must be either \"html\" or \"text\"";
}

[Daniel Bates]

(In reply to comment #11)
> [...]
> > Tools/Scripts/filter-build-webkit:118
> > +if (!$getOptionsResult || $showHelp) {
> 
> $showHelp is undefined if you haven't given -h/--help parameter, so please use defined($showHelp)
> 

$showHelp is being used as a boolean variable throughout this script. By <http://perldoc.perl.org/perlsyn.html#Truth-and-Falsehood>, an undefined value has the same truth value as 0. Hence, it's sufficient to leave this variable undefined and use boolean operations to determine its truth value. That is, we don't need to use defined().

[Mark Rowe (bdash)]

Comment on attachment 106446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106446&action=review

>> Tools/Scripts/filter-build-webkit:14
>> +# 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> 
> Apple Computer, Inc. => Apple, Inc.
> 
> This license appears to be outdated both in its reference to Apple Computer, Inc. as well as the presence of this third clause. The preferred license block for new files is <http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE>. That being said, you would know best as to which license block Apple uses.

Apparently he doesn’t.  I told him a few days ago that he’s using the wrong license file but he seems to have decided not to update it.

[Zoltan Horvath]

(In reply to comment #15)
> (In reply to comment #11)
> > [...]
> > > Tools/Scripts/filter-build-webkit:118
> > > +if (!$getOptionsResult || $showHelp) {
> > 
> > $showHelp is undefined if you haven't given -h/--help parameter, so please use defined($showHelp)
> > 
> 
> $showHelp is being used as a boolean variable throughout this script. By <http://perldoc.perl.org/perlsyn.html#Truth-and-Falsehood>, an undefined value has the same truth value as 0. Hence, it's sufficient to leave this variable undefined and use boolean operations to determine its truth value. That is, we don't need to use defined().

You're absolutely right! I like defined() because it is suggesting that the variable can be undefined and that fact can be useful for later usages. :) I'm okay with boolean operations here, since we don't need $showHelp after.

[Matthew Delaney]

Created attachment 106626 [details]
Patch

[Matthew Delaney]

Thanks for all the good stuff, Daniel! Here's a new patch up for review that incorporates all the input (including that missed from elsewhere).

[Daniel Bates]

Comment on attachment 106626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106626&action=review

This patch looks even better. I have a few more remarks.

> Tools/Scripts/filter-build-webkit:57
> +    HTML_FOOTER =><<HTMLFOOTER,

Nit: I would add an empty line before this line to help visually separate these constants and make clear where one constant ends and another begins (since both the here-document ending line, HTMLHEADER, and HTML_FOOTER are capitalized and have a prefix of HTML).

> Tools/Scripts/filter-build-webkit:111
> +open(OUTPUT_HANDLE, ">$outputPath") or die "Failed to open $outputPath";
> +if ($logUnfilteredOutput) {
> +    open(UNFILTERED_OUTPUT_HANDLE, ">$unfilteredOutputPath") or die "Failed to open $unfilteredOutputPath";
> +}

Nit: Even though you only have two file handles and Perl will automatically close them on exit, I suggest adding a corresponding close() call for each open() for good form.

> Tools/Scripts/filter-build-webkit:119
> +    chomp($line);

Does this still give us pretty output for VCExpress and Visual Studio builds? Do we need to strip CRLFs?

> Tools/Scripts/filter-build-webkit:146
> +        next if $line =~ /^\s*$/;
> +        next if $line =~ /^Build settings from command line:/;
> +        next if $line =~ /make: Nothing to be done for `all'\./;
> +        next if $line =~ /^JavaScriptCore\/create_hash_table/;
> +        next if $line =~ /JavaScriptCore.framework\/PrivateHeaders\/create_hash_table/;
> +        next if $line =~ /^JavaScriptCore\/pcre\/dftables/;
> +        next if $line =~ /^Creating hashtable for /;
> +        next if $line =~ /^Wrote output to /;
> +        next if $line =~ /^(touch|perl|cat|rm -f|bison|flex|python|\/usr\/bin\/g\+\+|gperf|echo|sed|if \[ \-f|WebCore\/generate-export-file) /;
> +        next if $line =~ /^UNDOCUMENTED: /;
> +        next if $line =~ /libtool.*has no symbols/;
> +        next if $line =~ /^# Lower case all the values, as CSS values are case-insensitive$/;
> +        next if $line =~ /^if sort /;
> +        next if $line =~ /^    /;

For your consideration, I would move these "next if" lines so that they come after "chomp($line)" (line 119) so as to make it a bit easier to see the control flow of this function.

> Tools/Scripts/filter-build-webkit:165
> +sub possiblyColored($$)
> +{
> +    my ($colors, $string) = @_;
> +
> +    if (-t STDOUT) {
> +        return colored([$colors], $string);
> +    } else {
> +        return $string;
> +    }
> +}

This code is identical to code in run-api-tests. Can we share this code instead of duplicating it? We tend to use VCSUtils.pm as a bit of a dumping ground for such functions even though it primarily encapsulates the version control functionality.

Ultimately we may want to consider either renaming VCSUtils.pm or defining a new module for such utility functions. This can be done in a separate bug.

[Matthew Delaney]

Created attachment 106679 [details]
Patch

[Matthew Delaney]

> Nit: I would add an empty line before this line to help visually separate these constants and make clear where one constant ends and another begins (since both the here-document ending line, HTMLHEADER, and HTML_FOOTER are capitalized and have a prefix of HTML).

Done.

> 
> > Tools/Scripts/filter-build-webkit:111
> > +open(OUTPUT_HANDLE, ">$outputPath") or die "Failed to open $outputPath";
> > +if ($logUnfilteredOutput) {
> > +    open(UNFILTERED_OUTPUT_HANDLE, ">$unfilteredOutputPath") or die "Failed to open $unfilteredOutputPath";
> > +}
> 
> Nit: Even though you only have two file handles and Perl will automatically close them on exit, I suggest adding a corresponding close() call for each open() for good form.

Done.

> > Tools/Scripts/filter-build-webkit:119
> > +    chomp($line);
> 
> Does this still give us pretty output for VCExpress and Visual Studio builds? Do we need to strip CRLFs?

I'm not sure; I don't have a windows build on hand to test. Though, I imagine this script will benefit from many improvements for use on windows and other platforms/ports. Not trying to be lazy, but I'd prefer to just "get it out there" right now for immediate use on cleaning up the build output of at least the mac build bots. I can file follow up bugs for any ideas for improvements.

> 
> > Tools/Scripts/filter-build-webkit:146
> > +        next if $line =~ /^\s*$/;
> > +        next if $line =~ /^Build settings from command line:/;
> > +        next if $line =~ /make: Nothing to be done for `all'\./;
> > +        next if $line =~ /^JavaScriptCore\/create_hash_table/;
> > +        next if $line =~ /JavaScriptCore.framework\/PrivateHeaders\/create_hash_table/;
> > +        next if $line =~ /^JavaScriptCore\/pcre\/dftables/;
> > +        next if $line =~ /^Creating hashtable for /;
> > +        next if $line =~ /^Wrote output to /;
> > +        next if $line =~ /^(touch|perl|cat|rm -f|bison|flex|python|\/usr\/bin\/g\+\+|gperf|echo|sed|if \[ \-f|WebCore\/generate-export-file) /;
> > +        next if $line =~ /^UNDOCUMENTED: /;
> > +        next if $line =~ /libtool.*has no symbols/;
> > +        next if $line =~ /^# Lower case all the values, as CSS values are case-insensitive$/;
> > +        next if $line =~ /^if sort /;
> > +        next if $line =~ /^    /;
> 
> For your consideration, I would move these "next if" lines so that they come after "chomp($line)" (line 119) so as to make it a bit easier to see the control flow of this function.

Done.

> 
> > Tools/Scripts/filter-build-webkit:165
> > +sub possiblyColored($$)
> > +{
> > +    my ($colors, $string) = @_;
> > +
> > +    if (-t STDOUT) {
> > +        return colored([$colors], $string);
> > +    } else {
> > +        return $string;
> > +    }
> > +}
> 
> This code is identical to code in run-api-tests. Can we share this code instead of duplicating it? We tend to use VCSUtils.pm as a bit of a dumping ground for such functions even though it primarily encapsulates the version control functionality.
> 

Done.

> Ultimately we may want to consider either renaming VCSUtils.pm or defining a new module for such utility functions. This can be done in a separate bug.

https://bugs.webkit.org/show_bug.cgi?id=67753

[Daniel Bates]

Comment on attachment 106679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106679&action=review

Colors in the air. Oh, everywhere. It comes in colors!

> Tools/Scripts/filter-build-webkit:113
> +open(OUTPUT_HANDLE, ">$outputPath") or die "Failed to open $outputPath";

Nit: I suggest appending the error message (in $!) to the die() message so as to provide a more descriptive reason for the failure. So, I would write this line as:

open(OUTPUT_HANDLE, ">$outputPath") or die "Failed to open $outputPath: $!";

> Tools/Scripts/filter-build-webkit:115
> +    open(UNFILTERED_OUTPUT_HANDLE, ">$unfilteredOutputPath") or die "Failed to open $unfilteredOutputPath";

Nit: Similarly, I would append the error message (in $!) to the die() message.

[Matthew Delaney]

http://trac.webkit.org/changeset/94746