Skip to content
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

Add GTest 1.8 compatibility and reduce duplicated code #914

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

v-lopez
Copy link
Contributor

@v-lopez v-lopez commented Jan 24, 2018

No description provided.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the very quick PR.

After reviewing the patch I only have one comment.

@@ -389,13 +325,12 @@ if(NOT GMOCK_FOUND)
endif()
_install(${ARGN})
endfunction()
add_subdirectory(${gtest_base_dir} ${gtest_lib_dir})
add_subdirectory(${base_dir} ${gtest_lib_dir})
set(_CATKIN_SKIP_INSTALL_RULES FALSE)
set_target_properties(${gtest_libs} ${gtest_main_libs}
PROPERTIES EXCLUDE_FROM_ALL 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property also needs to be set for the gmock targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I have ammended the commit to include this change.

@v-lopez v-lopez force-pushed the gtest-1.8-compatibility branch from d7aa040 to b5ffdba Compare January 24, 2018 17:30
@dirk-thomas
Copy link
Member

This looks good in a small workspace on Melodic. Thank you again for the patch!

@dirk-thomas dirk-thomas merged commit 8686d19 into ros:kinetic-devel Jan 24, 2018
"${_gmock_source_paths}" _gmock_found
_gmock_base_dir _gmock_include_dir _gmock_lib_dir
_gmock_libs _gmock_main_libs)
if(gmock_found)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line is incorrect. See #919.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants