Galactic: Support Citadel, Edifice and Fortress #8

chapulina · 2022-06-03T00:39:22Z

Builds on top of [Galactic] Backport github actions CI #11

My original intention was to make the Galactic branch compatible with Fortress. While at it, I also added Edifice support and CI for different combinations.

😱 Galactic + Citadel

I noticed that this package was released into Galactic compiled against libSDFormat 9, which is the version used on Gazebo Citadel. Although libSDFormat doesn't have "Ignition" or "Gazebo" in its name, it is one of Gazebo's libraries, and is versioned with it. Therefore, its use should follow the dependency requirements dictated by REP-2000, which pairs Galactic with Gazebo Edifice, and therefore libSDFormat 11.

I do understand that this is all not trivial for a ROS maintainer to follow though. The long-term solution is to only expose the correct SDF version through rosdep keys. This may only be possible if/when there's only one ROS distro per Ubuntu distro. In the short term, maybe some documentation and rosdistro gatekeeping could help?

Summary of changes

Support choosing the libSDFormat version using the GAZEBO_VERSION variable, similar to how it's done on packages like ros_ign and ign_ros2_control.
Kept Citadel as the default for backwards compatibility
Added Edifice, which should have been the default with Galactic. Even though it is EOL, ROS bindings will be supported until Galactic's EOL
Added Fortress, which is an LTS and I know several users have been using the Galactic + Fortress combination
~~Added GitHub actions CI - but since I don't have write access to this repository, I think it won't run unless someone enables it~~ Moved to [Galactic] Backport github actions CI #11
I also narrowed some SDF includes in some tests while debugging some issues, and left them here. They're not strictly necessary, but it reduces compilation time.

sloretz

This looks great! I'm happy to see the Galactic PR job passes too.

Would you be willing to target this at the ros2 branch? I'd prefer to merge there first and then backport to Galactic.

chapulina · 2022-06-03T18:26:05Z

Would you be willing to target this at the ros2 branch? I'd prefer to merge there first and then backport to Galactic.

What ROS versions does the ros2 branch support? Is it for Humble and Rolling? I can target a separate PR there for review, but it will look different from this.

Meanwhile, would you be ok enabling GitHub actions on this repo? I have no ⚡ here.

sloretz · 2022-06-03T18:30:45Z

What ROS versions does the ros2 branch support? Is it for Humble and Rolling?

Rolling, and I would branch off it for Humble.

but it will look different from this.

What would be different about it? The default SDFormat version?

sdformat_urdf/CMakeLists.txt

sloretz · 2022-06-03T18:38:23Z

Meanwhile, would you be ok enabling GitHub actions on this repo? I have no zap here.

Still trying to figure out how to do this :)

chapulina · 2022-06-03T18:51:54Z

Still trying to figure out how to do this :)

It should be here:

https://github.com/ros/sdformat_urdf/settings/actions

chapulina · 2022-06-03T18:53:00Z

What would be different about it? The default SDFormat version?

Yup, and I wouldn't bother with ifdefs to support SDF <= 11, unless you think that needs to be supported

sloretz · 2022-06-03T18:56:58Z

It should be here:

Already seems to be enabled. Is there a PR specific setting? We could try merging the workflow in a separate PR and see if the actions show up on this one.

Yup, and I wouldn't bother with ifdefs to support SDF <= 11, unless you think that needs to be supported

I wouldn't mind if the ifdefs were still there for older versions.

chapulina · 2022-06-03T18:58:04Z

Ok, let me open a separate PR for actions, against ros2

chapulina · 2022-06-03T19:26:31Z

Ok, let me open a separate PR for actions, against ros2

I wouldn't mind if the ifdefs were still there for older versions.

Ok, there are a few ways we can go about this.

Setup CI for the ros2 branch using SDF9 (Citadel), which is currently there.
- We don't ship Jammy binaries for Citadel, as it's not a supported version. So CI would need to also compile several dependencies from source, if that even works.
Setup CI for the ros2 branch, while updating it for SDF12, the version on Fortress, which is the official combination with Humble (and Rolling at the moment).
1. Keeping ifdefs for SDF9
2. Changing code to SDF12

I'd prefer to go with 2.ii. My preference would be to not support versions that aren't supported by upstream. Otherwise, how far back should we go? And if a code path isn't being tested on CI, it's hard to say it is supported, so we might as well remove it so it doesn't become rotten innards.

But I'd like to check if you have a preference first, @sloretz ?

chapulina · 2022-06-03T20:05:24Z

@sloretz , see option 2.ii in Fix Humble / Rolling build #9

sloretz · 2022-06-03T20:15:06Z

We don't ship Jammy binaries for Citadel, as it's not a supported version. So CI would need to also compile several dependencies from source, if that even works.

I agree we don't need to test Citadel on Jammy. Can CI be set up to only test libsdformat versions that are available on a platform?

But I'd like to check if you have a preference firs

I'd vote for adding .github/... files to ros2 first so we can try merging and seeing if jobs show up on the next PR.

I think the ros2 branch tests should be with SDFormat versions we might see on Jammy and Focal. The reason for Focal is it's a Recommended support platform for Humble and Rolling. I'd be happy to take care of backporting the CI PR to galactic (removing the Jammy tests).

My preference would be to not support versions that aren't supported by upstream. Otherwise, how far back should we go? And if a code path isn't being tested on CI, it's hard to say it is supported, so we might as well remove it so it doesn't become rotten innards.

You have a good point about upstream Gazebo not supporting all Gazebo releases available. I see this from the perspective of this package uses libsdformat independent of Gazebo, and it would like to build on Focal and Jammy. It could be built from source against versions 6, 9, 10, 11, and 12 on Focal (6 lacks features this package needs so it can be ruled out) and version 12 on Jammy. With that perspective I'd like to see Jammy tests that test version 12 and Focal tests that test versions 9-12.

The GAZEBO_VERSION logic seems like a nice helper for using consistent versions on the ROS buildfarm, but I don't think it's presence should mean sdformat_urdf supports a specific version of Gazebo.

chapulina · 2022-06-03T21:07:38Z

this package uses libsdformat independent of Gazebo

I'm using "Gazebo" with its new meaning, what we would refer to as "Ignition" until recently. - SDFormat is an Ignition / Gazebo library, despite it not being on the name. All SDFormat releases are tied to a Gazebo release, see SDF docs and Gazebo docs.

I don't think it's presence should mean sdformat_urdf supports a specific version of Gazebo.

Can you elaborate on what you mean here? Not sure if its a reiteration of the Gazebo vs SDFormat confusion above or a different point.

Can CI be set up to only test libsdformat versions that are available on a platform?

You mean minus EOL versions, right? SDF 10 and 11 are dead.

I'd vote for adding .github/... files to ros2 first

Yeah let's iterate on Fix Humble / Rolling build #9

The reason for Focal is it's a Recommended support platform for Humble and Rolling.

Ok, fair enough, so what are all the combinations we want to test on the ros2 branch?

~~Focal + Citadel (SDF 9) + Humble~~ Clarified in CI for Focal and Jammy #10
~~Focal + Citadel (SDF 9) + Rolling~~ Clarified in CI for Focal and Jammy #10
~~Focal + Fortress (SDF 12) + Humble~~ Clarified in CI for Focal and Jammy #10
~~Focal + Fortress (SDF 12) + Rolling~~ Clarified in CI for Focal and Jammy #10
Jammy + Fortress (SDF 12) + Humble
Jammy + Fortress (SDF 12) + Rolling

sloretz · 2022-06-03T22:40:19Z

Can you elaborate on what you mean here? Not sure if its a reiteration of the Gazebo vs SDFormat confusion above or a different point.

I think it's a reiteration of the above. If I understand correctly, it sounds like you're saying someone wouldn't normally say their package supports versions 9 through 12 of libsdformat in the same way someone wouldn't normally say they support version 2.x through 9.x of rclcpp. They would say they support Gazebo Citadel through Fortress or ROS Foxy through Galactic.

chapulina · 2022-06-03T22:57:33Z

I think it's a reiteration of the above. If I understand correctly, it sounds like you're saying someone wouldn't normally say their package supports versions 9 through 12 of libsdformat in the same way someone wouldn't normally say they support version 2.x through 9.x of rclcpp. They would say they support Gazebo Citadel through Fortress or ROS Foxy through Galactic.

Yeah it's like you'd say a package supports ROS Foxy through Humble instead of speaking in terms of the version of one specific dependency like rclcpp.

I see where you're coming from though. Since this repository cares explicitly about SDFormat, I think it wouldn't be unreasonable to talk in terms of SDF versions, although that does bring specific versions of other Gazebo libraries, which are defined according to the distribution.

For my specific use case, I'm compiling this in a workspace that also has ros_ign, so I found it simpler to use a single env var to configure all packages at once (ignore the fact that ros_ign still uses IGNITION_VERSION because it hasn't been migrated yet). As more packages in the ROS ecosystem start depending on Gazebo libraries and want to support multiple versions, I think that standardizing the env var would make everyone's lives easier.

* CI for Focal and Jammy Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Only run on pull_request updates Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Signed-off-by: Louise Poubel <louise@openrobotics.org>

sdformat_urdf/CMakeLists.txt

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina · 2022-06-06T21:36:01Z

CI is 🟢 for all versions 🚀

sloretz

Only comments I have are nitpicks about the documentation.

README.md

quarkytale

Works for me with the buoy project! Downloads the correct libraries (libsdformat12) and supports sdf version 1.8.

sloretz · 2022-06-08T00:14:45Z

changing the target branch to galactic so I don't accidentally close it.

Signed-off-by: Louise Poubel <louise@openrobotics.org> Co-authored-by: Shane Loretz <shane.loretz@gmail.com>

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina · 2022-06-08T00:32:02Z

Thanks, @sloretz , I've addressed all your comments and hopefully fixed the Gpr job 🤞🏽

sloretz · 2022-06-08T16:08:34Z

Thanks for iterating!

chapulina mentioned this pull request Jun 3, 2022

Plugins to view the buoy on RViz osrf/mbari_wec#6

Merged

sloretz mentioned this pull request Jun 3, 2022

sdformat12 support #7

Closed

sloretz reviewed Jun 3, 2022

View reviewed changes

sdformat_urdf/CMakeLists.txt Outdated Show resolved Hide resolved

chapulina mentioned this pull request Jun 3, 2022

Fix Humble / Rolling build #9

Closed

sloretz and others added 2 commits June 3, 2022 16:09

CI for Focal and Jammy (#10)

230eea3

* CI for Focal and Jammy Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Only run on pull_request updates Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Focal;Galactic;Citadel,Edifice,Fortress

a268207

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

chapulina mentioned this pull request Jun 3, 2022

[Galactic] Backport github actions CI #11

Merged

chapulina added 2 commits June 3, 2022 16:31

Galactic: Support Citadel, Edifice and Fortress

34c08d1

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Remove custom action

037f6ba

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina changed the base branch from galactic to sloretz__ci_galactic June 3, 2022 23:32

chapulina added 3 commits June 3, 2022 16:42

cascading logic

34b6c75

Signed-off-by: Louise Poubel <louise@openrobotics.org>

gazebo -> gz

c5afdc9

Signed-off-by: Louise Poubel <louise@openrobotics.org>

oops

6071003

Signed-off-by: Louise Poubel <louise@openrobotics.org>

sloretz reviewed Jun 3, 2022

View reviewed changes

sdformat_urdf/CMakeLists.txt Outdated Show resolved Hide resolved

chapulina mentioned this pull request Jun 6, 2022

Framework to view the buoy on RViz osrf/mbari_wec_gz#50

Merged

Reorder things

55e0b38

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina added 3 commits June 6, 2022 13:02

Fix tests with Edifice

a9f3948

Signed-off-by: Louise Poubel <louise@openrobotics.org>

no gz-utils for SDF 9

7a7f856

Signed-off-by: Louise Poubel <louise@openrobotics.org>

uncrustify

f0d5e64

Signed-off-by: Louise Poubel <louise@openrobotics.org>

sloretz reviewed Jun 7, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

quarkytale reviewed Jun 7, 2022

View reviewed changes

sloretz changed the base branch from sloretz__ci_galactic to galactic June 8, 2022 00:14

chapulina and others added 2 commits June 7, 2022 17:29

Apply suggestions from code review

1ac4b88

Signed-off-by: Louise Poubel <louise@openrobotics.org> Co-authored-by: Shane Loretz <shane.loretz@gmail.com>

Citadel

6fae5e8

Signed-off-by: Louise Poubel <louise@openrobotics.org>

sloretz approved these changes Jun 8, 2022

View reviewed changes

sloretz merged commit cc26b1f into ros:galactic Jun 8, 2022

chapulina deleted the chapulina/galactic/fortress branch June 8, 2022 16:41

chapulina mentioned this pull request Jul 8, 2022

Support Gazebo Garden #17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Galactic: Support Citadel, Edifice and Fortress #8

Galactic: Support Citadel, Edifice and Fortress #8

chapulina commented Jun 3, 2022 •

edited

Loading

sloretz left a comment

chapulina commented Jun 3, 2022

sloretz commented Jun 3, 2022

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 3, 2022

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 3, 2022

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022 •

edited

Loading

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 6, 2022

sloretz left a comment

quarkytale left a comment

sloretz commented Jun 8, 2022

chapulina commented Jun 8, 2022

sloretz commented Jun 8, 2022

Galactic: Support Citadel, Edifice and Fortress #8

Galactic: Support Citadel, Edifice and Fortress #8

Conversation

chapulina commented Jun 3, 2022 • edited Loading

😱 Galactic + Citadel

Summary of changes

sloretz left a comment

Choose a reason for hiding this comment

chapulina commented Jun 3, 2022

sloretz commented Jun 3, 2022

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 3, 2022

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 3, 2022

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022 • edited Loading

sloretz commented Jun 3, 2022

chapulina commented Jun 3, 2022

chapulina commented Jun 6, 2022

sloretz left a comment

Choose a reason for hiding this comment

quarkytale left a comment

Choose a reason for hiding this comment

sloretz commented Jun 8, 2022

chapulina commented Jun 8, 2022

sloretz commented Jun 8, 2022

chapulina commented Jun 3, 2022 •

edited

Loading

chapulina commented Jun 3, 2022 •

edited

Loading