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.
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.
How come that was not needed before? Janne, could you review this please?
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 on attachment 94257 [details] Export files under Symbian Qt WebKit build Thanks Janne for looking at this. r- from the comments.
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.
(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.
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.
(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 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.
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 :)
Laszlo could you please have a look?
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.
(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?
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.
> 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.
> 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.
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.
(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
(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 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.
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.
Created attachment 97483 [details] Export files as needed by the Symbian production build system Reapplied patch as suggested by Janne Koskinen.
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 on attachment 97483 [details] Export files as needed by the Symbian production build system cq+ as this worked on the buildbot.
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
(In reply to comment #25) I will update on latest webkit and resubmit the patch.
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 on attachment 98348 [details] Export files as needed by the Symbian production build system r=me.
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>
All reviewed patches have been landed. Closing bug.
Revision r89574 cherry-picked into qtwebkit-2.2 with commit abe135e <http://gitorious.org/webkit/qtwebkit/commit/abe135e>