RESOLVED FIXED 143293
progress towards CMake on Mac and Windows
https://bugs.webkit.org/show_bug.cgi?id=143293
Summary progress towards CMake on Mac and Windows
Alex Christensen
Reported 2015-03-31 22:43:59 PDT
More work in progress.
Attachments
Patch (33.00 KB, patch)
2015-03-31 23:57 PDT, Alex Christensen
no flags
Patch (33.00 KB, patch)
2015-04-01 09:14 PDT, Alex Christensen
no flags
Patch (33.05 KB, patch)
2015-04-01 10:22 PDT, Alex Christensen
no flags
Alex Christensen
Comment 1 2015-03-31 23:57:32 PDT
Alex Christensen
Comment 2 2015-04-01 00:06:33 PDT
I am under the impression that I did not change any bmalloc behavior for non-Windows systems. Why is the efl bot red?
Gyuyoung Kim
Comment 3 2015-04-01 00:08:38 PDT
Comment on attachment 249901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249901&action=review > Source/CMakeLists.txt:5 > +if (!WIN32) It looks this causes EFL build break. This seems not to work properly. CMake Error at Source/cmake/WebKitHelpers.cmake:28 (set_target_properties): set_target_properties Can not find target to add properties to: bmalloc Call Stack (most recent call first): Source/CMakeLists.txt:34 (WEBKIT_SET_EXTRA_COMPILER_FLAGS) > Source/cmake/OptionsWindows.cmake:8 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_VIEW_MODE_CSS_MEDIA OFF) Duplicated definition with 6 line ?
Gyuyoung Kim
Comment 4 2015-04-01 02:50:37 PDT
Comment on attachment 249901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249901&action=review >> Source/CMakeLists.txt:5 >> +if (!WIN32) > > It looks this causes EFL build break. This seems not to work properly. > > CMake Error at Source/cmake/WebKitHelpers.cmake:28 (set_target_properties): > set_target_properties Can not find target to add properties to: bmalloc > Call Stack (most recent call first): > Source/CMakeLists.txt:34 (WEBKIT_SET_EXTRA_COMPILER_FLAGS) ditto. > Source/WTF/wtf/CMakeLists.txt:226 > +if (!WIN32) Please use "if (NOT WIN32)".
Alex Christensen
Comment 5 2015-04-01 09:12:11 PDT
(In reply to comment #4) > > Source/WTF/wtf/CMakeLists.txt:226 > > +if (!WIN32) > > Please use "if (NOT WIN32)". Aha! Thanks! I'm still getting used to CMake. I write too much C++ code :)
Alex Christensen
Comment 6 2015-04-01 09:14:20 PDT
Alex Christensen
Comment 7 2015-04-01 10:22:34 PDT
Filip Pizlo
Comment 8 2015-04-01 11:21:03 PDT
Comment on attachment 249931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249931&action=review R=me. Consider making your FIXME's cite bugs, but it's no biggie. > Source/CMakeLists.txt:4 > +# FIXME: Port bmalloc to Windows. I've always liked to have FIXME's cite the bug. There's gotta be a bug for porting bmalloc to Windows. You could cite it here. > Source/ThirdParty/ANGLE/include/GLES2/gl2.h:473 > +// FIXME: Get WebGL working on Windows with CMake. Similar comment.
Chris Dumez
Comment 9 2015-04-01 11:22:21 PDT
Comment on attachment 249931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249931&action=review > Source/WTF/wtf/CMakeLists.txt:-182 > - WorkQueue.cpp Why don't we use this one on Windows? It is listed in Source/WTF/WTF.vcxproj/WTF.vcxproj. > Source/WebCore/PlatformMac.cmake:-86 > - platform/cf/SharedTimerCF.mm It is SharedTimerCF.cpp now
Alex Christensen
Comment 10 2015-04-01 11:25:30 PDT
(In reply to comment #9) > Comment on attachment 249931 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249931&action=review > > > Source/WTF/wtf/CMakeLists.txt:-182 > > - WorkQueue.cpp > > Why don't we use this one on Windows? It is listed in > Source/WTF/WTF.vcxproj/WTF.vcxproj. I'm not sure. It was causing compiling errors and I was under the impression it wasn't used on Windows, but looking into it I'm pretty sure that was a false assumption. I'll land it without this change. > > > Source/WebCore/PlatformMac.cmake:-86 > > - platform/cf/SharedTimerCF.mm > > It is SharedTimerCF.cpp now Thanks!
Alex Christensen
Comment 11 2015-04-01 11:37:47 PDT
Carlos Garcia Campos
Comment 12 2015-04-02 00:41:33 PDT
Comment on attachment 249931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249931&action=review This has broken GTK+ build, bots didn't notice it because it only affects clean builds. > Source/JavaScriptCore/CMakeLists.txt:1091 > - ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol/InspectorBackendCommands.js > + ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/inspector/InspectorBackendCommands.js Why did you change this path? GTK+ build still looks for inspector UI files in ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/ > Source/JavaScriptCore/CMakeLists.txt:1095 > - COMMAND ${PYTHON_EXECUTABLE} ${JavaScriptCore_INSPECTOR_SCRIPTS_DIR}/generate-inspector-protocol-bindings.py --outputDir "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}" --framework JavaScriptCore ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/CombinedDomains.json && mkdir -p ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector && cp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorBackendDispatchers.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorBackendDispatchers.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorFrontendDispatchers.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorFrontendDispatchers.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorProtocolObjects.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorProtocolObjects.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector && mkdir -p ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol && cp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorBackendCommands.js ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol > + COMMAND ${PYTHON_EXECUTABLE} ${JavaScriptCore_INSPECTOR_SCRIPTS_DIR}/generate-inspector-protocol-bindings.py --outputDir "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector" --framework JavaScriptCore ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/CombinedDomains.json > VERBATIM) InspectorBackendCommands.js is no longer copied to the expected directory ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol.
Zan Dobersek
Comment 13 2015-04-02 00:58:44 PDT
(In reply to comment #12) > Comment on attachment 249931 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249931&action=review > > This has broken GTK+ build, bots didn't notice it because it only affects > clean builds. > > > Source/JavaScriptCore/CMakeLists.txt:1091 > > - ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol/InspectorBackendCommands.js > > + ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/inspector/InspectorBackendCommands.js > > Why did you change this path? GTK+ build still looks for inspector UI files > in ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/ > > > Source/JavaScriptCore/CMakeLists.txt:1095 > > - COMMAND ${PYTHON_EXECUTABLE} ${JavaScriptCore_INSPECTOR_SCRIPTS_DIR}/generate-inspector-protocol-bindings.py --outputDir "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}" --framework JavaScriptCore ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/CombinedDomains.json && mkdir -p ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector && cp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorBackendDispatchers.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorBackendDispatchers.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorFrontendDispatchers.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorFrontendDispatchers.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorProtocolObjects.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorProtocolObjects.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector && mkdir -p ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol && cp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorBackendCommands.js ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol > > + COMMAND ${PYTHON_EXECUTABLE} ${JavaScriptCore_INSPECTOR_SCRIPTS_DIR}/generate-inspector-protocol-bindings.py --outputDir "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector" --framework JavaScriptCore ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/CombinedDomains.json > > VERBATIM) > > InspectorBackendCommands.js is no longer copied to the expected directory > ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/Protocol. Is the copying required? Can't we just use the file from DerivedSources/inspector/?
Carlos Garcia Campos
Comment 14 2015-04-02 01:10:01 PDT
It's required for the scripts used by GTK+, of course we can change that, but I think it's a different issue. The patch changed the output to be ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/inspector/InspectorBackendCommands.js but the file is actually written to ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/inspector/. Our script generate-inspector-gresource-manifest.py expects all passed files to have a WebInspectorUI/ dir at some point, to generate the xml file with paths relative to that. Then we can pass Source/WebInspectorUI and DerivedSources/WebInspectorUI as source dirs to glib-compile-resources
Alex Christensen
Comment 15 2015-04-02 12:15:09 PDT
(In reply to comment #12) > Why did you change this path? GTK+ build still looks for inspector UI files > in ${DERIVED_SOURCES_WEBINSPECTORUI_DIR}/UserInterface/ Sorry about that. The point of that change was to get rid of unix commands like cp and mkdir so we can use CMake on Windows. The ews bots were all green, and I thought these files were only used for building. If I broke something, we should change it back, or add a copy command somewhere. (In reply to comment #14) > It's required for the scripts used by GTK+ Are these scripts in WebKit or another repository?
Zan Dobersek
Comment 16 2015-04-02 12:33:56 PDT
(In reply to comment #15) > (In reply to comment #14) > > It's required for the scripts used by GTK+ > Are these scripts in WebKit or another repository? They are in WebKit, but only clean builds were affected. Carlos and I agreed that the current situation is more elegant, and that port-specific rules should be added if that port requires a file from DerivedSources/JavaScriptCore/inspector/ in some specific location. That's what I'll do tomorrow, in a separate bug.
Csaba Osztrogonác
Comment 17 2015-04-02 23:17:32 PDT
New bug report to fix the build: bug143361
Note You need to log in before you can comment on or make changes to this bug.