update to use rosidl_parser and .idl files rather than rosidl_adapter and .msg files

wjwwood commented 3 years ago

This pull request updates the ros1_bridge to use the .idl file versions of messages rather than .msg versions, switching to use rosidl_parser rather than rosidl_adapter respectively.

This also allows the bridge to work with messages that are defined as only .idl files (lacking a .msg version) as well, so long as the messages follow the conventional (<package_name>, msg, <message_name>) triplet.

The changes include:

changing the python scripts used to generate code for the bridge to prefer .idl files over .msg and to call into rosidl_parser rather than rosidl_adapter
update the template files to use the objects returned from rosidl_parser, which differ from rosdil_adapter
- this matches the changes made to most of the type support packages when .idl file support was added originally
change the CMake logic to use the idl files rather than the "interface files" which points to .msg versions and is empty if there is only a .idl version

To test this change I used Apex's fork of Dashing which uses .idl files more often than we do in upstream, and that's why I opened this against dashing first. When merged I'll forward port this to rolling.

wjwwood commented 3 years ago

CI:

Before my changes:
After my changes:

There are test failures due to flake8, but I think they're preexisting.

jacobperron commented 3 years ago

There are test failures due to flake8, but I think they're preexisting.

Yes, for Dashing I believe that is true.

wjwwood commented 3 years ago

Ok, merging thanks!

wjwwood commented 3 years ago

@jacobperron or @sloretz should I do a release for dashing or will you guys batch this into the next release?

sloretz commented 3 years ago

@jacobperron or @sloretz should I do a release for dashing or will you guys batch this into the next release?

Yes please to a release, with one question about the change. Is the Mapping class external API? The add_field_pair() change on the last parameter looks like a breaking change to me - both in the difference in type it accepts and the name change breaking someone calling it like a keyword arg.

I'm a little surprised this went onto dashing first instead of master and being backported.

jacobperron commented 3 years ago

Is the Mapping class external API? The add_field_pair() change on the last parameter looks like a breaking change to me - both in the difference in type it accepts and the name change breaking someone calling it like a keyword arg.

Good point. The class is publicly accessible, so it might be worth patching to be backwards compatible.

wjwwood commented 3 years ago

The Mapping class is externally accessible, but at the time I just assumed it was used only internally. Even after reconsidering it in this new light, I don't see how it could be used externally.

Patching it to be backwards compatible would be very difficult I think, because the objects being taken and added to the mapping class are later used in the template files which have also been changed to expect the new types. Also, it isn't possible to convert between the old types and the new ones that are expected, without writing a bunch of new code which is only used here (i.e. the alternative is converting .msg to .idl and then parsing the .idl files, which is what we do everywhere else AFAIK).

So, I'd actually prefer to either leave this change in (despite breaking public API, on the idea that no one could reasonably be using it) or remove this change in dashing and I go back to Apex to see if a dashing branch could be acceptable. Patching it to be API neutral would be hard to do.

What do you guys think? I'm happy to do either. I think the latter solution (reverting this and having Apex use a custom branch of dashing) is fine, though obviously getting the change in would be preferable.

I'm a little surprised this went onto dashing first instead of master and being backported.

Sorry @sloretz, I know we don't usually do it in that order, but this change was specifically required for Dashing and I had to test against that version so I opened this one first. In retrospect I should have done https://github.com/ros2/ros1_bridge/pull/296 first, sorry about that.

jacobperron commented 3 years ago

I'm alright leaving it in as-is.

sloretz commented 3 years ago

Even after reconsidering it in this new light, I don't see how it could be used externally.

I'm also fine leaving it as is.

wjwwood commented 3 years ago

Ok, I'll do a patch release then, thanks guys.

wjwwood commented 3 years ago

Patch release: https://github.com/ros/rosdistro/pull/27464

ros2 / ros1_bridge

update to use rosidl_parser and .idl files rather than rosidl_adapter and .msg files #292