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

Ignition Edifice #109

Merged
merged 4 commits into from
Apr 15, 2021
Merged

Ignition Edifice #109

merged 4 commits into from
Apr 15, 2021

Conversation

chapulina
Copy link
Contributor

Part of ros/rosdistro#28960

What I did:

  • Copied files from Citadel
  • Removed Gazebo 11, as well as libraries that are common between Citadel and Edifice (cmake2, math6, tools, plugin1)
  • Bumped the versions for all the others

Some things I noticed, but wasn't sure how to handle:

  • Focal is not including the ignition-citadel metapackage, so I didn't include ignition-edifice. I see the package has been uploaded though: http://packages.ros.org/ros/ubuntu/pool/main/i/ignition-citadel/. I wonder if the config file is wrong and the metapackage has been uploaded in an earlier version. Should I include ignition-edifice this time?
  • The format Package, Version | Package, Version is used in some places, and the format Package Package, Version is used in others. Which one is preferred? The 2nd seems easier to maintain.
  • There's a mismatch of sub-packages between Focal and Buster sometimes. For example, there are 6 packages for transport10 on Buster and only one on Focal. Should all 6 be added to Focal, or is there some globbing going on?

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I'm not sure why Aptly Importer / Bionic Aptly CONFIG_TESTER - debug true failed

@nuclearsandwich
Copy link
Contributor

* Focal is not including the `ignition-citadel` metapackage, so I didn't include `ignition-edifice`. I see the package has been uploaded though: [http://packages.ros.org/ros/ubuntu/pool/main/i/ignition-citadel](http://packages.ros.org/ros/ubuntu/pool/main/i/ignition-citadel/). I wonder if the config file is wrong and the metapackage has been uploaded in an earlier version. Should I include `ignition-edifice` this time?

Especially if there is a rosdep entry for the metapackage, let's make sure it is included in all import configurations. If there's no rosdep entry for it IMO it does not need to be included unless not including it would cause interference with the Ubuntu version of the metapackage.

* The format `Package, Version | Package, Version` is used in some places, and the format `Package  Package, Version` is used in others. Which one is preferred? The 2nd seems easier to maintain.

We've definitely been refining how we use the filter formula format. Package | Package, Version as seen in

((Package (% =libignition-common3) |\
Package (% =libignition-common3-av) |\
Package (% =libignition-common3-av-dev) |\
Package (% =libignition-common3-core-dev) |\
Package (% =libignition-common3-dev) |\
Package (% =libignition-common3-events) |\
Package (% =libignition-common3-events-dev) |\
Package (% =libignition-common3-graphics) |\
Package (% =libignition-common3-graphics-dev) |\
Package (% =libignition-common3-profiler) |\
Package (% =libignition-common3-profiler-dev)), \
$Version (% 3.11.1-1~focal)) |\
is the currently preferred format.

* There's a mismatch of sub-packages between Focal and Buster sometimes. For example, there are 6 packages for `transport10` on Buster and only one on Focal. Should all 6 be added to Focal, or is there some globbing going on?

I think buster is out of date because we were sprinting to get the focal configuration in shape for a deadline and then haven't yet returned to update buster. So at some point we should do that and focal is definitely the recommended template between the two.

@nuclearsandwich nuclearsandwich self-assigned this Apr 15, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

Thanks for the feedback, updated Focal in d4ccaff

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 focal config looks good to me.

The buster config is missing some source packages as identified by CI and I think it would ideally be updated to the same Package | Package, $Version format as the focal config.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I copied Focal into Buster and CI looks happy now

@nuclearsandwich nuclearsandwich merged commit 151d14e into master Apr 15, 2021
@nuclearsandwich nuclearsandwich deleted the chapulina/edifice branch April 15, 2021 23:37
@nuclearsandwich
Copy link
Contributor

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.

2 participants