Bug 171519

Summary: [PAL] Add symbol export macros for PAL
Product: WebKit Reporter: Yoshiaki Jitsukawa <yoshiaki.jitsukawa>
Component: PlatformAssignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, bfulgham, buildbot, commit-queue, don.olmstead, jbedard, mmaxfield, ossy, ryanhaddad
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
WIP Patch
none
WIP Patch 2
none
WIP Patch 2
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Xcode piece
none
WIP With XCode none

Description Yoshiaki Jitsukawa 2017-05-01 15:43:47 PDT
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.
Comment 1 Yoshiaki Jitsukawa 2017-05-01 16:48:51 PDT
Created attachment 308786 [details]
patch
Comment 2 Yoshiaki Jitsukawa 2017-05-01 16:50:49 PDT

*** This bug has been marked as a duplicate of bug 171523 ***
Comment 3 Don Olmstead 2017-06-09 15:39:15 PDT
Reopening as #171523 isn't able to land.
Comment 4 Don Olmstead 2017-06-09 15:44:17 PDT
Created attachment 312503 [details]
WIP Patch

WIP patch. Should fail on Apple platforms since no XCode changes were made.
Comment 5 Build Bot 2017-06-09 15:47:03 PDT
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.
Comment 6 Yoshiaki Jitsukawa 2017-06-11 19:36:33 PDT
Created attachment 312635 [details]
WIP Patch 2
Comment 7 Yoshiaki Jitsukawa 2017-06-11 20:31:04 PDT
Created attachment 312636 [details]
WIP Patch 2
Comment 8 Build Bot 2017-06-11 20:32:23 PDT
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.
Comment 9 Brent Fulgham 2017-06-12 10:54:44 PDT
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.
Comment 10 Don Olmstead 2017-06-12 12:25:24 PDT
(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.
Comment 11 Don Olmstead 2017-06-13 16:27:43 PDT
Created attachment 312825 [details]
WIP Patch
Comment 12 Build Bot 2017-06-13 16:29:38 PDT
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.
Comment 13 Don Olmstead 2017-06-13 16:36:06 PDT
Created attachment 312826 [details]
WIP Patch
Comment 14 Brent Fulgham 2017-06-14 09:02:06 PDT
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?
Comment 15 Don Olmstead 2017-06-14 14:25:12 PDT
(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.
Comment 16 Myles C. Maxfield 2017-06-15 13:28:10 PDT
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?
Comment 17 Don Olmstead 2017-06-15 13:33:20 PDT
(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.
Comment 18 Don Olmstead 2017-06-15 17:04:01 PDT
Created attachment 313028 [details]
WIP Patch

Throwing it at the bots to see if GTK/WPE work
Comment 19 Don Olmstead 2017-06-15 17:34:10 PDT
Created attachment 313033 [details]
WIP Patch
Comment 20 Don Olmstead 2017-06-16 14:30:12 PDT
Created attachment 313137 [details]
WIP Patch

Throwing at GTK and WPE bots
Comment 21 Don Olmstead 2017-06-17 17:44:12 PDT
Ok the Xcode projects just need to get sorted and I think this is good to go.
Comment 22 Myles C. Maxfield 2017-06-19 23:57:45 PDT
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.
Comment 23 Don Olmstead 2017-06-20 14:05:05 PDT
Created attachment 313437 [details]
WIP Patch

Fix nits and remove some unused code that snuck in.
Comment 24 Don Olmstead 2017-06-20 14:58:01 PDT
(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.
Comment 25 Myles C. Maxfield 2017-06-23 01:45:39 PDT
Created attachment 313699 [details]
Xcode piece
Comment 26 Don Olmstead 2017-06-26 11:57:58 PDT
Created attachment 313859 [details]
WIP With XCode

Merging Xcode piece and attempting Apple bots
Comment 27 Brent Fulgham 2017-06-26 12:49:23 PDT
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.
Comment 28 Build Bot 2017-06-26 13:35:19 PDT
Comment on attachment 313859 [details]
WIP With XCode

Rejecting attachment 313859 [details] from commit-queue.

don.olmstead@sony.com 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.
Comment 29 WebKit Commit Bot 2017-06-27 14:04:11 PDT
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
Comment 30 WebKit Commit Bot 2017-06-27 14:30:05 PDT
Comment on attachment 313859 [details]
WIP With XCode

Clearing flags on attachment: 313859

Committed r218843: <http://trac.webkit.org/changeset/218843>
Comment 31 WebKit Commit Bot 2017-06-27 14:30:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Csaba Osztrogonác 2017-06-27 23:33:23 PDT
(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