[webkit-reviews] review granted: [Bug 181664] [CMake] Remove WebCoreDerivedSources library target : [Attachment 331377] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 181664: [CMake] Remove WebCoreDerivedSources library target
https://bugs.webkit.org/show_bug.cgi?id=181664

Attachment 331377: Patch

https://bugs.webkit.org/attachment.cgi?id=331377&action=review




--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 331377
  --> https://bugs.webkit.org/attachment.cgi?id=331377
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331377&action=review

> Source/WebCore/CMakeLists.txt:-1842
> -    list(APPEND WebCore_DERIVED_SOURCES
${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.cpp)
> -    list(APPEND WebCore_DERIVED_SOURCES
${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.h)

Wait, what happened to these?

> Source/WebCore/CMakeLists.txt:-1926
> -# This is split into a separate library as a workaround for command line
length
> -# limits. This should no longer be needed when CMake supports Ninja response
> -# files on OS X.

You've verified this is no longer needed on macOS?

If not, we should get an Apple developer to check before committing this,
because EWS doesn't verify the CMake build. We don't want a problem to go
unnoticed for days and then eventually get reverted.

> Source/WebCore/CMakeLists.txt:1921
> +add_dependencies(WebCore WebCoreBindings)

I suspect this is no longer the best place in the file to have this line of
code. Can you look around for a better place to move it to?

> Source/WebKit/CMakeLists.txt:940
>  if (COMPILER_IS_GCC_OR_CLANG AND NOT APPLE)
> -    target_link_libraries(WebKit -Wl,--start-group WebCore
WebCoreDerivedSources -Wl,--end-group)
> +    target_link_libraries(WebKit -Wl,--start-group WebCore -Wl,--end-group)
>  endif ()

The point of -Wl,--start-group -Wl,--end-group is to avoid circular
dependencies between WebCore and WebCoreDerivedSources, because the linker will
discard symbols from WebCore that don't seem to be needed by WebKit, then blow
up because actually they were needed by WebCoreDerivedSources. It's problem #3
from
http://nibblestew.blogspot.com/2017/12/these-three-things-could-improve-linux.h
tml. Anyway, WebCore is already listed in WebKit_LIBRARIES, so it should be
safe to just remove all these lines now.

> Tools/TestWebKitAPI/PlatformWPE.cmake:66
> -target_link_libraries(TestWebCore ${test_webcore_LIBRARIES}
-Wl,--start-group WebCore WebCoreDerivedSources -Wl,--end-group)
> +target_link_libraries(TestWebCore ${test_webcore_LIBRARIES}
-Wl,--start-group WebCore -Wl,--end-group)

This should be simply:

target_link_libraries(TestWebCore ${test_webcore_LIBRARIES})