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

Use Gazebo ROS vendor packages #277

Merged
merged 8 commits into from
May 7, 2024
Merged

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Apr 23, 2024

Changes:

  • Update all package.xml and CMakeLists files to use Gazebo vendor packages that are now available in rolling and the upcoming jazzy.
  • Update Github Actions to run on Ubuntu Noble
  • Clean up code where both ign and gz headers were supported.

Changes:
* Update all package.xml and CMakeLists files to use Gazebo vendor
packages that are now available in rolling and the upcoming jazzy.
* Update Github Actions to run on Ubuntu Noble
* Clean up code where both ign and gz headers were supported.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey requested a review from ahcorde as a code owner April 23, 2024 21:12
@christophfroehlich
Copy link
Contributor

ros-rolling-ros-gz-sim is still not released, or is there an other issue with the config?

@azeey
Copy link
Contributor Author

azeey commented Apr 24, 2024

ros-rolling-ros-gz-sim is still not released, or is there an other issue with the config?

Yeah, I think that's the only issue.

@azeey azeey mentioned this pull request Apr 24, 2024
@azeey
Copy link
Contributor Author

azeey commented Apr 24, 2024

ros-rolling-ros-gz-sim is not available on ros2-testing.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

There are some errors on CI, do you mind to take a look ?

@azeey
Copy link
Contributor Author

azeey commented Apr 30, 2024

Has there been a release of ros2_control into rolling since ros-controls/ros2_control#1256? That seems to be the issue here.

@christophfroehlich
Copy link
Contributor

Has there been a release of ros2_control into rolling since ros-controls/ros2_control#1256? That seems to be the issue here.

yes, today ;) ros/rosdistro#41012

Copy link

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

That change looks like it would also need a README update.

Apart from that I tested building this locally on ubuntu noble with the testing repo enabled and did a quick test using ur_simulation_gz against this.

@azeey
Copy link
Contributor Author

azeey commented May 6, 2024

That change looks like it would also need a README update.

I've updated the README to add Jazzy. Let me know if there's a specific change you had in mind.

@azeey azeey requested a review from ahcorde May 6, 2024 21:33
@fmauch
Copy link

fmauch commented May 7, 2024

That change looks like it would also need a README update.

I've updated the README to add Jazzy. Let me know if there's a specific change you had in mind.

Yes, these are the changes that I had in mind, thank you for updating this. Currently, I find it a bit troublesome to find out which "rolling" means "rolling an noble" and which means "rolling on jammy, we haven't updated our docs, yet". The proposed changes make that clear in my opinion.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey
Copy link
Contributor Author

azeey commented May 7, 2024

I've fixed flake8 failures in affdb76. CI should hopefully be green now. Please take a look.

@ahcorde ahcorde merged commit 18fd2c7 into ros-controls:master May 7, 2024
4 checks passed
@azeey azeey deleted the gz_vendor_pkgs branch May 7, 2024 20:26
@christophfroehlich
Copy link
Contributor

@azeey will the vendor packages be released for distros < Jazzy as well? Up to this PR we were able to compile the rolling version of the ros2_control stack on iron and humble, but now this obviously fails https://github.com/ros-controls/ros2_control_ci/actions/runs/8994845511

@azeey
Copy link
Contributor Author

azeey commented May 9, 2024

We're currently not planning to release the vendor packages for earlier versions. If there is a lot of interest, we might consider releasing them, but they would require more work, so I don't think it will be anytime soon.

@azeey
Copy link
Contributor Author

azeey commented May 9, 2024

Up to this PR we were able to compile the rolling version of the ros2_control stack on iron and humble

I wasn't aware of this. I don't see a clean way to accomplish this with the vendor packages. In general, I was under the impression that best practice was to have a branch for each ROS version and backport changes if necessary. This is at least what the packages in the ROS core do. So I assumed master would only support Jazzy when it came out.

@christophfroehlich
Copy link
Contributor

Up to this PR we were able to compile the rolling version of the ros2_control stack on iron and humble

I wasn't aware of this. I don't see a clean way to accomplish this with the vendor packages. In general, I was under the impression that best practice was to have a branch for each ROS version and backport changes if necessary. This is at least what the packages in the ROS core do. So I assumed master would only support Jazzy when it came out.

You are right and in principle we use this scheme. But we wanted to have the opportunity to use new features in the LTS distros (which are the only ones used by the industry) without the extra work on backporting them with proper deprecations/without any ABI/API breaks. This is why we added these CI checks to see if we break our code to be compilable on the older LTS distros. This is now the case with this PR (which is fine for rolling/jazzy and makes totally sense).

I tried to just import all the vendor packages but it fails on dependencies not available on jammy ros-controls/ros2_control_ci#77 What would be necessary to add the vendor package at least for the humble LTS? (we can move the discussion to the ros2_control_ci 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