Bug 68510 - [EFL] build scripts modifications to support unit tests.
Summary: [EFL] build scripts modifications to support unit tests.
Status: RESOLVED DUPLICATE of bug 90606
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 68509
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-21 01:34 PDT by Krzysztof Czech
Modified: 2012-08-15 11:20 PDT (History)
7 users (show)

See Also:


Attachments
Added conditions for building gtest library and building unit tests. (4.39 KB, patch)
2011-09-21 01:37 PDT, Krzysztof Czech
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2011-09-21 01:43 PDT, Krzysztof Czech
rakuco: review-
Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2011-09-27 01:56 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2011-09-27 02:46 PDT, Krzysztof Czech
rakuco: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2011-09-21 01:34:49 PDT
Added conditions for building gtest library and building unit tests.
Comment 1 Krzysztof Czech 2011-09-21 01:37:07 PDT
Created attachment 108122 [details]
Added conditions for building gtest library and building unit tests.
Comment 2 Gyuyoung Kim 2011-09-21 01:40:12 PDT
Comment on attachment 108122 [details]
Added conditions for building gtest library and building unit tests.

Attachment 108122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9773598
Comment 3 WebKit Review Bot 2011-09-21 01:40:14 PDT
Attachment 108122 [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

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Krzysztof Czech 2011-09-21 01:43:17 PDT
Created attachment 108123 [details]
Patch
Comment 5 WebKit Review Bot 2011-09-21 01:45:38 PDT
Attachment 108123 [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

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Gyuyoung Kim 2011-09-21 01:47:22 PDT
Comment on attachment 108123 [details]
Patch

Attachment 108123 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9765667
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-09-21 07:20:48 PDT
Comment on attachment 108123 [details]
Patch

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

> ChangeLog:3
> +        [EFL] build scripts modifications to support unit tests.

Capital 'b' in build.

> ChangeLog:8
> +        Added conditions for building gtest library and building unit tests.

This could be moved to the * Source/CMakeLists.txt line.

> Source/CMakeLists.txt:147
> +# Add google unit tests (gtest)

I'd rather have all this in Source/WebKit/efl instead.

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

Is this needed for gtest to work?

> Source/WebKit/efl/tests/CMakeListsEfl.txt:39
> +ADD_LIBRARY(efl_test_launcher

What links against this library?
Comment 8 Krzysztof Czech 2011-09-22 01:13:41 PDT
(In reply to comment #7)
> (From update of attachment 108123 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108123&action=review
> 
> > ChangeLog:3
> > +        [EFL] build scripts modifications to support unit tests.
> 
> Capital 'b' in build.
> 
> > ChangeLog:8
> > +        Added conditions for building gtest library and building unit tests.
> 
> This could be moved to the * Source/CMakeLists.txt line.
> 
> > Source/CMakeLists.txt:147
> > +# Add google unit tests (gtest)
> 
> I'd rather have all this in Source/WebKit/efl instead.
> 
> > Source/WebKit/efl/tests/CMakeListsEfl.txt:37
> > +ADD_DEFINITIONS(-DGTEST_TEST_FRAMEWORK)
> 
> Is this needed for gtest to work?
This might be needed when gtest will be replaced by the other framework.
unit test launcher should be a separate component.
> 
> > Source/WebKit/efl/tests/CMakeListsEfl.txt:39
> > +ADD_LIBRARY(efl_test_launcher
> 
> What links against this library?
Unit tests links against this library.
Comment 9 Krzysztof Czech 2011-09-27 01:56:58 PDT
Created attachment 108813 [details]
Patch
Comment 10 Krzysztof Czech 2011-09-27 02:46:33 PDT
Created attachment 108819 [details]
Patch
Comment 11 Gyuyoung Kim 2011-10-19 00:28:48 PDT
Comment on attachment 108819 [details]
Patch

Attachment 108819 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10145307
Comment 12 Krzysztof Czech 2011-11-02 02:37:43 PDT
Build fails because this patch depends on 68509.
Comment 13 Krzysztof Czech 2011-11-14 01:37:25 PST
Dependency has been corrected. Could be tested again
Comment 14 Gyuyoung Kim 2011-11-15 00:29:03 PST
(In reply to comment #13)
> Dependency has been corrected. Could be tested again

I'm wondering if this patch can be built without Bug 68509's patch. If this patch is only able to be built based on Bug 68509's patch, you should wait for landing the Bug 68509's patch.
Comment 15 Krzysztof Czech 2011-11-16 00:32:29 PST
This patch cannot be build separately. It should wait till 68509 will be applied.
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-01-01 15:28:41 PST
Comment on attachment 108819 [details]
Patch

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?
Comment 17 Krzysztof Czech 2012-01-02 06:31:01 PST
I will merge this patch.
Comment 18 Thiago Marcos P. Santos 2012-08-15 11:20:31 PDT

*** This bug has been marked as a duplicate of bug 90606 ***