-
Notifications
You must be signed in to change notification settings - Fork 140
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
Provide a package that pulls the default Ignition version for each ROS distro #186
Comments
I would like to work on this issue.Could you please assign it to me. |
Hi @harshmahesheka , thank you for volunteering! Before jumping into an implementation, it would be good to share a concrete plan explaining what you're thinking about doing. This could be implemented in a few different ways, and it would be good to make sure everyone is onboard beforehand. |
Aside from having an invariant name, what is the difference between The only downside of using I don't think this is a dealbreaker for anything but packages in So this represents a different end of the trade-off between updating maintaining dependency declarations and keeping dependencies as tightly scoped as possible and there's no reason that this shouldn't be an option unless you're of the opinion that there should only be one "right" way to depend on ignition in ROS. |
I thought that I'd seen a response from @harshmahesheka regarding their proposed implementation, which I was going to comment on as well. I no longer see that response however. Was it removed? |
Hey @nuclearsandwich, I think we can have separate packages like ros_ign,ros_ign_gazebo as suggested so, we won't have to source everything but yeah it would create a few more packages. Also, I was trying my approach faced some errors, so I thought I will write a new detailed one. I guess I should have just edited that one. So, my approach was to create a package that will find and set the required ignition package version based on the ROS_DISTRO environment variable directly from CMakeLists.txt (Something similar to https://github.com/ros-simulation/gazebo_ros_pkgs/tree/noetic-devel/gazebo_dev just for ignition gazebo), Also I was thinking can we make an ign_cmake macros for this then we won't need different packages but at the same time project has to depend on ign_cmake. Also, this is a sample code that works for me https://github.com/harshmahesheka/ign-gazebo-dev |
When you say "works for me", and the package itself doesn't include tests can you please elaborate on what you tested and how it worked. Looking at that package, I do not see it accomplishing the goals laid out in this issue and providing more directed comments isn't easy without knowing what you're trying to demonstrate. |
Sorry for the confusion and ambiguity. I have updated the code and mainly there are two parts to it. First,package.xml has
with
in my other ignition package and it finds and sets the required gazebo correctly. Also for the first part, I did rosdep install on the package and also changed |
Hey @nuclearsandwich, if you could review my work that would be really helpful. |
The main advantage would be the invariant name. Think about automatically rolling packages to a new distribution.
Ok, so it sounds like we're leaning towards separate packages. I'd suffix them all with
Something we've talked about along these lines would be to make the metapackages (ign-fortress, @harshmahesheka 's suggested implementation sounds good to me unless @nuclearsandwich thinks this violates some principle of the ROS distributions. |
Yeah, a simple package for each library would be a cleaner way to do it. By this, we can avoid the issue of depending on more than you need and at the same time provide a convenient way for users to maintain and change their versions. |
One idea I have been toying with is exporting targets using CMake/find_package. I think we could have a metapackage
I think you could then downstream users could do something like
Which will both export the targets, as well as the |
I saw something similar in gazebosim/gz-cmake#69 here. It can surely decrease users' work but on the implementation side, I think rather than having a meta-package how about we simply add this argument in ign_find_package. Based on arguments ign_find_package will simply source and set the required variables. |
@harshmahesheka I was actually just discussing a similar proposal (having not seen the one above previously) offline with @chapulina and I think that it has a lot of potential to help with Gazebo usage both within ROS and more generally. With such a CMake helper a package in ROS can decide to declare a dependency on, for example, To resolve the case where different ROS versions target different Ignition versions we could do a couple of things:
|
I have done something similar in a draft pr here #240. Any kind of feedback or review will be very helpful. |
Each ROS distro is paired with a specific Ignition version as described on REP-2000. It's the same for Gazebo Classic.
We make sure that
ros_ign
releases match the official version. But since multiple Ignition rosdep keys may be available to packages across ROS distros (because those distros target the same platforms, i.e. Foxy and Galactic are on Ubuntu Focal), there's the potential for users mixing up versions unintentionally.In order to make downstream maintainers' lives easier, we could provide a package whose sole purpose is to pull the correct Ignition version for a given ROS distro. Another advantage is that as long as the underlying APIs don't change, users can seamlessly upgrade from one ROS distro to another without editing their packages.
Desired behavior
Instead of depending directly on a Gazebo version, i.e.
Depend on the wrapper:
Alternatives considered
ros_ign_math
ros_ign_gazebo
ros_ign_rendering
Implementation suggestion
This suggestion is inspired by the
gazebo_dev
package:https://github.com/ros-simulation/gazebo_ros_pkgs/tree/noetic-devel/gazebo_dev
Here's some history on it:
The text was updated successfully, but these errors were encountered: