Bug 61207 - [Qt] Export files under Symbian Qt WebKit build
Summary: [Qt] Export files under Symbian Qt WebKit build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-20 12:07 PDT by Joe Wild
Modified: 2011-06-27 07:00 PDT (History)
6 users (show)

See Also:


Attachments
Export files under Symbian Qt WebKit build (2.57 KB, patch)
2011-05-20 12:16 PDT, Joe Wild
benjamin: review-
Details | Formatted Diff | Diff
Export headers files to the bld.inf files for the Symbian build (2.52 KB, patch)
2011-05-24 13:47 PDT, Joe Wild
laszlo.gombos: review-
Details | Formatted Diff | Diff
fix predep from extra_compiler (793 bytes, patch)
2011-05-26 03:10 PDT, Janne Koskinen
no flags Details | Formatted Diff | Diff
Export files as needed by the Symbian production build system (1.54 KB, patch)
2011-06-16 13:29 PDT, Joe Wild
laszlo.gombos: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Export files as needed by the Symbian production build system (1.56 KB, patch)
2011-06-23 07:25 PDT, Joe Wild
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Wild 2011-05-20 12:07:16 PDT
Export files under Symbian Qt WebKit build because the it does not
have a "make install" step.

This set of changes should only affect the Symbian build.
Comment 1 Joe Wild 2011-05-20 12:16:16 PDT
Created attachment 94257 [details]
Export files under Symbian Qt WebKit build

Necessary on Symbian because the Symbian build does not have a "make install" step.
Comment 2 Benjamin Poulain 2011-05-23 02:43:41 PDT
How come that was not needed before?

Janne, could you review this please?
Comment 3 Janne Koskinen 2011-05-23 04:44:22 PDT
Normal exporting of headers to webkit\include has always worked.
This patch copies files additionally into <epocroot>\epoc32\include to accomondate tighter integration with the platform. Symbian platform all public headers are in epoc32\include. So you can think of this as convinience patch.

code part:

You don't need two loops. Just iterate through WEBKIT_INSTALL_HEADERS or better yet use extra compilers like how the webkit\include is getting copied. This unless you have some specific need why it export needs to happen in bld.inf.
Comment 4 Benjamin Poulain 2011-05-23 09:49:06 PDT
Comment on attachment 94257 [details]
Export files under Symbian Qt WebKit build

Thanks Janne for looking at this.

r- from the comments.
Comment 5 Joe Wild 2011-05-23 13:48:05 PDT
We need to export to 2 different sets of files to 2 different places,
Api/ and include/QtWebkit, so that is the reason for 2 loops.

Also, the production Symbian environment does not execute the install
step so that is why we need to do it here.  And this needs to
be done in the bld.inf file.  The build steps generated for
the INSTALL_HEADERS/inst_headers section is not executed in the
production Symbian build.

Let me know if you have any questions or there is anything else I can
clean up.
Comment 6 Janne Koskinen 2011-05-23 14:21:28 PDT
(In reply to comment #5)
> Also, the production Symbian environment does not execute the install
> step so that is why we need to do it here.  And this needs to

Thats why the extra compiler target is used to copy like this:
>inst_headers.commands = $$QMAKE_COPY ${QMAKE_FILE_NAME} ${QMAKE_FILE_OUT}
This creates xcopy/cp command.
>QMAKE_EXTRA_COMPILERS += inst_headers
Adds it as a separate make step, not part of install.

Just copy/paste inst_headers parts and create new extra compiler target say inst_headers_platform and use $$INSTALL_HEADERS as input. There is no need for those loops unless you are doing post processing on the headers in webkit/include before the export phase. Target would be something like this:
>inst_headers_platform.output = >$$MW_LAYER_PUBLIC_EXPORT_PATH(QtWebKit/QtWebKit/${QMAKE_FILE_BASE}${QMAKE_FILE_EXT})

At least this how I read the code. Your target directory is the same for both API an include so no need for the distinction.
Comment 7 Joe Wild 2011-05-24 13:47:34 PDT
Created attachment 94681 [details]
Export headers files to the bld.inf files for the Symbian build

Thank you for the good comments.  Reducing to one loop was a good idea.

I changed the double loop to a single loop.

I was not able to use QMAKE_EXTRA_COMPILERS to make these changes.
I looked into it more.  It's not just the missing install step. The
Symbian production build uses mostly the bld.inf files, so these change
need to go there.
Comment 8 Janne Koskinen 2011-05-25 06:13:40 PDT
(In reply to comment #7)
> Symbian production build uses mostly the bld.inf files, so these change
> need to go there.

bld.inf allows you to sbs export if not everything is compiled from scratch. That is the only reason to use it. r+ from me.
Comment 9 Simon Hausmann 2011-05-25 17:24:01 PDT
Comment on attachment 94681 [details]
Export headers files to the bld.inf files for the Symbian build

View in context: https://bugs.webkit.org/attachment.cgi?id=94681&action=review

> Source/WebKit.pri:27
> +isEmpty(OUTPUT_DIR): OUTPUT_DIR = $$PWD

Why is this needed in the .pri file?

I thought it belongs into the .pro files, and QtWebKit.pro has it indeed.
Comment 10 Janne Koskinen 2011-05-26 03:10:49 PDT
Created attachment 94952 [details]
fix predep from extra_compiler

Tried compiling QtWebkit2.2 branch which failed on Symbian and found that there is a bug in the QtWebkit.pro and extra_compiler parameters ... thats why it didn't work when you tried.
Attached fix for the copying.. apply that to rest of the patch and you have working bld.inf copying without loops :)
Comment 11 Alexis Menard (darktears) 2011-05-26 09:05:59 PDT
Laszlo could you please have a look?
Comment 12 Joe Wild 2011-05-26 12:01:09 PDT
Thanks.  Really good comments.

I will remove the redundant change in Source/WebKit.pri and use Janne's
suggested patch/correction to add the export/copy commands into the
bld.inf.

I just want to check this with our build manager.
Comment 13 Ademar Reis 2011-06-02 06:14:43 PDT
(In reply to comment #12)
> Thanks.  Really good comments.
> 
> I will remove the redundant change in Source/WebKit.pri and use Janne's
> suggested patch/correction to add the export/copy commands into the
> bld.inf.
> 
> I just want to check this with our build manager.

Ping?
Comment 14 Joe Wild 2011-06-02 11:20:12 PDT
Sorry.  We are still looking at this. The corrections suggested by Janne Koskinen in Comment #10 work just fine in my enviroment, but fail in our production environment.  The copy command fails in the production environment.

The copy command generated in my environment looks like:

C:/APPS/sbs/win32/cygwin/bin/cp.exe o:/qtwebkit_2.2/Source/WebKit/qt/Api/qwebframe.h o:/epoc32/include/mw/QtWebKit/qwebframe.h

And in the production environment is missing "o:" on the second file as

C:/APPS/sbs/win32/cygwin/bin/cp.exe o:/qtwebkit_2.2/Source/WebKit/qt/Api/qwebframe.h 
/epoc32/include/mw/QtWebKit/qwebframe.h

I have no good explanation for this, so will continue to investigate.
Comment 15 Janne Koskinen 2011-06-02 12:27:14 PDT
> C:/APPS/sbs/win32/cygwin/bin/cp.exe o:/qtwebkit_2.2/Source/WebKit/qt/Api/qwebframe.h 
> /epoc32/include/mw/QtWebKit/qwebframe.h
> 
> I have no good explanation for this, so will continue to investigate.

Few ideas to look for.
-Raptor version of both environments.
-Compare EPOCROOT environment variable.
-configure exe/sh is the same version?
I recall this type of issue, Need to check with Miikka as he has fixed (nearly?) all EPOCROOT related issues in QMake.
Comment 16 Janne Koskinen 2011-06-02 12:31:08 PDT
> Few ideas to look for.
> -Raptor version of both environments.
> -Compare EPOCROOT environment variable.
> -configure exe/sh is the same version?
> I recall this type of issue, Need to check with Miikka as he has fixed (nearly?) all EPOCROOT related issues in QMake.

One more. are you using back-slashes somewhere in the paths? It could well cause split parse error say: \o:\foo\bar.
\o would be ignored and : would get stripped.
Comment 17 Joe Wild 2011-06-03 13:44:14 PDT
This could be problem of how QT_MAKE_INSTALL_HEADERS are defined in the dev vs prod environments.  The dev environment includes the whole path with drive letter and the prod environment is missing the driver letter.
Comment 18 Joe Wild 2011-06-06 12:35:55 PDT
(In reply to comment #17)
> This could be problem of how QT_MAKE_INSTALL_HEADERS are defined in the dev vs prod environments.  The dev environment includes the whole path with drive letter and the prod environment is missing the driver letter.

Sorry. It is QT_INSTALL_HEADERS.

OPTION COMMAND $(GNUCP) 
p:/qtwebkit_2.2/Source/WebKit/qt/Api/qwebframe.h
p:/epoc32/include/mw/QtWebKit/qwebframe.h

From dev environment.
QT_INSTALL_HEADERS:P:/epoc32/include/mw

From prod environment.
QT_INSTALL_HEADERS:\epoc32/include/mw
Comment 19 Laszlo Gombos 2011-06-06 13:22:24 PDT
(In reply to comment #2)
> How come that was not needed before?

I looked into this and it turned out that this patch was actually delivered in production SW, even though it was never landed in WebKit trunk or QtWebKit 2.x branches. 

So the issue is not specific to QtWebKit 2.2; it is just this is the first time the changes are being up-streamed. This I think is an important information for the review.
Comment 20 Laszlo Gombos 2011-06-06 13:26:13 PDT
Comment on attachment 94681 [details]
Export headers files to the bld.inf files for the Symbian build

View in context: https://bugs.webkit.org/attachment.cgi?id=94681&action=review

r- because of the change in WebKit.pri. Janne's proposal seems to be a "cleaner" way to do address this issue, so we should understand better why Janne's proposal would not work before we proceed with this approach.

>> Source/WebKit.pri:27
>> +isEmpty(OUTPUT_DIR): OUTPUT_DIR = $$PWD
> 
> Why is this needed in the .pri file?
> 
> I thought it belongs into the .pro files, and QtWebKit.pro has it indeed.

I agree that this needs to be removed; so will r- for now.
Comment 21 Joe Wild 2011-06-14 10:07:58 PDT
The fix predep from extra_compiler submitted by Janne Kiskinen needs to 
be verified in the Symbian production environment.  It works fine in my
personal environment.  Once it is verified, I can submit a patch with
change log for review.
Comment 22 Joe Wild 2011-06-16 13:29:55 PDT
Created attachment 97483 [details]
Export files as needed by the Symbian production build system

Reapplied patch as suggested by Janne Koskinen.
Comment 23 Laszlo Gombos 2011-06-20 10:46:00 PDT
Comment on attachment 97483 [details]
Export files as needed by the Symbian production build system

r=me. Dihan will do a test run on the Symbian buildbot manually. We should only land the patch after the try-run.
Comment 24 Laszlo Gombos 2011-06-22 14:58:57 PDT
Comment on attachment 97483 [details]
Export files as needed by the Symbian production build system

cq+ as this worked on the buildbot.
Comment 25 WebKit Review Bot 2011-06-22 15:01:19 PDT
Comment on attachment 97483 [details]
Export files as needed by the Symbian production build system

Rejecting attachment 97483 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
it/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Laszlo Gombos', u'--fo..." exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Source/WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/qt/QtWebKit.pro
Hunk #1 FAILED at 321.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/qt/QtWebKit.pro.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Laszlo Gombos', u'--fo..." exit_code: 1

Full output: http://queues.webkit.org/results/8919811
Comment 26 Joe Wild 2011-06-22 15:04:06 PDT
(In reply to comment #25)
I will update on latest webkit and resubmit the patch.
Comment 27 Joe Wild 2011-06-23 07:25:34 PDT
Created attachment 98348 [details]
Export files as needed by the Symbian production build system

I rebased the same patch with the latest webkit so it can be applied.
Comment 28 Laszlo Gombos 2011-06-23 08:37:34 PDT
Comment on attachment 98348 [details]
Export files as needed by the Symbian production build system

r=me.
Comment 29 WebKit Review Bot 2011-06-23 08:45:10 PDT
Comment on attachment 98348 [details]
Export files as needed by the Symbian production build system

Clearing flags on attachment: 98348

Committed r89574: <http://trac.webkit.org/changeset/89574>
Comment 30 WebKit Review Bot 2011-06-23 08:45:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Ademar Reis 2011-06-27 07:00:09 PDT
Revision r89574 cherry-picked into qtwebkit-2.2 with commit abe135e <http://gitorious.org/webkit/qtwebkit/commit/abe135e>