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

Remove Gazebo Classic support and Update for MoveIt Jazzy/Rolling #228

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Jul 5, 2024

This PR removes Gazebo Classic support, as this breaks builds, and fixes the existing (new) Gazebo support.

For more details, see #217 (comment)

Critically:

@martinleroux
Copy link
Collaborator

Hi @sea-bass ,
Thank you for your contribution. We will examine your pull request as soon as we have a ROS expert available to do so.
In the meantime, I can however tell you that we require Humble builds to pass before we merge.

@sea-bass
Copy link
Contributor Author

sea-bass commented Jul 5, 2024

Hi @sea-bass , Thank you for your contribution. We will examine your pull request as soon as we have a ROS expert available to do so. In the meantime, I can however tell you that we require Humble builds to pass before we merge.

Yep, it seems the gz_ros2_control package is not being picked up by CI. Would appreciate a pointer on how to include repos from source, seems the .repos files didn't do it...

@aalmrad
Copy link
Collaborator

aalmrad commented Jul 5, 2024

Hi @sea-bass,
Thanks for the work you've put into this. After review, we believe that it would be preferable not to delete the Gazebo Classic support entirely. Instead, we would like to keep the support available but couple it with a warning for the users. This will provide an opportunity for those relying on this support to migrate to newer versions at their own pace. Your input has been helpful in identifying this part of the codebase that needs attention and we look forward to more of your contributions in the future.

@sea-bass
Copy link
Contributor Author

sea-bass commented Jul 5, 2024

Hi @sea-bass, Thanks for the work you've put into this. After review, we believe that it would be preferable not to delete the Gazebo Classic support entirely. Instead, we would like to keep the support available but couple it with a warning for the users. This will provide an opportunity for those relying on this support to migrate to newer versions at their own pace. Your input has been helpful in identifying this part of the codebase that needs attention and we look forward to more of your contributions in the future.

Sounds good. In that case, the gazebo_ros2_control package will have to be provided from source by anyone trying to use this package in Jazzy or later.

Of course, it's your choice, but this will make things harder for users to install on these later versions.

My recommendation would be to remove it from the main branch but keep it around on Iron and earlier branches.

@sea-bass sea-bass changed the title Remove Gazebo Classic support Remove Gazebo Classic support and Update for MoveIt Jazzy/Rolling Jul 25, 2024
@moriarty
Copy link
Collaborator

➕ yep probably best to create a branch where classic gazebo support is maintained and let main move forward and drop support.

Gazebo Classic is EOL in few months.

@martinleroux
Copy link
Collaborator

I am reviewing our open PRs. After some thought, I changed my mind and am inclined to agree with @moriarty after all.
Once the builds are fixed, we can merge. @aalmrad , please see that a branch with gazebo-classic is left available outside of main.

@aalmrad
Copy link
Collaborator

aalmrad commented Sep 25, 2024

Hello @sea-bass,

FYI a new branch called Gazebo-Classic-Support was created so that the Gazebo classic support can be removed on the main branch. After fixing the build issues, a merge can be done.

@sea-bass
Copy link
Contributor Author

Fantastic, thank you! Once the builds are back up, I'll take a look to see if anything else is missing in this PR.

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