[Webkit-unassigned] [Bug 72128] CMake color output is missing when using build-webkit

[bugzilla-daemon at webkit.org]

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


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114877|review?                     |review-
               Flag|                            |




--- Comment #11 from Daniel Bates <dbates at webkit.org>  2011-11-14 22:28:09 PST ---
(From update of attachment 114877)
View in context: https://bugs.webkit.org/attachment.cgi?id=114877&action=review

This patch is good. I have some remarks on how we can improve this patch.

> Tools/Scripts/filter-build-webkit:46
> +    CMAKE_STYLE_RED => 4,
> +    CMAKE_STYLE_GREEN => 5,
> +    CMAKE_STYLE_BLUE => 6,
> +    CMAKE_STYLE_CYAN=> 7,

Can we give these variables names that better reflect their purpose? For instance, we use style CMAKE_STYLE_GREEN whenever we are building/processing a source file. Maybe we should rename CMAKE_STYLE_GREEN to CMAKE_STYLE_BUILDING_SOURCE?

> Tools/Scripts/filter-build-webkit:75
>  sub setLogfileOption($$);
>  sub setOutputFormatOption($$);
>  sub usageAndExit();
> +sub processOutputWithCMakeColor($);

These forward declarations should be in sorted order.

>> Tools/Scripts/filter-build-webkit:103

> 
> I think merging with --[no-]color makes sence.

As far as I can tell the regular expressions for Xcodebuild don't overlap with the regular expressions for CMake. So, we could merge the CMake coloring mode into the command-line option color.

> Tools/Scripts/filter-build-webkit:229
> +    if ($line =~ /.*(Linking CXX|error:).*/) {

Does this regular expression match when linking non-C++ files (e.g. C files)? Would it be sufficient to match for "Linking"?

It's unnecessary to have ".*" at the beginning and end of the regular expression as Perl regular expressions match a substring. Also, the parentheses in the regular expression aren't necessary and don't seem to improve the readability of this expression. So, I would write the regular expression in this line as: /Linking CXX|error:/.

See <http://perldoc.perl.org/perlrequick.html#The-Guide> and <http://perldoc.perl.org/perlre.html> for more details.

> Tools/Scripts/filter-build-webkit:230
> +        printLine("$line", CMAKE_STYLE_RED);

It's sufficient to pass $line directly as the first argument of printLine() instead of using string interpolation:

printLine($line, CMAKE_STYLE_RED);

Notice the lack of double quotes around the first argument of printLine().

> Tools/Scripts/filter-build-webkit:231
> +    } elsif ($line =~ /.*(Building|Compiling|Preprocessing) CXX (object|source).*/) {

Does this regular expression match for non-C++ files (e.g. C files)?

As mentioned in the remark for line 229, it's unnecessary to prefix and append ".*" to the regular expression.

> Tools/Scripts/filter-build-webkit:232
> +        printLine("$line", CMAKE_STYLE_GREEN);

Remove the quotes around variable $line. See the remark on line 229 for more details.

> Tools/Scripts/filter-build-webkit:233
> +    } elsif ($line =~ /.*Generating.*/) {

The regular expression on this line can be written:

/Generating/

> Tools/Scripts/filter-build-webkit:234
> +        printLine("$line", CMAKE_STYLE_BLUE);

Remove the quotes around variable $line. See the remark on line 229 for more details.

> Tools/Scripts/filter-build-webkit:235
> +    } elsif ($line =~ /.*(Run.*(CPack|CMake)|Install.*project|Available install components).*/) {

Do we really want to match zero or more arbitrary characters between "Run" and "CPack", and "Run" and "CMake"? Would it be sufficient to match whitespace (\s)? Can we make this regular expression stronger?

A similar argument can be made for "Install.*project".

> Tools/Scripts/filter-build-webkit:236
> +        printLine("$line", CMAKE_STYLE_CYAN);

Remove the quotes around variable $line. See the remark on line 229 for more details.

> Tools/Scripts/filter-build-webkit:238
> +        printLine("$line", STYLE_PLAIN);

Remove the quotes around variable $line. See the remark on line 229 for more details.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.