ros2 / rosidl

Packages which provide the ROS IDL (.msg) definition and code generation.
Apache License 2.0
75 stars 125 forks source link

ROSIDL - Support generating files for fastddsgen without multiple definition errors #767

Open Ryanf55 opened 1 year ago

Ryanf55 commented 1 year ago

Bug report

Required Info:

Steps to reproduce issue

mkdir -p IDL/std_msgs/msg/
# Same for others
cp /opt/ros/humble/share/std_msgs/msg/Header.idl IDL/std_msgs/msg/
cp /opt/ros/humble/share/builtin_interfaces/msg/Time.idl IDL/builtin_interfaces/msg/
cp /opt/ros/humble/share/geographic_msgs/msg/GeoPoint.idl IDL/geographic_msgs/msg/
cp /opt/ros/humble/share/vision_msgs/msg/BoundingBox2D.idl IDL/vision_msgs/msg/
cp /opt/ros/humble/share/vision_msgs/msg/Pose2D.idl IDL/vision_msgs/msg/
cp /opt/ros/humble/share/vision_msgs/msg/Point2D.idl IDL/vision_msgs/msg/

fastddsgen -cs -replace -typeros2 -d gen/builtin_interfaces/msg -I IDL IDL/builtin_interfaces/msg/Time.idl
# Repeat for all the other files

OR, use ros2 run rosidl_adapter msg2idl.py /opt/ros/humble/share/std_msgs/msg/Header.msg to generate the IDL file into a directory if you want to work off the ROS messages, you get the same idl files.

Expected behavior

The generated IDL files do not have naming conflicts.

Actual behavior

[build] /ws/IDL/builtin_interfaces/msg/Time.idl:8:16: error: Illegal identifier: builtin_interfaces::msg::Time is already defined (Definition: com.eprosima.idl.parser.tree.TypeDeclaration@5b239d7d)

Illegal identifier: std_msgs::msg::Header is already defined (Definition: com.eprosima.idl.parser.tree.TypeDeclaration@515aebb0)

Additional information

Causes this downstream issue because the IDL file has conflicts.

https://github.com/eProsima/Fast-DDS-Gen/issues/52

clalancette commented 1 year ago

Just so you know, we don't use fastddsgen when generating code from the IDL files; the ROS 2 core does this itself. So this isn't functionality that is usually used.

That said, if you find a way to make this work we'd be happy to review a fix.

Ryanf55 commented 1 year ago

Just so you know, we don't use fastddsgen when generating code from the IDL files; the ROS 2 core does this itself. So this isn't functionality that is usually used.

That said, if you find a way to make this work we'd be happy to review a fix.

In a quick test, looks like a simple #pragma once at the top of every single ROS-generated IDL solved the problem on the EProsima side. I know from some research RTI supports it. Other vendors will need to be tested.

What's interesting is there are notes about this issue in the OMG IDL spec, but no proposed solution. #pragma once is not anywhere in the OMG IDL DDS spec, so I'm concerned it's not standard.

Thoughts?

clalancette commented 1 year ago

In a quick test, looks like a simple #pragma once at the top of every single ROS-generated IDL solved the problem on the EProsima side. I know from some research RTI supports it. Other vendors will need to be tested.

What's interesting is there are notes about this issue in the OMG IDL spec, but no proposed solution. #pragma once is not anywhere in the OMG IDL DDS spec, so I'm concerned it's not standard.

Yes, that's a definite concern. So we'll have to see what happens with other vendors, particularly CycloneDDS and GurumDDS.

I'll note that without additional work in rosidl, adding this won't have any effect for ROS 2 itself. But if this helps others use fastddsgen and the like outside of ROS 2, then it is definitely something we can consider adding.

emersonknapp commented 1 year ago

To be even more specific, the OMG IDL spec says

IDL shall be preprocessed according to the specification of the preprocessor in ISO/IEC 14882:2003.

That ISO is the C++03 spec, which says that

[A #pragma directive] causes the implementation to behave in an implementation-defined manner. Any pragma that is not recognized by the implementation is ignored.

That all to say, a spec-compliant IDL implementation is not required to implement #pragma once - which is why I never get to use this handy little doodad in my C++ code, "just in case" :)

Ryanf55 commented 1 year ago

To be even more specific, the OMG IDL spec says

IDL shall be preprocessed according to the specification of the preprocessor in ISO/IEC 14882:2003.

That ISO is the C++03 spec, which says that

[A #pragma directive] causes the implementation to behave in an implementation-defined manner. Any pragma that is not recognized by the implementation is ignored.

That all to say, a spec-compliant IDL implementation is not required to implement #pragma once - which is why I never get to use this handy little doodad in my C++ code, "just in case" :)

As far as I can tell, all the compilers for ROS tier 1 platforms support it.

com-server-ap commented 1 month ago

Up to now, the issue has not been resolved. just like Ryanf55 said: add #pragma once at the top of every idl, can ignore the issue.