compatibility to urdfdom < 0.4 #317

rhaschke · 2016-10-26T12:58:12Z

Similar patch as moveit/srdfdom#25 for MoveIt. Hopefully solves #18 (comment).

@130s Can you give it a try on Wily? I tested locally on Xenial with manually downgrading urdfdom to 0.3.

davetcoleman

Looks good to me except small requests

davetcoleman · 2016-10-26T13:14:25Z

moveit_core/package.xml

@@ -25,12 +25,12 @@
  <build_depend>eigen_conversions</build_depend>
  <build_depend>eigen_stl_containers</build_depend>
  <build_depend>libfcl-dev</build_depend>
-  <build_depend version_gte="0.5.1">geometric_shapes</build_depend>
+  <build_depend version_gte="0.5.2">geometric_shapes</build_depend>


can we just remove the version for geometric_shapes at this point?

Why should we? This explicitly states our requirements.
Sure, by default, in Kinetic this requirement should be met. But what about people building from source? Where do they find the required version?

Sure, but you could extend that argument to all of MoveIt's many dependencies - why do we need to explicitly specify it for this package? I thought the assumption is always if its missing an API call, get the latest version

Indeed, I would argue to specify the exact version everywhere ;-)

ok, i concede. out of curiosity, does the version_gte throw an error when building from source in a catkin workspace? that would be useful

Sadly it does not :(

Unfortunately, catkin only throws a warning:

catkin_package() version mismatch: the package.xml of 'moveit_core' build_depends on 'geometric_shapes >= 0.5.2', but 'geometric_shapes 0.5.1' found

well a warning is fine, too. i always fix warnings asap.

davetcoleman · 2016-10-26T13:15:51Z

moveit_core/urdfdom_compatibility.h.in

@@ -0,0 +1,78 @@
+/*********************************************************************


Does not seem like this belongs in the root folder of moveit_core, how about moveit_core/macros/include/moveit/macros/ ?

This file is not a usual include, but it's a template to generate the actual include. Hence, it should reside together with the CMakeLists.txt.

I disagree based on other instances of these .h.in files: moveit_resources, moveit_core, geometric_shapes - none have it in their root.

How about you put it in moveit_core/version since this is related to having the wrong version of urdfdom in Wily? I just don't like having a header file in the root of a package. Or if you don't like, a .cmake/ folder?

OK. I will put it into moveit_core/version.

davetcoleman · 2016-10-26T13:17:32Z

moveit_core/CMakeLists.txt

-find_package(urdfdom_headers 0.4 REQUIRED)
+find_package(urdfdom_headers REQUIRED)
+
+### Maintain compatibility to old urdfdom_headers (version < 4.0)


"Remove after Ubuntu Wily support has ended for Kinetic"

davetcoleman · 2016-10-26T13:17:40Z

moveit_core/urdfdom_compatibility.h.in

+*  POSSIBILITY OF SUCH DAMAGE.
+*********************************************************************/
+
+/* Robert Haschke */


"Remove after Ubuntu Wily support has ended for Kinetic"

davetcoleman · 2016-10-26T13:19:28Z

Travis error in moveit_ros_perception:

Errors << moveit_ros_perception:make /root/ws_moveit/logs/moveit_ros_perception/build.make.000.log
In file included from /root/ws_moveit/install/include/moveit/planning_scene/planning_scene.h:40:0,
                 from /root/ws_moveit/src/moveit/moveit_ros/perception/semantic_world/include/moveit/semantic_world/semantic_world.h:42,
                 from /root/ws_moveit/src/moveit/moveit_ros/perception/semantic_world/src/semantic_world.cpp:42:
/root/ws_moveit/install/include/moveit/robot_model/robot_model.h:46:42: fatal error: moveit/urdfdom_compatibility.h: No such file or directory 
compilation terminated.

rhaschke · 2016-10-26T13:32:28Z

I think, the whole commit 8a87c13 should be reverted after Wily support has ended.

davetcoleman · 2016-10-26T15:14:00Z

Sounds good to me - as usual creating an issue would be appropriate to track Wily support

130s · 2016-10-26T15:23:36Z

I can only think of testing this on Wily on virtual machine on this weekend at earliest, if that works.

davetcoleman · 2016-10-26T15:26:33Z

@130s does a prerelease test tell us if it builds on Wily? We'll need it to build on Xenial first through (Travis failed)

rhaschke · 2016-10-26T21:21:55Z

@wjwwood suggested to apply this PR more upstream, in the urdf package: #18 (comment), which I did here: ros/robot_model#160.

wjwwood · 2016-10-26T21:50:11Z

What is the benefit in removing this "after Wily support is dropped in Kinetic"? Why not remove it in Lunar? If you remove this code (or the equivalent now that most of it will be moved to urdf) during the Kinetic cycle you'll lose Debian Jessie support. Perhaps by Lunar you can remove it altogether since hopefully everyone will be using urdfdom 0.4 by then. That would actually be a good thing for you guys to help ensure if you want to remove this code.

130s · 2016-10-26T21:53:29Z

does a prerelease test tell us if it builds on Wily?

As of today I believe ROS prerelease test doesn't have an option to test PRs (it only tests merged commits from devel branch ros-infrastructure/prerelease_website#31).

We'll need it to build on Xenial first through (Travis failed)

+1

moved urdfdom_compatibility.h.in into version folder fixed export of include folder

rhaschke · 2016-10-27T09:13:16Z

I finally fixed the travis issue. Running a prerelease test for Wily now.

v4hn · 2016-10-27T11:22:28Z

What is the benefit in removing this "after Wily support is dropped in Kinetic"? Why not remove it in Lunar? If you remove this code (or the equivalent now that most of it will be moved to urdf) during the Kinetic cycle you'll lose Debian Jessie support

Let's try to support Jessie too. There's no need to keep compatibility with only one version of each system dependency.

lepalom · 2016-10-27T13:29:10Z

I'm sorry, but I have put the changes and it doesn't compile from sources in a Debian Jessie. I got:

In file included from /srv/robotica/ros/src/moveit/moveit_core/robot_model/src/joint_model_group.cpp:38:0:
/srv/robotica/ros/src/moveit/moveit_core/robot_model/include/moveit/robot_model/robot_model.h:76:26: error: ‘ModelInterfaceSharedPtr’ in namespace ‘urdf’ does not name a type
   RobotModel(const urdf::ModelInterfaceSharedPtr &urdf_model,
                          ^
/srv/robotica/ros/src/moveit/moveit_core/robot_model/include/moveit/robot_model/robot_model.h:104:15: error: ‘ModelInterfaceSharedPtr’ in namespace ‘urdf’ does not name a type
   const urdf::ModelInterfaceSharedPtr& getURDF() const
               ^
In file included from /srv/robotica/ros/src/moveit/moveit_core/robot_model/src/joint_model_group.cpp:38:0:
/srv/robotica/ros/src/moveit/moveit_core/robot_model/include/moveit/robot_model/robot_model.h:446:9: error: ‘ModelInterfaceSharedPtr’ in namespace ‘urdf’ does not name a type
   urdf::ModelInterfaceSharedPtr                 urdf_;
         ^
robot_model/CMakeFiles/moveit_robot_model.dir/build.make:123: recipe for target 'robot_model/CMakeFiles/moveit_robot_model.dir/src/joint_model_group.cpp.o' failed
make[2]: *** [robot_model/CMakeFiles/moveit_robot_model.dir/src/joint_model_group.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CMakeFiles/Makefile2:2062: recipe for target 'robot_model/CMakeFiles/moveit_robot_model.dir/all' failed
make[1]: *** [robot_model/CMakeFiles/moveit_robot_model.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2
<== Failed to process package 'moveit_core': 
  Command '['/srv/robotica/ros/install_isolated/env.sh', 'make', '-j2', '-l2']' returned non-zero exit status 2

Any idea?

lepalom · 2016-10-27T13:49:00Z

Sorry, I got an error. I comment the line:

#include<urdf_world/types.h>inmoveit_core/robot_model/include/moveit/robot_model/robot_model.h `

because 'urdf_world/types.h doesn't exist in liburdfdom-headers-dev 0.3.0-1 that it's the version shipped in Debian Jessie. If I put that line it doesn't build on Jessie because that file is not shipped.

Maybe it could be better to provide a backport of liburdfdom-headers-dev (and liburdf) for Jessie?

rhaschke · 2016-10-27T14:11:43Z

I'm still working on those issues. I've got a wily-docker container to test... Latest commit looks promising in wily. Hopefully, it doesn't break xenial.

lepalom · 2016-10-27T14:12:30Z

It worked for me on Jessie. I'm having another issue, but it's a bout fcl. Thanks for work on this.

davetcoleman · 2016-10-27T14:23:13Z

Are you planning on still keeping the compatibility header in this PR, as well as ros/robot_model#160?

rhaschke · 2016-10-27T15:12:40Z

Are you planning on still keeping the compatibility header in this PR, as well as ros/robot_model#160?

To speedup Kinetic release, we could go with our own compatibility workaround for now. As soon as ros/robot_model#160 is released, I can file another PR reverting current changes and proposing simpler ones.

davetcoleman · 2016-10-27T18:48:49Z

I tested this in a Docker container, built successfully

rhaschke added 2 commits October 26, 2016 14:12

require geometric_shapes 0.5.2

96d92aa

compatibility to urdfdom < 0.4

8a87c13

davetcoleman requested changes Oct 26, 2016

View reviewed changes

davetcoleman mentioned this pull request Oct 26, 2016

provide urdf_compatibility.h to define SharedPtr types ros/robot_model#160

Merged

rhaschke added 2 commits October 27, 2016 06:58

fixes

9a863aa

moved urdfdom_compatibility.h.in into version folder fixed export of include folder

fixed installation of urdf_compatibility.h

ddca989

workaround non-existing urdf_world/types.h

e3dc3f7

davetcoleman approved these changes Oct 27, 2016

View reviewed changes

davetcoleman merged commit cbc28f1 into moveit:kinetic-devel Oct 27, 2016

davetcoleman mentioned this pull request Oct 27, 2016

Kinetic Release #18

Closed

19 tasks

rhaschke deleted the urdfdom-compatibility branch October 27, 2016 18:51

rhaschke mentioned this pull request Oct 27, 2016

cleanup urdfdom compatibility #319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compatibility to urdfdom < 0.4 #317

compatibility to urdfdom < 0.4 #317

rhaschke commented Oct 26, 2016

davetcoleman left a comment

davetcoleman Oct 26, 2016

rhaschke Oct 26, 2016

davetcoleman Oct 26, 2016

rhaschke Oct 26, 2016

davetcoleman Oct 26, 2016

de-vri-es Oct 26, 2016

rhaschke Oct 26, 2016

davetcoleman Oct 26, 2016

davetcoleman Oct 26, 2016

rhaschke Oct 26, 2016

davetcoleman Oct 26, 2016

rhaschke Oct 26, 2016

davetcoleman Oct 26, 2016

davetcoleman Oct 26, 2016

davetcoleman commented Oct 26, 2016

rhaschke commented Oct 26, 2016

davetcoleman commented Oct 26, 2016

130s commented Oct 26, 2016

davetcoleman commented Oct 26, 2016

rhaschke commented Oct 26, 2016

wjwwood commented Oct 26, 2016

130s commented Oct 26, 2016

rhaschke commented Oct 27, 2016

v4hn commented Oct 27, 2016

lepalom commented Oct 27, 2016 •

edited by gavanderhoorn

Loading

lepalom commented Oct 27, 2016

rhaschke commented Oct 27, 2016

lepalom commented Oct 27, 2016

davetcoleman commented Oct 27, 2016

rhaschke commented Oct 27, 2016

davetcoleman commented Oct 27, 2016

		@@ -0,0 +1,78 @@
		/*********************************************************************

compatibility to urdfdom < 0.4 #317

compatibility to urdfdom < 0.4 #317

Conversation

rhaschke commented Oct 26, 2016

davetcoleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman commented Oct 26, 2016

rhaschke commented Oct 26, 2016

davetcoleman commented Oct 26, 2016

130s commented Oct 26, 2016

davetcoleman commented Oct 26, 2016

rhaschke commented Oct 26, 2016

wjwwood commented Oct 26, 2016

130s commented Oct 26, 2016

rhaschke commented Oct 27, 2016

v4hn commented Oct 27, 2016

lepalom commented Oct 27, 2016 • edited by gavanderhoorn Loading

lepalom commented Oct 27, 2016

rhaschke commented Oct 27, 2016

lepalom commented Oct 27, 2016

davetcoleman commented Oct 27, 2016

rhaschke commented Oct 27, 2016

davetcoleman commented Oct 27, 2016

lepalom commented Oct 27, 2016 •

edited by gavanderhoorn

Loading