Bug 143293 – progress towards CMake on Mac and Windows

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alex Christensen]

More work in progress.

[Alex Christensen]

Created attachment 249901 [details]
Patch

[Alex Christensen]

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 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 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]

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

Created attachment 249923 [details]
Patch

[Alex Christensen]

Created attachment 249931 [details]
Patch

[Filip Pizlo]

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 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]

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

http://trac.webkit.org/changeset/182243

[Carlos Garcia Campos]

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]

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

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]

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

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

New bug report to fix the build: bug143361