Bug 197206 - [CMake] Add WEBKIT_EXECUTABLE macro
Summary: [CMake] Add WEBKIT_EXECUTABLE macro
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-23 11:56 PDT by Don Olmstead
Modified: 2019-04-29 12:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch (24.20 KB, patch)
2019-04-23 12:15 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (24.22 KB, patch)
2019-04-23 12:37 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (24.27 KB, patch)
2019-04-23 12:42 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (24.27 KB, patch)
2019-04-23 12:54 PDT, Don Olmstead
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (3.14 MB, application/zip)
2019-04-23 14:30 PDT, Build Bot
no flags Details
Patch (16.46 KB, patch)
2019-04-24 12:07 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (16.46 KB, patch)
2019-04-24 14:16 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (16.03 KB, patch)
2019-04-26 10:35 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (17.89 KB, patch)
2019-04-26 12:31 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (56.24 KB, patch)
2019-04-26 13:55 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (20.14 KB, patch)
2019-04-26 13:58 PDT, Don Olmstead
annulen: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (20.01 KB, patch)
2019-04-26 16:17 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Build fix (556 bytes, patch)
2019-04-29 11:24 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Build fix (1.17 KB, patch)
2019-04-29 11:29 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2019-04-23 11:56:58 PDT
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.
Comment 1 Don Olmstead 2019-04-23 12:15:05 PDT Comment hidden (obsolete)
Comment 2 Don Olmstead 2019-04-23 12:37:12 PDT Comment hidden (obsolete)
Comment 3 Don Olmstead 2019-04-23 12:42:09 PDT Comment hidden (obsolete)
Comment 4 Don Olmstead 2019-04-23 12:54:35 PDT
Created attachment 368054 [details]
Patch

Adding again so Win and WPE pick it up hopefully.
Comment 5 Build Bot 2019-04-23 14:30:03 PDT
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
Comment 6 Build Bot 2019-04-23 14:30:04 PDT
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
Comment 7 Konstantin Tokarev 2019-04-24 03:23:06 PDT
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
Comment 8 Konstantin Tokarev 2019-04-24 03:45:47 PDT
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
Comment 9 Don Olmstead 2019-04-24 09:04:48 PDT
(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.
Comment 10 Don Olmstead 2019-04-24 12:07:02 PDT
Created attachment 368155 [details]
Patch

Add WEBKIT_WRAP_EXECUTABLE to move windows specific functionality out of WEBKIT_EXECUTABLE.
Comment 11 Konstantin Tokarev 2019-04-24 12:56:17 PDT
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
Comment 12 Don Olmstead 2019-04-24 14:16:37 PDT
Created attachment 368175 [details]
Patch

Fix typo
Comment 13 Konstantin Tokarev 2019-04-24 14:20:02 PDT
>if idea is to handle case-sensitive file system in a user-friendly way

oops I've meant "case-insensitive" there
Comment 14 Don Olmstead 2019-04-24 14:27:01 PDT
(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
Comment 15 Konstantin Tokarev 2019-04-24 14:46:49 PDT
(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 16 Konstantin Tokarev 2019-04-24 15:03:29 PDT
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
Comment 17 Don Olmstead 2019-04-25 10:52:25 PDT
(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.
Comment 18 Konstantin Tokarev 2019-04-25 10:59:30 PDT
Indeed
Comment 19 Don Olmstead 2019-04-25 11:02:58 PDT
(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.
Comment 20 Konstantin Tokarev 2019-04-25 11:19:09 PDT
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
Comment 21 Don Olmstead 2019-04-26 10:35:48 PDT
Created attachment 368332 [details]
Patch

Rebase
Comment 22 Konstantin Tokarev 2019-04-26 11:03:13 PDT
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 23 Konstantin Tokarev 2019-04-26 11:23:36 PDT
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?
Comment 24 Don Olmstead 2019-04-26 12:31:01 PDT
Created attachment 368342 [details]
Patch

Fix AppleWin build. Address review feedback.
Comment 25 Konstantin Tokarev 2019-04-26 12:42:39 PDT
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
Comment 26 Don Olmstead 2019-04-26 13:55:34 PDT
Created attachment 368347 [details]
Patch
Comment 27 Don Olmstead 2019-04-26 13:58:26 PDT
Created attachment 368348 [details]
Patch
Comment 28 WebKit Commit Bot 2019-04-26 16:06:53 PDT
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
Comment 29 Don Olmstead 2019-04-26 16:17:29 PDT
Created attachment 368360 [details]
Patch
Comment 30 WebKit Commit Bot 2019-04-26 17:06:44 PDT
Comment on attachment 368360 [details]
Patch

Clearing flags on attachment: 368360

Committed r244712: <https://trac.webkit.org/changeset/244712>
Comment 31 WebKit Commit Bot 2019-04-26 17:06:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Don Olmstead 2019-04-29 11:24:15 PDT
Reopening to land a build fix.
Comment 33 Don Olmstead 2019-04-29 11:24:36 PDT
Created attachment 368467 [details]
Build fix
Comment 34 Konstantin Tokarev 2019-04-29 11:28:40 PDT
ENOPATCH
Comment 35 Don Olmstead 2019-04-29 11:29:10 PDT
Created attachment 368470 [details]
Build fix
Comment 36 WebKit Commit Bot 2019-04-29 12:36:26 PDT
Comment on attachment 368470 [details]
Build fix

Clearing flags on attachment: 368470

Committed r244746: <https://trac.webkit.org/changeset/244746>
Comment 37 WebKit Commit Bot 2019-04-29 12:36:28 PDT
All reviewed patches have been landed.  Closing bug.