Bug 72692 – [CMAKE] Add the configuration of cmake to select between JSC and V8

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[jaehoon jeong]

Add the configuration of cmake to select between JSC and V8

[jaehoon jeong]

Created attachment 115749 [details]
Add the configuration of cmake to select between JSC and V8

[jaehoon jeong]

Created attachment 115750 [details]
Add the configuration of cmake to select between JSC and V8

[WebKit Review Bot]

Attachment 115749 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/CMakeLists.txt', u'So..." exit_code: 1

ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 115750 [details]
Add the configuration of cmake to select between JSC and V8

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

How does this interact with WebKit, WebCore and Tools, all of which include JavaScriptCore directories and need its headers?

> Source/CMakeLists.txt:52
> +SET(JS_ENGINE "JSC" CACHE STRING "choose which javascrit engine to use (one of ${ALL_JAVASCRIPT_ENGINES})")

typo: "javascrit"

[Raphael Kubo da Costa (:rakuco)]

If you don't CC people to your bugs, nobody is going to review them. I'm CC'ing Patrick and Daniel, who also work on CMake stuff.

[Patrick R. Gansterer]

Comment on attachment 115750 [details]
Add the configuration of cmake to select between JSC and V8

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

> ChangeLog:3
> +        [CMAKE] Add the configuration of cmake to select between JSC and V8.

not importent, but cmake is usually written as CMake

>> Source/CMakeLists.txt:52
>> +SET(JS_ENGINE "JSC" CACHE STRING "choose which javascrit engine to use (one of ${ALL_JAVASCRIPT_ENGINES})")
> 
> typo: "javascrit"

I'd prefer the long name JAVASCRIPT_ENGINE

> Source/CMakeLists.txt:56
> +    SET(WTF_USE_V8 1)

If you set WTF_USE_V8, i think you should set WTF_USE_JSC too

> Source/CMakeLists.txt:146
> +    ADD_SUBDIRECTORY(JavaScriptCore/wtf)

I think you should remove ADD_SUBDIRECTROY(wtf) from JavaScriptCore/CMakeLists.txt and always ADD_SUBDIRECTORY(JavaScriptCore/wtf) here

> Source/CMakeLists.txt:148
> +    ADD_SUBDIRECTORY(JavaScriptCore)

should be IF (WTF_USE_JSC) then

> Source/CMakeLists.txt:174
> +IF (NOT WTF_USE_V8)

IF (WTF_USE_JSC) instead

> Source/cmake/WebKitMacros.cmake:88
> +        SET(_outputfiles "${_outputfiles}" ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.h ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.h)

What about the calls to GENERATE_DOM_NAMES in WebCore/CMakeLists.txt? This will result in an error, since e.g. JSHTMLElementWrapperFactory.cpp won't get generated. Maybe there are more problems of this kind.

[jaehoon jeong]

Comment on attachment 115750 [details]
Add the configuration of cmake to select between JSC and V8

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

>>> Source/CMakeLists.txt:52
>>> +SET(JS_ENGINE "JSC" CACHE STRING "choose which javascrit engine to use (one of ${ALL_JAVASCRIPT_ENGINES})")
>> 
>> typo: "javascrit"
> 
> I'd prefer the long name JAVASCRIPT_ENGINE

ok, I will change it to "JAVASCRIPT_ENGINE" :)

>> Source/CMakeLists.txt:56

> 
> If you set WTF_USE_V8, i think you should set WTF_USE_JSC too

I will define WTF_USE_JSC and set it too.

>> Source/CMakeLists.txt:146
>> +    ADD_SUBDIRECTORY(JavaScriptCore/wtf)
> 
> I think you should remove ADD_SUBDIRECTROY(wtf) from JavaScriptCore/CMakeLists.txt and always ADD_SUBDIRECTORY(JavaScriptCore/wtf) here

ok... Regardless of javascript engine, I make ADD_SUBDIRECTORY(JavaScriptCore/wtf) default and remove it from JavaScriptCore/CMakeLists.txt

>> Source/CMakeLists.txt:174
>> +IF (NOT WTF_USE_V8)
> 
> IF (WTF_USE_JSC) instead

ok.. I replace it with "WTF_USE_JSC"

>> Source/cmake/WebKitMacros.cmake:88
>> +        SET(_outputfiles "${_outputfiles}" ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${_namespace}ElementFactory.h ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/${JAVASCRIPT_PREFIX}${_namespace}ElementWrapperFactory.h)
> 
> What about the calls to GENERATE_DOM_NAMES in WebCore/CMakeLists.txt? This will result in an error, since e.g. JSHTMLElementWrapperFactory.cpp won't get generated. Maybe there are more problems of this kind.

hm.. I don't know exactly what you mean,,
so I will upload other patches for v8 in this bug... and then please review them.

[jaehoon jeong]

Created attachment 116054 [details]
Add the configuration of cmake to select between JSC and V8

[jaehoon jeong]

Created attachment 116055 [details]
Add the configuration of cmake to select between JSC and V8

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 116055 [details]
Add the configuration of cmake to select between JSC and V8

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

> Source/WebCore/CMakeLists.txt:871
> -    html/shadow/DetailsMarkerControl.cpp
> +    html/shadow/DetailsMarkerControl.cpp    

Unneeded change.

> Source/WebCore/CMakeLists.txt:2429
> -ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +IF (WTF_USE_V8)
> +    ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${WTF_LIBRARY_NAME})
> +ELSEIF (WTF_USE_JSC)
> +    ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +ENDIF()

I think this section is not needed -- TARGET_LINK_LIBRARIES will automatically create the dependencies between the targets.

[jaehoon jeong]

Comment on attachment 116055 [details]
Add the configuration of cmake to select between JSC and V8

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

>> Source/WebCore/CMakeLists.txt:871
>> +    html/shadow/DetailsMarkerControl.cpp    
> 
> Unneeded change.

There are some spaces at the end of it.. I made a mistake,, :(

>> Source/WebCore/CMakeLists.txt:2429
>> +ENDIF()
> 
> I think this section is not needed -- TARGET_LINK_LIBRARIES will automatically create the dependencies between the targets.

If only JavaScriptCore_LIBRARY_NAME is setted as TARGET_LINK_LIBRARIES when build with v8.
Undefined reference errors occurr related with WTF libraries. so I think that this change need to be included.

[jaehoon jeong]

Created attachment 116312 [details]
Add the configuration of CMake to select between JSC and V8

[jaehoon jeong]

Created attachment 116313 [details]
Add the configuration of CMake to select between JSC and V8

[jaehoon jeong]

Created attachment 116314 [details]
This patch is the configuration of WebCore's CMake

[Gyuyoung Kim]

Comment on attachment 116313 [details]
Add the configuration of CMake to select between JSC and V8

Attachment 116313 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10602165

[jaehoon jeong]

Created attachment 116330 [details]
Add the configuration of CMake to select between JSC and V8

[jaehoon jeong]

Created attachment 116331 [details]
This patch is the configuration of WebCore's CMake

[Gyuyoung Kim]

Comment on attachment 116331 [details]
This patch is the configuration of WebCore's CMake

Attachment 116331 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10645183

[jaehoon jeong]

Created attachment 116486 [details]
Add the configuration of CMake to select between JSC and V8

[jaehoon jeong]

Created attachment 116487 [details]
This patch is the configuration of WebCore's CMake

[Gyuyoung Kim]

Why do you submit two patches to a Bug. Basically, we submit a patch to a Bug except for special case.

[jaehoon jeong]

(In reply to comment #21)
> Why do you submit two patches to a Bug. Basically, we submit a patch to a Bug except for special case.

At first, I had added just one patch related to Source/CMakeLists.txt and Source/cmake/WebKitMacros.cmake. but I thought that WebCore's CMake patch is also needed for review. so I added it as another patch.
If you think that just one patch is better... I'll make them one patch.. tanks. :)

[jaehoon jeong]

Created attachment 116490 [details]
Add the configuration of CMake to select between JSC and V8

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 116490 [details]
Add the configuration of CMake to select between JSC and V8

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

It'd be good if you could submit the same patch (without flagging cq? or r?) setting V8 as the default engine and then manually submit it for EWS analysis, just so that we can check if the V8 code path builds fine.

> Source/CMakeLists.txt:8
> +IF (WTF_USE_JSC)
> +    ADD_SUBDIRECTORY(JavaScriptCore)
> +ENDIF ()
> +
> +ADD_SUBDIRECTORY(JavaScriptCore/wtf)

It makes sense to build wtf first.

> Source/CMakeLists.txt:23
> +IF (WTF_USE_V8)
> +    WEBKIT_SET_EXTRA_COMPILER_FLAGS(${WTF_LIBRARY_NAME})

WTF needs the extra compiler flags regardless of the engine chosen.

> Source/WebCore/CMakeLists.txt:2441
> -ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +IF (WTF_USE_V8)
> +    ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${WTF_LIBRARY_NAME})
> +ELSEIF (WTF_USE_JSC)
> +    ADD_DEPENDENCIES(${WebCore_LIBRARY_NAME} ${JavaScriptCore_LIBRARY_NAME})
> +ENDIF()
>  TARGET_LINK_LIBRARIES(${WebCore_LIBRARY_NAME} ${WebCore_LIBRARIES})

My point with this in a previous review is: WebCore_LIBRARIES is set to WTF_LIBRARY_NAME when V8 is chosen, and JavaScriptCore_LIBRARY_NAME when JSC is chosen. This means that TARGET_LINK_LIBRARIES() will create a dependency between WebCore_LIBRARY_NAME and WebCore_LIBRARIES, which makes the ADD_DEPENDENCIES() calls redundant. What errors do you get when you remove the ADD_DEPENDENCIES() block?

[jaehoon jeong]

Comment on attachment 116490 [details]
Add the configuration of CMake to select between JSC and V8

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

>> Source/CMakeLists.txt:8
>> +ADD_SUBDIRECTORY(JavaScriptCore/wtf)
> 
> It makes sense to build wtf first.

I'll make WTF first.

>> Source/CMakeLists.txt:23
>> +    WEBKIT_SET_EXTRA_COMPILER_FLAGS(${WTF_LIBRARY_NAME})
> 
> WTF needs the extra compiler flags regardless of the engine chosen.

I'll make the WTF extra compiler flags to default option regardless of the engine.

>> Source/WebCore/CMakeLists.txt:2441
>>  TARGET_LINK_LIBRARIES(${WebCore_LIBRARY_NAME} ${WebCore_LIBRARIES})
> 
> My point with this in a previous review is: WebCore_LIBRARIES is set to WTF_LIBRARY_NAME when V8 is chosen, and JavaScriptCore_LIBRARY_NAME when JSC is chosen. This means that TARGET_LINK_LIBRARIES() will create a dependency between WebCore_LIBRARY_NAME and WebCore_LIBRARIES, which makes the ADD_DEPENDENCIES() calls redundant. What errors do you get when you remove the ADD_DEPENDENCIES() block?

OK... I understand what you mean. thanks
I remove ADD_DEPENDENCIES of WTF and JavaScirptCore

[jaehoon jeong]

Created attachment 116926 [details]
Add the configuration of CMake to select between JSC and V8

[Raphael Kubo da Costa (:rakuco)]

Looks OK, but I think it is essential to follow my suggestion in comment #24 and trigger an EWS build with V8 instead of JSC.

[Eric Seidel (no email)]

Comment on attachment 116926 [details]
Add the configuration of CMake to select between JSC and V8

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

> Source/CMakeLists.txt:4
> +ADD_SUBDIRECTORY(JavaScriptCore/wtf)

This directory doesnt' exist anymore.

[Patrick R. Gansterer]

V8 has been removed from tree