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

Add package.xml #413

Merged
merged 11 commits into from
Apr 17, 2024
Merged

Add package.xml #413

merged 11 commits into from
Apr 17, 2024

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Feb 23, 2024

🎉 New feature

Summary

Add package.xml based on dependencies found in .github/ci/packages.

This is to test out using vendor packages to provide Gazebo packages to ROS users (see gazebo-tooling/release-tools#1117).

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Feb 23, 2024
package.xml Outdated Show resolved Hide resolved
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

are package.xml files typically installed?

package.xml Outdated
<?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="2">
<name>gz-cmake3</name>
<version>3.5.0</version>
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have a plan for how to keep this version tag in sync with the version in the CMakeLists.txt.

Some options:

  1. Manually update version numbers in both places and add a CI check to verify consistency (presumably with a shared github action that we will use in other packages)
  2. Make this package.xml version string the single source of truth and add cmake logic to parse the package.xml file to get it (presumably using the existing code that ROS / ament is already using)

I won't block this pull request on that change, but I'd like to at least have an issue open where we continue this conversation

Copy link
Member

Choose a reason for hiding this comment

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

I think option 1 would be easier to implement

Copy link
Member

Choose a reason for hiding this comment

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

add cmake logic to parse the package.xml file to get it (presumably using the existing code that ROS / ament is already using)

I believe the following ament_cmake macro is used to invoke the ament_package python scripts:

Copy link
Member

Choose a reason for hiding this comment

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

I just hacked up a crude github workflow check for consistent versions in the CMakeLists.txt and package.xml file in branch package_xml...package_xml_ci_test

@j-rivero where would you like to see this CI configuration stored?

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 should have mentioned I've added a workflow in some of the other package.xml PRs. See https://github.com/gazebosim/gz-sim/actions/runs/8334724361

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've added an Action in gazebo-tooling/action-gz-ci#70 and updated this PR to use it.

@scpeters
Copy link
Member

are package.xml files typically installed?

for ROS packages, it looks like the package.xml files are installed to {install_prefix}/share/{package_name}/package.xml

@azeey
Copy link
Contributor Author

azeey commented Mar 15, 2024

are package.xml files typically installed?

for ROS packages, it looks like the package.xml files are installed to {install_prefix}/share/{package_name}/package.xml

For the purposes of creating the ROS vendor packages, I don't think we need to install package.xml. In general, I was hoping to avoid any changes to our CMake code to support the package.xml file and instead treat it as a convenient place to put our dependencies (so we can use rosdep) and also as the source of truth for generating/updating vendor packages with automated tools.

@scpeters
Copy link
Member

For the purposes of creating the ROS vendor packages, I don't think we need to install package.xml. In general, I was hoping to avoid any changes to our CMake code to support the package.xml file and instead treat it as a convenient place to put our dependencies (so we can use rosdep) and also as the source of truth for generating/updating vendor packages with automated tools.

sounds good

package.xml Show resolved Hide resolved
package.xml Show resolved Hide resolved
@azeey
Copy link
Contributor Author

azeey commented Apr 5, 2024

gazebo-tooling/action-gz-ci#70 adds validation of the package.xml using ament_xmllint and it seems like dashes (-) are not allowed in <name> tags 😱. Should we switch to underscores for just the package.xml?

@scpeters
Copy link
Member

scpeters commented Apr 5, 2024

gazebo-tooling/action-gz-ci#70 adds validation of the package.xml using ament_xmllint and it seems like dashes (-) are not allowed in <name> tags 😱. Should we switch to underscores for just the package.xml?

from https://ros.org/reps/rep-0149.html#name

The package name must start with a letter and contain only lowercase alphabetic, numeric or underscore characters

but also

The following recommended exemptions apply, which are optional for implementations:

  • Dashes may be permitted in package names. This is to support maintaining a consistent dependency name when transitioning back and forth between a system dependency and in-workspace package, since many rosdep keys contain dashes (inherited from the Debian/Ubuntu name).

so maybe ignore that specific linter complaint?

@azeey
Copy link
Contributor Author

azeey commented Apr 9, 2024

gazebo-tooling/action-gz-ci#70 adds validation of the package.xml using ament_xmllint and it seems like dashes (-) are not allowed in <name> tags 😱. Should we switch to underscores for just the package.xml?

from https://ros.org/reps/rep-0149.html#name

The package name must start with a letter and contain only lowercase alphabetic, numeric or underscore characters

but also

The following recommended exemptions apply, which are optional for implementations:

  • Dashes may be permitted in package names. This is to support maintaining a consistent dependency name when transitioning back and forth between a system dependency and in-workspace package, since many rosdep keys contain dashes (inherited from the Debian/Ubuntu name).

so maybe ignore that specific linter complaint?

This was discussed at today's ROS 2 meeting. The conclusion was that the REP is the authority here. The plan is to relax the naming requirement in the .xsd and potentially provide an opt-in linter that warns against use of dashes. See ros-infrastructure/rep#400.

In the meantime, we can comment out the ament_xml_lint from our CI and merge this.

azeey added 2 commits April 9, 2024 22:14
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey requested a review from j-rivero April 10, 2024 03:19
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey merged commit 116763e into gz-cmake3 Apr 17, 2024
9 checks passed
@azeey azeey deleted the package_xml branch April 17, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants