search

ros2 / rcl_interfaces

A repository for messages and services used by the ROS client libraries
Apache License 2.0
40 stars 43 forks source link

lifecycle_msgs: remove non-ASCII chars from field comments #147

Closed gavanderhoorn closed 2 years ago

gavanderhoorn commented 2 years ago

As per subject.

I'm working on an old platform with an ancient version of GCC which isn't capable of processing files with non-ASCII chars in them.

The unicode apostrophes caused build failures, so 48d66fbb removes them.

gavanderhoorn commented 2 years ago

This was never really a problem until we upgraded rosidl_generator_c et al. to post https://github.com/ros2/rosidl/pull/593.

clalancette commented 2 years ago

I'm torn on this one.

On the one hand, there is no reason that this needs to be Unicode; regular ASCII apostrophe will be just fine, and will fix your use case.

On the other hand, we most definitely should support Unicode inside of comments in .msg files. This was a test of that, though more of an implicit one rather than an explicit one.

I'm inclined to take this PR as-is, and deal with testing Unicode in comments in .msg files separately, but I'm not sure. @ros2/team thoughts?

gavanderhoorn commented 2 years ago

On the other hand, we most definitely should support Unicode inside of comments in .msg files. This was a test of that, though more of an implicit one rather than an explicit one.

Didn't https://github.com/ros2/rcl_interfaces/pull/80 already add those?

Edit: oh, hm, you wrote "in comments". #80 does not test comments.

My vote would obviously be: implicit tests are no tests, and fix those missing tests in separate PR(s) ;)

clalancette commented 2 years ago

My vote would be opening an issue on https://github.com/ros2/rosidl/issues about adding tests for non-ascii characters in comments in message files.

That works for me. @gavanderhoorn Would you mind opening up that issue? Then we can run CI on this.

gavanderhoorn commented 2 years ago

See https://github.com/ros2/rosidl/issues/713.

clalancette commented 2 years ago

CI:

gavanderhoorn commented 2 years ago

Thanks for merging.

Would this be back-portable to Humble as well?

clalancette commented 2 years ago

Yeah, that seems reasonable.

@Mergifyio backport humble

clalancette commented 2 years ago

@Mergifyio backport humble

mergify[bot] commented 2 years ago

backport humble

✅ Backports have been created

* [#148 lifecycle_msgs: remove non-ASCII chars from field comments (backport #147)](https://github.com/ros2/rcl_interfaces/pull/148) has been created for branch `humble`