Bug 72128 – CMake color output is missing when using build-webkit

• Component: Tools / Tests
• Status: UNCONFIRMED

[Ming Xie]

Currently, we're calling 'cmake --build' in buildCMakeGeneratedProject. According to this post:  
http://www.cmake.org/pipermail/cmake/2009-December/034051.html, 'cmake --build' will remove the nice color output for the 'make' command.

I'm considering to replace 'cmake --build' with the 'make' command. However, I would like to make sure such a change won't cause build failure to other ports using CMake. (e.g. Efl and WinCE)

[Antonio Gomes]

Ming, you can upload a patch and click APPLY TO EWS, and it will build the bots.

[Patrick R. Gansterer]

cmake --build . hides all the complexity of the build system (like devenv.exe / vsexpress.exe) on windows. So I'm not a big fan of such a change. I miss the colourfull output too, but I still prefere cmake --build since it's the same on _all_ platforms.

[Ming Xie]

(In reply to comment #2)
> cmake --build . hides all the complexity of the build system (like devenv.exe / vsexpress.exe) on windows. So I'm not a big fan of such a change. I miss the colourfull output too, but I still prefere cmake --build since it's the same on _all_ platforms.


An alternative solution is to add a CMake color filter in filter-build-webkit script.

I was debating which way is better as I found the root cause of this problem from the post. But since now we have concerns about portability on all platforms, I will finish my patch on filter-build-webkit.

Thanks for the feedback!

[Ming Xie]

Created attachment 114820 [details]
Patch

[Ming Xie]

(In reply to comment #4)
> Created an attachment (id=114820) [details]
> Patch

This patch adds a new subroutine processOutputWithCMakeColor() in filter-build-webkit.

Adding Matt Delayney to CC as he is the original author of filter-build-webkit.


(p.s. This is my first patch to WebKit.org, please leave me comments if there is anything I should fix or improve. Thanks!)

[Patrick R. Gansterer]

(In reply to comment #5)
> This is my first patch to WebKit.org, please leave me comments if there is anything I should fix or improve.

So a warm welcome here at the webkit community! (Hoping to see many patches in the future :-) )

Every patch needs a ChangeLog entry. See http://www.webkit.org/coding/contributing.html for more infor. Tools/Scripts/webkit-patch is your friend.


> This patch adds a new subroutine processOutputWithCMakeColor() in filter-build-webkit.

Only personal opinion: IMHO this hasn't to do anything with CMake. You only do some Regexp on the output. The original CMake colors depend on the real target type. IMHO the correct way to do this is a "--color" switch in the cake --build command. But it's harder to do there. ;-)
If you still want to do it this way I'd prefer to change the name of the parameter to "--color" only, since it's not CMake related and can be implemented for the other build systems too.

[Daniel Bates]

Comment on attachment 114820 [details]
Patch

r- since missing change log. As aforementioned by Patrick Gansterer, see <http://www.webkit.org/coding/contributing.html>.

[Ming Xie]

Created attachment 114877 [details]
Patch

[Ming Xie]

(In reply to comment #8)
> Created an attachment (id=114877) [details]
> Patch

Updated patch with ChangeLog entry.


(In reply to comment #6)
> Only personal opinion: IMHO this hasn't to do anything with CMake. You only do some Regexp on the output. The original CMake colors depend on the real target type. IMHO the correct way to do this is a "--color" switch in the cake --build command. But it's harder to do there. ;-)
> If you still want to do it this way I'd prefer to change the name of the parameter to "--color" only, since it's not CMake related and can be implemented for the other build systems too.

There is already an option(--[no]-color) in filter-build-webkit script. Therefore, I can take out my --useCMakeColor option if you guys believe this is ok.

Thanks for the warm welcome btw. :)

[Patrick R. Gansterer]

Comment on attachment 114877 [details]
Patch

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

> Tools/ChangeLog:9
> +        Add a new option("--useCMakeColor") in filter-build-webkit script. 
> +        This will add colors to the CMake output from build-webkit script.
> +
> +        Reviewed by NOBODY (OOPS!).

FYI: Usually the "long description" is after the "Reviewed by" line.

> Tools/Scripts/filter-build-webkit:103
> +  --useCMakeColor Enable CMake color scheme for filtering

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

[Daniel Bates]

Comment on attachment 114877 [details]
Patch

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.

[Soo-Hyun Choi]

build-webkit still doesn't allow us to print coloured output. anyone interested in working on this yet?

or, "cmake --build" inevitablly erase coloured output as paroga mentioned in https://bugs.webkit.org/show_bug.cgi?id=72128#c2 ??

[Patrick R. Gansterer]

https://github.com/Kitware/CMake/pull/58 fixes this problem in CMake. So all we have to do is to add a --use-stderr to the "cmake --build" call. I'll post a patch for build-webkit once the CMake change landed.

[Ming Xie]

(In reply to comment #13)
> https://github.com/Kitware/CMake/pull/58 fixes this problem in CMake. So all we have to do is to add a --use-stderr to the "cmake --build" call. I'll post a patch for build-webkit once the CMake change landed.

Awesome! FInally CMake fixes this. :-)

[Alexey Proskuryakov]

Please note that escape sequences in output cause trouble for EWS - browsers download build logs instead of displaying them inline.

So, any solution that adds color to build output needs to only do that when attached to an actual terminal, not when piping.