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.

Sign up for GitHub

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

Jump to bottom

Import Citadel gz packages #168

Merged
merged 7 commits into from
Aug 25, 2022
Merged

Import Citadel gz packages #168

merged 7 commits into from
Aug 25, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Aug 17, 2022 •
edited by nuclearsandwich
Loading

Summary:

  • Add all gz counterparts to ignition packages
  • Bump to the latest releases. ⚠️ I've included some versions that are staged to be released, but not there yet:
  • Also removed libraries from Fortress that are duplicates of Citadel, only for Focal. I think it makes sense to have a single source of truth.
  • I didn't update Buster on purpose, because that's EOL and we haven't been making new releases for it

Here's the current status of Citadel source packages on packages.osrf (this doesn't check that the gz packages exist, I've done that manually 🙃 ):

image

@chapulina
Import Citadel gz packages
0688282 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
syntax
4921fb5 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
syntax
b74510f 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
rendering 3.6.0-5
707f505 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
This was referenced Aug 17, 2022
Merged
Merged
@nuclearsandwich nuclearsandwich marked this pull request as ready for review August 17, 2022 23:04
@nuclearsandwich
Copy link
Contributor

  • Also removed libraries from Fortress that are duplicates of Citadel, only for Focal. I think it makes sense to have a single source of truth.

I agree that single-source of truth is an important objective.
The reason that there is duplication is that the unit-of-change for these configs is the individual config file so when updating a citadel-on-focal or fortress-on-focal if we want to make sure that each package is it is on the OSRF repos matches the import we need to break sole-source-of-truth to satisfy atomic unit-of-change. And I judge the latter to be more important.

Another possibility is to refactor all gazebo suites into a single config file for each (OS,version) combination so that packages shared among them are not duplicated but this would mean that we'd have to stabilize the OSRF repos an (OS,version) at a time rather than (OS,version,gazebo suite).

nuclearsandwich
nuclearsandwich requested changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Overall this is very close to what the generator script produces. I caught a couple of discrepancies which need to be resolved and I'm going to recommend not changing the fortress config although I understand the motivation behind it I think there is an overriding reason to leave it as is (described in thread).

config/ignition_citadel_gazebo11_ubuntu_focal.yaml Outdated Show resolved Hide resolved
config/ignition_citadel_gazebo11_ubuntu_focal.yaml
Package (= libgz-math6) |\
Package (= libgz-math6-dbg) |\
Package (= libgz-math6-dev) |\
Package (= libgz-math6-eigen3-dev) |\
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Package (= libgz-math6-eigen3-dev) |\
Package (= libgz-math6-eigen-dev) |\

According to the introspection performed by the generator script there is no package named libgz-math6-eigen3-dev but the libignition-math6-eigen3-dev package does use eigen3 instead of eigen. @j-rivero do you know if this is a typo / omission or if the plan is to rename the libgz... 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.

I thought I had fixed that

gazebo-release/gz-math6-release@3b85559

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, eigen3 should be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

The generator script is still finding the bare eigen package name on Focal. Looking at the package pool it appears that only jammy has the corrected package name.
eigen

config/ignition_fortress_ubuntu_focal.yaml Outdated Show resolved Hide resolved
config/ignition_fortress_ubuntu_focal.yaml Outdated Show resolved Hide resolved
config/ignition_fortress_ubuntu_focal.yaml Outdated Show resolved Hide resolved
config/ignition_fortress_ubuntu_focal.yaml Outdated Show resolved Hide resolved
j-rivero and others added 2 commits August 18, 2022 21:24
@j-rivero @nuclearsandwich
Update config/ignition_citadel_gazebo11_ubuntu_focal.yaml
86c794e 
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
@j-rivero
Revert changes in Fortress
adf0dce 
See reason at:
#168 (comment)

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
nuclearsandwich
nuclearsandwich requested changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

The eigen / eigen3 issue appears to've been fixed for jammy only so the focal package still lists plain eigen. Once that is corrected this is good to go (assuming no other releases happen in the meantime).

@j-rivero j-rivero mentioned this pull request Aug 24, 2022
Merged
@j-rivero
Copy link
Contributor

The eigen / eigen3 issue appears to've been fixed for jammy only so the focal package still lists plain eigen. Once that is corrected this is good to go (assuming no other releases happen in the meantime).

Yup, they are buggy right now. Fix in gazebo-release/gz-math6-release#17. I'll release a -3 and update this PR. Thanks for checking.

@nuclearsandwich
Copy link
Contributor

I didn't know that magic comments could close PRs across organization boundaries. Reopened after it was (presumably unintentionally) closed by a comment in gazebo-release/gz-math6-release#17

@j-rivero
Copy link
Contributor

I didn't know that magic comments could close PRs across organization boundaries. Reopened after it was (presumably unintentionally) closed by a comment in gazebo-release/gz-math6-release#17

Ouch, totally unintentionally yes.

@j-rivero
Copy link
Contributor

The data matches the output of the script in #144

@j-rivero j-rivero merged commit 68f838a into master Aug 25, 2022
@j-rivero j-rivero deleted the chapulina/citadel_gz branch August 25, 2022 21:30
@j-rivero
Copy link
Contributor

After import to bootstrap, I've checked the status of that repo in https://build.osrfoundation.org/job/ignition_citadel-install-pkg_ros_bootstrap-any-manual/2/ . Code coming from gazebo-tooling/release-tools#801. Seems fine to my eyes.

@j-rivero
Copy link
Contributor

Imported https://build.ros2.org/job/import_upstream/417/

@nuclearsandwich
Copy link
Contributor

Imported https://build.ros2.org/job/import_upstream/417/

In general please run import jobs for build.ros.org and build.ros2.org together so that future build.ros.org import jobs don't have excess changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich requested changes

@j-rivero j-rivero Awaiting requested review from j-rivero

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chapulina @nuclearsandwich @j-rivero