-
Notifications
You must be signed in to change notification settings - Fork 104
provide urdf_compatibility.h to define SharedPtr types #160
Conversation
Thanks @rhaschke, I can use this in rviz too. |
@@ -12,11 +12,24 @@ find_package(TinyXML REQUIRED) | |||
|
|||
catkin_package( | |||
LIBRARIES ${PROJECT_NAME} | |||
INCLUDE_DIRS include ${TinyXML_INLCLUDE_DIRS} | |||
INCLUDE_DIRS include ${TinyXML_INLCLUDE_DIRS} ${CATKIN_DEVEL_PREFIX}/include |
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.
Looks like CI is choking on this line. Are you sure it is necessary? I guess you want to do something like what gencpp
does for the generated message headers:
https://github.com/ros/gencpp/blob/indigo-devel/cmake/gencpp-extras.cmake.em#L49-L57
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.
This line is necessary to export the include path in devel-space context. However, I indeed missed to create that folder beforehand. I guess, it is created with configure_file()
, but I moved the whole block past catkin_package()
because of the use of CATKIN_PACKAGE_INCLUDE_DESTINATION
.
@wjwwood what kind of delay will we see in releasing this package into Kinetic? I'd like to get moveit released quickly and moving this PR here from moveit/moveit#317 seems like a potential big delay |
I'll make a release as soon as this patch is merged to expedite the process, though I would like to test this patch out with rviz before releasing to ensure it is sufficient. |
For MoveIt, it doesn't suffice to declare SharedPtr types in the compatibility header. At some places, MoveIt already includes |
With the latest commit, MoveIt seems to compile on Wily: I essentially removed all includes of |
@wjwwood I'm just building rviz on Wily with this patch. Some changes are required, which I will file as a PR as soon as build is successful. |
Testing this now. |
Cool it works for me in a Wily docker building rviz with this patch and ros-visualization/rviz#1064. Thanks again @rhaschke! I'll merge this now and start a release. |
I did a release with this pr just now: ros/rosdistro#13092 |
@wjwwood could we get this compatibility header released in indigo too? |
If someone does the pr to the right branch I'll look at it next time I spend time on |
* provide urdf_compatibility.h to define SharedPtr types * create exported include directory * include compatibility header in model.h
Wily ships with urdfdom 0.3, which doesn't yet declare SharedPtr types. To allow compatibility of Kinetic packages, which typically already rely on those types, we ship a
urdf_compatibility.h
header.@wjwwood suggested to apply this PR here, in the very first ROS package using urdfdom, and not in downstream packages like MoveIt: moveit/moveit#18 (comment).
Downstream packages simply need to
#include <urdf/urdf_compatibility.h>
.