Bug 61207 - [Qt] Export files under Symbian Qt WebKit build
[Qt] Export files under Symbian Qt WebKit build
 Status: RESOLVED FIXED None WebKit Unclassified WebKit Qt (show other bugs) 528+ (Nightly build) S60 Hardware S60 3rd edition P3 Normal Nobody Qt, QtTriaged

 Reported: 2011-05-20 12:07 PDT by Joe Wild 2011-06-27 07:00 PDT (History) 6 users (show) ademar benjamin koshuin laszlo.gombos menard webkit.review.bot

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.
 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. 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. Benjamin Poulain 2011-05-23 02:43:41 PDT How come that was not needed before? Janne, could you review this please? 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 \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 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. 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. 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. 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. 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. 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. 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 :) Alexis Menard (darktears) 2011-05-26 09:05:59 PDT Laszlo could you please have a look? 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. 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? 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. 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. 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. 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. 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 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. 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. 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. 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. 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. 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. 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 Joe Wild 2011-06-22 15:04:06 PDT (In reply to comment #25) I will update on latest webkit and resubmit the patch. 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. 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. 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:  WebKit Review Bot 2011-06-23 08:45:16 PDT All reviewed patches have been landed. Closing bug. Ademar Reis 2011-06-27 07:00:09 PDT Revision r89574 cherry-picked into qtwebkit-2.2 with commit abe135e