Summary: | [PAL] Add symbol export macros for PAL | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yoshiaki Jitsukawa <yoshiaki.jitsukawa> | ||||||||||||||||||||||||||
Component: | Platform | Assignee: | 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
Yoshiaki Jitsukawa
2017-05-01 15:43:47 PDT
Created attachment 308786 [details]
patch
*** This bug has been marked as a duplicate of bug 171523 *** Reopening as #171523 isn't able to land. Created attachment 312503 [details]
WIP Patch
WIP patch. Should fail on Apple platforms since no XCode changes were made.
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.
Created attachment 312635 [details]
WIP Patch 2
Created attachment 312636 [details]
WIP Patch 2
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 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. (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. Created attachment 312825 [details]
WIP Patch
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.
Created attachment 312826 [details]
WIP Patch
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? (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. 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? (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. Created attachment 313028 [details]
WIP Patch
Throwing it at the bots to see if GTK/WPE work
Created attachment 313033 [details]
WIP Patch
Created attachment 313137 [details]
WIP Patch
Throwing at GTK and WPE bots
Ok the Xcode projects just need to get sorted and I think this is good to go. 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. Created attachment 313437 [details]
WIP Patch
Fix nits and remove some unused code that snuck in.
(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. Created attachment 313699 [details]
Xcode piece
Created attachment 313859 [details]
WIP With XCode
Merging Xcode piece and attempting Apple bots
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 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 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 on attachment 313859 [details] WIP With XCode Clearing flags on attachment: 313859 Committed r218843: <http://trac.webkit.org/changeset/218843> All reviewed patches have been landed. Closing bug. (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 |