Upgrade to Rust 1.78

[maspe36]

We are currently on Rust 1.74, and now as of Rust 1.78, the u128 type is now FFI safe. Lets go ahead and upgrade.

Doing this upgrade will help us resolve #320 once and for all. As part of this upgrade, we should also remove the #![allow(improper_ctypes)] and #![allow(improper_ctypes_definitions)] lines from our rcl bindings as we no longer need to ignore these lints.

[Guelakais]

I'm currently working on. Is there a reason why you have set the rust version in cargo.toml for rclrs?

[maspe36]

That rust version is supposed to be the minimum supported version of rust that rclrs would work with. However, our CI does not use this same version so there is a chance that its actually misleading now.

I think with this upgrade to at least Rust 1.78 we'd want to upgrade rclrs to 1.78 in the cargo.toml as well since we would depend on u128 FFI safe functions.

[luca-della-vedova]

Something that we should probably think about is what ROS 2 / Ubuntu version we target and what Rust compiler is shipped with it.
The rustc shipped in Noble is 1.75, so having anything higher than that as a minimum supported version might put us at risk of not being able to build the packages on a stock Ubuntu distro that only relies on Debian packages and doesn't boostrap rustc from rustup.

[mxgrey]

@luca-della-vedova That's a good point. Since Noble is an LTS we should expect that the build farm will be targeting it for quite a while now, and any build farm integration that we hope to accomplish before 26.04 will probably be forced to rely on the version of Rust that shipped with Noble.

It seems the only motivation to go as high as 1.78 is this FFI safety for u128, but even with version 1.78, it's not guaranteed to be safe across all targets. Maybe it's worth putting some time into researching whether there's a more sound way to achieve FFI safety. Especially something that can work with version 1.75.

[maspe36]

Yeah, I think tying our minimum Rust version to what is shipped with Noble is reasonable if it helps reduce friction to get into the build farm.

boostrap rustc from rustup.

FWIW, rustup is also shipped in 24.04. All hope is not lost for moving beyond 1.75. Perhaps as the project matures and we are integrated into the build farm we can look at how to unblock Rust upgrades. The hope is still to have a single version of rclrs target many different ROS distros.

[mxgrey]

It turns out the FFI issue is thornier than we originally realized.

The real origin of the problem is that rosidl_dynamic_typesupport introduces float128 support, even though float128 is not a primitive type that's normally supported by rosidl.

This becomes a problem because bindgen does not properly support long double (float128). These uses of long double in the C API are quietly converted into u128 by bindgen, which is immediately a big problem for us, regardless of the u128 alignment issue that triggers the warning that we've been suppressing.

Currently our dyn_msg module is extremely minimal and doesn't include much of anything to actually inspect messages. I considered whether we can just filter out float128 from all the bindings that we generate to sidestep the error, but this indispensable struct contains a member that references float128, so the warnings would persist even if we try to filter.

My recommendation would be this:

• We rename this issue to something like "dyn_msg float128 support needs bindgen to fix long double"
• We disregard the v1.78 concern because it doesn't address the real problem
• We keep the improper_ctypes warning suppression, perhaps adding a comment placing the blame on the lack of long double support in bindgen
• We don't worry about this problem until someone starts fleshing out the dyn_msg module to really do type inspection
• We probably shouldn't support float128 at all until bindgen fixes long double because we shouldn't take accountability for the ABI issues that could arise

[mxgrey]

After doing even more research on long double, I've reached the conclusion that bindgen shouldn't support interpreting long doubles from a C API, now or ever, because the binary representation of a long double is ambiguous.

Moreover I believe ROS should not support transmitting long doubles for the same reason, and I've opened an issue ticket upstream recommended that they be removed from dynamic type support: ros2/rosidl_dynamic_typesupport#11

If that goes through, the following step would be to update the ROS IDL design doc to explicitly exclude long double from the subset of IDL that ROS supports (it's already not present in the ordinary builtin primitive types).

If that all goes according to plan, we'll be able to just remove the improper_ctypes suppression in the future and forget that float128 was ever a concern.

[esteve]

@mxgrey thank you so much for doing all this research, it'd be great to constraint rosidl_dynamic_typesupport and so we could remove improper_ctypes. Would you like to wait for ros2/rosidl_dynamic_typesupport#11 to be addressed before we update the version of Rust in our CI or would you rather update it now and then remove improper_ctypes in the future?

[mxgrey]

I think we should not increase the minimal version to 1.78 no matter what because it will leave us incompatible with the build farm which will be limited to 1.75 for the next two years or so.

In the PR I've increased it to 1.75 to reflect our need to maintain support for 1.75 for our build farm aspirations, but I don't mind reverting it to 1.74 and putting in a comment to remind us not to go past 1.75.