rclcpp icon indicating copy to clipboard operation
rclcpp copied to clipboard

rclcpp: check rcl version [humble]

Open stan-guer opened this issue 2 years ago • 4 comments

Since commit f87d548f95db4165525bd94e4d96b56b3c5478cc, rclcpp requires rcl_clock_time_started from rcl which is only availble since rcl version 5.3.4 for humble

Fixes issue #2249 for humble

stan-guer avatar Jul 30 '23 12:07 stan-guer

At a quick look, the approach seems syntactically correct. However if we do it for rcl, we should likely do it for a lot of other packages (and keep doing it in the future, I don't think that this is the first time that this happens).

Overall, I think that this is a larger change than it looks, and it's probably not justified without a larger commitment and discussion on how to apply it project-wise.

alsora avatar Jul 31 '23 18:07 alsora

However if we do it for rcl, we should likely do it for a lot of other packages (and keep doing it in the future, I don't think that this is the first time that this happens).

No, it is definitely not the first time. That said, it is somewhat rare in stable distributions, as we don't often backport features that require changes across both rcl and rclcpp.

Overall, I think that this is a larger change than it looks, and it's probably not justified without a larger commitment and discussion on how to apply it project-wise.

Yes and no (in my opinion). On the one hand, people taking piece-wise ROS package updates is a definite anti-pattern. On the other hand, this is a relatively light-weight way to enforce it in the packages. I don't think we should do this across the distribution (that would be way too much metadata to update all the time), but I'm OK doing it on a case-by-case basis.

clalancette avatar Jul 31 '23 18:07 clalancette

On the one hand, people taking piece-wise ROS package updates is a definite anti-pattern.

basically this could be one of the anti-pattern from distributor perspective, but i do not take this minority. Once it comes to production phase, developer would update the necessary packages only to avoid the instability from other packages in their own CI/CD. in case of this issue, that is rclcpp.

if we expect this is anti-pattern for user application, we need to add version dependencies to let the user know.

fujitatomoya avatar Jul 31 '23 19:07 fujitatomoya

On the one hand, people taking piece-wise ROS package updates is a definite anti-pattern.

To shortly interject here: this was not done by a person, our CI/CD server ran rosdep install ... --reinstall ... and one of the steps decided to update rclcpp, since it's a c++ codebase. I understand though that putting versions everywhere where they're missing and maintaining that correctly is daunting. I'm not sure where this lands on the spectrum of stability vs continuous development of ROS2.

stan-guer avatar Jul 31 '23 21:07 stan-guer