Bug 26224 - [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter
Summary: [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Rodrigo Belem
URL:
Keywords: Gtk, Qt
Depends on:
Blocks:
 
Reported: 2009-06-05 13:09 PDT by Andre Pedralho
Modified: 2011-04-19 05:15 PDT (History)
13 users (show)

See Also:


Attachments
A patch to fix the issue. (5.00 KB, patch)
2009-06-05 13:18 PDT, Andre Pedralho
no flags Details | Formatted Diff | Diff
The same patch updated on ToT. (3.66 KB, patch)
2009-06-19 13:19 PDT, Andre Pedralho
gns: review-
Details | Formatted Diff | Diff
Previous patch rebased and added more changes. (5.71 KB, patch)
2010-03-26 16:10 PDT, Rodrigo Belem
no flags Details | Formatted Diff | Diff
Updated patch to match the webkit requirements as requested by Antonio Gomes. (7.38 KB, patch)
2010-03-29 09:21 PDT, Rodrigo Belem
gns: review-
Details | Formatted Diff | Diff
Updated patch with the changes proposed by Gustavo Noronha and rebased with latest trunk. (7.46 KB, patch)
2010-03-30 08:45 PDT, Rodrigo Belem
no flags Details | Formatted Diff | Diff
Patch rebased with the latest trunk. (7.44 KB, patch)
2010-04-05 06:49 PDT, Rodrigo Belem
no flags Details | Formatted Diff | Diff
Updated patch with latest trunk and the requested changes (7.43 KB, patch)
2010-04-28 14:22 PDT, Rodrigo Belem
no flags Details | Formatted Diff | Diff
Patch with --prefix for gtk and --install-headers and --install-libs for qt (8.17 KB, patch)
2010-05-05 13:48 PDT, Rodrigo Belem
hausmann: review-
Details | Formatted Diff | Diff
Updated patch with latest requested changes (8.84 KB, patch)
2010-05-07 08:04 PDT, Rodrigo Belem
no flags Details | Formatted Diff | Diff
Updated patch with latest requested changes (8.93 KB, patch)
2010-05-07 10:47 PDT, Rodrigo Belem
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Updated patch with latest trunk (8.89 KB, patch)
2010-05-11 06:21 PDT, Rodrigo Belem
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Pedralho 2009-06-05 13:09:12 PDT
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"}.
Comment 1 Andre Pedralho 2009-06-05 13:18:36 PDT
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.
Comment 2 Eric Seidel (no email) 2009-06-14 02:26:12 PDT
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.
Comment 3 Andre Pedralho 2009-06-15 10:41:36 PDT
(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.
Comment 4 Adam Treat 2009-06-18 10:34:40 PDT
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
Comment 5 Tor Arne Vestbø 2009-06-19 03:11:43 PDT
Looks good, can you redo this on ToT? 
Comment 6 Andre Pedralho 2009-06-19 13:17:38 PDT
(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
Comment 7 Andre Pedralho 2009-06-19 13:19:21 PDT
Created attachment 31561 [details]
The same patch updated on ToT.
Comment 8 Eric Seidel (no email) 2009-06-20 01:44:50 PDT
(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?
Comment 9 Eric Seidel (no email) 2009-06-24 01:13:58 PDT
Comment on attachment 31010 [details]
A patch to fix the issue.

Removing treat's r+ from this obsolete patch.
Comment 10 Gustavo Noronha (kov) 2009-06-30 11:38:41 PDT
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 =).
Comment 11 Antonio Gomes 2010-01-19 11:44:57 PST
ossy, would any of you guys be interested in patching this bug ? (since you are hacking around these scripts lately)
Comment 12 Csaba Osztrogonác 2010-01-19 11:56:42 PST
(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)
Comment 13 Kenneth Rohde Christiansen 2010-01-20 04:07:12 PST
I would also like a way to skip the qmake step :-) like --skip-qmake :-)
Comment 14 Andre Pedralho 2010-03-25 16:19:30 PDT
Unfortunately I'm not be available to fix this bug for now, so I'm reassigning to default.
Comment 15 Rodrigo Belem 2010-03-26 16:10:02 PDT
Created attachment 51793 [details]
Previous patch rebased and added more changes.

Previous patch rebased and added more changes.
Comment 16 Antonio Gomes 2010-03-27 05:45:28 PDT
(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 <andre.pedralho@openbossa.org>" 

or 

"Patch by Rodrigo Belem <email@here.xxx> and
          Andre Pedralho <email@here.xxx> on 2009-09-07
"

or even

"2009-09-08  Rodrigo Belem <email@here.xxx>, Andre Pedralho <email@here.xxx>"
Comment 17 Rodrigo Belem 2010-03-29 09:21:36 PDT
Created attachment 51921 [details]
Updated patch to match the webkit requirements as requested by Antonio Gomes.
Comment 18 Rodrigo Belem 2010-03-29 09:25:58 PDT
CCing Simon Hausmann
Comment 19 Rodrigo Belem 2010-03-29 09:27:42 PDT
CCing Gustavo Noronha
Comment 20 Andre Pedralho 2010-03-29 11:02:46 PDT
(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).
Comment 21 Rodrigo Belem 2010-03-29 11:39:55 PDT
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.
Comment 22 Gustavo Noronha (kov) 2010-03-29 16:11:04 PDT
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 =).
Comment 23 Rodrigo Belem 2010-03-30 08:45:55 PDT
Created attachment 52043 [details]
Updated patch with the changes proposed by Gustavo Noronha and rebased with latest trunk.
Comment 24 Rodrigo Belem 2010-04-05 06:49:05 PDT
Created attachment 52531 [details]
Patch rebased with the latest trunk.
Comment 25 Simon Hausmann 2010-04-19 16:49:56 PDT
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
Comment 26 Rodrigo Belem 2010-04-19 17:33:08 PDT
(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
Comment 27 Simon Hausmann 2010-04-20 11:55:33 PDT
(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.
Comment 28 Simon Hausmann 2010-04-26 06:55:17 PDT
We shouldn't block the release because of this bug, as build-webkit is not used for package/release builds.
Comment 29 Rodrigo Belem 2010-04-28 14:22:08 PDT
Created attachment 54618 [details]
Updated patch with latest trunk and the requested changes
Comment 30 Rodrigo Belem 2010-05-05 13:48:35 PDT
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
Comment 31 Simon Hausmann 2010-05-05 13:57:55 PDT
(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?
Comment 32 Jocelyn Turcotte 2010-05-06 00:48:58 PDT
(In reply to comment #31)
> Looks okay to me. Jocelyn, what do you think?

*thumbs up*
Comment 33 Simon Hausmann 2010-05-07 06:34:35 PDT
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.
Comment 34 Rodrigo Belem 2010-05-07 08:04:23 PDT
Created attachment 55375 [details]
Updated patch with latest requested changes
Comment 35 Kenneth Rohde Christiansen 2010-05-07 10:23:46 PDT
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?
Comment 36 Rodrigo Belem 2010-05-07 10:47:55 PDT
Created attachment 55395 [details]
Updated patch with latest requested changes
Comment 37 WebKit Commit Bot 2010-05-08 10:42:53 PDT
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).
Comment 38 Rodrigo Belem 2010-05-10 16:58:45 PDT
(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?
Comment 39 Rodrigo Belem 2010-05-10 17:00:15 PDT
(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?".
Comment 40 Rodrigo Belem 2010-05-11 06:21:05 PDT
Created attachment 55695 [details]
Updated patch with latest trunk
Comment 41 WebKit Commit Bot 2010-05-12 01:57:56 PDT
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).
Comment 42 Rodrigo Belem 2010-05-12 07:17:07 PDT
(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?
Comment 43 Antonio Gomes 2010-05-12 08:26:59 PDT
Comment on attachment 55695 [details]
Updated patch with latest trunk

Committed r59235: <http://trac.webkit.org/changeset/59235>
Comment 44 Eric Seidel (no email) 2010-05-12 08:44:03 PDT
I'm not sure what went wrong.  Investigating.
Comment 45 Eric Seidel (no email) 2010-05-12 09:09:42 PDT
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  <rodrigo.belem@openbossa.org>

        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  <rodrigo.belem@openbossa.org>

        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.
Comment 46 Eric Seidel (no email) 2010-05-12 09:16:08 PDT
I filed bug 38998 about the script failure.  My apologies!
Comment 47 Simon Hausmann 2010-06-01 08:00:20 PDT
Revision r59235 cherry-picked into qtwebkit-2.0 with commit 5c45aa1fbfd654beab5d8ffdcf426e899b77f648