Bug 80062

Summary: WebKit.gyp:webkit should not depend on test code (!)
Product: WebKit Reporter: Ami Fischman <fischman>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: abarth, burg, cc-bugs, fishd, gustavo.noronha, gustavo, jamesr, japhet, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 82948    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Ami Fischman
Reported 2012-03-01 15:42:21 PST
http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/WebKit.gyp&exact_package=chromium&q=file:gyp%20file:third_party/webkit%20:gtest&type=cs&l=710 makes the webkit target depend on test code in the shared-library build. This is terrible for various reasons: - It violates the ODR by whole-sale linking in static libraries which are also linked into the main binary directly (http://crbug.com/112539) - It holds non-test code hostage to the standards of test-code (e.g. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/WebKit.gyp&exact_package=chromium&q=file:gyp%20file:third_party/webkit%20:gtest&type=cs&l=763) - It makes the build bigger than it needs to be The madness must stop.
Attachments
Patch (53.79 KB, patch)
2012-03-08 14:38 PST, Ami Fischman 		no flags
DetailsFormatted DiffDiff
Patch (58.77 KB, patch)
2012-03-08 15:25 PST, Ami Fischman 		no flags
DetailsFormatted DiffDiff
Patch (59.50 KB, patch)
2012-03-10 18:09 PST, Ami Fischman 		no flags
DetailsFormatted DiffDiff
Patch (63.59 KB, patch)
2012-03-10 19:26 PST, Ami Fischman 		no flags
DetailsFormatted DiffDiff
Patch (63.59 KB, patch)
2012-03-11 10:45 PDT, Ami Fischman 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Ami Fischman
Comment 1 2012-03-08 14:38:09 PST
Created attachment 130908 [details] Patch
WebKit Review Bot
Comment 2 2012-03-08 14:41:03 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Darin Fisher (:fishd, Google)
Comment 3 2012-03-08 14:48:38 PST
I think it is great that you are taking this on. I think it makes good sense to make our build of WebKit.DLL export symbols from WebCore that are used by our unit tests. I am concerned about using WTF_EXPORT outside of the WTF code base. I think it would be better to invent a similar define for each module. So, WebCore should have its own WEBCORE_EXPORT most likely. This may be something we need to discuss more broadly with the community as other ports use the other way of exporting symbols from WebCore (.def / WebCore.exp). -Darin
Ami Fischman
Comment 4 2012-03-08 15:25:39 PST
Created attachment 130918 [details] Patch
Ami Fischman
Comment 5 2012-03-08 15:29:05 PST
(In reply to comment #3) > I am concerned about using WTF_EXPORT outside of the WTF code base. I think it would be better to invent a similar define for each module. So, WebCore should have its own WEBCORE_EXPORT most likely. In looking for where to put the proposed new WEBCORE_EXPORT I found Source/WebCore/platform/PlatformExportMacros.h which already defines WEBKIT_EXPORTDATA as what I would have put in WEBCORE_EXPORT. So the second patch updates to use that instead of WTF_EXPORT{,DATA}. SGTY? > This may be something we need to discuss more broadly with the community as other ports use the other way of exporting symbols from WebCore (.def / WebCore.exp). ISTM there's already quite a lot of use of _EXPORT macros (across ports) so not sure if this is needed. Adam: do you have an opinion on this?
Early Warning System Bot
Comment 6 2012-03-08 16:33:01 PST
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11906194
Adam Barth
Comment 7 2012-03-08 16:34:08 PST
> Adam: do you have an opinion on this? I agree with fishd that this is a change we should discuss with the boarder community before making. Different ports have different needs for exporting symbols from WebCore. It's not obvious me to what's best. The simplest solution would be to create a Chromium equivalent to WebCore.exp.in that lists the symbols that Chromium wants to export from WebCore. However, maintaining that file is a pain and I wouldn't wish to impose the maintenance of another such file on the rest of the community.
Build Bot
Comment 8 2012-03-08 17:52:38 PST
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11915210
Build Bot
Comment 9 2012-03-08 17:54:57 PST
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11911212
Ami Fischman
Comment 10 2012-03-08 19:53:21 PST
(In reply to comment #7) > I agree with fishd that this is a change we should discuss with the boarder community before making. Sent email to webkit-dev. Let the opinions flow!
Gustavo Noronha (kov)
Comment 11 2012-03-08 21:30:50 PST
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11903360
Collabora GTK+ EWS bot
Comment 12 2012-03-08 22:35:55 PST
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11894426
Darin Fisher (:fishd, Google)
Comment 13 2012-03-09 14:39:18 PST
Comment on attachment 130918 [details] Patch I would probably prefer to introduce a WEBCORE_EXPORT macro, possibly renaming WEBKIT_EXPORTDATA to be that. That seems clearer to me. Otherwise, this patch looks to correctly implement the idea of exporting functions from WebCore to support the Chromium port's unit tests.
Ami Fischman
Comment 14 2012-03-09 16:26:11 PST
(In reply to comment #13) > (From update of attachment 130918 [details]) > I would probably prefer to introduce a WEBCORE_EXPORT macro, possibly renaming WEBKIT_EXPORTDATA to be that. That seems clearer to me. Went to see how much work it'd be to do this, and AFAICT *nobody* uses it today. How worried do I need to be about hidden codebases? (safari, other ports not part of the third_party/WebKit tree in a chromium checkout) I'll do the rename as part of this patch if there are really no other use sites.
Adam Barth
Comment 15 2012-03-09 16:27:36 PST
> How worried do I need to be about hidden codebases? The WebKit project explicitly doesn't care about hidden codebases. That's the carrot for joining the community.
Ami Fischman
Comment 16 2012-03-10 18:09:47 PST
Created attachment 131197 [details] Patch
Ami Fischman
Comment 17 2012-03-10 18:10:53 PST
(In reply to comment #16) > Created an attachment (id=131197) [details] > Patch Renamed s/WEBKIT_EXPORTDATA/WEBCORE_EXPORT/g and added an explicit #include to hopefully greenify the red EWS runs from the last patch.
Early Warning System Bot
Comment 18 2012-03-10 18:32:13 PST
Comment on attachment 131197 [details] Patch Attachment 131197 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11937252
Gustavo Noronha (kov)
Comment 19 2012-03-10 19:06:45 PST
Comment on attachment 131197 [details] Patch Attachment 131197 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11939261
Ami Fischman
Comment 20 2012-03-10 19:26:23 PST
Created attachment 131200 [details] Patch Even more explicit includes of PlatformExportMacros.h
Ami Fischman
Comment 21 2012-03-11 10:45:32 PDT
Created attachment 131245 [details] Patch rename WEBCORE_EXPORT to WEBCORE_EXPORT_PRIVATE per morrita@ preference
Adam Barth
Comment 22 2012-05-21 15:20:52 PDT
Comment on attachment 131245 [details] Patch I'm clearing the review flag from this patch since it's not clear how we should proceed here.
Ami Fischman
Comment 23 2012-05-21 15:24:35 PDT
Thanks Adam. FTR, my plan is to wait for bug 82948 to finish getting squashed before re-visiting this.
Adam Barth
Comment 24 2012-05-21 15:34:54 PDT
Makes sense.
Note You need to log in before you can comment on or make changes to this bug.