Bug 181664 – [CMake] Remove WebCoreDerivedSources library target

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fujii Hironori]

[CMake] Remove WebCoreDerivedSources library target

After unified source build has been introduced,
WebCoreDerivedSources causes complicated unnecessary
recompilation issue in CMake Visual Studio build (Bug 181117).

The simplest solution is just removing WebCoreDerivedSources
library target if possible.

[Fujii Hironori]

Created attachment 331365 [details]
WIP patch

[Fujii Hironori]

Created attachment 331373 [details]
WIP patch

[Fujii Hironori]

WebCoreDerivedSources library has been introduced in Bug 151399 to avoid command line limit of CMake Ninja build.
I have confirmed CMake Ninja can build with this patch.

[Fujii Hironori]

Created attachment 331377 [details]
Patch

[Michael Catanzaro]

(In reply to Fujii Hironori from comment #3)
> WebCoreDerivedSources library has been introduced in Bug 151399 to avoid
> command line limit of CMake Ninja build.
> I have confirmed CMake Ninja can build with this patch.

Awesome!

[Michael Catanzaro]

Comment on attachment 331377 [details]
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.html. 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})

[Alex Christensen]

Comment on attachment 331377 [details]
Patch

(In reply to Fujii Hironori from comment #3)
> I have confirmed CMake Ninja can build with this patch.
Have you confirmed that CMake Ninja can build with this patch on Mac?  This change was done because linking WebCore on Mac with Ninja exceeded the command line character limit of something.  Ninja on Linux and Windows worked fine without this patch, so verifying on those systems is not enough.

[Alex Christensen]

I verified that WebCore links on Mac using Ninja with this patch.  It's ok once Michael's comments have been addressed.

[Fujii Hironori]

Comment on attachment 331377 [details]
Patch

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

Thank you for the review.

>> Source/WebCore/CMakeLists.txt:-1842
>> -    list(APPEND WebCore_DERIVED_SOURCES ${DERIVED_SOURCES_WEBCORE_DIR}/${_objectName}Builtins.h)
> 
> Wait, what happened to these?

This causes duplicated symbol errors in Mac CMake build.
WebCoreJSBuiltins.cpp is a unified source for those files.
And, WebCoreJSBuiltins.cpp is already included to compile.

[Fujii Hironori]

Created attachment 331456 [details]
Patch

[Fujii Hironori]

(In reply to Michael Catanzaro from comment #6)

I can't remove "-Wl,--start-group WebCore -Wl,--end-group".
generate-gtkdoc reports an error:

> [16/74] cd /home/fujii/work/webkit/ga/WebKitBuild/Release && /usr/bin/cmake -E env CC=/usr/lib/ccache/cc "CFLAGS=-fdiagnostics-color=always -Wno-expansion-to-defined -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wextra -Wall  -fno-strict-aliasing -fno-exceptions -Wno-unused-parameter" /home/fujii/work/webkit/ga/Tools/gtk/generate-gtkdoc --skip-html && touch docs-build-no-html.stamp
> FAILED: docs-build-no-html.stamp 
> cd /home/fujii/work/webkit/ga/WebKitBuild/Release && /usr/bin/cmake -E env CC=/usr/lib/ccache/cc "CFLAGS=-fdiagnostics-color=always -Wno-expansion-to-defined -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wextra -Wall  -fno-strict-aliasing -fno-exceptions -Wno-unused-parameter" /home/fujii/work/webkit/ga/Tools/gtk/generate-gtkdoc --skip-html && touch docs-build-no-html.stamp
> Unescaped left brace in regex is deprecated here (and will be fatal in Perl 5.30), passed through in regex; marked by <-- HERE in m/(.*?){ <-- HERE / at /home/fujii/work/webkit/ga/WebKitBuild/DependenciesGTK/Root/bin/gtkdoc-scan line 735.
> /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so: undefined reference to `WebCore::mediaControlsGtkJavaScript'
> /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so: undefined reference to `WebCore::mediaControlsLocalizedStringsJavaScript'
> /home/fujii/work/webkit/ga/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so: undefined reference to `WebCore::mediaControlsBaseJavaScript'
> collect2: error: ld returned 1 exit status
> Linking of scanner failed: 
> Traceback (most recent call last):
>   File "/home/fujii/work/webkit/ga/Tools/gtk/generate-gtkdoc", line 203, in <module>
>     saw_warnings = generate_documentation(webkitdom_generator)
>   File "/home/fujii/work/webkit/ga/Tools/gtk/generate-gtkdoc", line 151, in generate_documentation
>     return generate_doc(generator, arguments.skip_html)
>   File "/home/fujii/work/webkit/ga/Tools/gtk/generate-gtkdoc", line 138, in generate_doc
>     generator.generate(not skip_html)
>   File "/home/fujii/work/webkit/ga/Tools/gtk/gtkdoc.py", line 143, in generate
>     self._run_gtkdoc_scangobj()
>   File "/home/fujii/work/webkit/ga/Tools/gtk/gtkdoc.py", line 336, in _run_gtkdoc_scangobj
>     env=env, cwd=self.output_dir)
>   File "/home/fujii/work/webkit/ga/Tools/gtk/gtkdoc.py", line 209, in _run_command
>     % (args[0], process.returncode))
> Exception: gtkdoc-scangobj produced a non-zero return code 1

[Fujii Hironori]

(In reply to Fujii Hironori from comment #11)

I found the answer in Bug 138655
WebCorePlatformGTK should be specified before WebCore for the linker.

[Fujii Hironori]

Created attachment 331460 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331460 [details]
Patch

Clearing flags on attachment: 331460

Committed r227049: <https://trac.webkit.org/changeset/227049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36573453>

[Fujii Hironori]

*** Bug 181117 has been marked as a duplicate of this bug. ***