Bug 74481 – REGRESSION(r102663): generate-bindings.pl runs every time

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kentaro Hara]

generate-bindings.pl should not run unless any IDL file is updated.

[Kentaro Hara]

[What is happening?]
In Chromium/{Windows, Mac, Linux} build, generate-bindings.pl runs at every build for (not all but) 17 IDL files, e.g. dom/EventListener.idl and dom/EventTarget.idl.

[What is the cause?]
Those 17 IDL files do not need .h and .cpp files. Thus, we stopped generating them at r102663. However, since WebCore.gyp has the build rule that ".h and .cpp files should be generated from each .idl file", WebCore.gyp continues to invoke generate-bindings.pl to generate .h and .cpp files at every build (because generate-bindings.pl never generates them).

[What was written in those _meaningless_ .h and .cpp files before r102663?]
Before r102663, generate-bindings.pl normally generates .h and .cpp files based on the IDL file, but the .cpp file is not included in DerivedSources*.cpp. Actually the content of .h and .cpp files does not matter because they are never compiled. 

[How should we fix it?]
Two options are possible.

(a) Fix build scripts (e.g. WebCore.gyp) so that they do not try to generate .h and .cpp files for those IDLs.
(b) Fix generate-bindings.pl so that it generates .h and .cpp files even for those IDLs. The content of .h and .cpp should be "This file is generated just to prevent build scripts from trying to generate this file at every build" instead of apparently meaningful contents.

I guess that (b) is better. In case of (a), we need to fix all build scripts (for Mac, Qt, Gtk, ...).

[Kentaro Hara]

Created attachment 119154 [details]
Patch

[Kentaro Hara]

Created attachment 119155 [details]
Patch

[Kentaro Hara]

Created attachment 119163 [details]
Patch

[Adam Barth]

Comment on attachment 119163 [details]
Patch

Ok.  This seems like a slight hack, but it seems fine.

[Peter Kasting]

I think comment 1 fix (a) is much better because it's not a hack.

We shouldn't optimize for saving a bit of effort now at the cost of leaving a confusing construction in the build system perpetually.

[Kentaro Hara]

(In reply to comment #6)
>> (a) Fix build scripts (e.g. WebCore.gyp) so that they do not try to generate .h and .cpp files for those IDLs.
>> (b) Fix generate-bindings.pl so that it generates .h and .cpp files even for those IDLs. The content of .h and .cpp should be "This file is generated just to prevent build scripts from trying to generate this file at every build" instead of apparently meaningful contents.
>>
> I think comment 1 fix (a) is much better because it's not a hack.
> 
> We shouldn't optimize for saving a bit of effort now at the cost of leaving a confusing construction in the build system perpetually.

Peter: Thanks for the comment. But I would feel that (b) is better in this case. I am afraid that (a) requires non-trivial efforts not only to me but also to future developers.

- Before r102663, all platforms (Chromium, AppleWebKit, Qt, Gtk, Win, Efl/WinCE/BlackBerry) are generating apparently meaningful but actually unused .h and .cpp files for those 17 IDL files, since those build scripts have a rule like this (e.g. makefile):

    %.h: %.idl
        ....

- Thus, in order to prevent those unused .h and .cpp files from being generated, we need to explicitly write a list of the IDL files in *each* build script like this:

    EXCLUDING_BINDING_IDL_FILES = dom/EventListener.idl dom/EventTarget.idl ....

In addition, a developer needs to update this list in all build scripts every time the developer adds a new IDL file that does not require .h and .cpp file. Rather than this complexity, I guess that (b) is simpler and more modest solution for this problem.

WDYT?

[Peter Kasting]

> - Before r102663, all platforms (Chromium, AppleWebKit, Qt, Gtk, Win, Efl/WinCE/BlackBerry) are generating apparently meaningful but actually unused .h and .cpp files for those 17 IDL files, since those build scripts have a rule like this (e.g. makefile):
> 
>     %.h: %.idl
>         ....
> 
> - Thus, in order to prevent those unused .h and .cpp files from being generated, we need to explicitly write a list of the IDL files in *each* build script like this:
> 
>     EXCLUDING_BINDING_IDL_FILES = dom/EventListener.idl dom/EventTarget.idl ....

Maybe we should back up a bit.  In principle, how do we know whether we need to run the script on one of these "silent" IDL files?  Clearly the script must do *something* or we could eliminate the IDL files entirely.  So what does it do?

Answering this question might make clear how we could write an alternative build step that properly determines whether these IDL files need "compilation" that does not look at a bogus .h or .cpp file.

[Kentaro Hara]

(In reply to comment #8)
> Maybe we should back up a bit.  In principle, how do we know whether we need to run the script on one of these "silent" IDL files?  Clearly the script must do *something* or we could eliminate the IDL files entirely.  So what does it do?
> 
> Answering this question might make clear how we could write an alternative build step that properly determines whether these IDL files need "compilation" that does not look at a bogus .h or .cpp file.

There are several reasons for excluding those silent IDL files, as commented in the 'bindings_idl_files!' target in WebCore.gyp.

Another reason that we are most focusing on in this patch and in the upcoming patches is the [Supplemental] IDL. If A.idl includes [Supplemental=B] IDL, it indicates that attributes and methods in A are implemented in B (Article: http://www.mail-archive.com/[e-mail]/msg04682.html  Meta bug: https://bugs.webkit.org/show_bug.cgi?id=72138). In this case we do not need to generate .h and .cpp files from A.idl, but we cannot know it until we parse A.idl and find the [Supplemental] IDL. So I guess that the possible options are

- (a) listing all silent IDL files in build scripts
- (b) generate "empty" .h and .cpp files

[Peter Kasting]

I am going to leave things in Adam's capable hands.  I am sure that not only is he capable of understanding the principles behind my comments but he understands what you're doing with the IDL files far better than I do.  Whatever route he thinks is best is fine.

[Kentaro Hara]

(In reply to comment #10)
> I am going to leave things in Adam's capable hands.  I am sure that not only is he capable of understanding the principles behind my comments but he understands what you're doing with the IDL files far better than I do.  Whatever route he thinks is best is fine.

Adam: WDYT?

[Adam Barth]

> Adam: WDYT?

I think the approach in this patch is ok, but less than ideal.  Having these generated files isn't really causing much harm, especially if the comments in the files explain what's going on.  It might even be better to have these stub files than to have another list of IDL files in each build system that needs to be maintained.

Given the amount of trouble its been to teach the build system about Supplemental, my inclination is to move forward with this approach.  I would file a bug to revisit this issue once we've taught the other build systems about Supplemental.

[WebKit Review Bot]

Comment on attachment 119163 [details]
Patch

Clearing flags on attachment: 119163

Committed r102987: <http://trac.webkit.org/changeset/102987>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.