rosidl_cmake: install interface files to same folder as idl#935
Conversation
fujitatomoya
left a comment
There was a problem hiding this comment.
i think this is reasonable fix and consistent behavior.
IMO, keeping message files under msg/ is NOT a hard requirement for the development.the rosidl_generate_interfaces() CMake macro technically accepts paths with subfolders (like msg/ros2/ldmrs/SickLdmrsObject.msg), so it doesn't reject them at build time. (packages build and the messages work at runtime for publishing/subscribing.) however, the ros2interface tooling assumes the standard layout.
the PR fixes real root cause and provides more consistency that two interfaces can't have the same name within a package anyway, so the subfolder separation provides no real benefit.
lgtm with green CI.
i would like to have another approval from other maintainers before merge.
|
Pulls: #935 |
|
I assume this cannot be back-ported since it changes the file layout. I would work on a patch for |
|
I'm a bit concerned about this change. A lot of downstream tools (e.g., Could you take a look at some |
|
@christophebedard I think there is a misunderstanding here. To clarify, I am using the term Interface file to refer to the
If you want to retrieve the files, you can use get_interface_path, but it only returns the IDL files and not the Interface files. I will provide an example:
The following AMENT resources are generated: The contents of The files are installed as following: On my humble machine I get the following output from python: This shows that installing the interface files into a different folder than the IDL files causes a problem when using these functions. Regarding hard-coding, I assume that packages simply do This pr simply moves the Interface file into the same folder as the IDL file. Thanks for the tip. I realized that the AMENT |
|
@christophebedard friendly ping. |
|
@fujitatomoya, I fixed the CI. I forgot to change the logic at the generation for the AMENT resource. Could you trigger the CI Build again? |
|
@christophebedard @fujitatomoya friendly ping. |
|
@christophebedard can you check the comment on #935 (comment) |
|
@fujitatomoya @christophebedard friendly reminder. I would like to merge it before Lyrical since otherwise we will carry this issue over. |
|
@Mergifyio rebase |
Signed-off-by: Tim Wendt <[email protected]>
|
Deprecation notice: This pull request comes from a fork and was rebased using |
✅ Branch has been successfully rebased |
c54cb40 to
c9673b4
Compare
|
Pulls: #935 |
|
@techtasie thanks for the overview.
Why not change the IDL file generated for a |
|
@christophebedard What would be the advantage of that? There cannot be two interfaces with the same name anyway, and it would break the hardcoded paths some packages use, as I explained in my comment. Furthermore, I find it a bit confusing that it’s only the name of the parent directory and not the same relative path as the source file; this feels unintuitive to me. In the end, it doesn’t matter that much. We just have to make a decision. I prefer my implementation, but the final decision lies with the maintainers. |
|
I agree with you that some flexibility with interface file locations would be nice (and I've actually implemented that in the past), but I'm just concerned about where this is going (not strictly this PR, but others that will probably follow), since the potential for breaking things downstream is relatively high. However, like Tomoya said above, we actually do support "non-standard" interface file paths; it just breaks tools like I won't block this PR if @fujitatomoya is fine with it. I might recommend adding tests if there is still some time left, though. We add test interface files to |
|
I think the Windows build failure is unrelated and should be resolved when ros2/rmw_zenoh#969 gets merged |
|
@christophebedard robag is already testing for these kind of Interface. We could probably steal the test from there. https://github.com/ros2/rosbag2/tree/rolling/rosbag2_test_msgdefs/nested_sub_dir/action I don't get why the aarch build is failing. It looks like the collection of test results failed. But if this pr gets merged there is no difference anymore between these types of messages and they should be indistinguishable. I could add a test to makes sure that an nested interface looks the same as normal ones. |
|
There was another Windows build failure, which has now been fixed by ros2/rmw_connextdds#225, so I retriggered the Windows job. |
Description
Changes the install location for the interface files to
share/${PROJECT_NAME}/<msg/srv/action>. The current logic of installing toshare/${PROJECT_NAME}/${_parent_folder}separates idl and interface files into separate folders which breaksros2 interface show. Furthermore there can't be two interfaces with the same name anyways, therefore separating them into sub folders is not providing any advantages and is just confusing.Fixes #933
Is this user-facing behavior change?
Did you use Generative AI?
ChatGPT 5.2 has been used to explain regex.
Additional Information
Also affects ros2/ros2cli#1186 and RobotWebTools/rclnodejs#1388