Bug 26224

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 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.

(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 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

Looks good, can you redo this on ToT?

(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

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

(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 on attachment 31010 [details] A patch to fix the issue. Removing treat's r+ from this obsolete patch.

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 =).

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

(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)

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

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

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

(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>"

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

CCing Simon Hausmann

CCing Gustavo Noronha

(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).

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 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 =).

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

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

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

(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

(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.

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

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

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

(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?

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

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.

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

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?

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

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).

(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?

(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?".

Created attachment 55695 [details] Updated patch with latest trunk

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).

(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 on attachment 55695 [details] Updated patch with latest trunk Committed r59235: <http://trac.webkit.org/changeset/59235>

I'm not sure what went wrong. Investigating.

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.

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

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