Bug 13499

Summary: [qmake] Make it possible to use qmake for other ports
Product: WebKit Reporter: Holger Freyther <zecke>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, zack
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Other   
OS: All   
Attachments:
Description Flags
Add some OUTPUT directories to the include path
none
Scope files belonging to the Qt port
none
Add the OUTPUTDIR to the INCLUDEPATH.
zack: review+
Scope the qt-port. To allow other ports to reuse the qmake system
none
Make it possible to compile the Gdk port using qmake
mrowe: review-
Scope the qt-port. To allow other ports to reuse the qmake system
zack: review+
Make it possible to compile the Gdk port using qmake
none
Make it possible to compile the Gdk port using qmake
zack: review+
Folded patch to add Gdk port to the qmake buildsystem none

Description Holger Freyther 2007-04-26 06:38:32 PDT
qmake is a simple but still capable buildsystem. Besides being written using Qt there is no strict requirement on Qt and in theory and even in practice it can be used to build other stuff as well.
Comment 1 Holger Freyther 2007-04-26 06:40:25 PDT
Created attachment 14200 [details]
Add some OUTPUT directories to the include path

On my build some auto-generated files were not found. I had to extend the INCLUDEPATH to look into these two directories.
Comment 2 Holger Freyther 2007-04-26 06:42:54 PDT
Created attachment 14201 [details]
Scope files belonging to the Qt port

Scope files belonging to the Qt port to allow other ports to use qmake.
Comment 3 Oliver Hunt 2007-04-27 19:57:13 PDT
Comment on attachment 14200 [details]
Add some OUTPUT directories to the include path

Is pcre.pri qt specific? (eg. is it only used for qmake?)
Comment 4 Oliver Hunt 2007-04-27 19:59:39 PDT
Comment on attachment 14201 [details]
Scope files belonging to the Qt port

You'll probably need a person with more qt knowledge for this one sorry -- try lars, zackr or niko (i think he has reviewer privs)
Comment 5 Holger Freyther 2007-04-28 02:58:50 PDT
(In reply to comment #4)
> (From update of attachment 14201 [details] [edit])
> You'll probably need a person with more qt knowledge for this one sorry -- try
> lars, zackr or niko (i think he has reviewer privs)
What is the appropriate way to get their attention? Set them on the CC list of this bug and wait for them to respond or mail them privately and point them to the bug?

Comment 6 Holger Freyther 2007-04-28 03:01:07 PDT
(In reply to comment #3)
> (From update of attachment 14200 [details] [edit])
> Is pcre.pri qt specific? (eg. is it only used for qmake?)

The .pri gets included by a .pro file. I would say it is qmake specific and is not limited to the Qt port.

Comment 7 Holger Freyther 2007-05-01 06:57:29 PDT
Looking at webkitdirs.pm I wonder if the Qt port really wants to force the name of the checkout to "WebKit". But that is up to them I guess.
Comment 8 Holger Freyther 2007-05-01 06:59:36 PDT
Created attachment 14291 [details]
Add the OUTPUTDIR to the INCLUDEPATH.

Updated the patch
Comment 9 Holger Freyther 2007-05-01 07:02:27 PDT
Created attachment 14292 [details]
Scope the qt-port. To allow other ports to reuse the qmake system

Updated the patch to apply to the current HEAD of trunk. Moved some files around, scoped some more defines
Comment 10 Holger Freyther 2007-05-01 07:03:59 PDT
Created attachment 14293 [details]
Make it possible to compile the Gdk port using qmake

Add Gdk,Cairo,ImageDecoders to the qmake project files. Update the build-webkit script to allow building the Gdk/Gtk-Port using qmake.
Comment 11 Holger Freyther 2007-05-01 07:09:16 PDT
Zack, here is an updated patch and as you wished I have added another port. If you agree with the changes I could fold the series and generate a proper ChangeLog, or feel free to create that yourself.
Comment 12 Simon Hausmann 2007-05-01 07:52:40 PDT
I think it would be nice if the Qt port would remain the default for the qmake build. I like the ability of doing a quick build of WebKit without being forced to use build-webkit by just running qmake && make. With the proposed patches it is neccessary to pass "CONFIG+=qt-port". Just my two cents :)
Comment 13 Holger Freyther 2007-05-01 08:38:04 PDT
(In reply to comment #12)
> I think it would be nice if the Qt port would remain the default for the qmake
> build. I like the ability of doing a quick build of WebKit without being forced
> to use build-webkit by just running qmake && make. With the proposed patches it
> is neccessary to pass "CONFIG+=qt-port". Just my two cents :)
> 

I sort of agree and build-webkit will already prefer the Qt port (when both QTDIR and the other env var is set).

so something like !gdk-port:CONFIG += qt-port in WebCore.pro should give the wanted effect?
Comment 14 Mark Rowe (bdash) 2007-05-02 02:57:26 PDT
Comment on attachment 14291 [details]
Add the OUTPUTDIR to the INCLUDEPATH.

Why is this not needed for the Qt port already?
Comment 15 Mark Rowe (bdash) 2007-05-02 03:10:56 PDT
Comment on attachment 14293 [details]
Make it possible to compile the Gdk port using qmake

+    if ((isGdk()) and ($path =~ /WebCore/)) {

I dont think you need so many parenthesis here.

+sub isGdk()
+{
+    return defined($ENV{'BUILD_GDK'})
+}

It would be great if this could be controlled by a build-webkit command-line argument.  It's more natural to do 'build-webkit --gdk' than 'BUILD_GDK=1 build-webkit' IMO.

+sub buildQMakeGdkProject($$)

Inside this function there are a few instances of "if(..)" -- our style guidelines call for a space before the parenthesis.

+gdk-port:CONFIG  += link_pkgconfig

The += is indented weirdly here.

+gdk-port:DEFINES += BUILDING_GDK__=1 BUILDING_CAIRO__
+gdk-port:CONFIG  += link_pkgconfig

Lining up the +='s with whitespace goes against our normal code style guidelines, so it may not be a good idea in build scripts either.

The duplication of the INCLUDEPATH and gdk-port:* settings in WebCore.pro and WebKit.pri isn't ideal. It'd be great if there were some way around it.
Comment 16 Mark Rowe (bdash) 2007-05-02 03:12:59 PDT
Comment on attachment 14292 [details]
Scope the qt-port. To allow other ports to reuse the qmake system

This looks ok to me but I'm not a qmake expert. I'll let Zack decide whether this change is correct.
Comment 17 Holger Freyther 2007-05-02 03:19:11 PDT
(In reply to comment #14)
> (From update of attachment 14291 [details] [edit])
> Why is this not needed for the Qt port already?
I don't know for sure. The build-webkit forces one to name the directory of the checkout WebKit. I happened to have directories with the name of WebKit-work-on-that and was forced to invoke qmake manually. Probably due my weird invocation and path setup I needed the patch. I argue that the patch in itself is correct (probably unusual) and allows me to build with my setup.

Yesterday I found the root-cause and wrote about it in Comment #7. The build-webkit script appends ../ until the directory is called WebKit. But the name of my checkout is WebKit-work-on-xyz so this part of the script fails and will cause malfunction on compilation. I can open another bug report to sanitize the behaviour of build-webkit.
Comment 18 Holger Freyther 2007-05-02 15:55:14 PDT
Created attachment 14311 [details]
Scope the qt-port. To allow other ports to reuse the qmake system

Updated patch.
This makes WebKit.pri contain common options,and WebCore/WebCore.pro includes the WebKit.pri now. This will be used by the Gdk+ port to share these options between the GdkLauncher and the library.

Removes unncessary () from the build-webkit script

Make the qt-port the default port.
Comment 19 Holger Freyther 2007-05-02 16:04:05 PDT
Created attachment 14312 [details]
Make it possible to compile the Gdk port using qmake

Update as requested.

The webkitdirs.pm/build-webkit script should follow the CodingStyle. I have decided to leave the prefix in as this is something both Qt and Gdk port will need to take a look. I have added --gdk support to the script, I was not able to use GetOption as I had to share the option between webkitdirs.pm and build-webkit

Simplify the INCLUDEPATH and other shared resources by sharing them in WebKit.pri

PS: For the Qt port I had to change a method signature from float,float to WebCore::FloatSize in DragImageQt.cpp and for the Gdk I had to add an empty stub for ImageBuffer::context. These two changes will not be found in the above patch.
Comment 20 Holger Freyther 2007-05-03 00:01:41 PDT
Created attachment 14313 [details]
Make it possible to compile the Gdk port using qmake

For some reason the first three lines of the WebCore/WebCore.pro patch was missing.
Comment 21 Simon Hausmann 2007-05-03 07:28:46 PDT
I think the patch looks fine, although I have one suggestion for perhaps a future patch: The duplicated qt-port lines could be simplified.

Old:
qt-port: FOO += BAR
qt-port: BLUB -= MEH
qt-port: SOME += MORE

New:

qt-port {
    FOO += BAR
    BLUB -= MEH
    SOME += MORE
}

I think that would look cleaner.
Comment 22 Holger Freyther 2007-05-03 11:41:10 PDT
(In reply to comment #21)
> I think the patch looks fine, although I have one suggestion for perhaps a
> future patch: The duplicated qt-port lines could be simplified.

Simon, feel free to either poke zack to review:+ or do a review:- yourself and I can update to use 'scope {}'.
Comment 23 Zack Rusin 2007-05-03 12:21:39 PDT
Comment on attachment 14311 [details]
Scope the qt-port. To allow other ports to reuse the qmake system

Of course without the DragImageQt.cpp patch.

Plus technically I'd give it a '-' because I wanted sources ordered alphabetically like they were and your patch breaks that but I don't feel like looking at this again. Please fix it after this lands.
Comment 24 Holger Freyther 2007-05-03 13:06:54 PDT
Created attachment 14322 [details]
Folded patch to add Gdk port to the qmake buildsystem

Folded all three patches into one and generated the ChangeLog. Does this need review or can it be landed directly?
A new patch taken Simon's and Zack's comment into account will follow atfer this  patch has been landed.
Comment 25 Simon Hausmann 2007-05-03 15:26:04 PDT
Landed in r21235. Zack, can you close this bug? (my bugzilla karma is not sufficient)