Bug 137394 – [GTK] Make forwarding headers generation depend on source code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

So that forwarding headers script is only called when source code changes, instead of for every build.

[Carlos Garcia Campos]

Created attachment 239206 [details]
Patch

[Martin Robinson]

Comment on attachment 239206 [details]
Patch

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

> Source/WebKit2/PlatformGTK.cmake:854
> +
> +file(GLOB_RECURSE WebKit2_HEADERS
> +    *.h
> +)

This is run during the configuration phase, so doesn't this break if you add a new header without re-running cmake?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 239206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239206&action=review
> 
> > Source/WebKit2/PlatformGTK.cmake:854
> > +
> > +file(GLOB_RECURSE WebKit2_HEADERS
> > +    *.h
> > +)
> 
> This is run during the configuration phase, so doesn't this break if you add a new header without re-running cmake?

Yes, that's only another problem of not adding headers to the makefiles, it happens with other things like rules using wilcard characters, if you add a new file, you need to force a configure for that to work. This is still much better than running the forwarding headers script 3 times every build. The situation of adding a new header after the configure phase that requires a new forwarding header is not common at all.

[Carlos Garcia Campos]

Committed r174422: <http://trac.webkit.org/changeset/174422>

[Csaba Osztrogonác]

(In reply to comment #4)
> Committed r174422: <http://trac.webkit.org/changeset/174422>

After this change, the GTK EWS stucked with this strange error:

ninja: error: '../../Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h', needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to make it

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > Committed r174422: <http://trac.webkit.org/changeset/174422>
> 
> After this change, the GTK EWS stucked with this strange error:
> 
> ninja: error: '../../Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h', needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to make it

Yes, this is indeed one of the cases when this fails because of the wildcard characters, a header file was removed, but non of our makefiles changed (it was an efl header), so a new cmake configure was required. Already forced a clean build on the bots. If this case happens more often that what I expected, I'll roll out the patch.

[Csaba Osztrogonác]

One more problem: https://trac.webkit.org/changeset/177764

ninja: error: '../../Source/WebKit2/Shared/API/Cocoa/WKRenderingProgressEventsInternal.h', needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to make it

I triggered clean build on the buildbots, but the EWS still needs a kick.

[Csaba Osztrogonác]

One more error due to this bug after https://trac.webkit.org/changeset/181777:
ninja: error: '../../Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.h', needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to make it

It isn't good to have incremental build issue after 
all header deleting/renaming in Webkit2 :-/

[Carlos Garcia Campos]

(In reply to comment #8)
> One more error due to this bug after
> https://trac.webkit.org/changeset/181777:
> ninja: error:
> '../../Source/WebKit2/UIProcess/mac/WebMediaPlaybackTargetPickerProxyMac.h',
> needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to
> make it
> 
> It isn't good to have incremental build issue after 
> all header deleting/renaming in Webkit2 :-/

This hasn't happened that often, IMO, so for me it's still worth it.

[Csaba Osztrogonác]

I kicked the bots with https://trac.webkit.org/changeset/181786 ,
but it isn't good to have to land this kind of fixes or poking
the EWS maintainers to kick the bots. We really need a proper fix.

Why was it problem that forwaring header generator ran all the time?
It is quite quick. I understand that all seconds count during working
on a task locally. But can't we run this generator unconditionally
only on the bots somehow?

[Csaba Osztrogonác]

(In reply to comment #10)
> I kicked the bots with https://trac.webkit.org/changeset/181786 ,
> but it isn't good to have to land this kind of fixes or poking
> the EWS maintainers to kick the bots. We really need a proper fix.
> 
> Why was it problem that forwaring header generator ran all the time?
> It is quite quick. I understand that all seconds count during working
> on a task locally. But can't we run this generator unconditionally
> only on the bots somehow?

- empty build on GTK takes 3-4 seconds (fw header generator doesn't run at all)
- empty build on EFL takes 6-7 seconds (fw header generator always run 10 times)

It is trivial to refactor fw header generator to run only once for each project:
WebKit2, WTR, TestWebKitAPI. It reduced the EFL build time to 5 seconds for me,
I'm going to upload the patch soon to bug142907.

[Csaba Osztrogonác]

One more incrementail build issue due to this change:
https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/56936

[Csaba Osztrogonác]

and again:
https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/57167

Do you really want to have this issue again and again on the bots?
I can't understand don't we run this generator unconditonally?

It's only 3-4 seconds per build and it guarantess we won't
have this ugly issue ever. But it's your choice, of course.

[Csaba Osztrogonác]

I kicked the GTK bots with this commit - http://trac.webkit.org/changeset/182326.
But I'm against to do this kind of changes, and done last time.

[Michael Catanzaro]

TBH I'm with Ossy on this one. The time I lose doing an initial build after a pull, discovering that it failed, figuring out it's an incremental build issue, and then running a clean build is not really worth the benefit. Bug #142907 looks like a better approach.

[Carlos Garcia Campos]

Ok, let's optimize and reduce the amount of times the script is run instead.

[Csaba Osztrogonác]

(In reply to comment #16)
> Ok, let's optimize and reduce the amount of times the script is run instead.

bug142907 reduces the amount from 4 to 3 on GTK and from 10 to 3 on EFL.

[Csaba Osztrogonác]

(In reply to comment #17)
> (In reply to comment #16)
> > Ok, let's optimize and reduce the amount of times the script is run instead.
> 
> bug142907 reduces the amount from 4 to 3 on GTK and from 10 to 3 on EFL.

With this fix the empty EFL build takes only 5 seconds. Additionally
I measured the runtime of the forwarding header generator script itself:
(these three execution can be run in parallel)
- 0.12 seconds for WTR
- 0.17 seconds for TestWebKitAPI
- 1.05 seconds for WebKit2

[Csaba Osztrogonác]

And again after https://trac.webkit.org/changeset/182873:
ninja: error: '../../Source/WebKit2/UIProcess/API/Cocoa/_WKWebsiteDataStoreInternal.h', needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to make it

(on the other hand we really don't need forwarding header for cocoa headers ...)

[Csaba Osztrogonác]

I filed two bug reports:
- make forwarding header generator run unconditionally: bug143819
- make forwarding header generate less unnecessary headers: bug143820

(In reply to comment #19)
> And again after https://trac.webkit.org/changeset/182873:
> ninja: error:
> '../../Source/WebKit2/UIProcess/API/Cocoa/_WKWebsiteDataStoreInternal.h',
> needed by 'WebKit2-forwarding-headers.stamp', missing and no known rule to
> make it
> 
> (on the other hand we really don't need forwarding header for cocoa headers
> ...)

The forwarding header generator didn't generate forwarding header
for Source/WebKit2/UIProcess/API/Cocoa/_WKWebsiteDataStoreInternal.h,
but the *.h rule added false dependency for this header.