[webkit-reviews] review denied: [Bug 68510] [EFL] build scripts modifications to support unit tests. : [Attachment 108819] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Krzysztof
<k.czech at samsung.com>'s request for review:
Bug 68510: [EFL] build scripts modifications to support unit tests.
https://bugs.webkit.org/show_bug.cgi?id=68510

Attachment 108819: Patch
https://bugs.webkit.org/attachment.cgi?id=108819&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=108819&action=review


I don't see why this change should be in a separate patch instead of being sent
together with bug 68509: this one will fail without the code, and the buildbot
will not really test the patch in bug 68509 without the buildsystem changes.
Could you please close this report and merge this patch with the one in bug
68509 (further discussions could then go in that report)?

> Source/CMakeLists.txt:27
> +SET(GTEST_DIR "${THIRDPARTY_DIR}/gtest")

No need to set this for all ports.

> Source/CMakeLists.txt:146
>  #
-----------------------------------------------------------------------------
> +# GTest headers
> +#
-----------------------------------------------------------------------------
> +INCLUDE_DIRECTORIES(${GTEST_DIR} ${GTEST_DIR}/include)

Ditto.

> Source/CMakeLists.txt:156
> +# Add EFL unit tests
> +#
-----------------------------------------------------------------------------
> +INCLUDE_IF_EXISTS(${WEBKIT_DIR}/efl/tests/CMakeLists${PORT}.txt)

This should not be here. Just add a `add_subdirectory(tests)` call in
Sources/WebKit/efl/CMakeListsEfl.txt (you need to rename
tests/CMakeListsEfl.txt to tests/CMakeLists.txt for this to work, which
actually makes sense).

> Source/WebKit/efl/CMakeListsEfl.txt:267
> +INCLUDE_DIRECTORIES(${GTEST_DIR} ${GTEST_DIR}/include)
> +
> +SET(GTEST_SOURCES ${THIRDPARTY_DIR}/gtest/src)
> +ADD_LIBRARY(gtest
> +	       ${GTEST_SOURCES}/gtest.cc
> +	       ${GTEST_SOURCES}/gtest-death-test.cc
> +	       ${GTEST_SOURCES}/gtest_main.cc
> +	       ${GTEST_SOURCES}/gtest-filepath.cc
> +	       ${GTEST_SOURCES}/gtest-port.cc
> +	       ${GTEST_SOURCES}/gtest-test-part.cc
> +	       ${GTEST_SOURCES}/gtest-typed-test.cc
> +)

This could go into tests/CMakeLists.txt.

> Source/WebKit/efl/tests/CMakeListsEfl.txt:35
> +SET(DEFAULT_THEME_PATH ${CMAKE_BINARY_DIR}/WebKit/efl/DefaultTheme)
> +ADD_DEFINITIONS(-DDEFAULT_THEME_PATH=\"${DEFAULT_THEME_PATH}\")

There is now THEME_BINARY_DIR with the same value.

> Source/WebKit/efl/tests/CMakeListsEfl.txt:37
> +ADD_DEFINITIONS(-DGTEST_TEST_FRAMEWORK)

What is this used for?