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

Update Ignition Edifice packages using generator script. #146

Merged
merged 5 commits into from
Apr 15, 2022

Conversation

nuclearsandwich
Copy link
Contributor

@nuclearsandwich nuclearsandwich commented Apr 9, 2022

This supersedes #145 which updates version numbers but does not actually match the current packages in the packages.osrfoundation.org repository.

The first commit on this PR, a3e52e8 updates the format of the config file while not changing any contents so that it's easier to diff between the updated, generated content, and the previous content.

I recommend that reviewers use custom compare links to review the changes without formatting differences.

@nuclearsandwich nuclearsandwich requested review from j-rivero and chapulina and removed request for j-rivero and chapulina April 9, 2022 15:00
@nuclearsandwich nuclearsandwich marked this pull request as draft April 9, 2022 15:02
@nuclearsandwich
Copy link
Contributor Author

Currently includes a draft of #147 cherry-picked. I want to rebase that out once #147 is merged.

@nuclearsandwich nuclearsandwich marked this pull request as ready for review April 10, 2022 13:44
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

The version numbers look good to me.

We also need to upgrade ign-cmake2 and ign-math6, which are common between Citadel and Edifice, see https://github.com/ros-infrastructure/reprepro-updater/pull/145/files#diff-e5e0a0f10c0f27937b901ac927c2c6b40354686df642be49ec8c2811db37dbd1. I see you added math6 here. Should it be moved to the Citadel file or should ign-cmake2 be duplicated here?

config/ignition_edifice_ubuntu_focal.yaml Show resolved Hide resolved
@nuclearsandwich nuclearsandwich force-pushed the nuclearsandwich/edifice-updates-2022-04-05 branch from ee0632d to 9b21a9f Compare April 14, 2022 21:48
@@ -2,119 +2,148 @@ name: ignition_edifice_debian_buster
method: http://packages.osrfoundation.org/gazebo/debian-stable
suites: [buster]
component: main
architectures: [amd64, source]
architectures: [amd64, armhf, arm64, source]
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 would like to just standardize on pulling these architectures. Although I could be convinced to give up hope for armhf ever coming back.

Copy link
Contributor

Choose a reason for hiding this comment

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

From time to time Jose tries to fix them, but it's not a priority for us, so I'd be ok removing them if they cause headaches.

Comment on lines 7 to 8
(Package (= ignition-edifice)
), $Version (% 1.0.2-*) |\
(Package (= ignition-edifice) \
), $Version (% 1.0.0-1*) |\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's odd that the current repository appears to have regressed in version compared to what was configured previously.

Looking at the ROS 2 repository: http://repo.ros2.org/ubuntu/main/pool/main/i/ignition-edifice/

It does not appear that ignition-edifice on Buster ever had a successful 1.0.2 import since the import job will not downgrade a package.

ignition-edifice_1.0.0-1~buster.debian.tar.xz      16-Apr-2021 13:57                1300
ignition-edifice_1.0.0-1~buster.dsc                16-Apr-2021 13:57                1582
ignition-edifice_1.0.0-1~buster_amd64.deb          16-Apr-2021 13:57                2084

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not surprised, when Buster builds fail we usually take a long time to go back and fix them.

It does not appear that ignition-edifice on Buster ever had a successful 1.0.2 import

That's one of the reasons I say that these yaml files don't really correspond to the state of the packages. Imports have been failing silently since we started this process 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire purpose of the exercise is to get closer.
If I could find a non-exhaustive way to do it I'd fail the import on a missing match.

Comment on lines 29 to 37
Package (= libignition-gazebo5-plugins)
), $Version (% 5.3.0-*) |\
Package (= libignition-gazebo5-plugins) \
), $Version (% 5.2.0-1*) |\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's another "downgrade" but in this case the imported package is actually still at 5.0.0 so this will affect an upgrade.

ignition-gazebo5_5.0.0-1~buster.debian.tar.xz      16-Apr-2021 13:57                2068
ignition-gazebo5_5.0.0-1~buster.dsc                16-Apr-2021 13:57                1858
libignition-gazebo5-dev_5.0.0-1~buster_amd64.deb   16-Apr-2021 13:57              137996
libignition-gazebo5-plugins_5.0.0-1~buster_amd6..> 16-Apr-2021 13:57             3013128
libignition-gazebo5_5.0.0-1~buster_amd64.deb       16-Apr-2021 13:57             3906496

Comment on lines 53 to 73
(Package (= ignition-physics4) |\
Package (= libignition-physics4) |\
Package (= libignition-physics4-bullet-dev) |\
Package (= libignition-physics4-bullet) |\
Package (= libignition-physics4-bullet-dbgsym) |\
Package (= libignition-physics4-bullet-dev) |\
Package (= libignition-physics4-core-dev) |\
Package (= libignition-physics4-dartsim) |\
Package (= libignition-physics4-dartsim-dbgsym) |\
Package (= libignition-physics4-dartsim-dev) |\
Package (= libignition-physics4-dbgsym) |\
Package (= libignition-physics4-dev) |\
Package (= libignition-physics4-heightmap-dev) |\
Package (= libignition-physics4-mesh-dev) |\
Package (= libignition-physics4-sdf-dev) |\
Package (= libignition-physics4-tpe) |\
Package (= libignition-physics4-tpe-dbgsym) |\
Package (= libignition-physics4-tpe-dev) |\
Package (= libignition-physics4-tpelib) |\
Package (= libignition-physics4-tpelib-dev)
), $Version (% 4.3.0-*) |\
Package (= libignition-physics4-tpelib-dbgsym) |\
Package (= libignition-physics4-tpelib-dev) \
), $Version (% 4.2.0-2*) |\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously configured: 4.3.0
Actually available in the repository: 4.2.0

Currently imported in ROS: 4.1.0

@nuclearsandwich
Copy link
Contributor Author

The CI failure indicates that ign-math6 is missing a suitable source package for the imported versions. Until that is sorted out we've decided not to update the debian buster config or update the import there at all.

@nuclearsandwich nuclearsandwich force-pushed the nuclearsandwich/edifice-updates-2022-04-05 branch from 05d7ccc to ba4d0c1 Compare April 14, 2022 23:58
…adel import only.

Following discussion, with the script to generate these configs it makes
sense to have each one include the entire collection rather than
de-conflict them into disjoint sets.
@nuclearsandwich nuclearsandwich force-pushed the nuclearsandwich/edifice-updates-2022-04-05 branch from 0dee206 to 45c7020 Compare April 15, 2022 00:08
@nuclearsandwich
Copy link
Contributor Author

Due to problems with the way the "Get changed files" action behaved I ended up rebasing out all changes to the Debian Buster configuration.

@nuclearsandwich nuclearsandwich merged commit 0ff57e8 into master Apr 15, 2022
@nuclearsandwich nuclearsandwich deleted the nuclearsandwich/edifice-updates-2022-04-05 branch April 15, 2022 00:10
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.

None yet

2 participants