Bug 50174

Summary: Get rid of prefix header dependency for WebKit2 build system
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: WebKit2Assignee: Greg <greg.coletta>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, buildbot, commit-queue, eric, greg.coletta, kbalazs, kenneth, loki, rgabor, sam, s.mathur, webkit-ews, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 53400    
Bug Blocks: 50251    
Attachments:
Description Flags
Adding config.h to every WebKit2 cpp file.
levin: review-
Adding config.h to every WebKit2 cpp file.
none
Adding config.h to every WebKit2 cpp file.
laszlo.gombos: review+, laszlo.gombos: commit-queue-
rebaselined patch for cq to commit
commit-queue: commit-queue-
try again, this time patch generated from svn
commit-queue: commit-queue-
rebaseline WebKit2Common.vsprops none

Description Laszlo Gombos 2010-11-29 13:06:50 PST
WebKit(1) does not need "prefix header" support from the build system (see config.h), but WebKit2 has this dependency. This bug is to track building WebKit2 without "prefix header" support from the compiler.

The topic has been discussed in the webkit-dev mailing list - https://lists.webkit.org/pipermail/webkit-dev/2010-July/013427.html.

RVCT 2.2 with the current Symbian (and likely other embedded) build system does not seems to support "prefix header". Builds with icecc would also take advantage of this work.
Comment 1 Balazs Kelemen 2010-11-29 15:37:22 PST
I agree that a config.h style solution would be more straightforward.
The last feedback I got from the Apple guys was that they do not want
another config.h for WebKit2. However, distcc and icecc are happy with 
the prefix header at least with gcc (only precompiled header is a 
problem). Does RVCT 2.2 really luck the  -include option or something 
equivalent?
Comment 2 Andras Becsi 2010-11-29 16:08:28 PST
(In reply to comment #1)
> Does RVCT 2.2 really lack the  -include option or something equivalent?

RVCT 4.0 has a -include option but drops a warning:
Warning: C3910W: Old syntax, please use '-I'.

So I guess previous versions also supported -include, but it might behave like -I, which prepends a directory to the include path.

As far as I know RVCT has a --pch option since 2.0, and at least 4.0 has a
--use_pch option, which should behave similar to gcc's -include.
Comment 3 Balazs Kelemen 2010-11-30 02:05:24 PST
Gabor(s), do you know how to simulate -include with rvct?
Comment 4 Laszlo Gombos 2010-11-30 04:51:51 PST
RVCT (including 2.2) does have a compiler option for prefix headers - it is called "--preinclude". There is more on this here: http://www.keil.com/support/man/docs/armccref/armccref_CHDIIAEI.htm

However there is a bug in the 2.2 version of RVCT that it only takes the last instance of the preinclude header into account (which typically points to RVCT2_2.h). I have not found a way to convince RVCT to take our own preinclude header with the existing Qt/Symbian toolchain.

As noted RVCT is not the only environment where this is an issue.
Comment 5 Siddharth Mathur 2010-12-13 09:46:42 PST
If you happen to compile for Symbian OS, then a provisional solution with RVCT 2.2 is to add the following line at the end of your $$EPOCROOT/epoc32/include/rvct/rvct.h

#ifdef BUILDING_WEBKIT
#include "..\..\..\webkit\WebKit2\WebKit2Prefix.h"
#endif
Comment 6 Greg 2010-12-23 10:58:36 PST
Created attachment 77351 [details]
Adding config.h to every WebKit2 cpp file.
Comment 7 Early Warning System Bot 2010-12-23 11:11:51 PST
Attachment 77351 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7257124
Comment 8 Build Bot 2010-12-23 13:28:44 PST
Attachment 77351 [details] did not build on win:
Build output: http://queues.webkit.org/results/7235150
Comment 9 David Levin 2011-01-03 09:13:31 PST
Comment on attachment 77351 [details]
Adding config.h to every WebKit2 cpp file.

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

r- because this breaks some builds. 

Also, does this change have consensus with the folks working on WebKit2? This seems like it should be resolved on webkit-dev (rather than a bug/patch).

> Tools/Scripts/webkitpy/style/checker.py:161
> +    # Certain directories have other idiosyncracies.

Nit: "other" would no longer apply here.
Comment 10 Greg 2011-01-03 09:25:03 PST
Thanks for the review David, I'm looking into the build failures.

The topic has been raised on webkit-dev mailing list - 
https://lists.webkit.org/pipermail/webkit-dev/2010-November/015162.html . 

No notable objections has been received so far.
Comment 11 Greg 2011-01-21 12:03:00 PST
Created attachment 79772 [details]
Adding config.h to every WebKit2 cpp file.
Comment 12 Greg 2011-01-21 14:40:28 PST
Created attachment 79797 [details]
Adding config.h to every WebKit2 cpp file.
Comment 13 WebKit Review Bot 2011-01-21 14:42:30 PST
Attachment 79797 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/config.h:117:  "WebCore/config.h" already included at Source/WebKit2/config.h:28  [build/include] [4]
Total errors found: 1 in 361 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Balazs Kelemen 2011-01-23 06:01:06 PST
Looks absolutely good to me but we should convince the Apple folks about
the need of this change.
Comment 15 Laszlo Gombos 2011-01-26 06:18:03 PST
This looks good to me as well. I plan to r+ it and land it on Thursday, unless someone object.
Comment 16 Laszlo Gombos 2011-01-27 19:07:09 PST
Comment on attachment 79797 [details]
Adding config.h to every WebKit2 cpp file.

r=me. cq- as the patch needs to be re-baselined.
Comment 17 Laszlo Gombos 2011-01-27 19:18:35 PST
Created attachment 80402 [details]
rebaselined patch for cq to commit
Comment 18 WebKit Commit Bot 2011-01-27 19:19:57 PST
Comment on attachment 80402 [details]
rebaselined patch for cq to commit

Rejecting attachment 80402 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'ap..." exit_code: 2

Last 500 characters of output:
h
|+++ Source/WebKit2/config.h
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file Source/WebKit2/win/WebKit2Common.vsprops
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/win/WebKit2Common.vsprops.rej
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/webkitpy/style/checker.py

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7542395
Comment 19 Laszlo Gombos 2011-01-27 19:43:49 PST
Created attachment 80404 [details]
try again, this time patch generated from svn
Comment 20 WebKit Commit Bot 2011-01-27 20:26:45 PST
Comment on attachment 80404 [details]
try again, this time patch generated from svn

Rejecting attachment 80404 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'ap..." exit_code: 2

Last 500 characters of output:
cess/qt/WebProcessQt.cpp
patching file Source/WebKit2/WebProcess/win/WebProcessMainWin.cpp
patching file Source/WebKit2/WebProcess/win/WebProcessWin.cpp
patching file Source/WebKit2/win/WebKit2Common.vsprops
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/win/WebKit2Common.vsprops.rej
patching file Tools/ChangeLog
patching file Tools/Scripts/webkitpy/style/checker.py

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7525396
Comment 21 Laszlo Gombos 2011-01-27 21:05:20 PST
Created attachment 80415 [details]
rebaseline WebKit2Common.vsprops
Comment 22 WebKit Commit Bot 2011-01-27 21:08:26 PST
Comment on attachment 80415 [details]
rebaseline WebKit2Common.vsprops

Rejecting attachment 80415 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:
cess/qt/WebProcessQt.cpp
patching file Source/WebKit2/WebProcess/win/WebProcessMainWin.cpp
patching file Source/WebKit2/WebProcess/win/WebProcessWin.cpp
patching file Source/WebKit2/win/WebKit2Common.vsprops
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/win/WebKit2Common.vsprops.rej
patching file Tools/ChangeLog
patching file Tools/Scripts/webkitpy/style/checker.py

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7546380
Comment 23 WebKit Review Bot 2011-01-27 22:36:19 PST
http://trac.webkit.org/changeset/76916 might have broken SnowLeopard Intel Release (Tests)
Comment 24 Laszlo Gombos 2011-01-28 00:08:07 PST
Comment on attachment 80415 [details]
rebaseline WebKit2Common.vsprops

Landed manually as http://trac.webkit.org/changeset/76916.