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

Remove unused splitting of .srv files in CMake #753

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jun 11, 2023

It used to be that rosidl generators acted on ROS IDL (.msg/.srv/.action) files directly, and generated code from them. This removes now purposeless code that used to be part of that pipeline.

IIUC, how this used to work is the rosidl_generate_interfaces() macro would split .srv files into two: one for the request and one for the response. The generators would then generate code for the split .msg files. Notice in the past the request and response messages were put into a variable called _idl_files, meaning ROS IDL, and which was then passed to the generators.

This code seems to have survived the transition to OMG IDL, but it no longer has a purpose. Notice the split services are now put into a variable called _non_idl_files, (now meaning they're not OMG IDL files). These files aren't passed to the generators. They do get installed - but AFAIK that's just so other's can see what the original ROS IDL of a message was. Nothing actually uses these ..._Request.msg and ..._Response.msg files anymore.

With the OMG IDL pipeline, .srv files are converted to OMG IDL files. The service still gets split into two in a way. rosidl_adapter parses the .srv file to produce a python object representing the request and the response, and then uses one of the .msg templates for the request and response in the OMG IDL file, but the generators downstream only see that as an OMG IDL file containing Request and Response structs.

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Copy link
Contributor Author

sloretz commented Jun 11, 2023

CI (build: all packages test: --packages-above rosidl_cmake)

  • Linux Build Status
  • Linux-aarch64 Build Status (CI machine crashed?) Rerun? all aarch64 nodes are down Build Status
  • Windows Build Status

@sloretz sloretz merged commit 69efae0 into rolling Jun 12, 2023
@delete-merged-branch delete-merged-branch bot deleted the sloretz__remove_service_splitting branch June 12, 2023 18:00
@emersonknapp
Copy link
Collaborator

lol omg i was so confused for 30 mins there just now. I literally just had a test start failing on me because I was using these. I was like "how is something possibly passing on Humble but not in Rolling??". I hadn't pulled locally, and Rpr was fine - but the CI started failing.
ros2/rosbag2#1341

Funny that you did this on the exact week I decided to start depending on it.

I see the motivation to clean up a dangling thing... now I need to reconsider how I'm going to handle getting "implicit subinterface" schemas more generally - I was just going to punt on actions and handle services since the .msg was already there.

achim-k added a commit to foxglove/ros-foxglove-bridge that referenced this pull request Apr 23, 2024
Request and Response message files are not available anymore as of ros2/rosidl#753. This patch parses the service's .srv file instead.
jtbandes pushed a commit to foxglove/ros-foxglove-bridge that referenced this pull request May 3, 2024
### Changelog
Fix service definition parsing on ROS rolling

### Description
Prior to this change, we used the auto-generated `_Request.msg` and
`_Response.msg` message definitions for parsing a service's request and
response definition. However, these message definitions have been
removed in ros2/rosidl#753 causing foxglove
bridge to raise warnings that the service definition could not be found.
This PR changes the service definition parsing to lookup the service
definition using the `.srv` file and splitting the content using the
`===` seperator to obtain the request and response definition.
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.

3 participants