Bug 31273 – [Qt][Symbian] Make sure WebKit headers are included before platform headers on Symbian

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Norbert Leser]

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

[Kenneth Rohde Christiansen]

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?

[Norbert Leser]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Filed bug 31280 about the perl error.  This might be a false rejection.

[Adam Barth]

Comment on attachment 42790 [details]
Patch for USERINCLUDE paths for symbian

Let's try again.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Norbert Leser]

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.

[Eric Seidel (no email)]

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.

[Norbert Leser]

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

[WebKit Commit Bot]

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)

[Eric Seidel (no email)]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

OK.  I'm just gonna land this by hand, and let some other bug feel the brunt of the current commit-queue headaches. :)

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Ha!  There is actually a real error in the ChangeLog in the end.  I'll fix and land this by hand.

[Eric Seidel (no email)]

Committed r50970: <http://trac.webkit.org/changeset/50970>

[Norbert Leser]

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!

[Eric Seidel (no email)]

The trouble was self-inflicted. :)  I run the commit-queue and aside from the JSC crashes, these failures were all my fault. :)

[Janne Koskinen]

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.

[Norbert Leser]

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

[Janne Koskinen]

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

[Norbert Leser]

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

[Laszlo Gombos]

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.

[Eric Seidel (no email)]

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.

[Laszlo Gombos]

Comment on attachment 43203 [details]
2nd update of patch file for bug #31273

clearing flags on the already committed patch to avoid confusion.

[Laszlo Gombos]

http://trac.webkit.org/changeset/62120 committed as a temporary workaround to get the builds going.

[Diego Gonzalez]

Should this bug be marked as fixed o wait for a definitive solution?

[Laszlo Gombos]

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.

[Laszlo Gombos]

Comment on attachment 64739 [details]
proposed solution

patch does the trick for me.

[Ariya Hidayat]

Janne: any comment about the latest patch/fix from Laszlo?

[Simon Hausmann]

Comment on attachment 64739 [details]
proposed solution

Landing manually

[Simon Hausmann]

Committed r65877: <http://trac.webkit.org/changeset/65877>

[WebKit Commit Bot]

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

[Simon Hausmann]

Revision r50970 cherry-picked into qtwebkit-2.1 with commit 73452d5a19888918ffb19249ffd4a5c9e45f5c7c
Revision r65877 cherry-picked into qtwebkit-2.1 with commit 72b063270b1f7b6b909429ff9b02fdeb42b89cc4

[Laszlo Gombos]

REOPEN as this hasn't been resolved for JavaScriptCore. Patch will follow.

[Laszlo Gombos]

Created attachment 77880 [details]
proposed patch

[WebKit Commit Bot]

Comment on attachment 77880 [details]
proposed patch

Clearing flags on attachment: 77880

Committed r74978: <http://trac.webkit.org/changeset/74978>

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 77880 [details]:

http/tests/appcache/simple.html bug 51891 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[Laszlo Gombos]

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.

[Kenneth Rohde Christiansen]

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.

[Laszlo Gombos]

Comment on attachment 78284 [details]
fix for WebKit2

Committed as http://trac.webkit.org/changeset/75320.

[Laszlo Gombos]

Closing the bug as this has been resolved for JavaScriptCore, WebCore and WebKit2.