Bug 31273

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?

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

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

Comment on attachment 42790 [details] Patch for USERINCLUDE paths for symbian Let's try again.

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

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.

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

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

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

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

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

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

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!

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

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.

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

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

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

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 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 on attachment 43203 [details] 2nd update of patch file for bug #31273 clearing flags on the already committed patch to avoid confusion.

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

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

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 on attachment 64739 [details] proposed solution patch does the trick for me.

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

Comment on attachment 64739 [details] proposed solution Landing manually

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

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

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

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

Created attachment 77880 [details] proposed patch

Comment on attachment 77880 [details] proposed patch Clearing flags on attachment: 77880 Committed r74978: <http://trac.webkit.org/changeset/74978>

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.

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 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 on attachment 78284 [details] fix for WebKit2 Committed as http://trac.webkit.org/changeset/75320.

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