Bug 80062 – WebKit.gyp:webkit should not depend on test code (!)

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Ami Fischman]

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.

[Ami Fischman]

Created attachment 130908 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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]

Created attachment 130918 [details]
Patch

[Ami Fischman]

(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 on attachment 130918 [details]
Patch

Attachment 130918 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11906194

[Adam Barth]

> 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 on attachment 130918 [details]
Patch

Attachment 130918 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11915210

[Build Bot]

Comment on attachment 130918 [details]
Patch

Attachment 130918 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11911212

[Ami Fischman]

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

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

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

Created attachment 131197 [details]
Patch

[Ami Fischman]

(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 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 on attachment 131197 [details]
Patch

Attachment 131197 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11939261

[Ami Fischman]

Created attachment 131200 [details]
Patch

Even more explicit includes of PlatformExportMacros.h

[Ami Fischman]

Created attachment 131245 [details]
Patch

rename WEBCORE_EXPORT to WEBCORE_EXPORT_PRIVATE per morrita@ preference

[Adam Barth]

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]

Thanks Adam.  FTR, my plan is to wait for bug 82948 to finish getting squashed before re-visiting this.

[Adam Barth]

Makes sense.