-
Notifications
You must be signed in to change notification settings - Fork 84
Patch subfolder regex to support hyphens #1389
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates the ROS2 ament-install path parsing logic in rosidl_gen to correctly handle package subfolders that include hyphens (fixing the mrpt_msgs msg-ros2 case reported in #1388).
Changes:
- Broadened the ament-path regex used to extract the package name from file paths.
- Broadened the ament-path regex used to extract the interface subfolder from file paths (now supports hyphens).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (amentExecuted) { | ||
| return filePath.match(/\w+\/share\/\w+\/(\w+)\//)[1]; | ||
| return filePath.match(/[^/]+\/share\/[^/]+\/([^/]+)\//)[1]; | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a regression test for ament-index paths where the interface subfolder contains a hyphen (e.g., .../share/mrpt_msgs/msg-ros2/Foo.msg). Current test suite exercises findPackagesInDirectory, but doesn’t guarantee coverage for hyphenated subfolders, so this could regress without CI catching it.
| if (amentExecuted) { | ||
| return filePath.match(/\w+\/share\/(\w+)\//)[1]; | ||
| return filePath.match(/[^/]+\/share\/([^/]+)\//)[1]; | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ament path parsing regex is duplicated between getPackageName and getSubFolder. Consider centralizing the parsing (e.g., split on /share/ and then on /) to reduce the chance of future divergence when path rules change again.
|
Hi @techtasie Thanks for submitting the PR, can you please also add a test for the For your reference:
Please ignore the errors for the rolling build. |
|
@minggangw, while investigating some issues with other packages (SICKAG/sick_scan_xd#547), I guess this is what happens when you run Even with the fixed .msg files and The same error occurs for mrpt_msgs, as you can see in my test commit. The problem seems to be that there are now multiple .msg files in two locations: one next to the .idl file and one in the parent folder at the root of the share directory. It is still a reference error since the files are in different folders, with the additional issue that After seeing all this, I think it will probably have to be fixed in the ROS IDL generator. If I will keep you posted! |
|
@techtasie I check the current failures on arm64 platform, it's because we will test all messges in rclnodejs/test/test-message-object.js Lines 43 to 51 in 70cfe1f
The problem is GraphSlamAgent doesn't has .idl and the test failed finally as it reports:
Error: Cannot find module '../../generated/mrpt_msgs/mrpt_msgs__msg__GraphSlamAgent.js' |
|
@minggangw i just installed the mrpt msgs into the arm64 package to test out if the build fails on the action too. The package they are using is this one: But when I extract it: I find these files just in a different folder. That is what I meant to say with the reference is broken. |
Public API Changes
None
Description
MRPT puts messages into a subfolder containing a hyphen.
#1388