Bug 51339

Summary: [Qt] Redesign the build system
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: PlatformAssignee: Andras Becsi <abecsi>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, benjamin, eric, hausmann, kling, laszlo.gombos, ossy, s.mathur, webkit-ews
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 52891    
Bug Blocks: 50251    
Attachments:
Description Flags
first try
none
Patch
none
patch part1
none
patch part2
none
patch part2 v2
laszlo.gombos: review-
patch part2 v3
none
patch part2
none
patch part2 v4 none

Description Balazs Kelemen 2010-12-20 09:16:31 PST
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.
Comment 1 Balazs Kelemen 2010-12-20 09:19:19 PST
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?
Comment 2 Laszlo Gombos 2011-01-04 08:58:37 PST
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.
Comment 3 Laszlo Gombos 2011-01-09 12:05:26 PST
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.
Comment 4 Kenneth Rohde Christiansen 2011-01-10 12:12:35 PST
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.
Comment 5 Simon Hausmann 2011-01-11 02:49:18 PST
(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.
Comment 6 Balazs Kelemen 2011-01-11 04:21:44 PST
(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.
Comment 7 Laszlo Gombos 2011-01-11 18:02:30 PST
Comment on attachment 78354 [details]
first try

Canceling patch review, bbandix can take it from here.
Comment 8 Andras Becsi 2011-01-12 04:59:23 PST
(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.
Comment 9 Andras Becsi 2011-02-14 12:51:52 PST
Created attachment 82353 [details]
Patch
Comment 10 Andras Becsi 2011-02-14 13:04:18 PST
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.
Comment 11 Early Warning System Bot 2011-02-14 13:40:15 PST
Attachment 82353 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7908697
Comment 12 Andras Becsi 2011-02-15 07:44:11 PST
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.
Comment 13 Csaba Osztrogon√°c 2011-02-15 08:07:21 PST
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
Comment 14 Andras Becsi 2011-02-15 08:20:50 PST
Comment on attachment 82452 [details]
patch part1

Thanks Ossy.
First part landed in http://trac.webkit.org/changeset/78566
Comment 15 Andras Becsi 2011-02-15 10:43:09 PST
Created attachment 82486 [details]
patch part2

The second part. This also needs a clean build.
Comment 16 Laszlo Gombos 2011-02-15 14:40:42 PST
> 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 ?
Comment 17 Andras Becsi 2011-02-15 14:58:00 PST
(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.
Comment 18 Andras Becsi 2011-02-18 08:15:45 PST
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.
Comment 19 Early Warning System Bot 2011-02-18 08:39:28 PST
Attachment 82960 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7934232
Comment 20 Balazs Kelemen 2011-02-18 15:27:50 PST
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.
Comment 21 Andras Becsi 2011-02-19 05:22:08 PST
(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?
Comment 22 Balazs Kelemen 2011-02-19 08:32:59 PST
(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.
Comment 23 Laszlo Gombos 2011-02-20 10:15:44 PST
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.
Comment 24 Laszlo Gombos 2011-02-20 10:17:44 PST
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.
Comment 25 Laszlo Gombos 2011-02-21 05:33:31 PST
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.
Comment 26 Andras Becsi 2011-02-21 08:14:54 PST
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.
Comment 27 Andras Becsi 2011-02-21 12:49:15 PST
Created attachment 83206 [details]
patch part2

Corrected the mobility issues, thanks Laszlo for spotting them.
Comment 28 Laszlo Gombos 2011-02-21 22:13:39 PST
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 ?
Comment 29 Andras Becsi 2011-02-22 02:11:35 PST
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.
Comment 30 Laszlo Gombos 2011-02-22 04:49:21 PST
Comment on attachment 83290 [details]
patch part2 v4

Looks great, r=me.
Comment 31 Andras Becsi 2011-02-22 07:15:22 PST
Committed r79320: <http://trac.webkit.org/changeset/79320>
Comment 32 Andras Becsi 2011-02-22 07:17:05 PST
Comment on attachment 83290 [details]
patch part2 v4

Clearing flags.
Comment 33 Andras Becsi 2011-02-22 11:25:29 PST
(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.
Comment 34 Csaba Osztrogon√°c 2011-02-22 12:26:40 PST
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.
Comment 35 Andras Becsi 2011-02-22 13:23:23 PST
(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.