Bug 197206 – [CMake] Add WEBKIT_EXECUTABLE macro

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 368050 [details]
Patch

[Don Olmstead]

Created attachment 368052 [details]
Patch

[Don Olmstead]

Created attachment 368053 [details]
Patch

[Don Olmstead]

Created attachment 368054 [details]
Patch

Adding again so Win and WPE pick it up hopefully.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Konstantin Tokarev]

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

[Konstantin Tokarev]

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

[Don Olmstead]

(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.

[Don Olmstead]

Created attachment 368155 [details]
Patch

Add WEBKIT_WRAP_EXECUTABLE to move windows specific functionality out of WEBKIT_EXECUTABLE.

[Konstantin Tokarev]

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

[Don Olmstead]

Created attachment 368175 [details]
Patch

Fix typo

[Konstantin Tokarev]

>if idea is to handle case-sensitive file system in a user-friendly way

oops I've meant "case-insensitive" there

[Don Olmstead]

(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

[Konstantin Tokarev]

(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

[Konstantin Tokarev]

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

[Don Olmstead]

(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.

[Konstantin Tokarev]

Indeed

[Don Olmstead]

(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.

[Konstantin Tokarev]

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

[Don Olmstead]

Created attachment 368332 [details]
Patch

Rebase

[Konstantin Tokarev]

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}

[Konstantin Tokarev]

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?

[Don Olmstead]

Created attachment 368342 [details]
Patch

Fix AppleWin build. Address review feedback.

[Konstantin Tokarev]

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

[Don Olmstead]

Created attachment 368347 [details]
Patch

[Don Olmstead]

Created attachment 368348 [details]
Patch

[WebKit Commit Bot]

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

[Don Olmstead]

Created attachment 368360 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 368360 [details]
Patch

Clearing flags on attachment: 368360

Committed r244712: <https://trac.webkit.org/changeset/244712>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

Reopening to land a build fix.

[Don Olmstead]

Created attachment 368467 [details]
Build fix

[Konstantin Tokarev]

ENOPATCH

[Don Olmstead]

Created attachment 368470 [details]
Build fix

[WebKit Commit Bot]

Comment on attachment 368470 [details]
Build fix

Clearing flags on attachment: 368470

Committed r244746: <https://trac.webkit.org/changeset/244746>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.