Bug 163868 – [CMake] generate-bindings-all.pl uses USES_TERMINAL which leaves a noisy line in interactive Ninja build

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

We used to have a ninja build that was silent until the final stages of the build, when generating GtkDoc. That's no longer the case:

[0/4006] Generate bindings (WebCoreBindings)
Preprocess IDL
[34/4006] Generate bindings (WebKitTestRunnerInjectedBundleBindings)
[35/3990] Generate bindings (WebKitTestRunnerBindings)
[197/3988] Building CXX object Source/...vaScriptCore.dir/b3/B3BasicBlock.cpp.o^C

I don't think this extra output is helpful. It could be hidden behind some debug or verbose build flag, or just removed.

[Michael Catanzaro]

Created attachment 292545 [details]
Patch

[Fujii Hironori]

preprocess-idls.pl is executed unnecessarily because of
WriteFileIfChanged of preprocess-idls.pl, which is not useful
anymore but harmful for CMake-based port.
This is a bug I plan to fix.

[Fujii Hironori]

I can not test you patch now. I think your patch does not solve following output:

> [34/4006] Generate bindings (WebKitTestRunnerInjectedBundleBindings)
> [35/3990] Generate bindings (WebKitTestRunnerBindings)

Isn't it?

[Michael Catanzaro]

(In reply to comment #3)
> I can not test you patch now. I think your patch does not solve following
> output:
> 
> > [34/4006] Generate bindings (WebKitTestRunnerInjectedBundleBindings)
> > [35/3990] Generate bindings (WebKitTestRunnerBindings)
> 
> Isn't it?

Hm, you must be right; I didn't try a clean build.

[Fujii Hironori]

I tested your patch.
This is a incremental build log without any source code change.

> fujii@localhost $ ./Tools/Scripts/build-webkit --gtk --debug --64-bit
> [0/949] Generate bindings (WebCoreBindings)
> [2/38] Generate bindings (WebKitTestRunnerInjectedBundleBindings)
> [3/22] Generate bindings (WebCoreTestSupportBindings)
> [4/10] Generate bindings (WebKitTestRunnerBindings)
> [7/7] cd /home/fujii/work/webkit/w1/WebKitBuild/Debug/Tools/T...rivedSources/ForwardingHeaders --platform gtk --platform soup
> 
> ====================================================================
>  WebKit is now built (00m:02s). 
>  To run MiniBrowser with this newly-built code, use the
>  "Tools/Scripts/run-minibrowser" script.
> ====================================================================
> fujii@localhost $

Your patch does not suppress the build log because I specify
USES_TERMINAL option.

[Fujii Hironori]

Created attachment 292574 [details]
Patch

[Fujii Hironori]

Created attachment 292575 [details]
Patch

[Fujii Hironori]

Created attachment 292576 [details]
Patch

* Preserve BYPRODUCTS option which is needed for Ninja

[Fujii Hironori]

Michael,

I think the output of generate-bindings-all.pl is helpful to know
which IDL bindings are regenerated.
Are the following four lines annoying? That is issued by Ninja.

> [0/949] Generate bindings (WebCoreBindings)
> [2/38] Generate bindings (WebKitTestRunnerInjectedBundleBindings)
> [3/22] Generate bindings (WebCoreTestSupportBindings)
> [4/10] Generate bindings (WebKitTestRunnerBindings)

[Michael Catanzaro]

(In reply to comment #9)
> Michael,
> 
> I think the output of generate-bindings-all.pl is helpful to know
> which IDL bindings are regenerated.
> Are the following four lines annoying? That is issued by Ninja.
> 
> > [0/949] Generate bindings (WebCoreBindings)
> > [2/38] Generate bindings (WebKitTestRunnerInjectedBundleBindings)
> > [3/22] Generate bindings (WebCoreTestSupportBindings)
> > [4/10] Generate bindings (WebKitTestRunnerBindings)

I don't see why it's helpful. I would indeed prefer these to be silenced.

[Fujii Hironori]

For example, a new IDL StaticRange.idl was added in
<http://trac.webkit.org/changeset/207670>.  If a new IDL is
added, supplemental_dependency.tmp will be updated.

In this case, non-CMake-based port (Mac port) regenerates all
bindings. See this build log:

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20%28Build%29/builds/11132/steps/compile-webkit/logs/stdio

On the other hand, in CMake-based port regenerating bindings
which is needed.

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Build%29/builds/64008/steps/compile-webkit/logs/stdio

> [1/3] InputEvent.idl
> [2/3] StaticRange.idl
> [3/3] DOMWindow.idl

We can confirmed three IDL files was processed by seeing build log of
BuildBot.

[Michael Catanzaro]

But the buildbots print verbose output. I don't mind if it shows up in the logs on the buildbot, I just don't want to see it on a separate line when running ninja myself. :)

We should also clean up GtkDoc to not do this, but that's a preexisting issue.

[Fujii Hironori]

I'll try to find a solution.

[Fujii Hironori]

Created attachment 293519 [details]
Patch

[Michael Catanzaro]

Comment on attachment 293519 [details]
Patch

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

> Source/cmake/WebKitMacros.cmake:48
> +option(SHOW_PROGRESS_OF_GENERATE_BINDINGS "Show progress of generating bindings" ON)

OK, thanks. But please name it SHOW_BINDINGS_GENERATION_PROGRESS and change the default to OFF. Then it can be turned on on the bots.

[Fujii Hironori]

Created attachment 293525 [details]
Patch

Thank you for review.
* Renamed to SHOW_BINDINGS_GENERATION_PROGRESS
* Set OFF as default SHOW_BINDINGS_GENERATION_PROGRESS
* Enable SHOW_BINDINGS_GENERATION_PROGRESS unless interactive Ninja build

[Fujii Hironori]

Comment on attachment 293525 [details]
Patch

Oops. I mistakenly clear r+ flag.

[Michael Catanzaro]

Comment on attachment 293525 [details]
Patch

Thank you for working on this!

[WebKit Commit Bot]

Comment on attachment 293525 [details]
Patch

Clearing flags on attachment: 293525

Committed r208220: <http://trac.webkit.org/changeset/208220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.