Bug 61207 – [Qt] Export files under Symbian Qt WebKit build

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Joe Wild]

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.

[Joe Wild]

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.

[Benjamin Poulain]

How come that was not needed before?

Janne, could you review this please?

[Janne Koskinen]

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.

[Benjamin Poulain]

Comment on attachment 94257 [details]
Export files under Symbian Qt WebKit build

Thanks Janne for looking at this.

r- from the comments.

[Joe Wild]

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.

[Janne Koskinen]

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

[Joe Wild]

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.

[Janne Koskinen]

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

[Simon Hausmann]

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.

[Janne Koskinen]

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

[Alexis Menard (darktears)]

Laszlo could you please have a look?

[Joe Wild]

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.

[Ademar Reis]

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

[Joe Wild]

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.

[Janne Koskinen]

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

[Janne Koskinen]

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

[Joe Wild]

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.

[Joe Wild]

(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

[Laszlo Gombos]

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

[Laszlo Gombos]

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.

[Joe Wild]

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.

[Joe Wild]

Created attachment 97483 [details]
Export files as needed by the Symbian production build system

Reapplied patch as suggested by Janne Koskinen.

[Laszlo Gombos]

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.

[Laszlo Gombos]

Comment on attachment 97483 [details]
Export files as needed by the Symbian production build system

cq+ as this worked on the buildbot.

[WebKit Review Bot]

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

[Joe Wild]

(In reply to comment #25)
I will update on latest webkit and resubmit the patch.

[Joe Wild]

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.

[Laszlo Gombos]

Comment on attachment 98348 [details]
Export files as needed by the Symbian production build system

r=me.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r89574 cherry-picked into qtwebkit-2.2 with commit abe135e <http://gitorious.org/webkit/qtwebkit/commit/abe135e>