-
Notifications
You must be signed in to change notification settings - Fork 938
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
fixing the test utilities in moveit core #1409
Conversation
Of interest to @rhaschke and @BryceStevenWilley |
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.
Originally, libmoveit_test_utils was intended to be used during testing mode only.
That's why it wasn't installed. I'm fine though, to change this if we accept moveit_resources
to become a dependency of moveit_core
.
moveit_core/utils/CMakeLists.txt
Outdated
@@ -1,35 +1,30 @@ | |||
set(MOVEIT_LIB_NAME moveit_utils) | |||
|
|||
find_package(moveit_resources REQUIRED) |
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.
Lifting moveit_resources to the normal build, requires an appropriate dependency change in package.xml
- one reason, why we decided to build this lib in testing mode only.
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.
Good point.
It seems to me that elevating moveit_resources to a normal dependency is inevitable if we want to be able to use libmoveit_test_utils outside of the moveit_core package. At that point, I'm not entirely sure we need moveit_test_utils to be separate from moveit_utils.
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.
libmoveit_test_utils should be definitely different from moveit_utils, as it depends on gtest!
One option to avoid the moveit_resources
dependency, is to find another way (e.g. using rospack
) to find the path to the moveit_resources
package.
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.
The most recent commit keeps moveit_resources as a test_depend and replaces MOVEIT_TEST_RESOURCES_DIR
with ros::packages::getPath(moveit_resources)
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.
Great work! If you replaced all occurrences of MOVEIT_TEST_RESOURCES_DIR
, in future we might want to remove the corresponding generator code from cmake as well, which I always considered clumsy.
f3b47bd
to
298e601
Compare
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.
LGTM. Let's wait for Travis to verify this as well 😉
Great fixup Mike! |
* fixing the test utilities in moveit core * replacing MOVEIT_TEST_RESOURCES_DIR with getPath(moveit_resources)
This fixes an issue with the test utils where it was only getting built during testing and wasn't being exported as a library.
This enables moveit/moveit_tutorials#295 to build and run as expected