Use find_package(Threads) when building with cmake

[klimek, 2009-12-30 09:58:09]

First draft, will fix formatting after I test it on Windows tonight ;-)

[wan, 2009-12-30 17:46:02]

Thanks for the quick turn-around!

http://codereview.appspot.com/183093/diff/1/2
File CMakeLists.txt (right):

http://codereview.appspot.com/183093/diff/1/2#newcode118
CMakeLists.txt:118: find_package(PythonInterp)
For my education, could you explain what the benefit of this change is?

http://codereview.appspot.com/183093/diff/1/2#newcode123
CMakeLists.txt:123: # cxx_test_with_flags(name cxx_flags libs srcs...)
I try to structure the script such that a definition is done right before it's
first needed (roughly).  The thinking is that there are different kinds of
users: some only care to build gtest itself, some want to build the samples too,
some want to build some basic tests for gtest, and yet some want to build
everything.  We shouldn't make the user read stuff that he doesn't need to know.

In this case, if a user only wants to build tests that use standard compiler
flags, he shouldn't need to know about cxx_test_with_flags.  Therefore I'd like
to move this function back to the next section, even though it means that we
can't define function cxx_test in terms of it.

http://codereview.appspot.com/183093/diff/1/2#newcode202
CMakeLists.txt:202: cxx_test_with_flags(gtest-death-test_test ""
The compiler flags shouldn't be empty.  They should be consistent with what's
used to build gtest_main.

Also, does this work on Linux?  As I remember, I had to add -pthread both when
compiling gtest-death-test_test.cc and when linking gtest-death-test_test.

[klimek, 2009-12-31 10:16:11]

http://codereview.appspot.com/183093/diff/1/2
File CMakeLists.txt (right):

http://codereview.appspot.com/183093/diff/1/2#newcode118
CMakeLists.txt:118: find_package(PythonInterp)
On 2009/12/30 17:46:02, wan wrote:
> For my education, could you explain what the benefit of this change is?

It's the cmake idiom
(http://cmake.org/cmake/help/cmake-2-8-docs.html#command:find_package) for
finding installed software. While without any parameters it probably doesn't
matter, people with cmake experience will understand what we're doing here ...

http://codereview.appspot.com/183093/diff/1/2#newcode123
CMakeLists.txt:123: # cxx_test_with_flags(name cxx_flags libs srcs...)
On 2009/12/30 17:46:02, wan wrote:
> I try to structure the script such that a definition is done right before it's
> first needed (roughly).  The thinking is that there are different kinds of
> users: some only care to build gtest itself, some want to build the samples
too,
> some want to build some basic tests for gtest, and yet some want to build
> everything.  We shouldn't make the user read stuff that he doesn't need to
know.
> 
> In this case, if a user only wants to build tests that use standard compiler
> flags, he shouldn't need to know about cxx_test_with_flags.  Therefore I'd
like
> to move this function back to the next section, even though it means that we
> can't define function cxx_test in terms of it.

First, I don't understand your reasoning about the "user" here. The "user" who
just wants to build doesn't need to read this file at all - she will use ccmake,
read the description of the options, switch some options, and then do a "make".
If you're concerned about the level of abstraction, I would propose to put those
"lowlevel" methods into a googletest.cmake module and just put the declarative
part in this file.

The reason I did this is because the method is already non-trivial, and I was
trying to fix a linking issue (for target_link_libraries, I added a comment),
and had to do message-debugging to find out that I've been changing the wrong
method. In general I strongly feel that the duplication is bad here ;-) But of
course I'll change it back if you insist on putting it back in...

http://codereview.appspot.com/183093/diff/1/2#newcode202
CMakeLists.txt:202: cxx_test_with_flags(gtest-death-test_test ""
On 2009/12/30 17:46:02, wan wrote:
> The compiler flags shouldn't be empty.  They should be consistent with what's
> used to build gtest_main.

Ah, I see. Thanks for catching.

> 
> Also, does this work on Linux?  As I remember, I had to add -pthread both when
> compiling gtest-death-test_test.cc and when linking gtest-death-test_test.

So far I only tested it on linux. Yes, it works. The problem you had was
probably that if you don't use "target_link_libraries", but add the -lpthread
manually to the linker flags (like you did with -pthread), the -lpthread is in
the wrong place of the gcc command line, which breaks the linking.

[Gavin, 2010-01-04 10:50:27]

http://codereview.appspot.com/183093/diff/1/2
File CMakeLists.txt (right):

http://codereview.appspot.com/183093/diff/1/2#newcode22
CMakeLists.txt:22: cmake_minimum_required(VERSION 2.8)
Are any features used that are only present in version 2.8, or could a lower
version number be used to improve compatibility with existing CMake
installations?

[wan, 2010-01-04 18:05:30]

http://codereview.appspot.com/183093/diff/1/2
File CMakeLists.txt (right):

http://codereview.appspot.com/183093/diff/1/2#newcode118
CMakeLists.txt:118: find_package(PythonInterp)
On 2009/12/31 10:16:11, klimek wrote:
> On 2009/12/30 17:46:02, wan wrote:
> > For my education, could you explain what the benefit of this change is?
> 
> It's the cmake idiom
> (http://cmake.org/cmake/help/cmake-2-8-docs.html#command:find_package) for
> finding installed software. While without any parameters it probably doesn't
> matter, people with cmake experience will understand what we're doing here ...

Thanks for explaining!

http://codereview.appspot.com/183093/diff/1/2#newcode123
CMakeLists.txt:123: # cxx_test_with_flags(name cxx_flags libs srcs...)
On 2009/12/31 10:16:11, klimek wrote:
> On 2009/12/30 17:46:02, wan wrote:
> > I try to structure the script such that a definition is done right before
it's
> > first needed (roughly).  The thinking is that there are different kinds of
> > users: some only care to build gtest itself, some want to build the samples
> too,
> > some want to build some basic tests for gtest, and yet some want to build
> > everything.  We shouldn't make the user read stuff that he doesn't need to
> know.
> > 
> > In this case, if a user only wants to build tests that use standard compiler
> > flags, he shouldn't need to know about cxx_test_with_flags.  Therefore I'd
> like
> > to move this function back to the next section, even though it means that we
> > can't define function cxx_test in terms of it.
> 
> First, I don't understand your reasoning about the "user" here. The "user" who
> just wants to build doesn't need to read this file at all - she will use
ccmake,
> read the description of the options, switch some options, and then do a
"make".
> If you're concerned about the level of abstraction, I would propose to put
those
> "lowlevel" methods into a googletest.cmake module and just put the declarative
> part in this file.

A user of gtest doesn't want to just build gtest itself alone.  He needs to
build his own tests that use gtest.  Therefore, a user of the cmake script will
likely need to read and tweak the script such that it can compile his own tests
and link them with gtest.  Many users don't have a lot of experience with cmake.
 Therefore I want to make this script as readable to a cmake-newbie as possible,
within reason.

> The reason I did this is because the method is already non-trivial, and I was
> trying to fix a linking issue (for target_link_libraries, I added a comment),
> and had to do message-debugging to find out that I've been changing the wrong
> method. In general I strongly feel that the duplication is bad here ;-) But of
> course I'll change it back if you insist on putting it back in...

It's not a huge deal.  We can worry about the readability later.

http://codereview.appspot.com/183093/diff/1/2#newcode202
CMakeLists.txt:202: cxx_test_with_flags(gtest-death-test_test ""
On 2009/12/31 10:16:11, klimek wrote:
> On 2009/12/30 17:46:02, wan wrote:
> > The compiler flags shouldn't be empty.  They should be consistent with
what's
> > used to build gtest_main.
> 
> Ah, I see. Thanks for catching.

Do you have a new patch that fixes this?

> > Also, does this work on Linux?  As I remember, I had to add -pthread both
when
> > compiling gtest-death-test_test.cc and when linking gtest-death-test_test.
> 
> So far I only tested it on linux. Yes, it works. The problem you had was
> probably that if you don't use "target_link_libraries", but add the -lpthread
> manually to the linker flags (like you did with -pthread), the -lpthread is in
> the wrong place of the gcc command line, which breaks the linking.

Thanks.  When I get your new patch, I'll test and submit it.

[klimek, 2010-01-04 18:36:00]

On Mon, Jan 4, 2010 at 7:05 PM,  <wan@google.com> wrote:
>
> http://codereview.appspot.com/183093/diff/1/2
> File CMakeLists.txt (right):
>
> http://codereview.appspot.com/183093/diff/1/2#newcode118
> CMakeLists.txt:118: find_package(PythonInterp)
> On 2009/12/31 10:16:11, klimek wrote:
>>
>> On 2009/12/30 17:46:02, wan wrote:
>> > For my education, could you explain what the benefit of this change
>
> is?
>
>> It's the cmake idiom
>> (http://cmake.org/cmake/help/cmake-2-8-docs.html#command:find_package)
>
> for
>>
>> finding installed software. While without any parameters it probably
>
> doesn't
>>
>> matter, people with cmake experience will understand what we're doing
>
> here ...
>
> Thanks for explaining!
>
> http://codereview.appspot.com/183093/diff/1/2#newcode123
> CMakeLists.txt:123: # cxx_test_with_flags(name cxx_flags libs srcs...)
> On 2009/12/31 10:16:11, klimek wrote:
>>
>> On 2009/12/30 17:46:02, wan wrote:
>> > I try to structure the script such that a definition is done right
>
> before it's
>>
>> > first needed (roughly).  The thinking is that there are different
>
> kinds of
>>
>> > users: some only care to build gtest itself, some want to build the
>
> samples
>>
>> too,
>> > some want to build some basic tests for gtest, and yet some want to
>
> build
>>
>> > everything.  We shouldn't make the user read stuff that he doesn't
>
> need to
>>
>> know.
>> >
>> > In this case, if a user only wants to build tests that use standard
>
> compiler
>>
>> > flags, he shouldn't need to know about cxx_test_with_flags.
>
> Therefore I'd
>>
>> like
>> > to move this function back to the next section, even though it means
>
> that we
>>
>> > can't define function cxx_test in terms of it.
>
>> First, I don't understand your reasoning about the "user" here. The
>
> "user" who
>>
>> just wants to build doesn't need to read this file at all - she will
>
> use ccmake,
>>
>> read the description of the options, switch some options, and then do
>
> a "make".
>>
>> If you're concerned about the level of abstraction, I would propose to
>
> put those
>>
>> "lowlevel" methods into a googletest.cmake module and just put the
>
> declarative
>>
>> part in this file.
>
> A user of gtest doesn't want to just build gtest itself alone.  He needs
> to build his own tests that use gtest.  Therefore, a user of the cmake
> script will likely need to read and tweak the script such that it can
> compile his own tests and link them with gtest.  Many users don't have a
> lot of experience with cmake.  Therefore I want to make this script as
> readable to a cmake-newbie as possible, within reason.

I think if a /user/ of gtest wants to build her own tests, she'll just use
find_package(GTest)
See
http://public.kitware.com/cgi-bin/viewcvs.cgi/Modules/FindGTest.cmake?revisio...

I don't really understand yet why I would use a different project's
build infrastructure for my own project, but maybe I'm
misunderstanding something here...

>> The reason I did this is because the method is already non-trivial,
>
> and I was
>>
>> trying to fix a linking issue (for target_link_libraries, I added a
>
> comment),
>>
>> and had to do message-debugging to find out that I've been changing
>
> the wrong
>>
>> method. In general I strongly feel that the duplication is bad here
>
> ;-) But of
>>
>> course I'll change it back if you insist on putting it back in...
>
> It's not a huge deal.  We can worry about the readability later.
>
> http://codereview.appspot.com/183093/diff/1/2#newcode202
> CMakeLists.txt:202: cxx_test_with_flags(gtest-death-test_test ""
> On 2009/12/31 10:16:11, klimek wrote:
>>
>> On 2009/12/30 17:46:02, wan wrote:
>> > The compiler flags shouldn't be empty.  They should be consistent
>
> with what's
>>
>> > used to build gtest_main.
>
>> Ah, I see. Thanks for catching.
>
> Do you have a new patch that fixes this?

I uploaded it before sending out the comments.
I can see the new patch if I go back to the issue and then click on
"View" again...
Is there a different way of uploading than just using the upload script again?

Cheers,
/Manuel

>
>> > Also, does this work on Linux?  As I remember, I had to add -pthread
>
> both when
>>
>> > compiling gtest-death-test_test.cc and when linking
>
> gtest-death-test_test.
>
>> So far I only tested it on linux. Yes, it works. The problem you had
>
> was
>>
>> probably that if you don't use "target_link_libraries", but add the
>
> -lpthread
>>
>> manually to the linker flags (like you did with -pthread), the
>
> -lpthread is in
>>
>> the wrong place of the gcc command line, which breaks the linking.
>
> Thanks.  When I get your new patch, I'll test and submit it.
>
> http://codereview.appspot.com/183093
>



-- 
Manuel Klimek (http://go/klimek)

[wan, 2010-01-04 19:58:24]

On Mon, Jan 4, 2010 at 10:35 AM, Manuel Klimek <klimek@google.com> wrote:
> On Mon, Jan 4, 2010 at 7:05 PM,  <wan@google.com> wrote:
>>
> I think if a /user/ of gtest wants to build her own tests, she'll just use
> find_package(GTest)
> See
http://public.kitware.com/cgi-bin/viewcvs.cgi/Modules/FindGTest.cmake?revisio...

Yes, but she still needs to read gtest's CMakeLists.txt to learn how
to use it.  Since she's likely new to CMake, she'll probably want to
copy and paste snippets from gtest as examples (that's what the
'samples' section is about).  That requires she to have some basic
understanding of the script.  Therefore I'm trying to make the script
easy to understand even for CMake-newbies.

>> Do you have a new patch that fixes this?
>
> I uploaded it before sending out the comments.
> I can see the new patch if I go back to the issue and then click on
> "View" again...
> Is there a different way of uploading than just using the upload script again?

Sorry, I missed it!

-- 
Zhanyong

[wan, 2010-01-05 16:31:38]

This is committed in r360.  Cheers,

2010/1/4 Zhanyong Wan (λx.x x) <wan@google.com>:
> On Mon, Jan 4, 2010 at 10:35 AM, Manuel Klimek <klimek@google.com> wrote:
>> On Mon, Jan 4, 2010 at 7:05 PM,  <wan@google.com> wrote:
>>>
>> I think if a /user/ of gtest wants to build her own tests, she'll just use
>> find_package(GTest)
>> See
http://public.kitware.com/cgi-bin/viewcvs.cgi/Modules/FindGTest.cmake?revisio...
>
> Yes, but she still needs to read gtest's CMakeLists.txt to learn how
> to use it.  Since she's likely new to CMake, she'll probably want to
> copy and paste snippets from gtest as examples (that's what the
> 'samples' section is about).  That requires she to have some basic
> understanding of the script.  Therefore I'm trying to make the script
> easy to understand even for CMake-newbies.
>
>>> Do you have a new patch that fixes this?
>>
>> I uploaded it before sending out the comments.
>> I can see the new patch if I go back to the issue and then click on
>> "View" again...
>> Is there a different way of uploading than just using the upload script
again?
>
> Sorry, I missed it!
>
> --
> Zhanyong
>



-- 
Zhanyong