Currently when creating executables the CMake follows along with a lot of the conventions with WEBKIT_FRAMEWORK. This should be formalized into WEBKIT_EXECUTABLE. The reason for a macro rather than just writing out things is Windows. The Windows executables all have a SHARED lib and then an executable that loads that lib. That logic should go into the macro.
Created attachment 368050 [details] Patch
Created attachment 368052 [details] Patch
Created attachment 368053 [details] Patch
Created attachment 368054 [details] Patch Adding again so Win and WPE pick it up hopefully.
Comment on attachment 368054 [details] Patch Attachment 368054 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11974894 New failing tests: imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-abort.https.html
Created attachment 368064 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
I don't think we should introduce new macros like WEBKIT_EXECUTABLE. In CMake it's quite easy to create targets without intermediate variables, for example ImageDiff could be defined like this: # Tools/ImageDiff/CMakeLists.txt add_executable(ImageDiff ImageDiff.cpp PlatformImage.cpp ) target_include_directories(ImageDiff PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS() # Tools/ImageDiff/Cairo.cmake target_sources(ImageDiff PRIVATE cairo/PlatformImageCairo.cpp ) target_include_directories(ImageDiff SYSTEM PRIVATE ${CAIRO_INCLUDE_DIRS}) target_link_libraries(ImageDiff ${CAIRO_LIBRARIES}) # Tools/ImageDiff/PlatformMac.cmake target_sources(ImageDiff PRIVATE cg/PlatformImageCG.cpp ) target_link_libraries(ImageDiff CoreFoundation CoreGraphics CoreText ) IMO project becomes easier to read and understand without extra variables and macros
If we change modules like FindCairo to create imported targets, code above will simplify further as all CAIRO_* variables will be gone and target_link_libraries(ImageDiff Cairo) will do all work
(In reply to Konstantin Tokarev from comment #7) > I don't think we should introduce new macros like WEBKIT_EXECUTABLE. In > CMake it's quite easy to create targets without intermediate variables, for > example ImageDiff could be defined like this: > > # Tools/ImageDiff/CMakeLists.txt > > add_executable(ImageDiff > ImageDiff.cpp > PlatformImage.cpp > ) > target_include_directories(ImageDiff PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) > > WEBKIT_INCLUDE_CONFIG_FILES_IF_EXISTS() > > # Tools/ImageDiff/Cairo.cmake > > target_sources(ImageDiff PRIVATE > cairo/PlatformImageCairo.cpp > ) > > target_include_directories(ImageDiff SYSTEM PRIVATE ${CAIRO_INCLUDE_DIRS}) > target_link_libraries(ImageDiff ${CAIRO_LIBRARIES}) > > # Tools/ImageDiff/PlatformMac.cmake > > target_sources(ImageDiff PRIVATE > cg/PlatformImageCG.cpp > ) > > target_link_libraries(ImageDiff > CoreFoundation > CoreGraphics > CoreText > ) > > > IMO project becomes easier to read and understand without extra variables > and macros I agree but as I noted Windows throws a big wrench into this. On Windows the SOURCES aren't in the executable they're in a library that is loaded by the executable. That's the point of the macro which wraps this up in a way that formalizes this behavior.
Created attachment 368155 [details] Patch Add WEBKIT_WRAP_EXECUTABLE to move windows specific functionality out of WEBKIT_EXECUTABLE.
Comment on attachment 368155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368155&action=review > Source/JavaScriptCore/b3/air/testair.cpp:2219 > +#if OS(WINDOWS) I would prefer it to be PLATFORM(WIN) instead of OS(WINDOWS) to affect only AppleWin and WinCairo ports, but for ports existing in trunk that would not matter > Source/JavaScriptCore/dfg/testdfg.cpp:86 > + return !filter || !!strstr(testName, filter); that's weird if idea is to handle case-sensitive file system in a user-friendly way, it should also be done on macOS. But on macOS it's also possible to have case-sensitive FS. And even NTFS can have case-sensitive mode > Source/cmake/WebKitMacros.cmake:219 > + set(${_target}_DEPENDENCIES ${target_name}) I guess ${_target_name} was meant here
Created attachment 368175 [details] Patch Fix typo
>if idea is to handle case-sensitive file system in a user-friendly way oops I've meant "case-insensitive" there
(In reply to Konstantin Tokarev from comment #11) > Comment on attachment 368155 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368155&action=review > > > Source/JavaScriptCore/b3/air/testair.cpp:2219 > > +#if OS(WINDOWS) > > I would prefer it to be PLATFORM(WIN) instead of OS(WINDOWS) to affect only > AppleWin and WinCairo ports, but for ports existing in trunk that would not > matter > > > Source/JavaScriptCore/dfg/testdfg.cpp:86 > > + return !filter || !!strstr(testName, filter); > > that's weird > > if idea is to handle case-sensitive file system in a user-friendly way, it > should also be done on macOS. But on macOS it's also possible to have > case-sensitive FS. And even NTFS can have case-sensitive mode There is no implementation of strcasestr on Windows so its working around that. The added code was already in another file. > > Source/cmake/WebKitMacros.cmake:219 > > + set(${_target}_DEPENDENCIES ${target_name}) > > I guess ${_target_name} was meant here
(In reply to Don Olmstead from comment #14) > (In reply to Konstantin Tokarev from comment #11) > > Comment on attachment 368155 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=368155&action=review > > > > > Source/JavaScriptCore/b3/air/testair.cpp:2219 > > > +#if OS(WINDOWS) > > > > I would prefer it to be PLATFORM(WIN) instead of OS(WINDOWS) to affect only > > AppleWin and WinCairo ports, but for ports existing in trunk that would not > > matter > > > > > Source/JavaScriptCore/dfg/testdfg.cpp:86 > > > + return !filter || !!strstr(testName, filter); > > > > that's weird > > > > if idea is to handle case-sensitive file system in a user-friendly way, it > > should also be done on macOS. But on macOS it's also possible to have > > case-sensitive FS. And even NTFS can have case-sensitive mode > > There is no implementation of strcasestr on Windows so its working around > that. The added code was already in another file. I think we could use WTF::equalLettersIgnoringASCIICase() from StringCommon.h
Comment on attachment 368175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368175&action=review > Source/JavaScriptCore/b3/air/testair.cpp:2219 > +#if OS(WINDOWS) DLLLauncherMain is used only in PlatformWin.cmake files, so #if PLATFORM(WIN) should be used here
(In reply to Konstantin Tokarev from comment #16) > Comment on attachment 368175 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368175&action=review > > > Source/JavaScriptCore/b3/air/testair.cpp:2219 > > +#if OS(WINDOWS) > > DLLLauncherMain is used only in PlatformWin.cmake files, so #if > PLATFORM(WIN) should be used here Technically the JSCOnly windows port is using it as well.
Indeed
(In reply to Konstantin Tokarev from comment #15) > (In reply to Don Olmstead from comment #14) > > (In reply to Konstantin Tokarev from comment #11) > > > Comment on attachment 368155 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=368155&action=review > > > > > > > Source/JavaScriptCore/b3/air/testair.cpp:2219 > > > > +#if OS(WINDOWS) > > > > > > I would prefer it to be PLATFORM(WIN) instead of OS(WINDOWS) to affect only > > > AppleWin and WinCairo ports, but for ports existing in trunk that would not > > > matter > > > > > > > Source/JavaScriptCore/dfg/testdfg.cpp:86 > > > > + return !filter || !!strstr(testName, filter); > > > > > > that's weird > > > > > > if idea is to handle case-sensitive file system in a user-friendly way, it > > > should also be done on macOS. But on macOS it's also possible to have > > > case-sensitive FS. And even NTFS can have case-sensitive mode > > > > There is no implementation of strcasestr on Windows so its working around > > that. The added code was already in another file. > > I think we could use WTF::equalLettersIgnoringASCIICase() from StringCommon.h I see this construct in all the JavaScriptCore test*.cpp files. I'm not sure if there's a particular reason why WTF string pieces are not used.
testdfg.cpp is already using WTF::dataLog(), so I suppose it's just a coincidence that non-portable strcasestr is used instead of WTF function
Created attachment 368332 [details] Patch Rebase
Comment on attachment 368332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368332&action=review > Source/cmake/WebKitMacros.cmake:149 > +macro(WEBKIT_TARGET _target _target_name) _target is a name that we use as a part of variable names and BUILDING_* macro names _target_name is a name of CMake target These two values may only be different if we use "wrapped target" hack I think parameter names need to be changed to reflect their meaning better, and explainging comment should be added > Source/cmake/WebKitMacros.cmake:195 > +macro(WEBKIT_WRAP_EXECUTABLE _target) It seems like this macro won't have a use outside of Windows ports, so it can be moved to OptionsWin > Source/cmake/WebKitMacros.cmake:208 > + WEBKIT_TARGET(${_target} ${_target_name}) ${_target} and ${_target_name} should be renamed to something more specific like ${_wrapper_target} and ${_wrapped_target}
Comment on attachment 368332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368332&action=review > Source/JavaScriptCore/shell/CMakeLists.txt:77 > + WEBKIT_ADD_TARGET_PROPERTIES(jsc LINK_FLAGS "${JSC_LINK_FLAGS}") Typo: jsc_LINK_FLAGS Do we use it anywhere though?
Created attachment 368342 [details] Patch Fix AppleWin build. Address review feedback.
Comment on attachment 368342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368342&action=review > Source/JavaScriptCore/shell/PlatformWin.cmake:2 > add_definitions(-DWIN_CAIRO) We shouldn't have any add_definitions() in target-oriented project. Use target_compile_definitions(). Other possible option is to use set_source_files_properties() for DLLLauncherMain.cpp
Created attachment 368347 [details] Patch
Created attachment 368348 [details] Patch
Comment on attachment 368348 [details] Patch Rejecting attachment 368348 [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', 368348, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/12009794
Created attachment 368360 [details] Patch
Comment on attachment 368360 [details] Patch Clearing flags on attachment: 368360 Committed r244712: <https://trac.webkit.org/changeset/244712>
All reviewed patches have been landed. Closing bug.
Reopening to land a build fix.
Created attachment 368467 [details] Build fix
ENOPATCH
Created attachment 368470 [details] Build fix
Comment on attachment 368470 [details] Build fix Clearing flags on attachment: 368470 Committed r244746: <https://trac.webkit.org/changeset/244746>