Bug 31273 - [Qt][Symbian] Make sure WebKit headers are included before platform headers on Symbian
Summary: [Qt][Symbian] Make sure WebKit headers are included before platform headers o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-11-09 13:11 PST by Norbert Leser
Modified: 2011-01-08 07:02 PST (History)
9 users (show)

See Also:


Attachments
Patch for USERINCLUDE paths for symbian (2.36 KB, patch)
2009-11-09 13:11 PST, Norbert Leser
no flags Details | Formatted Diff | Diff
2nd update of patch file for bug #31273 (2.45 KB, patch)
2009-11-13 14:24 PST, Norbert Leser
no flags Details | Formatted Diff | Diff
proposed solution (2.46 KB, patch)
2010-08-18 11:05 PDT, Laszlo Gombos
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
proposed patch (2.08 KB, patch)
2011-01-04 05:20 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
fix for WebKit2 (2.94 KB, patch)
2011-01-07 15:35 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert Leser 2009-11-09 13:11:18 PST
Created attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Added macros for USERINCLUDE paths within symbian blocks to guarantee inclusion of respective header files from local path first (to avoid clashes with same names of header files in system include path).
Comment 1 Kenneth Rohde Christiansen 2009-11-09 13:23:00 PST
Comment on attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Looks plausable. How do you ensure that you don't have this problem with other includes?
Comment 2 Norbert Leser 2009-11-09 13:36:17 PST
(In reply to comment #1)
> (From update of attachment 42790 [details])
> Looks plausable. How do you ensure that you don't have this problem with other
> includes?

We have verified that it is currently not the case, but unfortunately, there is no guarantee that this will happen with other include file name clashes in the future, at least not with this current version of the symbian tools chain.
Comment 3 WebKit Commit Bot 2009-11-09 17:16:14 PST
Comment on attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Rejecting patch 42790 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 1
Last 500 characters of output:
.pm line 397, <> line 66.
Use of uninitialized value in subtraction (-) at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 397, <> line 66.
Use of uninitialized value in subtraction (-) at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 398, <> line 66.
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/WebCore.pro
Hunk #1 FAILED at 12.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/WebCore.pro.rej
Comment 4 Eric Seidel 2009-11-09 17:20:45 PST
Filed bug 31280 about the perl error.  This might be a false rejection.
Comment 5 Adam Barth 2009-11-12 18:15:32 PST
Comment on attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Let's try again.
Comment 6 WebKit Commit Bot 2009-11-12 18:20:40 PST
Comment on attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Rejecting patch 42790 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 1
Last 500 characters of output:
.pm line 396, <> line 66.
Use of uninitialized value in subtraction (-) at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 396, <> line 66.
Use of uninitialized value in subtraction (-) at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 397, <> line 66.
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/WebCore.pro
Hunk #1 FAILED at 12.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/WebCore.pro.rej
Comment 7 Eric Seidel 2009-11-13 11:47:02 PST
There is definitely a bug in svn-apply here, but I don't think it's causing this failure.  I think the patch simply no longer applies.
Comment 8 Norbert Leser 2009-11-13 14:24:22 PST
Created attachment 43203 [details]
2nd update of patch file for bug #31273

Created a new patch (no content change). Apparently, the committer process thought there is a merge conflict.
Comment 9 Eric Seidel 2009-11-13 14:27:43 PST
Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

It is possible I've broken svn-apply with my recent fixes to the fixChangeLog function, and that this patch is triggering a bug there.
Comment 10 Norbert Leser 2009-11-13 14:34:04 PST
(In reply to comment #9)
> (From update of attachment 43203 [details])
> It is possible I've broken svn-apply with my recent fixes to the fixChangeLog
> function, and that this patch is triggering a bug there.

It more likely is a perceived conflict with checkin to bug#31272, which added a couple of lines to WebCore.pro at the same place as this one does.
Comment 11 WebKit Commit Bot 2009-11-13 14:44:45 PST
Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

Rejecting patch 43203 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
ld/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCompositionEvent.o /Users/eseidel/Projects/CommitQueue/WebKitBuild/Release/DerivedSources/WebCore/JSCompositionEvent.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSEventCustom.o /Users/eseidel/Projects/CommitQueue/WebCore/bindings/js/JSEventCustom.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(2 failures)
Comment 12 Eric Seidel 2009-11-13 14:47:03 PST
Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

My continued apologies.  I've reverted back to the old commit-queue machine until I can figure out the build issue.  Hopefully the next run will not hit the JSC crasher. :(
Comment 13 WebKit Commit Bot 2009-11-13 15:11:29 PST
Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

Rejecting patch 43203 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Last 500 characters of output:
outTests
Testing 11622 test cases.
fast/canvas/canvas-longlived-context.html -> timed out
Sampling process 46111 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 46111 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 4894 tests run.
838.61s total testing time

4893 test cases (99%) succeeded
1 test case (<1%) timed out
3 test cases (<1%) had stderr output
Comment 14 Eric Seidel 2009-11-13 15:17:40 PST
OK.  I'm just gonna land this by hand, and let some other bug feel the brunt of the current commit-queue headaches. :)
Comment 15 Eric Seidel 2009-11-13 15:19:30 PST
Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

Rejecting patch 43203 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
tory/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/JavaScriptCore.pri
	M	WebCore/ChangeLog
	M	WebCore/WebCore.pro
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 16 Eric Seidel 2009-11-13 15:20:05 PST
Ha!  There is actually a real error in the ChangeLog in the end.  I'll fix and land this by hand.
Comment 17 Eric Seidel 2009-11-13 15:21:45 PST
Committed r50970: <http://trac.webkit.org/changeset/50970>
Comment 18 Norbert Leser 2009-11-13 15:29:03 PST
Wow, that was a rough landing ;)
I am sorry for the trouble you had to go through for this little patch (I thought) - and thanks, for the time!
Comment 19 Eric Seidel 2009-11-13 15:34:11 PST
The trouble was self-inflicted. :)  I run the commit-queue and aside from the JSC crashes, these failures were all my fault. :)
Comment 20 Janne Koskinen 2009-11-16 02:01:19 PST
sigh.. why on earth this was landed.
I know what it tries to solve, but did you try this on 3.1,3.2,5.0,5.1 MCL, product and public SDK variants ?
Patch alters the inclusion order and is most likely to break.
Another reason not to have it is that it is specific to only some directories. If this kinda of fix is really necessary (I've yet to see any broken builds due to this) then do it for all directories. i.e. use userinclude . and try it on all above variants. userinclude . makes it consistent will solve all #include "localheader.h" cases.
Comment 21 Norbert Leser 2009-11-16 04:39:50 PST
(In reply to comment #20)
> sigh.. why on earth this was landed.
> I know what it tries to solve, but did you try this on 3.1,3.2,5.0,5.1 MCL,
> product and public SDK variants ?
> Patch alters the inclusion order and is most likely to break.
> Another reason not to have it is that it is specific to only some directories.
> If this kinda of fix is really necessary (I've yet to see any broken builds due
> to this) then do it for all directories. i.e. use userinclude . and try it on
> all above variants. userinclude . makes it consistent will solve all #include
> "localheader.h" cases.

On which targets did it break for you? What was the error? (The targets I did not try it on are 3.x).

I agree with you that it would be preferable to apply this change to all localheader include cases. I am not sure what you mean with using "userinclude" and how to easily achieve that with current qmake for symbian? I used USERINCLUDE by mapping via MMP_RULES. If there is another way of generally applying userinclude, I'd love to use that.

The patch for these particular include paths on targets beyond 5.0 due to changes in the (abld) tools chain. The first SYSTEMINCLUDE always becomes /epoc32/include, regardless of declarations in mmp files. That is, we must use USERINCLUDE to guarantee inclusion before that.
Comment 22 Janne Koskinen 2009-11-16 05:28:15 PST
(In reply to comment #21)

I haven't tried the patch yet so don't know if it broke anything.
Headers are moved about in MCL releases and I would assume this could potentially break anyways.

> I agree with you that it would be preferable to apply this change to all
> localheader include cases. I am not sure what you mean with using "userinclude"

MMP_RULES += "USERINCLUDE ."

or change each include not under epoc32\include to be user includes.

Would be the only 2 alternatives I can live with :)

> 
> The patch for these particular include paths on targets beyond 5.0 due to
> changes in the (abld) tools chain. The first SYSTEMINCLUDE always becomes
> /epoc32/include, regardless of declarations in mmp files. That is, we must use
> USERINCLUDE to guarantee inclusion before that.

This was a bug in SBSv2 and also on abld and has been fixed from SBSv2 already. It was added to fix some silly test case without thinking...

Toolchain is bugged, but we are trying to fix it instead of trying to work around it.
Comment 23 Norbert Leser 2009-11-16 06:10:00 PST
(In reply to comment #22)
> (In reply to comment #21)
> 
> I haven't tried the patch yet so don't know if it broke anything.
> Headers are moved about in MCL releases and I would assume this could
> potentially break anyways.
> 
> > I agree with you that it would be preferable to apply this change to all
> > localheader include cases. I am not sure what you mean with using "userinclude"
> 
> MMP_RULES += "USERINCLUDE ."
> 
> or change each include not under epoc32\include to be user includes.
> 
> Would be the only 2 alternatives I can live with :)

Wouldn't your first option still require some of the webkit code to be changed to define includes relative to ".", such as #include "profiler/profiler.h"? That is what I wanted to avoid (I believe there was a request with this patch earlier this year which was either rejected or reversed).

The 2nd option seems to be very invasive, if I understand that right (would that require to special case for SYMBIAN almost all include paths in JavaScripCore.pri?

> > 
> > The patch for these particular include paths on targets beyond 5.0 due to
> > changes in the (abld) tools chain. The first SYSTEMINCLUDE always becomes
> > /epoc32/include, regardless of declarations in mmp files. That is, we must use
> > USERINCLUDE to guarantee inclusion before that.
> 
> This was a bug in SBSv2 and also on abld and has been fixed from SBSv2 already.
> It was added to fix some silly test case without thinking...
> 
> Toolchain is bugged, but we are trying to fix it instead of trying to work
> around it.

I am with you, to fix the toolchain, but we are facing the problem of build breaks right now and I have no idea when the fixed toolchain will be available. Besides, it is not a specific but a structural issue that the symbian toolchain uses USERINCLUDE paths instead of honoring the "localinclude.h" property. I haven't seen any attempt to fix that, and we have to work around that, somehow, in the least intrusive way (I still think, if we proof that these 3 USERINCLUDE declarations of this patch don't break other targets, it seems to be a useful workaround for the current situation).
Comment 24 Laszlo Gombos 2010-06-28 14:48:54 PDT
As anticipated we now have more directories to special case (bridge, platform/animation). I agree with Janne that we need to find a better approach. 

Re-opening the bug with the intention of reverting 50970 and other possible workarounds. We need to re-evaulate which tool chain issues are fixed and which ones are remains to be fixed.
Comment 25 Eric Seidel 2010-06-29 03:15:47 PDT
Comment on attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 42790 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 26 Laszlo Gombos 2010-06-29 07:40:22 PDT
Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

clearing flags on the already committed patch to avoid confusion.
Comment 27 Laszlo Gombos 2010-06-29 11:30:39 PDT
http://trac.webkit.org/changeset/62120 committed as a temporary workaround to get the builds going.
Comment 28 Diego Gonzalez 2010-07-21 05:51:42 PDT
Should this bug be marked as fixed o wait for a definitive solution?
Comment 29 Laszlo Gombos 2010-08-18 11:05:39 PDT
Created attachment 64739 [details]
proposed solution

On Symbian PREPEND_INCLUDEPATH is the best way to make sure that WebKit headers are included before platform headers. On all other platforms continue to use INCLUDEPATH (as before).

This patch also removed the workarounds that are put in place now that we have a better solution.

Still testing the patch will put up for review once testing is finished.
Comment 30 Laszlo Gombos 2010-08-18 13:10:43 PDT
Comment on attachment 64739 [details]
proposed solution

patch does the trick for me.
Comment 31 Ariya Hidayat 2010-08-22 01:28:47 PDT
Janne: any comment about the latest patch/fix from Laszlo?
Comment 32 Simon Hausmann 2010-08-24 01:50:47 PDT
Comment on attachment 64739 [details]
proposed solution

Landing manually
Comment 33 Simon Hausmann 2010-08-24 01:52:16 PDT
Committed r65877: <http://trac.webkit.org/changeset/65877>
Comment 34 WebKit Commit Bot 2010-08-24 02:31:37 PDT
Comment on attachment 64739 [details]
proposed solution

Rejecting patch 64739 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
mmitQueue/LayoutTests
Testing 20899 test cases.
media/video-autoplay.html -> timed out
Sampling process 77784 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 77784 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 17221 tests run.
679.55s total testing time

17220 test cases (99%) succeeded
1 test case (<1%) timed out
34 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3740592
Comment 35 Simon Hausmann 2010-08-24 07:14:23 PDT
Revision r50970 cherry-picked into qtwebkit-2.1 with commit 73452d5a19888918ffb19249ffd4a5c9e45f5c7c
Revision r65877 cherry-picked into qtwebkit-2.1 with commit 72b063270b1f7b6b909429ff9b02fdeb42b89cc4
Comment 36 Laszlo Gombos 2011-01-04 05:19:40 PST
REOPEN as this hasn't been resolved for JavaScriptCore. Patch will follow.
Comment 37 Laszlo Gombos 2011-01-04 05:20:41 PST
Created attachment 77880 [details]
proposed patch
Comment 38 WebKit Commit Bot 2011-01-04 11:16:52 PST
Comment on attachment 77880 [details]
proposed patch

Clearing flags on attachment: 77880

Committed r74978: <http://trac.webkit.org/changeset/74978>
Comment 39 WebKit Commit Bot 2011-01-04 12:47:31 PST
The commit-queue encountered the following flaky tests while processing attachment 77880 [details]:

http/tests/appcache/simple.html bug 51891 (authors: andersca@apple.com and ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 40 Laszlo Gombos 2011-01-07 15:35:46 PST
Created attachment 78284 [details]
fix for WebKit2

As it turns out WebKit2 part of the build needs this fix as well for Symbian. I wanted to minimize the "Symbian only" sections, that is why this work is being done piecemeal on a need bases.
Comment 41 Kenneth Rohde Christiansen 2011-01-07 15:39:39 PST
Comment on attachment 78284 [details]
fix for WebKit2

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

> WebKit2/WebKit2.pro:168
>  
> +symbian {
> +    PREPEND_INCLUDEPATH = $$WEBKIT2_INCLUDEPATH $$PREPEND_INCLUDEPATH
> +} else {
> +    INCLUDEPATH = $$WEBKIT2_INCLUDEPATH $$INCLUDEPATH
> +}

I think it would be nice with a comment here.
Comment 42 Laszlo Gombos 2011-01-08 07:01:52 PST
Comment on attachment 78284 [details]
fix for WebKit2

Committed as http://trac.webkit.org/changeset/75320.
Comment 43 Laszlo Gombos 2011-01-08 07:02:54 PST
Closing the bug as this has been resolved for JavaScriptCore, WebCore and WebKit2.