[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.
Created attachment 331365 [details] WIP patch
Created attachment 331373 [details] WIP patch
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.
Created attachment 331377 [details] Patch
(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!
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})
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.
I verified that WebCore links on Mac using Ninja with this patch. It's ok once Michael's comments have been addressed.
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.
Created attachment 331456 [details] Patch
(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
(In reply to Fujii Hironori from comment #11) I found the answer in Bug 138655 WebCorePlatformGTK should be specified before WebCore for the linker.
Created attachment 331460 [details] Patch
Comment on attachment 331460 [details] Patch Clearing flags on attachment: 331460 Committed r227049: <https://trac.webkit.org/changeset/227049>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36573453>
*** Bug 181117 has been marked as a duplicate of this bug. ***