Skip to content
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

Some updates to the Melodic REP-0003. #160

Merged
merged 6 commits into from
Mar 27, 2018
Merged

Some updates to the Melodic REP-0003. #160

merged 6 commits into from
Mar 27, 2018

Conversation

clalancette
Copy link
Contributor

Specifically, update the current packages in the distributions,
and also add a note about Gazebo support.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Specifically, update the current packages in the distributions,
and also add a note about Gazebo support.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
rep-0003.rst Outdated
@@ -337,6 +337,8 @@ Requirements:

" ! " means that this package will be at least this version (since these distributions have not yet been released); this may change as those releases get closer.

Note that Gazebo is a special case, since it spans versions 7, 8, and 9. The only long-term supported platform (Bionic) has Gazebo 9, so packages should target that. Packages are encouraged to additionally support Gazebo 7, but it is not required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a matter of wording but Debian Stretch is LTS and supported, isn't it ?
Or has the decision of shipping Gazebo 9 on Debian Stretch been made ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a matter of wording. Debian Stretch is only "recommended" support, not "required" support for Melodic. I'll change the wording to say that Bionic is the only long-term supported required platform, or something to that effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at the wording in be648e8 and let me know if it is clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packages are encouraged to additionally support Gazebo 7, but it is not required.

Gazebo 7 and 8 based on the previous sentence as some "recommended platforms" ship with Gazebo 8 at the moment.

I still think we should confirm where we stand on Debian Stretch (use 7 from upstream or distribute 9 in the ROS apt repo) before considering merging this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the language to say "7 and 8". After talking to @j-rivero , he is pretty confident that we can get Gazebo 9 packages built for Debian Stretch. So I've updated the document to have Debian Stretch be 9.0.0, with a star to indicate it is an OSRF package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j-rivero What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mikael is right, using gazebo9 we won't be able to provide the Debian Stretch users with the option of having Lunar and Melodic desktop-full (or supporting Gazebo at the same time). We have discussed this before but since there was not collision in Ubuntu, we did not find a compromise for the future.

I'm afraid that the option of using gazebo7 for Debian in Stretchwill make the things more difficult since there is two API bumps between versions in Ubuntu/Debian Stretch. I don't know how hard it is the limitation of not having two ROS versions on Debain Stretch but seems to me that i could be better than having to deal with two different versions of Gazebo in ROS Melodic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using gazebo7 [..] will make the things more difficult since there is two API bumps between versions in Ubuntu/Debian Stretch.

Agreed, we discussed it recently and as we didn't know in details how big the API breakage was we didn't settle on it. Note that this issue (of supporting both Gazebo 7 and 9 APIs in Melodic) also exists with Artful that is a platform for which we "require support".

If the consensus is that it'll be too much of a burden for maintainers to support both APIs I suggest the following:

  • Shipping Gazebo 9 on Debian Stretch and add a note to tell user that this ROS version is not side by side installable with the previous one (not sure what the right place for this is but I think it should be made pretty visible as this never happened before AFAIK)
  • Move Artful to the "recommended support" platforms and create separate build files for it to allow people to blacklist it if they don't intend to support it.
  • If we keep Artful, (even only in recommended support), we would need at least desktop-full on all platforms we build debs for. Do you think it's doable to have a version of gazebo_ros_pkgs in melodic that will support both APIs?
    • If that's not the case we may want either:
      • Reconsider Artful support as a whole for Melodic
      • Consider shiping Gazebo 9 on Artful as well

If the API breakage is not significant to justify diverging from upstream:

  • Use upstream on Stretch
  • Use upstream on Artful and keep it in the "required support" section

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's doable to have a version of gazebo_ros_pkgs in melodic that will support both APIs?

The current lunar-devel branch supports gazebo7, gazebo8 and gazebo9 and it will be the base of melodic-devel so yes, we can keep the support if we have a reason for that.

If the API breakage is not significant to justify diverging from upstream:

The API changes between 7.x and 9.x are many:
https://bitbucket.org/osrf/gazebo/src/default/Migration.md?fileviewer=file-view-default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our offline conversations, @j-rivero agreed that he would build Gazebo 9 packages for Ubuntu Artful and Debian Stretch. Thus, I have updated this PR to call for Gazebo 9 across the board (with the exception of Fedora, which is only recommended anyway). I also did a small update to the package versions in Bionic and Fedora. I think this should be ready to review now.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
OSRF will build Gazebo 9 packages for Debian Stretch.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@mikaelarguedas
Copy link
Contributor

Now that we target Gazebo 9 across the board, the note about Gazebo support spanning 7, 8 and 9 should be removed or updated

@clalancette
Copy link
Contributor Author

Now that we target Gazebo 9 across the board, the note about Gazebo support spanning 7, 8 and 9 should be removed or updated

Right, good call. I'll remove it since I don't think we need it anymore.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Copy link
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I didnt double-check the package versions though as I don't think it's necessary

@tfoote
Copy link
Member

tfoote commented Mar 22, 2018

I'd like to revisit the discussion for OpenCV. In particular we updated targets for everything to be 3.4 in #139 In particular comments at #139 (review) #139 (review)

We only pushed ahead to 3.3 based on this request: https://discourse.ros.org/t/opencv-3-3/2674 and that upgrade caused significant issues and debugging time. And we've had several request to fall back to 3.2 during the process. We thought that the new release would be fully backwards compatible but it was not and with the knowledge that I have now, I would have not chosen to make the upgrade if I could choose again.

With 3.2 available for our LTS platform (bionic) and 3.1 on the soon to be EOL'd platform (artful) the only major outlier is that stretch is older and only has 2.4.9.

I'd like to suggest that we fall back to using the system dependencies again instead of packaging OpenCV within ROS packages. On most of our platforms we're within a minor release.

To make things close to compatible I would propose that we backport buster's 3.2 to stretch, and then by July we'll only need to support 3.2 or higher. Which I believe is already available in Fedora and other platforms. And if there's something major blocking on Artful's 3.1 we could backport bionic's 3.2 or blacklist opencv3 on artful as it's very close to EOL.

There are significant advantages to using the upstream packages. In particular we are compatible with other system dependencies that rely on the system opencv installation. There are also standard replacement packages that enable hardware acceleration for systems with CUDA, Neon, etc maintained by the greater community. This would allow ROS users to leverage those builds.

We will also avoid significant build time on the ROS buildfarm, as well as maintaining a parallel custom package which has cost us a lot of time in this last cycle. In the past there's been issues with contrib not being available and the opencv version upstream being very out of date. Also as there's now the same major version of opencv on most platforms we need to be much more careful about declaring conflicts and making sure that our installation and the system one do not collide.

If people really do need to have the latest and greatest version of opencv, replacing the system version is relatively straight forward, it will just require that that any package depending on the custom version of opencv be compile from source. This is standard development processes, and with recent containerization technologies can even be relatively easily shared and reproduced.

I know that there are people that are strongly pushing for the bleeding edge latest things, but a large part of being in a distro is to be stable and solid for those not pushing the envelope. The new features will become available, but to provide the stability of a distro I think that we should not try to significantly push ahead of the upstream packaging.

@mikaelarguedas @vrabaud @clalancette

@clalancette
Copy link
Contributor Author

I'd like to suggest that we fall back to using the system dependencies again instead of packaging OpenCV within ROS packages. On most of our platforms we're within a minor release.

I'm totally fine with this, as I agree the benefits of sticking to the system packages outweigh the benefits of having a newer package available. I'd even go further and suggest that we make sure to stick with whatever version we choose for the lifetime of the distribution, to make sure we don't have a repeat of what happened with OpenCV 3.3.

That all being said, I'm not one of the bleeding edge users that really want newer OpenCV features, so I'd like to hear from @vrabaud before we change this.

@clalancette
Copy link
Contributor Author

By the way, I'm going to merge this PR up so we can get the REP-3 updated properly for Melodic. I'll open a separate issue reiterating @tfoote 's points about OpenCV so that @vrabaud can respond.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants