Bug 26224 – [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter

• Component: Tools / Tests
• Status: CLOSED FIXED

[Andre Pedralho]

WebKit build system should have a parameter to set the prefix folder to install binaries. It would substitute or be an additional option to $ENV{"WebKitInstallationPrefix"}.

[Andre Pedralho]

Created attachment 31010 [details]
A patch to fix the issue.

It adds the --prefix option in WebKitTools/Scripts/webkit-build script and adds it to the build option list. In WebKitTools/Scripts/webkitdirs.pm it is used as a parameter for autogen.sh,for example:
"WebKitTools/Scripts/webkit-build --gtk --prefix=/usr" which will generate
"./autogen.sh --prefix=/usr"

It will does nothing harming to Qt as it is just printing it.

[Eric Seidel (no email)]

Comment on attachment 31010 [details]
A patch to fix the issue.

Other ports (like mac) just pass *all* unrecognized arguments on to the build system.  It seems Qt should follow the same model, and then we don't need to add this argument to the common code.  Or?

--prefix is really more of a configuration step than a build step it seems.

[Andre Pedralho]

(In reply to comment #2)
> (From update of attachment 31010 [details] [review])
> Other ports (like mac) just pass *all* unrecognized arguments on to the build
> system.  It seems Qt should follow the same model, and then we don't need to
> add this argument to the common code.  Or?
> 
Actually the build-webkit script leverages those unrecognized arguments.

> --prefix is really more of a configuration step than a build step it seems.
> 
That's true. I'm trying to pass it along to the autogen.sh for the GTK build and the PREFIX for Qt.

[Adam Treat]

Comment on attachment 31010 [details]
A patch to fix the issue.

Given that Qt and GTK already are already passing this when an environment variable is detected, I don't see the harm in allowing a flag too.

r=me

[Tor Arne Vestbø]

Looks good, can you redo this on ToT? 

[Andre Pedralho]

(In reply to comment #5)
> Looks good, can you redo this on ToT? 
> 
Sure, I can. However I need to remind that those changes are not enough to set WebKit Qt target dir. We need to set it right in the .pro files below. Other way, the value in the --prefix= argument will be overridden by the values of QT_INSTALL_{LIBS,PREFIX,HEADERS,PLUGINS} vars.

grep -nHRI QT_INSTALL . --include=*.pro
./WebCore/WebCore.pro:86:        SQLITE3SRCDIR = $$[QT_INSTALL_PREFIX]/src/3rdparty/sqlite/
./WebCore/WebCore.pro:1302:            INCLUDEPATH += $$[QT_INSTALL_PREFIX]/src/3rdparty/sqlite/
./WebCore/WebCore.pro:2149:    target.path = $$[QT_INSTALL_LIBS]
./WebCore/WebCore.pro:2151:    headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
./WebCore/WebCore.pro:2153:    prf.path = $$[QT_INSTALL_PREFIX]/mkspecs/features
./WebCore/WebCore.pro:2160:        dlltarget.commands = $(COPY_FILE) $(DESTDIR)$(TARGET) $$[QT_INSTALL_BINS]
./WebCore/WebCore.pro:2174:        lib_replace.replace = $$[QT_INSTALL_LIBS]
./WebKit/qt/Plugins/Plugins.pro:12:target.path = $$[QT_INSTALL_PLUGINS]/imageformats

[Andre Pedralho]

Created attachment 31561 [details]
The same patch updated on ToT.

[Eric Seidel (no email)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 31010 [details] [review] [review])
> > Other ports (like mac) just pass *all* unrecognized arguments on to the build
> > system.  It seems Qt should follow the same model, and then we don't need to
> > add this argument to the common code.  Or?
> > 
> Actually the build-webkit script leverages those unrecognized arguments.

What does that sentence mean?

> > --prefix is really more of a configuration step than a build step it seems.
> > 
> That's true. I'm trying to pass it along to the autogen.sh for the GTK build
> and the PREFIX for Qt.

What does that mean?  Why does this need to be in build-webkit?

[Eric Seidel (no email)]

Comment on attachment 31010 [details]
A patch to fix the issue.

Removing treat's r+ from this obsolete patch.

[Gustavo Noronha (kov)]

Comment on attachment 31561 [details]
The same patch updated on ToT.

> +  --prefix=<target>                 Override WebKit installation directory

You probably want to note here that this only affects GTK+ and Qt. Also, you're missing a ChangeLog entry =).

[Antonio Gomes]

ossy, would any of you guys be interested in patching this bug ? (since you are hacking around these scripts lately)

[Csaba Osztrogonác]

(In reply to comment #11)
> ossy, would any of you guys be interested in patching this bug ? (since you are
> hacking around these scripts lately)

I will see it later. (in 3-4 hours)

[Kenneth Rohde Christiansen]

I would also like a way to skip the qmake step :-) like --skip-qmake :-)

[Andre Pedralho]

Unfortunately I'm not be available to fix this bug for now, so I'm reassigning to default.

[Rodrigo Belem]

Created attachment 51793 [details]
Previous patch rebased and added more changes.

Previous patch rebased and added more changes.

[Antonio Gomes]

(In reply to comment #15)
> Created an attachment (id=51793) [details]
> Previous patch rebased and added more changes.
> 
> Previous patch rebased and added more changes.

belem, patch looks Ok overall, but there are some requirement for all WebKit patches.

It needs a ChangeLog entry. For that run WebKitTools/Scripts/prepare-ChangeLog --bug 26224

It will create a ChangeLog entry in WebKitTools/ChangeLog . There you put the more details you can, explaining the change.

Also there is not need to sign-off patches. You add can add something like 

"Based on the previous work of André Pedralho <[e-mail]>" 

or 

"Patch by Rodrigo Belem <[e-mail]> and
          Andre Pedralho <[e-mail]> on 2009-09-07
"

or even

"2009-09-08  Rodrigo Belem <[e-mail]>, Andre Pedralho <[e-mail]>"

[Rodrigo Belem]

Created attachment 51921 [details]
Updated patch to match the webkit requirements as requested by Antonio Gomes.

[Rodrigo Belem]

CCing Simon Hausmann

[Rodrigo Belem]

CCing Gustavo Noronha

[Andre Pedralho]

(In reply to comment #17)
> Created an attachment (id=51921) [details]
> Updated patch to match the webkit requirements as requested by Antonio Gomes.

Belem, as far as I could see your patch just sets the installation prefix in the build script, which is enough to install the GTK and EFL ports builds in the right place. However, some time ago it would not be enough to set it for the Qt port. In addition, you need to verify for all the ports if it is still working. (Those were the requirements Kenneth asked me once).

[Rodrigo Belem]

Hi Pedralho,

(In reply to comment #20)
> (In reply to comment #17)
> > Created an attachment (id=51921) [details] [details]
> > Updated patch to match the webkit requirements as requested by Antonio Gomes.
> 
> Belem, as far as I could see your patch just sets the installation prefix in
> the build script, which is enough to install the GTK and EFL ports builds in
> the right place. However, some time ago it would not be enough to set it for
> the Qt port. In addition, you need to verify for all the ports if it is still
> working. (Those were the requirements Kenneth asked me once).

In this first block I added the support in the QtWebkit build system to change the prefix.

diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
index 2c71ed4..994edc9 100644
--- a/WebCore/WebCore.pro
+++ b/WebCore/WebCore.pro
@@ -2837,8 +2837,13 @@ WEBKIT_INSTALL_HEADERS = $$WEBKIT_API_HEADERS $$WEBKIT_CLASS_HEADERS
 
 !symbian {
     headers.files = $$WEBKIT_INSTALL_HEADERS
-    headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
-    target.path = $$[QT_INSTALL_LIBS]
+    !isEmpty(INSTALL_PREFIX) {
+        headers.path = $$INSTALL_PREFIX/include/qt4/QtWebKit
+	target.path = $$INSTALL_PREFIX/lib
+    } else {
+        headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
+        target.path = $$[QT_INSTALL_LIBS]
+    }
 
     INSTALLS += target headers
 } else {


And in this block I get the value from --prefix= and pass to the qmake args as INSTALL_PREFIX=$prefix


+    my $prefix;
     for my $i (0 .. $#buildParams) {
         my $opt = $buildParams[$i];
         if ($opt =~ /^--qmake=(.*)/i ) {
@@ -1319,6 +1323,8 @@ sub buildQMakeProject($@)
             push @buildArgs, $1;
         } elsif ($opt =~ /^--makeargs=(.*)/i ) {
             $makeargs = $1;
+        } elsif ($opt =~ /^--prefix=(.*)/i ) {
+            $prefix = $1;
         } else {
             push @buildArgs, $opt;
         }
@@ -1326,7 +1332,8 @@ sub buildQMakeProject($@)
 
     my $make = qtMakeCommand($qmakebin);
     my $config = configuration();
-    my $prefix = $ENV{"WebKitInstallationPrefix"};
+    $prefix = $ENV{"WebKitInstallationPrefix"} if !defined($prefix);
+    push @buildArgs, "INSTALL_PREFIX=" . $prefix if defined($prefix);



Thanks for your comment.

[Gustavo Noronha (kov)]

Comment on attachment 51921 [details]
Updated patch to match the webkit requirements as requested by Antonio Gomes.

 2841         headers.path = $$INSTALL_PREFIX/include/qt4/QtWebKit
 2842 	target.path = $$INSTALL_PREFIX/lib

This seems to be indented poorly. Tabs?

 222   --prefix=<PREFIX>                 Install files in PREFIX
221223   --makeargs=<arguments>            Optional Makefile flags

--makeargs uses a lower-cased argument place-holder, perhaps we should keep this consistent. You could use <path>, for instance. This still misses an indication that this only affects some ports, I think.

Except for these two nits, the GTK+ part of it looks good to me. I'll r- since you're not a committer, and we'll need an updated patch anyway. If someone wants to look at the Qt changes, and say r+ when these nits are addressed, the GTK+ side looks good, like I said =).

[Rodrigo Belem]

Created attachment 52043 [details]
Updated patch with the changes proposed by Gustavo Noronha and rebased with latest trunk.

[Rodrigo Belem]

Created attachment 52531 [details]
Patch rebased with the latest trunk.

[Simon Hausmann]

Comment on attachment 52531 [details]
Patch rebased with the latest trunk.



> -        headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
> -        target.path = $$[QT_INSTALL_LIBS]
> +        !isEmpty(INSTALL_PREFIX) {
> +            headers.path = $$INSTALL_PREFIX/include/qt4/QtWebKit
> +            target.path = $$INSTALL_PREFIX/lib

Hmm, it looks for you the prefix is a substitute for INSTALL_ROOT. Why not use INSTALL_ROOT instead?


Simon

[Rodrigo Belem]

(In reply to comment #25)
> (From update of attachment 52531 [details])
> 
> 
> > -        headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
> > -        target.path = $$[QT_INSTALL_LIBS]
> > +        !isEmpty(INSTALL_PREFIX) {
> > +            headers.path = $$INSTALL_PREFIX/include/qt4/QtWebKit
> > +            target.path = $$INSTALL_PREFIX/lib
> 
> Hmm, it looks for you the prefix is a substitute for INSTALL_ROOT. Why not use
> INSTALL_ROOT instead?
> 

Hi Simon,

INSTALL_PREFIX and INSTALL_ROOT are different.

A small example:

setting INSTALL_PREFIX to /usr/local
and
setting INSTALL_ROOT to /tmp

will be /tmp/usr/local/other/internal/path

[Simon Hausmann]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 52531 [details] [details])
> > 
> > 
> > > -        headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
> > > -        target.path = $$[QT_INSTALL_LIBS]
> > > +        !isEmpty(INSTALL_PREFIX) {
> > > +            headers.path = $$INSTALL_PREFIX/include/qt4/QtWebKit
> > > +            target.path = $$INSTALL_PREFIX/lib
> > 
> > Hmm, it looks for you the prefix is a substitute for INSTALL_ROOT. Why not use
> > INSTALL_ROOT instead?
> > 
> 
> Hi Simon,
> 
> INSTALL_PREFIX and INSTALL_ROOT are different.
> 
> A small example:
> 
> setting INSTALL_PREFIX to /usr/local
> and
> setting INSTALL_ROOT to /tmp
> 
> will be /tmp/usr/local/other/internal/path

Ah that's true, it is a real prefix for you.

The thing I don't like then is the hardcoding of include/qt4, which certainly reflects the Debian way of packaging, but doesn't necessary confirm with other distributions.

[Simon Hausmann]

We shouldn't block the release because of this bug, as build-webkit is not used for package/release builds.

[Rodrigo Belem]

Created attachment 54618 [details]
Updated patch with latest trunk and the requested changes

[Rodrigo Belem]

Created attachment 55152 [details]
Patch with --prefix for gtk and --install-headers and --install-libs for qt

This patch maintains the --prefix= for gtk, but removes it for qt. In the place of --prefix it was added the --install-headers and --install-libs.

The install-headers and install-libs expect for a full path, and by default the values will be the following

qmake -query QT_INSTALL_HEADERS
qmake -query QT_INSTALL_LIBS

[Simon Hausmann]

(In reply to comment #30)
> Created an attachment (id=55152) [details]
> Patch with --prefix for gtk and --install-headers and --install-libs for qt
> 
> This patch maintains the --prefix= for gtk, but removes it for qt. In the place
> of --prefix it was added the --install-headers and --install-libs.
> 
> The install-headers and install-libs expect for a full path, and by default the
> values will be the following
> 
> qmake -query QT_INSTALL_HEADERS
> qmake -query QT_INSTALL_LIBS

Looks okay to me. Jocelyn, what do you think?

[Jocelyn Turcotte]

(In reply to comment #31)
> Looks okay to me. Jocelyn, what do you think?

*thumbs up*

[Simon Hausmann]

Comment on attachment 55152 [details]
Patch with --prefix for gtk and --install-headers and --install-libs for qt


>      !symbian {
>          headers.files = $$WEBKIT_INSTALL_HEADERS
> -        headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
> -        target.path = $$[QT_INSTALL_LIBS]
> +        isEmpty(INSTALL_HEADERS) {
> +            headers.path = $$[QT_INSTALL_HEADERS]/QtWebKit
> +        } else {
> +            headers.path = $$INSTALL_HEADERS/QtWebKit
> +        }
> +
> +        isEmpty(INSTALL_LIBS) {
> +            target.path = $$[QT_INSTALL_LIBS]
> +        } else {
> +            target.path = $$INSTALL_LIBS
> +        }

This needs to be done also for the Symbian path. Maybe it's easiest to do
it like this:

isEmpty(INSTALL_HEADERS): INSTALL_HEADERS = $$[QT_INSTALL_HEADERS]

and then later use that variable in both blocks.

The rest of the patch looks good to me, so r- only because of the missing Symbian block.

[Rodrigo Belem]

Created attachment 55375 [details]
Updated patch with latest requested changes

[Kenneth Rohde Christiansen]

Comment on attachment 55375 [details]
Updated patch with latest requested changes

WebKitTools/Scripts/build-webkit:231
 +    --install-headers=<path>          Set installation path for the QtWebKit headers
Maybe set installation path for the headers (Qt only)

instead?

WebKitTools/Scripts/build-webkit:234
 +    --prefix=<path>                   Set installation prefix to the given path (Gtk only)
Was this here before?

[Rodrigo Belem]

Created attachment 55395 [details]
Updated patch with latest requested changes

[WebKit Commit Bot]

Comment on attachment 55395 [details]
Updated patch with latest requested changes

Rejecting patch 55395 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55395, '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
cgi?id=55395&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=26224&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 55395 from bug 26224.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Rodrigo Belem]

(In reply to comment #37)
> (From update of attachment 55395 [details])
> Rejecting patch 55395 from commit-queue.
> 
> Unexpected failure when landing patch!  Please file a bug against webkit-patch.
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55395, '--parent-command=commit-queue', '--no-update']" exit_code: 1
> Last 500 characters of output:
> cgi?id=55395&action=edit
> Fetching: https://bugs.webkit.org/show_bug.cgi?id=26224&ctype=xml
> Processing 1 patch from 1 bug.
> Cleaning working directory
> Processing patch 55395 from bug 26224.
> NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
> ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).


This is not a problem in my patch. There is a wrong entry in WebCore/ChangeLog there were commited in the revision 59085 and in git it is 60f54909e4e4c92c5731e3d1a111da6fbb41875a.

How should I do?

[Rodrigo Belem]

(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 55395 [details] [details])
> > Rejecting patch 55395 from commit-queue.
> > 
> > Unexpected failure when landing patch!  Please file a bug against webkit-patch.
> > Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55395, '--parent-command=commit-queue', '--no-update']" exit_code: 1
> > Last 500 characters of output:
> > cgi?id=55395&action=edit
> > Fetching: https://bugs.webkit.org/show_bug.cgi?id=26224&ctype=xml
> > Processing 1 patch from 1 bug.
> > Cleaning working directory
> > Processing patch 55395 from bug 26224.
> > NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
> > ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> 
> This is not a problem in my patch. There is a wrong entry in WebCore/ChangeLog there were commited in the revision 59085 and in git it is 60f54909e4e4c92c5731e3d1a111da6fbb41875a.
> 
> How should I do?

I mean "How should I proceed?".

[Rodrigo Belem]

Created attachment 55695 [details]
Updated patch with latest trunk

[WebKit Commit Bot]

Comment on attachment 55695 [details]
Updated patch with latest trunk

Rejecting patch 55695 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55695, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
cgi?id=55695&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=26224&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 55695 from bug 26224.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Rodrigo Belem]

(In reply to comment #41)
> (From update of attachment 55695 [details])
> Rejecting patch 55695 from commit-queue.
> 
> Unexpected failure when landing patch!  Please file a bug against webkit-patch.
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55695, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
> Last 500 characters of output:
> cgi?id=55695&action=edit
> Fetching: https://bugs.webkit.org/show_bug.cgi?id=26224&ctype=xml
> Processing 1 patch from 1 bug.
> Cleaning working directory
> Processing patch 55695 from bug 26224.
> NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
> ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).



I don't know what is wrong. What should I do to fix this?

[Antonio Gomes]

Comment on attachment 55695 [details]
Updated patch with latest trunk

Committed r59235: <http://trac.webkit.org/changeset/59235>

[Eric Seidel (no email)]

I'm not sure what went wrong.  Investigating.

[Eric Seidel (no email)]

I tested with:

% git checkout revision_before_your_commit
% webkit-patch apply-from-bug 26224
% webkit-patch commit-message
Parsing ChangeLog: /Projects/WebKit/WebCore/ChangeLog
Parsing ChangeLog: /Projects/WebKit/WebKitTools/ChangeLog
2010-05-12  Rodrigo Belem  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter
        https://bugs.webkit.org/show_bug.cgi?id=26224

        This patch adds the ability, in the QtWebkit build system, to change
        the installation path.

        * WebCore.pro:
2010-05-12  Rodrigo Belem  <[e-mail]>

        Reviewed by Kenneth Rohde Christiansen.

        [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter
        https://bugs.webkit.org/show_bug.cgi?id=26224

        Added more parameters to build-webkit script, the --prefix for gkt
        and --install-libs, --install-headers for qt. Now it is possible
        to change the install prefix for gtk and install path for qt.

        * Scripts/build-webkit:
        * Scripts/webkitdirs.pm:

It appears svn-apply failed to set the reviewer correctly on the first ChangeLog.  I'm not sure why.

[Eric Seidel (no email)]

I filed bug 38998 about the script failure.  My apologies!

[Simon Hausmann]

Revision r59235 cherry-picked into qtwebkit-2.0 with commit 5c45aa1fbfd654beab5d8ffdcf426e899b77f648