Bug 38054 – [Qt] Build dependency problems

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Laszlo Gombos]

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 ?

[Jocelyn Turcotte]

(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.

[Csaba Osztrogonác]

One more false positive alarm because of this bug. :/

See https://bugs.webkit.org/show_bug.cgi?id=42027 (comment25,26)

[Mahesh Kulkarni]

How about checking for 'DEFINES +=' in any *.pri or *.pro?
If found trigger a clean build or else normal?

[Csaba Osztrogonác]

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. :-/

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

One more problem because of this bug: https://bugs.webkit.org/show_bug.cgi?id=63472

[Jocelyn Turcotte]

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)

[Jocelyn Turcotte]

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.

[Kristóf Kosztyó]

Created attachment 105925 [details]
proposed fix

[Jocelyn Turcotte]

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.

[Csaba Osztrogonác]

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.

[Kristóf Kosztyó]

Created attachment 105935 [details]
proposed fix

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Landed in http://trac.webkit.org/changeset/94288

[Csaba Osztrogonác]

(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