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

Fix same named types overriding typesources #759

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

emersonknapp
Copy link
Collaborator

When two interface types in the same package had the same name stem (e.g. msg/Empty.msg and srv/Empty.srv), both types would receive the same TypeSources in their generated description. Can be considered undefined behavior, but it's processed alphabetically so the msg consistently received the srv sources.

This fixes it, adds a regression test, and adds a new basic test for .idl files just to make sure that continues to work OK.
Bug exists in Iron, this would need to be backported.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
rosidl_pycommon/rosidl_pycommon/__init__.py Outdated Show resolved Hide resolved
@@ -34,6 +34,7 @@ if(BUILD_TESTING)
${test_interface_files_ACTION_FILES}
${test_interface_files_MSG_FILES}
${test_interface_files_SRV_FILES}
msg/BasicIdl.idl
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be totally clear, this is an unrelated test, correct? I'm actually fine with adding it in here for testing purposes, but I just want to make sure I'm not missing something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just wanted to make extra sure, here in rosidl repo, that the new changes didn't break anything for .idl files, which are handled slightly differently as they don't go through rosidl_adapter. The intention for this extra file is to serve that purpose, since there were not yet any .idl files being tested by this package.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good with green CI.

@emersonknapp
Copy link
Collaborator Author

Pulls: #759
Gist: https://gist.githubusercontent.com/emersonknapp/67c942266b93571ebb90a33aef5d2ddc/raw/a35bc75291715af8472773be5c927306a1474c62/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12433

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@emersonknapp emersonknapp merged commit 89c6713 into rolling Jul 26, 2023
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/fix-same-name-types branch July 26, 2023 05:01
@emersonknapp
Copy link
Collaborator Author

@Mergifyio backport iron

@mergify
Copy link

mergify bot commented Jul 26, 2023

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 26, 2023
* Fix same named types overriding typesources

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
(cherry picked from commit 89c6713)
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