Provide a package that pulls the default Ignition version for each ROS distro

chapulina commented 3 years ago

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>ignition-gazebo5</depend>

Depend on the wrapper:

<depend>ros_ign_dev</depend>

Alternatives considered

Keep the status quo: depending on specific versions of the simulator
Instead of offering a single package that installs all of Ignition, we could have one per library, i.e.
- 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:

harshmahesheka commented 2 years ago

I would like to work on this issue.Could you please assign it to me.

chapulina commented 2 years ago

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.

nuclearsandwich commented 2 years ago

Aside from having an invariant name, what is the difference between ros_ign_dev and the ignition-fortress package? Does ignition-fortress not include all dev dependencies?

The only downside of using ros_ign_dev for any ROS packages which depend on any part of Ignition is that it establishes a pattern of depending on more than you strictly need. A package which only needs ignition-math and ignition-cmake doesn't need the entire array of physics backend plugins to build but if the ros_ign_dev package is going to depend on the entire ignition suite then those packages would all be be pulled in by that dependency.

I don't think this is a dealbreaker for anything but packages in ros_core and desktop which should be kept as small as possible including only the packages that are actually needed to meet those variants.

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.

nuclearsandwich commented 2 years ago

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?

harshmahesheka commented 2 years ago

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

nuclearsandwich commented 2 years ago

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.

harshmahesheka commented 2 years ago

Sorry for the confusion and ambiguity. I have updated the code and mainly there are two parts to it. First,package.xml has <depend condition="$ROS_DISTRO == foxy">ignition-gazebo6</depend> which will make the package depend on required ign-gazebo version based on Ros distribution and other packages can depend on it.Secondly, ignition_gazebo_dev-extras.cmake under cmake find and set required ign gazebo package so, other packages can simply go find_package(ign-gazebo-dev REQUIRED) instead of finding ign-gazebo which will make their packages furthermore compatible with different ROS distros. For testing, I have replaced

find_package(ignition-gazebo6 REQUIRED)
set(IGN_GAZEBO_VER ${ignition-gazebo6_VERSION_MAJOR})

with

find_package(ign-gazebo-dev REQUIRED)

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 <depend>ignition-gazebo6</depend> with <depend>ros_ign_dev</depend> on my other packages and it gives expected behaviour.You can check the code at https://github.com/harshmahesheka/ign-gazebo-dev .Also if I am still wrong at my interpretation of the issue please guide me in what it means.

harshmahesheka commented 2 years ago

Hey @nuclearsandwich, if you could review my work that would be really helpful.

chapulina commented 2 years ago

Aside from having an invariant name, what is the difference between ros_ign_dev and the ignition-fortress package? Does ignition-fortress not include all dev dependencies?

The main advantage would be the invariant name. Think about automatically rolling packages to a new distribution.

downside of using ros_ign_dev for any ROS packages which depend on any part of Ignition is that it establishes a pattern of depending on more than you strictly need.

we can have separate packages like ros_ign,ros_ign_gazebo as suggested

Ok, so it sounds like we're leaning towards separate packages. I'd suffix them all with _dev and probably keep them in a nested directory so they don't pollute the root of this repository. Also, considering we're renaming everything to gz soon, this should be implemented as ros_gz_<lib>_dev so we don't need to migrate.

I was thinking can we make an ign_cmake macros for this

Something we've talked about along these lines would be to make the metapackages (ign-fortress, ign-garden...) export the IGN_<LIBNAME>_VER CMake variables. That could then be used by ROS. But it would suffer from the issue of depending on more than you need.

@harshmahesheka 's suggested implementation sounds good to me unless @nuclearsandwich thinks this violates some principle of the ROS distributions.

harshmahesheka commented 2 years ago

But it would suffer from the issue of depending on more than you need.

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.

mjcarroll commented 2 years ago

Something we've talked about along these lines would be to make the metapackages (ign-fortress, ign-garden...) export the IGN__VER CMake variables. That could then be used by ROS. But it would suffer from the issue of depending on more than you need.

One idea I have been toying with is exporting targets using CMake/find_package.

I think we could have a metapackage gz, and export subcomponents such as:

gz::utils
gz::utils::cli
gz::common
gz::common::graphics

I think you could then downstream users could do something like

find_package(gz VERSION fortress REQUIRED COMPONENTS utils utils::cli)

Which will both export the targets, as well as the <LIBNAME>_VER variables

harshmahesheka commented 2 years ago

find_package(gz VERSION fortress REQUIRED COMPONENTS utils utils::cli)

I saw something similar in https://github.com/ignitionrobotics/ign-cmake/issues/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.

nuclearsandwich commented 2 years ago

@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, gazebo-fortress in its package.xml and then use that CMake workflow to initialize the dependency targets.

To resolve the case where different ROS versions target different Ignition versions we could do a couple of things:

We could have a metapackage, ros_gazebo_dev for example, which has conditional dependencies for each ROS version.

<!-- SOON <depend condition="$ROS_DISTRO == rolling">ignition-garden</depend> -->
<depend condition="$ROS_DISTRO == rolling or $ROS_DISTRO == humble">ignition-fortress</depend>
<depend condition="$ROS_DISTRO == galactic">ignition-edifice</depend>
<depend condition="$ROS_DISTRO == foxy">ignition-citadel</depend>

harshmahesheka commented 2 years ago

We could have a metapackage, ros_gazebo_dev for example, which has conditional dependencies for each ROS version.

I have done something similar in a draft pr here https://github.com/ignitionrobotics/ros_ign/pull/240. Any kind of feedback or review will be very helpful.

gazebosim / ros_gz