Add catkin package(s) to provide the default version of Gazebo#473
Add catkin package(s) to provide the default version of Gazebo#473meyerj wants to merge 4 commits into
Conversation
| camera_info_manager | ||
| std_msgs | ||
| DEPENDS | ||
| gazebo |
There was a problem hiding this comment.
This line had no effect anyway. catkin_package(DEPENDS ...) is case-sensitive, but gazebo-config.cmake only sets the capitalized GAZEBO_INCLUDE_DIRS and GAZEBO_LIBRARIES variables (@PKG_NAME@ is GAZEBO).
Basically catkin adds ${depend_name}_INCLUDE_DIRS and ${depend_name}_LIBRARIES for each dependency to the exported include directories and libraries for this package (source code in catkin_package.cmake:163). There will be no error or warning message because gazebo_FOUND is set by cmake before. Maybe this is worth a patch in catkin, like adding a check that at least one of the two variables is non-empty or making DEPENDS case insensitive by checking the lower- and upper-case variants, too (@dirk-thomas)? Or at least improve the documentation and mention the case dependency explicitly?
For gazebo_plugins I think that not exporting the Gazebo dependency at all is the better option because simply adding include directories and libraries would not be sufficient anyway.
There was a problem hiding this comment.
Since all CMake variables are case sensitive I don't think it would be good to make anything in catkin case insensitive.
catkin is supposed to already check the arguments and ensure the passed name as been found before. I tried this specific example and the condition checking this was incorrect. ros/catkin#813 addresses this and with it applied the above CMake will result in the intendend error message that gazebo was not found (because gazebo_FOUND is not set).
|
Looks fantastic. I'm not over the moon about the name. I don't know if this is possible, but I pictured that a single package named
That would make the most compelling case for users, if the majority of Gazebo-using ROS packages could simply |
|
Unfortunately having a single wrapper package only for build- and run-time dependencies is not sufficient, at least not for releases. The catkin A derived package like I have not yet verified if my assumptions are true and whether bloom correctly resolves these dependencies but I do not think that there is a way to achieve the same binary package hierarchy without a separate However, we could rename the |
|
The travis and ROS builds failed during |
|
I really like this idea. On a related note that I expect is beyond the scope of this PR, we have also been dealing with the issue of supporting extra gazebo versions through debian packages hosted on http://packages.osrfoundation.org (ie. cc: @j-rivero |
|
I am aware of the alternative gazebo packages in the http://packages.osrffoundation.org repository and we are using them for Gazebo 6 in ROS indigo at Intermodalics with a custom rosdep file. Works quite well. I do not think that this PR would break these alternative packages if the By the way, the latest |
Travis calls The job at http://build.ros.org/ (Ipr__gazebo_ros_pkgs__ubuntu_trusty_amd64/22) does not recognize the transitive build dependency The jobs at http://build.osrfoundation.org/ seem to always install a certain version of Gazebo independent of the declared package.xml dependencies and rosdep. |
|
Thanks Johannes (et all) for the idea and the PR. I think that is a good improvement over the current setup.
Maybe not mandatory but since we have the distinction between build/run-time I think that an user should expect to get the gazebo application as a run-time dependency of gazebo_dev.
AFAIK, we already have two different rosdep keys that map the libgazeboX-dev and gazebo.
The gazebo rosdep key seems to be unique for the different gazeboX (at least for gazebo, gazebo2 and gazebo7). What would be the benefit of this package?
We are building them in the osrfoundation.org build farm (managed by Gazebo team). The process is mainly to overwrite rosdep rules (and some magic to rename packages) and run bloom on top, publishing the results in an alternative -release repos (i.e.gazebo6_ros_pkgs-release). And generate the packages using that metadata. I can not see a problem with the introduction of this new
I understand the same. @dirk-thomas any idea about what we are doing wrong? |
|
@meyerj That looks indeed like a bug in the build farm. While for a single package it makes sense to:
While I will look into a fix for the build farm tomorrow so you shouldn't need to change the dependency type. |
|
I have temporarily modified the PR job to use the branch of the I was wondering why the PR failed in the pre-merge step until I noticed that it currently has merge conflicts. @meyerj Can you please fix them? The following PR build should pass then. |
…e installed Gazebo version
The sdformat library is an indirect dependency of Gazebo and does not need to be linked explicitly.
rosdep install should not complain about packages found in the source-space or in the ROS_PACKAGE_PATH.
1e4e93b to
28bc241
Compare
Apparently I did not fetch the newest versions of the indigo-devel, jade-devel and kinetic-devel branches before I started with this. Rebased now and the job seems to build successfully. Thanks for the quick fix, Dirk!
I added the run-time dependency in 06d2dc3 (indigo), 809c3c0 (jade) and 08b1feb (kinetic).
Yes, but in the general rosdep/base.yaml file. What would be needed is a key which resolves to different binary packages depending on the distribution. rosdep would allow this by adding a tag to the sources list file, but with the current 20-default.list file it only looks at the distribution.yaml file for released ROS packages. Not sure, but I think there is no way to achieve something like without changes to the 20-default.list or even rosdep, the underlying Python packages or the REPs they are based on.
Currently the rosdep keys in base.yaml can only differentiate between OS versions, but not between ROS distributions. If every OS would only support one Gazebo version, there would be no problem. But because for example trusty can run all kind of different versions and the default version depends on whether ROS indigo or jade is installed (or even both), the Releasing a separate package |
|
Thanks for the explanation Johannes. Looks like the rosdep change would imply a good bunch of modifications out of the scope of this PR, so let's focus on merge the changes are they are now. Looks to me that the changes proposed are fully backward compatible with third party code out there, please correct me if you see any risk. My plan:
Sounds good? |
+1 I already linked them above, but here are the diff's for jade and kinetic again: I did not create three separate pull requests, but they should be equivalent. The only differences are the dependencies declared in gazebo_dev/package.xml. |
|
Looks like this was merged in all releases we would want to touch and can close the issue |
…bo version to generic gazebo_dev See also ros-simulation/gazebo_ros_pkgs#571 and ros-simulation/gazebo_ros_pkgs#473.
As a follow-up of the discussion in tu-darmstadt-ros-pkg/hector_gazebo#30 (@mikepurvis, @tfoote, @meyerj), I created a proof-of-concept catkin package
gazebo_dev.The new package contains no code and its main purpose is to depend on the respective ROS distro's default version of the Gazebo development library package. Other packages that want to link to the Gazebo libraries can in turn depend on
gazebo_dev, removing the need to maintain separate branches for different ROS and Gazebo versions as long as the source code is compatible. The current practice of depending ongazebo_rospulls in some unnecessary dependencies (see original discussion).This patch is for ROS indigo and Gazebo 2 and there is no separate
libgazebo2-devpackage/key, but it is quite straight-forward to adapt gazebo_dev/package.xml for newer releases by replacing the<build_export_depend>line with the appropriatelibgazeboX-devrosdep key (jade, kinetic).Secondary, the package provides a cmake script exposed via catkin's CFG_EXTRAS mechanism so that the
gazebo_devpackage can be used in the same way as any other catkin package without the need to find the Gazebo libraries or apply C++ flags explicitly. I modified the other packages in this repository to only depend ongazebo_devand removed the obsolete cmake code. It should be noted that all packages that directly depend ongazebo_devwill compile in C++11 mode because of the addition ofGAZEBO_CXX_FLAGStoCMAKE_CXX_FLAGS. That's why this dependency has not been exported via theCATKIN_DEPENDSkey ofcatkin_package()in thegazebo_pluginspackage. Derived packages have to depend ongazebo_devexplicitly.Possible extensions would be a check in gazebo_dev-extras.cmake whether the found Gazebo version is indeed the expected one or even to provide a way to override the required version by means of cmake variables.
The
gazebo_devpackage does not declare a run-time dependency for the same Gazebo version at the moment. I am not sure if this is required. A rosdep keygazebo_defaultcould be established that points to the correct gazebo binary package, so that derived packages like gazebo_plugins can build_depend ongazebo_devand exec_depend ongazebo_default. Or is there a need for a separate run-time only package with that name, which would provide the per-distrogazebo_defaultkey? LikeThis patch also converts all package manifest to format 2 according to REP 140 to be able to use the new
<build_export_depend>tag. Furthermore it removes the unnecessary dependencymessage_generationfrom packagegazebo_ros.I also created a branch indigo-gazebo-dev in hector_gazebo (diff) which applies a patch to use the new
gazebo_devpackage and which compiles successfully in ROS indigo, jade and kinetic with the respective default Gazebo versions.