[CMake] Make gtest a shared library

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

It's nicer to make it a shared library because it might improve linking time and we don't need to force gtest users to link with gtest dependencies.

[Thiago Marcos P. Santos]

Created attachment 151680 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 151680 [details]
Patch

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

I wonder if this is really needed -- gtest is only needed by unit tests, whose linking time isn't very important, and pthreads would need to be present anyway for gtest to be built.

> Source/cmake/gtest/CMakeLists.txt:26
> +IF(CMAKE_USE_PTHREADS_INIT)

You need FIND_PACKAGE(Threads) to use this. Plus, shouldn't you just always link to the thread library?

[Thiago Marcos P. Santos]

Comment on attachment 151680 [details]
Patch

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

>> Source/cmake/gtest/CMakeLists.txt:26
>> +IF(CMAKE_USE_PTHREADS_INIT)
> 
> You need FIND_PACKAGE(Threads) to use this. Plus, shouldn't you just always link to the thread library?

FIND_PACKAGE(Threads) is done at Options${Platform} like the other FIND_PACKAGEs.

pthreads won't happen if you are building on Windows, remember that we are not the only customers of CMake build system and I want to make this reusable.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 151680 [details]
Patch

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

>>> Source/cmake/gtest/CMakeLists.txt:26
>>> +IF(CMAKE_USE_PTHREADS_INIT)
>> 
>> You need FIND_PACKAGE(Threads) to use this. Plus, shouldn't you just always link to the thread library?
> 
> FIND_PACKAGE(Threads) is done at Options${Platform} like the other FIND_PACKAGEs.
> 
> pthreads won't happen if you are building on Windows, remember that we are not the only customers of CMake build system and I want to make this reusable.

Hmm, but don't you need to link to the equivalent thread library in other systems as well?

[Thiago Marcos P. Santos]

Created attachment 152071 [details]
Patch

Rebased.

[Thiago Marcos P. Santos]

(In reply to comment #4)
> (From update of attachment 151680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151680&action=review
> 
> >>> Source/cmake/gtest/CMakeLists.txt:26
> >>> +IF(CMAKE_USE_PTHREADS_INIT)
> >> 
> >> You need FIND_PACKAGE(Threads) to use this. Plus, shouldn't you just always link to the thread library?
> > 
> > FIND_PACKAGE(Threads) is done at Options${Platform} like the other FIND_PACKAGEs.
> > 
> > pthreads won't happen if you are building on Windows, remember that we are not the only customers of CMake build system and I want to make this reusable.
> 
> Hmm, but don't you need to link to the equivalent thread library in other systems as well?

I don't think threading support is mandatory, but don't expect for example that thread-safe death tests will be available in this case.

[Gyuyoung Kim]

Comment on attachment 152071 [details]
Patch

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

[Thiago Marcos P. Santos]

Created attachment 152512 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152512 [details]
Patch

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

[Thiago Marcos P. Santos]

(In reply to comment #9)
> (From update of attachment 152512 [details])
> Attachment 152512 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13253265

[ 90%] /usr/bin/ld: ../../bin/test_ewk_view: hidden symbol `Building CXX object Source/WebKit2/CMakeFiles/ewebkit2.dir/UIProcess/WebGrammarDetail.cpp.o
WTF::fastMalloc(unsigned long)' in ../../lib/libwtf_efl.a(FastMalloc.cpp.o) is referenced by DSO
/usr/bin/ld: final link failed: Bad value


Weird, I don't get this error locally.

[Thiago Marcos P. Santos]

Created attachment 152532 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152532 [details]
Patch

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

[Thiago Marcos P. Santos]

Created attachment 152535 [details]
Patch

One more attempt to make EFL EWS happy. I really could not reproduce this linking error. :(

[Gyuyoung Kim]

Comment on attachment 152535 [details]
Patch

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

[Thiago Marcos P. Santos]

Created attachment 152555 [details]
Patch

One more attempt.

[Thiago Marcos P. Santos]

(In reply to comment #15)
> Created an attachment (id=152555) [details]
> Patch
> 
> One more attempt.

EWS is sane. Don't know why I was not having this linking errors on my desktop. The problem was gtest has now to link with WTF, since it uses it wrappers for memory allocation.

[Daniel Bates]

Comment on attachment 152555 [details]
Patch

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

r=me

> ChangeLog:10
> +        dependencies like pthreads (and avoid linking errors when it is not

Nit: "and avoid" => "which produces" OR "which causes"

[Thiago Marcos P. Santos]

Created attachment 152970 [details]
Patch

Updated patch. Thanks for reviewing.

[WebKit Review Bot]

Comment on attachment 152970 [details]
Patch

Clearing flags on attachment: 152970

Committed r122943: <http://trac.webkit.org/changeset/122943>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.