Skip to content

#44821: include rti-connext-7.3.0 in base.yaml#44918

Merged
nuclearsandwich merged 10 commits intoros:masterfrom
lobolanja:feature/44821-update-connext
Apr 8, 2025
Merged

#44821: include rti-connext-7.3.0 in base.yaml#44918
nuclearsandwich merged 10 commits intoros:masterfrom
lobolanja:feature/44821-update-connext

Conversation

@lobolanja
Copy link
Copy Markdown
Contributor

If you're making a new release with bloom please use bloom to create the pull request automatically (except for the naming review request which must be made manually).
If you've already run the release bloom has a --pull-request-only option you can use.-->

**As I understand it, previous updates to the rti-connext-dds packages were handled by OSRF—we provided the binaries, and they added them to the ROS bootstrap repositories. @mjcarroll can you confirm that? **

Please add the following dependency to the rosdep database.

Package name:

rti-connext-dds-7.3.0

Package Upstream Source:

TODO (waiting clarification from OSRF on how to proceed)

Purpose of using this:

This new package will bring new features, improved stability, and bug fixes compared to rti-connext-dds-6.0.1. We’ve also already had ROS 2 users requesting this update.

Distro packaging links:

Links to Distribution Packages

Please Add This Package to be indexed in the rosdistro.

ROSDISTRO NAME

The source is here:

http://sourcecode_url

Checks

  • All packages have a declared license in the package.xml
  • This repository has a LICENSE file
  • This package is expected to build on the submitted rosdistro

@lobolanja lobolanja requested a review from a team as a code owner March 31, 2025 13:19
@github-actions github-actions bot added the rosdep Issue/PR is for a rosdep key label Mar 31, 2025
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for sending a pull request to ROS distro!

This is an automated tool that helps check your pull request for correctness.
This tool checks a number of attributes associated with your ROS package and generates a report that helps our reviewers merge your pull request in a timely fashion. Here are a few things to consider when sending adding or updating a package to ROS Distro.
ROS Distro includes a very helpful CONTRIBUTING.md file that we recommend reading if it is your first time submitting a package.
Please also read the ROS Distro review guidelines which summarizes this release process.

ROS Distro Considerations

Package Considerations

Having your package included in a ROS Distro is a badge of quality, and we recommend that package developers strive to create packages of the highest quality. We recommend package developers review the following resources before submitting their package.

Need Help?

Please post your questions to Robotics Stack Exchange or refer to the #infra-help channel on our Discord server.


For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ❌ There are problems with explicitly provided platforms:
    • One or more explicitly provided platforms are no longer supported

For changes related to yamllint:

  • ❌ One or more linter violations were added to YAML files

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ❌ There are problems with explicitly provided platforms:
    • One or more explicitly provided platforms are no longer supported

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

…ntains a license file to be used out of the box by ros users
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • 📝 There are problems with the names of new rosdep keys:
    • New key names should typically match the Ubuntu package name
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

@lobolanja
Copy link
Copy Markdown
Contributor Author

@nuclearsandwich do we need to address this PR before RMW freeze?

@mjcarroll
Copy link
Copy Markdown
Member

One approval from me, I'll let @nuclearsandwich get the second.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

rosdep/base.yaml Outdated
Comment on lines +8480 to +8481
ubuntu:
noble: [rti-connext-dds-7.3.0-ros]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In order to maintain forward compatibility we usually invert the definition here:

Suggested change
ubuntu:
noble: [rti-connext-dds-7.3.0-ros]
ubuntu:
'*': [rti-connext-dds-7.3.0-ros]
jammy: null
focal: null

This will allow us to use this package in 26.04 without requiring re-definition. The method by which we currently populate bootstrap repositories will insure this package is propagated to 26.04 when we first create it. However, if the Connext maintainers prefer explicitly defining which distros are supported for each rosdep definition I'm alright with that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let’s keep it as simple as possible. Some of the changes I made were in response to issues reported by GitHub Actions, so I was just trying to keep things compliant with the “tests” until I could get a proper human review. 🙂

Thanks for the detailed explanation @nuclearsandwich

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nuclearsandwich Just a quick question to avoid any issues—can you confirm whether I should use rti-connext-dds-7.3.0 instead of rti-connext-dds-7.3.0-ros in this PR in rti_connext_dds_cmake_module/package.xml?

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

…ros will be keep as a implementation detail)
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • ✅ New rosdep keys are named appropriately
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

use rti-connext-dds-7.3.0-ros since this is the debian package name is going to be installed.

Co-authored-by: Steven! Ragnarök <122943030+nuclearsandwich-ai@users.noreply.github.com>
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • 📝 There are problems with the names of new rosdep keys:
    • New key names should typically match the Ubuntu package name
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

ubuntu:
'*': [rti-connext-dds-6.0.1]
bionic: null
rti-connext-dds-7.3.0:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This key does not match the Ubuntu package name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nuclearsandwich @mjcarroll it is fine tho have this "warning" ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this is fine. I think it's better to stick to the package name historical convention for now.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

For changes related to rosdep:

  • 📝 There are problems with the names of new rosdep keys:
    • New key names should typically match the Ubuntu package name
  • ✅ Platforms for new rosdep rules are valid

For changes related to yamllint:

  • ✅ All new lines of YAML pass linter checks

@nuclearsandwich nuclearsandwich merged commit 6cc35b1 into ros:master Apr 8, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rosdep Issue/PR is for a rosdep key

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants