Bug 171519 – [PAL] Add symbol export macros for PAL

• Component: Platform
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

Some sources under the WebCore/platform directory export symbols by using the WEBCORE_EXPORT macro. In case we want to move such files to PAL, we'll need PAL_EXPORT or something like that which doesn't depend on WebCore.

[Yoshiaki Jitsukawa]

Created attachment 308786 [details]
patch

[Yoshiaki Jitsukawa]

*** This bug has been marked as a duplicate of bug 171523 ***

[Don Olmstead]

Reopening as #171523 isn't able to land.

[Don Olmstead]

Created attachment 312503 [details]
WIP Patch

WIP patch. Should fail on Apple platforms since no XCode changes were made.

[Build Bot]

Attachment 312503 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/config.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yoshiaki Jitsukawa]

Created attachment 312635 [details]
WIP Patch 2

[Yoshiaki Jitsukawa]

Created attachment 312636 [details]
WIP Patch 2

[Build Bot]

Attachment 312636 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/config.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 312636 [details]
WIP Patch 2

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

I think the Xcode project changes look right, but I'm not sure "pal/PAL..." is a good naming scheme.

> Source/WebCore/PAL/pal/crypto/CryptoDigest.h:28
> +#include <pal/PALExportMacros.h>

I wonder if this should be "pal/PALExportMacros.h". Seems like "pal/ExportMacros.h" would be enough to distinguish things.

[Don Olmstead]

(In reply to Brent Fulgham from comment #9)
> Comment on attachment 312636 [details]
> WIP Patch 2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312636&action=review
> 
> I think the Xcode project changes look right, but I'm not sure "pal/PAL..."
> is a good naming scheme.
> 
> > Source/WebCore/PAL/pal/crypto/CryptoDigest.h:28
> > +#include <pal/PALExportMacros.h>
> 
> I wonder if this should be "pal/PALExportMacros.h". Seems like
> "pal/ExportMacros.h" would be enough to distinguish things.

I can change the name. It sounds better to me.

With the XCode projects there's the -DSTATICALLY_LINKED_WITH_PAL=1 that isn't there. The CryptoDigest class had no exports previously so I'm guessing things will break when there are things in PAL that were using WEBCORE_EXPORT.

I can see if I can hunt down a Mac and apply the patch to that.

[Don Olmstead]

Created attachment 312825 [details]
WIP Patch

[Build Bot]

Attachment 312825 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/PAL/config.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 312826 [details]
WIP Patch

[Brent Fulgham]

Comment on attachment 312826 [details]
WIP Patch

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

> Tools/TestWebKitAPI/config.h:29
> +#include <pal/ExportMacros.h>

I don't think we have a 'pal' directory in the archive (at least yet). Does this patch depend on another one that creates this directory?

[Don Olmstead]

(In reply to Brent Fulgham from comment #14)
> Comment on attachment 312826 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312826&action=review
> 
> > Tools/TestWebKitAPI/config.h:29
> > +#include <pal/ExportMacros.h>
> 
> I don't think we have a 'pal' directory in the archive (at least yet). Does
> this patch depend on another one that creates this directory?

I'm currently trying to work all this out.

Windows builds seems to want to copy stuff into a ForwardingHeaders directory. Not sure the historic context for it all.

Will keep throwing it at the bots till I can get it worked out.

[Myles C. Maxfield]

I think this is generally a good direction to go. I look forward to seeing what you can do to make EWS happy.

What is DSTATICALLY_LINKED_WITH_PAL for?

[Don Olmstead]

(In reply to Myles C. Maxfield from comment #16)
> I think this is generally a good direction to go. I look forward to seeing
> what you can do to make EWS happy.
> 
> What is DSTATICALLY_LINKED_WITH_PAL for?

It follows along with whats in JSC.

If WebCore is shared and PAL is static then PAL_EXPORT is WTF_EXPORT when building WebCore.

If WebCore is shared and PAL is shared then PAL_EXPORT is WTF_IMPORT when building WebCore.

[Don Olmstead]

Created attachment 313028 [details]
WIP Patch

Throwing it at the bots to see if GTK/WPE work

[Don Olmstead]

Created attachment 313033 [details]
WIP Patch

[Don Olmstead]

Created attachment 313137 [details]
WIP Patch

Throwing at GTK and WPE bots

[Don Olmstead]

Ok the Xcode projects just need to get sorted and I think this is good to go.

[Myles C. Maxfield]

Comment on attachment 313137 [details]
WIP Patch

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

I don't know how the CMake ports do it, but the Xcode ports give WTF an extra build step, which rsyncs the headers to ${DSTROOT}/usr/local/include/wtf and ${DSTROOT}/usr/local/include is in the search path. Maybe there's something more elegant we can do, but we definitely don't want to have to modify every project that depends on WebCore to update its include directories.

> Source/WebCore/PAL/pal/PlatformWin.cmake:7
> +#set(PAL_PRE_BUILD_COMMAND "${CMAKE_BINARY_DIR}/DerivedSources/PAL/preBuild.cmd")
> +#file(WRITE "${PAL_PRE_BUILD_COMMAND}" "@xcopy /y /s /d /f \"${PAL_DIR}/pal/*.h\" \"${DERIVED_SOURCES_DIR}/ForwardingHeaders/pal\" >nul 2>nul\n@xcopy /y /s /d /f \"${DERIVED_SOURCES_DIR}/pal/*.h\" \"${DERIVED_SOURCES_DIR}/ForwardingHeaders/pal\" >nul 2>nul\n")
> +#file(MAKE_DIRECTORY ${DERIVED_SOURCES_DIR}/ForwardingHeaders/pal)

We don't leave around commented code in our files.

> Source/WebKit2/config.h:-36
> -#include <wtf/Platform.h>
> -
>  #include <WebCore/PlatformExportMacros.h>
> +#include <pal/ExportMacros.h>
>  #include <runtime/JSExportMacros.h>
>  #include <wtf/DisallowCType.h>
> -#include <wtf/ExportMacros.h>

WebKit2 should not have to be modified at all.

> Tools/DumpRenderTree/config.h:26
> +#include <pal/ExportMacros.h>

Ditto.

> Tools/Scripts/update-webkit-wincairo-libs:41
> +#system("perl", $command, $winCairoLibsURL, ".") == 0 or die;

Ditto (from above).

> Tools/TestWebKitAPI/CMakeLists.txt:9
> +    PAL${DEBUG_SUFFIX}

Shouldn't PAL's .a file be linked into WebKit's shared library? This shouldn't be necessary.

> Tools/TestWebKitAPI/config.h:33
> +#include <pal/ExportMacros.h>

Ditto. wtf/ExportMacros.h isn't in this list, so it must be possible to do this without modifying this file.

> Tools/WebKitTestRunner/config.h:25
> +#if defined(HAVE_CONFIG_H) && HAVE_CONFIG_H && defined(BUILDING_WITH_CMAKE)

Ditto.

[Don Olmstead]

Created attachment 313437 [details]
WIP Patch

Fix nits and remove some unused code that snuck in.

[Don Olmstead]

(In reply to Myles C. Maxfield from comment #22)
> Comment on attachment 313137 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313137&action=review
> 
> I don't know how the CMake ports do it, but the Xcode ports give WTF an
> extra build step, which rsyncs the headers to
> ${DSTROOT}/usr/local/include/wtf and ${DSTROOT}/usr/local/include is in the
> search path. Maybe there's something more elegant we can do, but we
> definitely don't want to have to modify every project that depends on
> WebCore to update its include directories.

I think there's going to need to be something along these lines done. For example there are references to <WebCore/FileSystem.h> in WebKit2. After the ExportMacros is sorted then I plan on moving over FileSystem.

Is this something you can handle?

> > Source/WebCore/PAL/pal/PlatformWin.cmake:7
> > +#set(PAL_PRE_BUILD_COMMAND "${CMAKE_BINARY_DIR}/DerivedSources/PAL/preBuild.cmd")
> > +#file(WRITE "${PAL_PRE_BUILD_COMMAND}" "@xcopy /y /s /d /f \"${PAL_DIR}/pal/*.h\" \"${DERIVED_SOURCES_DIR}/ForwardingHeaders/pal\" >nul 2>nul\n@xcopy /y /s /d /f \"${DERIVED_SOURCES_DIR}/pal/*.h\" \"${DERIVED_SOURCES_DIR}/ForwardingHeaders/pal\" >nul 2>nul\n")
> > +#file(MAKE_DIRECTORY ${DERIVED_SOURCES_DIR}/ForwardingHeaders/pal)
> 
> We don't leave around commented code in our files.

Removed

> > Source/WebKit2/config.h:-36
> > -#include <wtf/Platform.h>
> > -
> >  #include <WebCore/PlatformExportMacros.h>
> > +#include <pal/ExportMacros.h>
> >  #include <runtime/JSExportMacros.h>
> >  #include <wtf/DisallowCType.h>
> > -#include <wtf/ExportMacros.h>
> 
> WebKit2 should not have to be modified at all.
> 
> > Tools/DumpRenderTree/config.h:26
> > +#include <pal/ExportMacros.h>
> 
> Ditto.

WebKit2 will end up including headers from PAL.

> > Tools/Scripts/update-webkit-wincairo-libs:41
> > +#system("perl", $command, $winCairoLibsURL, ".") == 0 or die;
> 
> Ditto (from above).
> 

Removed. This snuck in.

> > Tools/TestWebKitAPI/CMakeLists.txt:9
> > +    PAL${DEBUG_SUFFIX}
> 
> Shouldn't PAL's .a file be linked into WebKit's shared library? This
> shouldn't be necessary.

Removed.

> > Tools/TestWebKitAPI/config.h:33
> > +#include <pal/ExportMacros.h>
> 
> Ditto. wtf/ExportMacros.h isn't in this list, so it must be possible to do
> this without modifying this file.
> 
> > Tools/WebKitTestRunner/config.h:25
> > +#if defined(HAVE_CONFIG_H) && HAVE_CONFIG_H && defined(BUILDING_WITH_CMAKE)
> 
> Ditto.

I removed references in config.h to wtf/ExportMacros.h and wtf/Platform.h in any project depending on WTF. wtf/ExportMacros.h includes wtf/Platform.h so there's no reason to have it there. The other projects include wtf/ExportMacros.h to get the WTF_EXPORT definition so there's no reason to include those files directly within the config. as they are redundant.

https://bugs.webkit.org/show_bug.cgi?id=173294

This patch includes similar changes to above to remove redundant includes in the affected files.

[Myles C. Maxfield]

Created attachment 313699 [details]
Xcode piece

[Don Olmstead]

Created attachment 313859 [details]
WIP With XCode

Merging Xcode piece and attempting Apple bots

[Brent Fulgham]

Comment on attachment 313859 [details]
WIP With XCode

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

This looks reasonable to me. r=me.

> Source/WebCore/PAL/Configurations/CopyPALHeaders.xcconfig:24
> +PRIVATE_HEADERS_FOLDER_PATH = usr/local/include/pal;

Is this always true? I guess individual ports could override this.

[Build Bot]

Comment on attachment 313859 [details]
WIP With XCode

Rejecting attachment 313859 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 313859 [details]
WIP With XCode

Rejecting attachment 313859 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 313859, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4007948

[WebKit Commit Bot]

Comment on attachment 313859 [details]
WIP With XCode

Clearing flags on attachment: 313859

Committed r218843: <http://trac.webkit.org/changeset/218843>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to WebKit Commit Bot from comment #30)
> Comment on attachment 313859 [details]
> WIP With XCode
> 
> Clearing flags on attachment: 313859
> 
> Committed r218843: <http://trac.webkit.org/changeset/218843>

It broke the Apple Mac cmake build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/2917