Make middleware selection more independent of build-time package availability

[rotu]

Defer list of available RMWs to build time of downstream packages so you can build a dependent package even if RMW_IMPLEMENTATION is set to a value not available when the rmw_implementation package was built.

Remove CMake Config flag "RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING".
Add CMake Config flag "RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION" so you can statically link a middleware, even if multiple present at build time. This value can be explicitly set, but by default preserves the behavior of "statically link if one middleware found".

Add a scary message if the user tries to change the implementation and built with RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION=ON

[rotu]

Alternative to #65

[rotu]

I’m not 100% sure, with the changes in this PR, if it’s still necessary to list middlewares in package.xml.

https://answers.ros.org/question/332359/why-does-rmw_implementation-have-a-build-dependency-on-certain-middleware-packages/

[dirk-thomas]

As mentioned on the other ticket (#65 (comment)) imo it should be preserved that when only a single RMW implementation is present that by default rmw_implementation statically links against it. Since this patch changes that behavior I don't think that is a desired.

[rotu]

I believe that change is desirable. Behavior-changing optimizations such as direct vs indirect loading of RMW should be explicit rather than implicit.

The previous behavior is unintuitive (#58) and undocumented.

If I’m tuning performance in real life, I’m going to want to compare multiple RMWs, and that means probably (1) having them checked out in my workspace at compile time or installed via apt, even if I’m only using one (2) toggling poco with the same RMW.

[dirk-thomas]

I believe that change is desirable.

I disagree on this. If a user builds from source with a single RMW impl the most common case is that they want to use only that RMW impl. and then it is imo preferred that it performs the static linking since the user shouldn't "pay for" what they don't use / need.

(1) having them checked out in my workspace at compile time or installed via apt

For this the case of a single RMW impl at build time doesn't apply.

So in which scenario are you using a binary which was built with a single RMW implementation and then want to add other RMW implementations later? All binaries by Open Robotics use more than one RMW impl.

[rotu]

since the user shouldn't "pay for" what they don't use / need

That’s the thing: you can’t tell when rmw_implementation is built whether the user will need dynamic linking when rmw_implementation is actually used in an overlay.

The previous behavior is an example of premature optimization.

In any case, if you install via apt, all users are forced to “pay for” this feature since the OSRF binaries are built with multiple typesupports.

So in which scenario are you using a binary which was built with a single RMW implementation and then want to add other RMW implementations later?

A hypothetical: I’ve built my underlay to zoom under FastRTPS, and I’d like to try out CycloneDDS. Now I install FastRTPS alongside CycloneDDS, and inadvertently either change the FastRTPS performance by linking differently or accidentally compare statically linked FastRTPS to dynamically linked CycloneDDS.

[rotu]

Not to mention if you rosdep install, you get the other RMWs! So the previous behavior seems to discourage meaningful use of rosdep.

[dirk-thomas]

That’s the thing: you can’t tell when rmw_implementation is built whether the user will need dynamic linking when rmw_implementation is actually used in an overlay.

Correct. That is why we have to make a decision at built time which might not be the right one in some cases. My point is that I think the current strategy aim to do the right thing for the common use cases.

In any case, if you install via apt, all users are forced to “pay for” this feature since the OSRF binaries are built with multiple typesupports.

Not to mention if you rosdep install, you get the other RMWs! So the previous behavior seems to discourage meaningful use of rosdep.

We simply decided that providing support for multiple RMW impl. in the provided binary packages is beneficial to the majority of the community.

I’ve built my underlay to zoom under FastRTPS, and I’d like to try out CycloneDDS. Now I install FastRTPS alongside CycloneDDS, and inadvertently either change the FastRTPS performance by linking differently or accidentally compare statically linked FastRTPS to dynamically linked CycloneDDS.

I you want to measure performance (which is not a common use case compared to the overall community which just wants to use the binaries) you need to know what you are doing anyway. So if you don't want to compare apples with oranges you need to compile both RMW impl either each on their own or both together to have an exact comparison.

Let's just agree that we disagree on what each of us thinks the right choice is here. Maybe someone else wants to comment with their opinion to get a broader set of perspectives.

[rotu]

Let's just agree that we disagree on what each of us thinks the right choice is here. Maybe someone else wants to comment with their opinion to get a broader set of perspectives.

👍 @nuclearsandwich, could you weigh in?

[clalancette]

My opinion: we should default to always building dynamically, with an option to build one statically if the user wants. That is, I think we should take this PR. Most of the time, users are not concerned with absolute performance, and thus having dynamic loading be the default no matter how many DDS vendors you have installed seems to be the most consistent thing to do.

[nuclearsandwich]

Not to mention if you rosdep install, you get the other RMWs! So the previous behavior seems to discourage meaningful use of rosdep.

I don't think that this has any bearing on the default linking behavior for rmw_implementations. Providing more complete group semantics and implementing them across the tooling is a separate (but valid) issue.

I agree with @dirk-thomas that anyone profiling will need to be paying attention to the rmw binding style and that which one is the default is less important. Our documentation for building from source focuses on the use of the default rmw. It might be worth adding something here: https://index.ros.org/doc/ros2/Installation/Dashing/Linux-Development-Setup/#install-more-dds-implementations-optional which mentions that if you first built a workspace with only one rmw implementation that you may need to reconfigure rmw_implementation to pick up the second one.

I also think defaulting to skipping poco makes sense as a default of "don't pay for what you aren't using" and yes someone may want to use it down the road and may not be fully aware of the change in behavior but I think that's a better situation than requiring people to opt-in to the optimization.

[rotu]

Even (especially) if the performance difference is extreme, we should provide a way to affirmatively opt in to static linking. There isn't one currently. ("Oops! I switched to another DDS but accidentally left the first RMW package installed with apt so it's building with dynamic linking and there's no way to tell").

Either it's important enough to make link type explicit or it's not. I don't see an argument for "this is important but not important enough to allow the user to turn ON static linking, only off".

[nuclearsandwich]

I don't see an argument for "this is important but not important enough to allow the user to turn ON static linking, only off".

I got caught up in the discussion of changing the default that I wasn't considering this. I would be in favor of a build option that disables poco usage in favor of a specific rmw implementation as long as it doesn't disrupt the current default behavior.

[rotu]

Reverted to the static build if one RMW is found at build time. I think this is the wrong call, but at the end of the day, I don't think it will make much difference.

Please rereview

[rotu]

Rebased and revised commit message:

Defer list of available RMWs to build time of downstream packages so you can build a dependent package even if RMW_IMPLEMENTATION is set to a value not available when the rmw_implementation package was built.

Remove CMake Config flag "RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING".
Add CMake Config flag "RMW_IMPLEMENTATION_BAKED" so you can statically link a middleware, even if multiple present at build time. This value can be explicitly set, but by default preserves the behavior of "statically link if one middleware found".

Downgrade missing rmw library to a warning, since some RMW's like rmw_cyclonedds_cpp and rmw_fastrtps_dynamic_cpp (?) do not need to inject build dependencies.
Add a scary message if the user tries to change the implementation and built with RMW_IMPLEMENTATION_BAKED=ON

Signed-off-by: Dan Rose dan@digilabs.io

[rotu]

Rebased, reworded, and cleaned up unnecessary whitespace changes, as well as functional issues you pointed out. Thanks for your patience, @dirk-thomas!

[dirk-thomas]

CI builds with only FastRTPS:

• Linux
• Linux-aarch64
• macOS
• Windows

CI builds with FastRTPS and Connext:

• Linux
• Linux-aarch64
• macOS
• Windows (unrelated)

[dirk-thomas]

Thanks for this improvement and for iterating on the patch!