Bug 51339 – [Qt] Redesign the build system

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Balazs Kelemen]

Currently we build the lib in one pass from the jscore static lib, WebCore and the API.
For WebKit2 we followed that policy so we created a static lib target to link into the final lib.
Obviously, that does not match the real dependencies between the components so we end up in weird problems:
https://bugs.webkit.org/show_bug.cgi?id=50519, https://bugs.webkit.org/show_bug.cgi?id=50861#c12.
I think we should solve this by adapting the build targets to the dependencies.

[Balazs Kelemen]

My idea is to build a static lib from JSC, WebCore and WebKit2 and then
creating the shared lib from the static libs and the sources of the API (with or without the WK2 API).
What do you think?

[Laszlo Gombos]

I like this idea. In fact this is exactly how we're building WebKit2 for Symbian at the moment as the "whole-archive" machinery to expose symbols does not seems to be available for the current Symbian toolchain.

[Laszlo Gombos]

Created attachment 78354 [details]
first try

This approach is slightly simpler than proposed earlier in https://bugs.webkit.org/show_bug.cgi?id=51339#c1. In the attached patch there is no separate .pro file for building WebCore library, instead that is combined with building the final WebKit library. 

With this patch (and some other patches under review) WebKit2 links with the Symbian toolchain.

[Kenneth Rohde Christiansen]

Comment on attachment 78354 [details]
first try

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

> Source/WebCore/WebCore.pro:192
> +
> +    HEADERS += \
> +        $$WK2_DIR/UIProcess/API/C/qt/WKNativeEvent.h \

Our WebCore.pro is becoming too polluted... here we are adding more WebKit2 stuff in WebCore, we want to move away from that.

[Simon Hausmann]

(In reply to comment #1)
> My idea is to build a static lib from JSC, WebCore and WebKit2 and then
> creating the shared lib from the static libs and the sources of the API (with or without the WK2 API).
> What do you think?

Good plan.

[Balazs Kelemen]

(In reply to comment #5)
> (In reply to comment #1)
> > My idea is to build a static lib from JSC, WebCore and WebKit2 and then
> > creating the shared lib from the static libs and the sources of the API (with or without the WK2 API).
> > What do you think?
> 
> Good plan.

Fine. bbandix has been started working on the this, currently he is waiting for
the source tree reorganizing to be finished.

[Laszlo Gombos]

Comment on attachment 78354 [details]
first try

Canceling patch review, bbandix can take it from here.

[Andras Becsi]

(In reply to comment #7)
> (From update of attachment 78354 [details])
> Canceling patch review, bbandix can take it from here.

Thanks.
I agree with Kenneth that WebCore.pro is getting too chaotic that's why we discussed a new approach with Balazs.
In the coming weeks the remaining directories should move to Source, then I'll resume working on this.

[Andras Becsi]

Created attachment 82353 [details]
Patch

[Andras Becsi]

The patch needs a clean build, since the whole build system got revamped.

Adding WebKit2 API files to the final build phase would be dangerous because of name clashes between WebCore and WebKit2 sources and headers, so I left the WebKit2 build as it was and link to the static lib with whole-archive.

I hope this also works on Symbian, if not, then we need to export symbols.

[Early Warning System Bot]

Attachment 82353 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7908697

[Andras Becsi]

Created attachment 82452 [details]
patch part1

After a discussion with ossy I split the patch.
So this is now the first part, which moves code generator rules to CodeGenerators.pri and makes WebCore.pri to a project include file for commonly needed options.

[Csaba Osztrogonác]

Comment on attachment 82452 [details]
patch part1

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

Good cleanup! r=me. 
Please do a clean build on the buildbots if it is necessary.

> Source/WebCore/CodeGenerators.pri:655
> +inspectorBackendStub.wkAddOutputToSources = false

Please add a comment to ChangeLog about this change before landing.

> Source/WebCore/CodeGenerators.pri:662
> +injectedScriptSource.wkAddOutputToSources = false

ditto

> Source/WebCore/CodeGenerators.pri:671
> +tokenizer.wkAddOutputToSources = false

ditto

[Andras Becsi]

Comment on attachment 82452 [details]
patch part1

Thanks Ossy.
First part landed in http://trac.webkit.org/changeset/78566

[Andras Becsi]

Created attachment 82486 [details]
patch part2

The second part. This also needs a clean build.

[Laszlo Gombos]

> Adding WebKit2 API files to the final build phase would be dangerous because of name clashes between WebCore and WebKit2 sources and headers, so I left the WebKit2 build as it was and link to the static lib with whole-archive.
> 
> I hope this also works on Symbian, if not, then we need to export symbols.

I do not think this will work for Symbian, but perhaps we can address WebKit2 in a subsequent patch (step 3).

I think we should keep building bindings as part of WebCore.pro. What is the rational of moving bindings in QtWebKit.pro ?

[Andras Becsi]

(In reply to comment #16)
> > Adding WebKit2 API files to the final build phase would be dangerous because of name clashes between WebCore and WebKit2 sources and headers, so I left the WebKit2 build as it was and link to the static lib with whole-archive.
> > 
> > I hope this also works on Symbian, if not, then we need to export symbols.
> 
> I do not think this will work for Symbian, but perhaps we can address WebKit2 in a subsequent patch (step 3).
> 
> I think we should keep building bindings as part of WebCore.pro. What is the rational of moving bindings in QtWebKit.pro ?

Moving bindings and bridge to the final build step ensures linking to the jsc static library without getting some symbols stripped away and without using whole-archive. When I left them in WebCore.pro I got linking problems with WebKit2 and minimal builds.

[Andras Becsi]

Created attachment 82960 [details]
patch part2 v2

I finally managed to also move the WebKit2 API to the final step, and moved bridge and bindings back to WebCore.pro as Laszlo suggested.

WebKit2/WebProcess/qt/WebProcessMainQt.cpp had to be moved to the final step as well, so the WK2 and JSC library don't need whole-archive any more.

In theory later changes could introduce trouble with the clashing filenames of WebCore and WebKit2 (especially with header includes), but this risk is minimal since only the API is in the final step.

This patch should make it possible to also work on the Windows version of Qt WebKit2.

This most certainly needs a clean build.

[Early Warning System Bot]

Attachment 82960 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7934232

[Balazs Kelemen]

Comment on attachment 82960 [details]
patch part2 v2

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

Great! This is what we need.

> Source/WebKit/qt/QtWebKit.pro:41
> +webkit2:addWebKit2Lib(../../WebKit2)
> +
> +addWebCoreLib(../../WebCore)
> +
> +!v8:addJavaScriptCoreLib(../../JavaScriptCore)

Is this the best order? I believe that this determines the location of each unit in the final shared lib
and that can affect to performance. Do somebody have an idea what would be the ideal order?

> Source/WebKit/qt/QtWebKit.pro:124
> +} else {
> +    DESTDIR = $$OUTPUT_DIR/lib
> +    !static: DEFINES += QT_MAKEDLL
> +    symbian: TARGET =$$TARGET$${QT_LIBINFIX}
> +}
> +
> +unix|win32-g++*:QMAKE_PKGCONFIG_REQUIRES = QtCore QtGui QtNetwork
> +unix:!mac:*-g++*:QMAKE_CXXFLAGS += -ffunction-sections -fdata-sections
> +unix:!mac:*-g++*:QMAKE_LFLAGS += -Wl,--gc-sections
> +linux*-g++*:QMAKE_LFLAGS += $$QMAKE_LFLAGS_NOUNDEF
> +
> +!static: DEFINES += QT_MAKEDLL

Once defined QT_MAKEDLL conditionally and then unconditionally. I guess it should be removed from the else case.

[Andras Becsi]

(In reply to comment #20)
> (From update of attachment 82960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review
> 
> Great! This is what we need.
> 
> > Source/WebKit/qt/QtWebKit.pro:41
> > +webkit2:addWebKit2Lib(../../WebKit2)
> > +
> > +addWebCoreLib(../../WebCore)
> > +
> > +!v8:addJavaScriptCoreLib(../../JavaScriptCore)
> 
> Is this the best order? I believe that this determines the location of each unit in the final shared lib
> and that can affect to performance. Do somebody have an idea what would be the ideal order?
> 

I'm not sure whether this can affect the performance. I can experiment with the order and measure the performance if this is a concern of yours. 

> > Source/WebKit/qt/QtWebKit.pro:124
> > +} else {
> > +    DESTDIR = $$OUTPUT_DIR/lib
> > +    !static: DEFINES += QT_MAKEDLL
> > +    symbian: TARGET =$$TARGET$${QT_LIBINFIX}
> > +}
> > +
> > +unix|win32-g++*:QMAKE_PKGCONFIG_REQUIRES = QtCore QtGui QtNetwork
> > +unix:!mac:*-g++*:QMAKE_CXXFLAGS += -ffunction-sections -fdata-sections
> > +unix:!mac:*-g++*:QMAKE_LFLAGS += -Wl,--gc-sections
> > +linux*-g++*:QMAKE_LFLAGS += $$QMAKE_LFLAGS_NOUNDEF
> > +
> > +!static: DEFINES += QT_MAKEDLL
> 
> Once defined QT_MAKEDLL conditionally and then unconditionally. I guess it should be removed from the else case.

Good catch, I can correct this when landing.

Laszlo, could you take a look whether this approach could work on Symbian too?

[Balazs Kelemen]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 82960 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82960&action=review
> > 
> > Great! This is what we need.
> > 
> > > Source/WebKit/qt/QtWebKit.pro:41
> > > +webkit2:addWebKit2Lib(../../WebKit2)
> > > +
> > > +addWebCoreLib(../../WebCore)
> > > +
> > > +!v8:addJavaScriptCoreLib(../../JavaScriptCore)
> > 
> > Is this the best order? I believe that this determines the location of each unit in the final shared lib
> > and that can affect to performance. Do somebody have an idea what would be the ideal order?
> > 
> 
> I'm not sure whether this can affect the performance. I can experiment with the order and measure the performance if this is a concern of yours. 
 
Fine, but it should not block the patch, we can change the order later.

[Laszlo Gombos]

Comment on attachment 82960 [details]
patch part2 v2

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

Looks great. I have to say r- because this badly breaks Symbian and because this approach repeats a few build rules (in WebCore.pro and QtWebKit.pro) instead of reused (e.g. WebCore.pri).

Also the patch no longer applies against the trunk.

> Source/WebCore/WebCore.pro:-25
> -        QMAKE_CXXFLAGS.ARMCC += --fpu softvfp+vfpv2 --fpmode fast

CFLAGS should stay in WebCore.pro (or perhaps WebCore.pri) - that's where v8 object files are built.

> Source/WebCore/WebCore.pro:-56
> -        MMP_RULES += ALWAYS_BUILD_AS_ARM

I think MMP_RULES should stay in WebCore.pro (or perhaps WebCore.pri) as well.

> Source/WebCore/WebCore.pro:-59
> -        QMAKE_CXXFLAGS -= --thumb

Ditto.

> Source/WebCore/WebCore.pro:-61
> -    CONFIG(release, debug|release): QMAKE_CXXFLAGS.ARMCC += -OTime -O3

Ditto.

> Source/WebCore/WebCore.pro:-77
> -    !static: DEFINES += QT_MAKEDLL

Removing QT_MAKEDLL define will cause PluginViewSymbian.cpp no longer build - as QWEBKIT_EXPORT will no be defined. qtwebkit_setPluginCreatedCallback is the only exported symbol from WebCore (only on Symbian). I think the change in pro file is OK, however the exported symbol should be moved to QtWebKit.pro.

> Source/WebKit/qt/QtWebKit.pro:120
> +unix:!mac:*-g++*:QMAKE_CXXFLAGS += -ffunction-sections -fdata-sections

I think this CFLAGS rule should go to WebCore.pri as it should be applied to both WebCore and QtWebKit.

>>> Source/WebKit/qt/QtWebKit.pro:124
>>> +!static: DEFINES += QT_MAKEDLL
>> 
>> Once defined QT_MAKEDLL conditionally and then unconditionally. I guess it should be removed from the else case.
> 
> Good catch, I can correct this when landing.
> 
> Laszlo, could you take a look whether this approach could work on Symbian too?

Happy to continue to help; Ossy has a working WebKit(1) Symbian environment as well.

> Source/WebKit/qt/QtWebKit.pro:161
> +    webkitbackup.sources = ../WebKit/qt/symbian/backup_registration.xml

This relative path needs to be now relative to QtWebKit.pro - should be "symbian/backup_registration.xml".

> Source/WebKit/qt/QtWebKit.pro:166
> +         declarativeImport.sources += ../WebKit/qt/declarative/qmldir

This relative path needs to be now relative to QtWebKit.pro - should be "declarative/qmldir".

> Source/WebKit/qt/QtWebKit.pro:404
> +contains(DEFINES, ENABLE_NETSCAPE_PLUGIN_API=1) {

Most of this block should go to WebCore.pri instead.

> Source/WebKit/qt/QtWebKit.pro:405
> +    unix {

WebCore.pro (where this block is copied from) had a symbian block before the unix block - as the symbian block is omitted here and symbian will test true for unix which will break the symbian build. This is a good example why we should not copy these rules into both pro files and instead move them to WebCore.pri.

Use unix:!symbian instead.

> Source/WebKit/qt/QtWebKit.pro:412
> +                DEFINES += MOZ_PLATFORM_MAEMO=5

Should go to WebCore.pri instead of repeating it here. MOZ_PLATFORM_MAEMO is used in npapi.h which is included in both WebCore and QtWebKit directory.

> Source/WebKit/qt/QtWebKit.pro:417
> +                DEFINES += MOZ_PLATFORM_MAEMO=6

Ditto.

> Source/WebKit/qt/QtWebKit.pro:420
> +            DEFINES += ENABLE_NETSCAPE_PLUGIN_METADATA_CACHE=1

Ditto.

> Source/WebKit/qt/QtWebKit.pro:485
> +        }

Section above is old. This block is already newer in r78996 which was used to create this patch. This is also a good example why we should move this rules from WebCore.pro into WebCore.pri and not copy them around.

> Source/WebKit/qt/QtWebKit.pro:505
> +    MOBILITY *= sensors

I think these CONFIG and MOBILITY rules should go to WebCore.pri as well.

> Source/WebKit/qt/QtWebKit.pro:642
> +*-g++*:QMAKE_CXXFLAGS -= -std=c++0x -std=gnu++0x

Should stay in WebCore.pro or moved to WebCore.pri.

[Laszlo Gombos]

Comment on attachment 82960 [details]
patch part2 v2

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

> Source/WebCore/WebCore.pro:15
> +DEFINES += BUILDING_WEBCORE

This define is not used anywhere I think we should remove this line.

[Laszlo Gombos]

Comment on attachment 82960 [details]
patch part2 v2

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

One more..

> Source/WebKit/qt/QtWebKit.pro:414
> +                SOURCES += $PWD/WebCoreSupport/QtMaemoWebPopup.cpp

Should be $$PWD instead of $PWD.

> Source/WebKit/qt/QtWebKit.pro:495
> +        $PWD/WebCoreSupport/DeviceOrientationProviderQt.h

Ditto.

> Source/WebKit/qt/QtWebKit.pro:502
> +        $PWD/WebCoreSupport/DeviceOrientationProviderQt.cpp

Ditto.

[Andras Becsi]

Created attachment 83168 [details]
patch part2 v3

Addressed the mentioned issues. Ossy launched a build for Symbian, but since it will last a while I upload the patch now.

[Andras Becsi]

Created attachment 83206 [details]
patch part2

Corrected the mobility issues, thanks Laszlo for spotting them.

[Laszlo Gombos]

Comment on attachment 83206 [details]
patch part2

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

Looks really good. This version of the patch no longer breaks the Symbian WebKit(1) builds. Made a few more comments for consideration.

> Source/WebCore/WebCore.pri:162
> +    !CONFIG(production):CONFIG-=def_files

This line should go to QtWebKit.pro as it is only needed for final linking.

> Source/WebKit/qt/QtWebKit.pro:380
> +                CONFIG += x11

CONFIG += x11 changes the include path so I think it should go to WebCore.pri.

> Source/WebKit/qt/QtWebKit.pro:381
> +                LIBS += -lXrender

We might want to consider keeping some (or all) LIB rules together (in WebCore.pri) with the corresponding CONFIG or DEFINE rules just to increase the readability (it seems LIBS is ignored for static libs). I do not really have a strong opinion on this one.

> Source/WebKit/qt/QtWebKit.pro:400
> +contains(DEFINES, ENABLE_SQLITE=1) {

This whole block should be moved to WebCore.pri instead; some of the rules below are manipulating the includepath for files under WebCore.

> Source/WebKit/qt/QtWebKit.pro:546
> +win32:!win32-g++*:contains(QMAKE_HOST.arch, x86_64):{

Shouldn't this block stay in WebCore.pro ?

[Andras Becsi]

Created attachment 83290 [details]
patch part2 v4

Addressed Laszlo's suggestions, WebCore.pri contains now the LIB and CONFIG rules.
Thanks Laszlo for the comprehensive review.

[Laszlo Gombos]

Comment on attachment 83290 [details]
patch part2 v4

Looks great, r=me.

[Andras Becsi]

Committed r79320: <http://trac.webkit.org/changeset/79320>

[Andras Becsi]

Comment on attachment 83290 [details]
patch part2 v4

Clearing flags.

[Andras Becsi]

(In reply to comment #31)
> Committed r79320: <http://trac.webkit.org/changeset/79320>

Had to do a follow-up (http://trac.webkit.org/changeset/79334), because I screwed up something with webkit-patch.

[Csaba Osztrogonác]

Andras, this patch broke inspector tests, because somehow qrc_InspectorBackendStub.o isn't linked to libQtWebKit.so :-/

qrc_InspectorBackendStub.o is in libwebcore.a, but missing from so.

Reopen to fix this regression.

[Andras Becsi]

(In reply to comment #34)
> Andras, this patch broke inspector tests, because somehow qrc_InspectorBackendStub.o isn't linked to libQtWebKit.so :-/
> 
> qrc_InspectorBackendStub.o is in libwebcore.a, but missing from so.
> 
> Reopen to fix this regression.

Fix landed in http://trac.webkit.org/changeset/79352.
Sorry for the inconvenience.