-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix build when included with cmake_add_subdirectory #32
base: rolling
Are you sure you want to change the base?
Conversation
8365d53
to
474fdb5
Compare
still broken:
|
@nuclearsandwich why do both both this and https://github.com/ament/googletest provide googletest? |
I haven't the foggiest idea. Working backwards from what I do know, I don't believe this package "provides" googletest but rather keeps its own copy as this is a pure CMake package not an ament_cmake package and so does not use the ament set of packages for testing. A quick browse through the package CMake code suggests that this is how googletest upstream recommends that source be included into consuming projects. |
@rotu this package was meant to be used outside of ROS 2 as well as within it, and therefore doesn't depend on ament or the ament mechanisms for googletest. It would be possible for ament to use this, but it's currently not the case. |
I just came across this when cleaning up some other issues, I'll see about incorporating this in the next release. |
Thanks, @wjwwood. For your convenience, I've resolved merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rotu sorry for the delay, but I had one question after re-reviewing it.
@@ -45,6 +45,9 @@ set(OSRF_TESTING_TOOLS_CPP_REQUIRE_GOOGLETEST_VERSION_SETUP) | |||
# @public | |||
# | |||
macro(osrf_testing_tools_cpp_require_googletest) | |||
if (TARGET gtest OR TARGET gtest_main) | |||
return() | |||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? I assume there was a duplicate target, but honestly I'm not sure this is the right way to handle it. If the target already exists then either osrf_testing_tools_cpp_require_googletest()
was called more than once (which is explicitly not allowed) or it was created by another piece of CMake logic, which is also bad, since it might not have the version osrf_testing_tools_cpp_require_googletest()
would have given.
Currently breaks when built as a sub-project. This fixes that.