Use find_package(Threads) when building with cmake (issue183093)
kli...@google.com
Message:
First draft, will fix formatting after I test it on Windows tonight ;-)
Please review this at http://codereview.appspot.com/183093
Affected files:
M CMakeLists.txt
w...@google.com
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.
kli...@google.com
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.