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

[gazebo-migration] Make gazebo_ros_pkgs optional in nav2_system_tests #4095

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Feb 3, 2024


Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A
Primary OS tested on Ubuntu 22
Robotic platform tested on No
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Leverage common environment variable GZ_VERSION to control which gazebo is linked to in system_tests
  • Add author warning when some system tests are disabled
  • Making gazebo classic optional is one of the first things to help migration
  • This allows users to build all of nav2 with new gazebo installed without any special flags
  • Conditionally skip rosdep for gazebo classic based on the same environment variable
  • This is backward compatible to humble and should not result in any change in behavior for existing CI
  • Allow developers to use new gazebo binaries on their Ubuntu 22.04 OS, build nav2 from source, and not deal with the dependency conflict.

Description of documentation updates required from your changes

  • If you like the proposal, I can add user docs

Future work that may be required in bullet points

  • Continue this pattern to make all of gazebo classic optional in NAV2
  • Port user examples to new gazebo
  • Port system tests to new gazebo
  • Remove gazebo dependencies 11 entirely

Screenshots of warnings when you are missing gazebo classic

These will show up on colcon build if you have the GZ_VERSION environment variable set to something other than an empty string.

image

image

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

* This allows users to build all of nav2 with new gazebo installed without any special flags
* Leverage common environment variable GZ_VERSION
* Add author warning when some tests are disabled
* Making gazebo classic optional is one of the first things to help migration

Signed-off-by: Ryan Friedman <[email protected]>
Copy link

codecov bot commented Feb 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (18e9b2b) 90.25% compared to head (3c5784c) 90.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4095      +/-   ##
==========================================
+ Coverage   90.25%   90.27%   +0.01%     
==========================================
  Files         417      417              
  Lines       18842    18842              
==========================================
+ Hits        17006    17009       +3     
+ Misses       1836     1833       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 5, 2024

I disagree with this charge writ large. The system tests are ... system tests. Not making the simulation tests as part of it defeats much of the purpose of the package. There's no reason AFAIK to do this.

For the GZ migration, the nav2 bringup is largely decoupled from the system tests which will continue to use gazebo classic until we upgrade those separately (but one step at a time). My understandign is that 24.04 will have gazebo classic binaries distributed by someone else, so there's no a step function where they instantly stop working. If there was, then this would make sense as a temporary work around, but I might just prefer to put a COLCON_IGNORE file in instead to disable it all rather than pretending like we have any system tests running, since those remaining tests are not the bulk of them

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Feb 10, 2024

I think it would be helpful to understand what the migration plan for ROS packages is when we have a dependency conflict. The oversight of whoever allowed gazebo classic into Ubuntu 22 and then let the new gazebo in and depend on a conflicting debian package is a disaster.

The PR here proposes a path for a smooth migration plan, but since you aren't a fan... it would be helpful to know how you plan to migrate to new gazebo on rolling. I was expecting a rolling platform shift to Ubuntu noble, which does not come with gazebo classic, but that hasn't happened yet?
By January, 2024 - Rolling platform shift: https://docs.ros.org/en/rolling/Releases/Release-Jazzy-Jalisco.html#release-timeline

@SteveMacenski
Copy link
Member

We have pending PRs for gazebo2 that are not yet complete. That's a stepping stone to get our bringup to use gazebo2 and use those changes as a model to update our tests. The gazebo and gazebo2 bringup files will exist in parallel in Jazzy and probably for the immediately foreseeable future, but the gazebo2 tests will be fully transitioned over once they are stable

@SteveMacenski SteveMacenski added the wontfix This will not be worked on label Feb 13, 2024
@Ryanf55 Ryanf55 deleted the makegazebo-ros-pkgs-optional-in-system-tests branch February 14, 2024 00:30
@azeey
Copy link
Contributor

azeey commented Feb 20, 2024

My understandign is that 24.04 will have gazebo classic binaries distributed by someone else, so there's no a step function where they instantly stop working

@SteveMacenski do you know who would be doing that? I'm not aware of any source for Gazebo classic binaries on 24.04 atm.

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 20, 2024

Canonical / Ubuntu is doing it I was told my OSRF folks a few times. I don't have first hand documentation of that, more general conversations with your team and mentioning frustration with that. Maybe I'm mistaken / misrecalling?

@azeey
Copy link
Contributor

azeey commented Feb 22, 2024

Yeah, it seems like there's some confusion. We've posted https://discourse.ros.org/t/gazebo-classic-end-of-life-ros-2-jazzy/36239 to hopefully clear things up. As stated there, Gazebo Classic binaries will not be available on Ubuntu 24.04.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants