Bug 38054

Summary: [Qt] Build dependency problems
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Blocker CC: hausmann, jturcotte, laszlo.gombos, maheshk, ossy
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 67646, 67706    
Bug Blocks:    
Attachments:
Description Flags
proposed fix
jturcotte: review-
proposed fix none

Description Laszlo Gombos 2010-04-23 12:03:13 PDT
The extend of the problem is not well understood, but we have frequent false breaks on the minimal bot. For example r58178 cased the following failure:

make[1]: Leaving directory `/home/webkitbuildbot/slaves/release32bitMinimal/buildslave/qt-linux-release-minimal/build/WebKitBuild/Release/JavaScriptCore'
cd WebCore/ && make -f Makefile 
make[1]: Entering directory `/home/webkitbuildbot/slaves/release32bitMinimal/buildslave/qt-linux-release-minimal/build/WebKitBuild/Release/WebCore'
rm -f libQtWebKit.so.4.6.2 libQtWebKit.so libQtWebKit.so.4 libQtWebKit.so.4.6

[...]

/release/PluginPackageNone.o: In function `WebCore::PluginPackage::determineQuirks(WebCore::String const&)':
PluginPackageNone.cpp:(.text._ZN7WebCore13PluginPackage15determineQuirksERKNS_6StringE+0x0): multiple definition of `WebCore::PluginPackage::determineQuirks(WebCore::String const&)'
obj/release/PluginPackage.o:PluginPackage.cpp:(.text._ZN7WebCore13PluginPackage15determineQuirksERKNS_6StringE+0x0): first defined here
obj/release/PluginView.o: In function `WebCore::PluginView::handleEvent(WebCore::Event*)':
PluginView.cpp:(.text._ZN7WebCore10PluginView11handleEventEPNS_5EventE+0xb1): undefined reference to `WebCore::PluginView::handleFocusInEvent()'
PluginView.cpp:(.text._ZN7WebCore10PluginView11handleEventEPNS_5EventE+0xe9): undefined reference to `WebCore::PluginView::handleFocusOutEvent()'
obj/release/PluginView.o: In function `WebCore::PluginView::stop()':
PluginView.cpp:(.text._ZN7WebCore10PluginView4stopEv+0x3cd): undefined reference to `NPN_MemFree'
PluginView.cpp:(.text._ZN7WebCore10PluginView4stopEv+0x3d8): undefined reference to `NPN_MemFree'
collect2: ld returned 1 exit status
make[1]: *** [../lib/libQtWebKit.so.4.6.2] Error 1
make[1]: Leaving directory `/home/webkitbuildbot/slaves/release32bitMinimal/buildslave/qt-linux-release-minimal/build/WebKitBuild/Release/WebCore'


Link to the full buildlog: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/582/steps/compile-webkit/logs/stdio


The list of DEFINES has changed (as WebCore.pri has changed) but none of the source files has been rebuilt. As a top gap measure maybe we can make all WebCore files dependent on WebCore.pri, WebCore.pro, WebKit.pri ?
Comment 1 Jocelyn Turcotte 2010-05-06 02:14:56 PDT
(In reply to comment #0)
> The list of DEFINES has changed (as WebCore.pri has changed) but none of the
> source files has been rebuilt. As a top gap measure maybe we can make all
> WebCore files dependent on WebCore.pri, WebCore.pro, WebKit.pri ?

The problem is that this will trigger an almost full rebuild each time one of these files will change, and 95% of the times the change is not related to preprocessor input data.

I don't know how much less efficient the build bot would be, it looks like at least one of these files get modified around once or twice a day. It might be not that bad.
Comment 2 Csaba Osztrogonác 2010-08-18 12:15:19 PDT
One more false positive alarm because of this bug. :/

See https://bugs.webkit.org/show_bug.cgi?id=42027 (comment25,26)
Comment 3 Mahesh Kulkarni 2010-08-18 19:18:08 PDT
How about checking for 'DEFINES +=' in any *.pri or *.pro?
If found trigger a clean build or else normal?
Comment 4 Csaba Osztrogonác 2011-06-16 05:07:56 PDT
Moidifying it to P1/blocker. It is so annoying bug in our build system. :-S

https://bugs.webkit.org/attachment.cgi?id=97432 in https://bugs.webkit.org/show_bug.cgi?id=59085 killed our EWS, and I have no idea how to fix it ...

+} else {
+    # We have to disable SVG Fonts, which rely on the fast path.
+    DEFINES -= ENABLE_SVG_FONTS=1
+    DEFINES += ENABLE_SVG_FONTS=0

When EWS try to build the patch, this change doesn't trigger rebuilding all source contains ENABLE_SVG_FONTS defines. It cause an inconsistent state 
in WebKitBuild. :-/
Comment 5 Csaba Osztrogonác 2011-06-16 11:16:52 PDT
A typical example what happens if Qt EWS doesn't work because of this crazy bug: https://bugs.webkit.org/show_bug.cgi?id=62748
Comment 6 Csaba Osztrogonác 2011-07-26 05:37:27 PDT
One more problem because of this bug: https://bugs.webkit.org/show_bug.cgi?id=63472
Comment 7 Jocelyn Turcotte 2011-07-26 07:43:10 PDT
I think that adding all the .pro and .pri files to the dependency of each .o file would be less annoying for the bots, but extremely more annoying for developers (adding or removing any line to i.e. SOURCES or HEADERS in those files will trigger a full rebuild on their machine).

The "cleanest" solution I would see is to centralize the DEFINES modifications in one file (called config.pri or something) and make that file a dependency for every copiled .o
There is also no way I know of with qmake to add an additional dependency to all object files, so we might have to hack some QMAKE_EXTRA_TARGETS that would depend on this file and do something like touching all config.h files

That's hacky, ugly, probably going to be another thing making this build system a burden and I personally prefer it the way it is right now, but that's because I'm not the one that has to reset those bots each time an incremental build isn't possible.

To explore other solutions, how often are those bots failing in normal cases? What would be the cost of automatically forcing a clean build when an incremental build failed?

Something like:
build-webkit --qt || (rm -rf $WEBKITOUTPUTDIR ; build-webkit --qt)
Comment 8 Jocelyn Turcotte 2011-07-26 07:52:36 PDT
Another idea would be to stop using command line defines and have instead a DerivedSources generator that would create an header file with defines instead (like ENABLE* and USE*). This header could be included in each config.h within a PLATFORM(QT) block.

More complex defines logic should be in static headers to be handled by the preprocessor instead of qmake.
Comment 9 Kristóf Kosztyó 2011-09-01 02:00:15 PDT
Created attachment 105925 [details]
proposed fix
Comment 10 Jocelyn Turcotte 2011-09-01 02:32:23 PDT
Comment on attachment 105925 [details]
proposed fix

Looks like this should fix it, simple and efficient!
One thing that should be changed is to move the CONFIG(compute_defaults) { } part in Source/WebCore/features.pri to the end of the file and a comment should be added to say that nothing should be placed after it.

For example right now, since ENABLE_XHTMLMP is defined after the compute_defaults part, changing it won't trigger a clean build. Beside this it LGTM.
Comment 11 Csaba Osztrogonác 2011-09-01 02:46:29 PDT
Great work. I agree with Jocelyn, it would be good if you move CONFIG(compute_defaults) {...} to the end of features.pri with a comment.

Please upload a new patch with this fix and I'll review it.
Comment 12 Kristóf Kosztyó 2011-09-01 03:48:36 PDT
Created attachment 105935 [details]
proposed fix
Comment 13 Csaba Osztrogonác 2011-09-01 05:17:10 PDT
Comment on attachment 105935 [details]
proposed fix

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

LGTM, r=me.

> Source/WebCore/features.pri:266
> +# Nothing place after this!

"Don't place anything after this!" would be better
Comment 14 Csaba Osztrogonác 2011-09-01 05:24:03 PDT
Landed in http://trac.webkit.org/changeset/94288
Comment 15 Csaba Osztrogonác 2011-09-02 04:36:20 PDT
(In reply to comment #14)
> Landed in http://trac.webkit.org/changeset/94288

And a trivial fix landed in http://trac.webkit.org/changeset/94407