Bug 74900 – Change the build flow of AppleWebKit and AppleWin to use the [Supplemental] IDL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kentaro Hara]

This is the final step for bug 74599. We change the build flow of AppleWebKit as follows, and thus enable the [Supplemental] IDL.

    - Previous build flow:
            foreach $idl (all IDL files) {
                generate-bindings.pl depends on $idl;
                generate-bindings.pl reads $idl;
                generate-bindings.pl generates .h and .cpp files for $idl;
            }

    - New build flow (See the discussions in bug 72138 for more details):
            resolve-supplemental.pl depends on all IDL files;
            resolve-supplemental.pl reads all IDL files;
            resolve-supplemental.pl resolves the dependency of [Supplemental=XXXX];
            resolve-supplemental.pl outputs supplemental_dependency.tmp;
            foreach $idl (all IDL files) {
                generate-bindings.pl depends on $idl and supplemental_dependency.tmp;
                generate-bindings.pl reads $idl;
                generate-bindings.pl reads supplemental_dependency.tmp;
                generate-bindings.pl generates .h and .cpp files for $idl,
                    including all attributes in the IDL files that are implementing $idl;
            }

[Kentaro Hara]

Created attachment 119964 [details]
Patch

[Kentaro Hara]

Created attachment 119990 [details]
WIP patch to see if win build passes

[Kentaro Hara]

Created attachment 120001 [details]
WIP patch to see if win build passes

[Kentaro Hara]

Created attachment 120008 [details]
WIP: Please ignore this one-line patch

[Kentaro Hara]

Created attachment 120119 [details]
WIP patch to see if build succeeds

[Kentaro Hara]

Created attachment 120254 [details]
WIP patch to see if build passes

[Kentaro Hara]

It finally passed the win build. Review?

[Adam Barth]

(In reply to comment #7)
> It finally passed the win build. Review?

Yay!

[Adam Barth]

Comment on attachment 120254 [details]
WIP patch to see if build passes

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

I'm marking this R+, but I'm slightly unsure that we'll re-build JSDOMWindow.h when DOMWindowWebSocket.idl changes.

> Source/WebCore/ChangeLog:24
> +                generate-bindings.pl depends on $idl and supplemental_dependency.tmp;

Will this step correctly run if only the supplemental IDL file is changed?

[Kentaro Hara]

Comment on attachment 120254 [details]
WIP patch to see if build passes

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

>> Source/WebCore/ChangeLog:24
>> +                generate-bindings.pl depends on $idl and supplemental_dependency.tmp;
> 
> Will this step correctly run if only the supplemental IDL file is changed?

I confirmed that it correctly runs. This is because

- generate-bindings.pl depends on $idl and supplemental_dependency.tmp
- and supplemental_dependency.tmp depends on *all* IDL files.

In other words, given that *any* of IDL files is updated, then generate-bindings.pl runs for *all* IDL files. Perhaps is this a problem?


By the way, I found that generate-bindings.pl runs for DOMWindowWebSocket.idl and DOMWindowWebAudio.idl at every build even if no IDL files are updated. I am investigating the reason.

[Adam Barth]

> In other words, given that *any* of IDL files is updated, then generate-bindings.pl runs for *all* IDL files. Perhaps is this a problem?

That seems slightly unfortunate, but I think it's ok for this iteration.  Once we get this working better, we'll probably want to improve that.  One option is to write the output h/cpp file to a temp file and then only overwrite the real output file if the bytes differ.  That will prevent us from having to rebuild all downstream dependencies.

[Adam Barth]

As simpler solution might be to shard supplemental_dependency.tmp by interface.  We could have a DOMWindow.supplemental_dependencies file that JSDOMWindow.h would depend upon.  That file would only get overwritten by resolve-dependencies if it changed.

[Adam Barth]

Anyway, I think its ok to move forward and get this system working with this approach.  We can optimize the incremental builds after we have everything working.

[Kentaro Hara]

(In reply to comment #13)
> Anyway, I think its ok to move forward and get this system working with this approach.  We can optimize the incremental builds after we have everything working.

I agree, thanks. I'll commit it after preventing generate-bindings.pl from running for DOMWindow*.idl at every build.

[Kentaro Hara]

(In reply to comment #14)
> (In reply to comment #13)
> > Anyway, I think its ok to move forward and get this system working with this approach.  We can optimize the incremental builds after we have everything working.
> 
> I agree, thanks. I'll commit it after preventing generate-bindings.pl from running for DOMWindow*.idl at every build.

I found that the cause exists in generate-bindings.pl. In case where the generator is ObjC and generate-bindings.pl needs to generate empty .h and .cpp, it should generate DOM****.h and DOM****.cpp, but currently it generates ObjC****.h and ObjC****.cpp. Anyway, to make this patch simpler, I guess we should fix it in another patch. (Note: This is not a serious problem. Even if we commit the patch without fixing it, nothing bad happens except that generate-bindings.pl always runs for the two supplemental IDLs and generates empty .h and .cpp.)

Committed. Thanks!

[WebKit Review Bot]

Comment on attachment 120254 [details]
WIP patch to see if build passes

Clearing flags on attachment: 120254

Committed r103519: <http://trac.webkit.org/changeset/103519>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Looks like this caused bug 75546.

[Kentaro Hara]

(In reply to comment #11)
> > In other words, given that *any* of IDL files is updated, then generate-
bindings.pl runs for *all* IDL files. Perhaps is this a problem?

This has been becoming an issue in our team and I'd like to fix it.

> One option is to write the output h/cpp file to a temp file and then only overwrite the real output file if the bytes differ.  That will prevent us from having to rebuild all downstream dependencies.

I'll take this approach.

[Darin Adler]

(In reply to comment #19)
> (In reply to comment #11)
> > One option is to write the output h/cpp file to a temp file and then only overwrite the real output file if the bytes differ.  That will prevent us from having to rebuild all downstream dependencies.
> 
> I'll take this approach.

This can be harder than it sounds. If you do it in the most obvious way you can end up regenerating the .h and .cpp files over and over again. There has to be some other target used for make logic. Maybe that’s obvious to you, but I’ve run into that problem many times trying to do something like this.

[Kentaro Hara]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #11)
> > > One option is to write the output h/cpp file to a temp file and then only overwrite the real output file if the bytes differ.  That will prevent us from having to rebuild all downstream dependencies.
> > 
> > I'll take this approach.
> 
> This can be harder than it sounds. If you do it in the most obvious way you can end up regenerating the .h and .cpp files over and over again. There has to be some other target used for make logic. Maybe that’s obvious to you, but I’ve run into that problem many times trying to do something like this.

I am not 100% sure but, as I mentioned in bug 76836, I guess that we can make the change simply by changing WriteData() in CodeGenerator*pm so that it writes .h/.cpp to a temp file and then overwrites the real .h/.cpp only when the bytes differ (We can prepare the subroutine that does it in CodeGenerator.pm and call it from WriteData() in each CodeGenerator*.pm). IMO, I'd like to avoid making a change on Makefiles because we need to touch various build scripts (make, cmake, qmake, GYP, ...)