[webkit-reviews] review denied: [Bug 44081] Add standalone script that filters the output of build-webkit to be more human-readable : [Attachment 106446] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 6 21:59:54 PDT 2011
Daniel Bates <dbates at webkit.org> has denied Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 44081: Add standalone script that filters the output of build-webkit to be
more human-readable
https://bugs.webkit.org/show_bug.cgi?id=44081
Attachment 106446: Patch
https://bugs.webkit.org/attachment.cgi?id=106446&action=review
------- Additional Comments from Daniel Bates <dbates at webkit.org>
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#L29
4> and one example call site is
<http://trac.webkit.org/browser/trunk/Tools/Scripts/run-api-tests?rev=93240#L18
6>.
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\"";
}
More information about the webkit-reviews
mailing list