disable plugins/tests which need rmw_fastrtps_cpp if unavailable

[ivanpauno]

Fixes #136.

[ivanpauno]

CI (only opensplice):

• Linux

[ivanpauno]

I think this is ready to merge

[Karsten1987]

I don't think that's the right approach.
We dynamically, but hardcoded, load rmw_fastrtps_cpp inside the code: https://github.com/ros2/rosbag2/blob/master/rosbag2_converter_default_plugins/src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp#L63

---

I believe we should always find rmw_fastrtps_cpp and don't compile the library at all if fastrtps is not available. With this patch, we currently build the library correctly, but it fails on runtime due to the missing libraries. That also explains the set of test failures (the segfaults) of the CI job you've linked.
When fast-rtps is not available, we should further disable all tests which rely on the converter plugins, as they are the only source requiring fastrtps.

[ivanpauno]

I don't think that's the right approach.
We dynamically, but hardcoded, load rmw_fastrtps_cpp inside the code: https://github.com/ros2/rosbag2/blob/master/rosbag2_converter_default_plugins/src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp#L63

I believe we should always find rmw_fastrtps_cpp and don't compile the library at all if fastrtps is not available. With this patch, we currently build the library correctly, but it fails on runtime due to the missing libraries. That also explains the set of test failures (the segfaults) of the CI job you've linked.
When fast-rtps is not available, we should further disable all tests which rely on the converter plugins, as they are the only source requiring fastrtps.

Ok, I will check what has to be disabled an update the PR.

[ivanpauno]

@Karsten1987 In bb38b7a, I completely disabled the build of rosbag2_converter_default_plugins if you don't have fastrtps. I also disabled the three tests in rosbag2_tests which I think that they depend at runtime in the former package.

I also saw in the previous CI that test_rosbag2_node and test_play were failing, but I'm not sure if they also depend on the converter plugin, or if it was a random error related to using opensplice.
Can you check please?
Thanks!

[ivanpauno]

@Karsten1987 Friendly ping

[Karsten1987]

Sorry @ivanpauno I am currently pretty underwater and won't have time to deep dive into it for the next two weeks.

It makes sense to me to completely disable the converter plugin tests when fast rtps is not available. I could further imagine (without having precisely looked at the code) that some end-to-end tests might have to be disabled or compiled differently when the converters are not present in the current setup.

[ivanpauno]

Sorry @ivanpauno I am currently pretty underwater and won't have time to deep dive into it for the next two weeks.

It makes sense to me to completely disable the converter plugin tests when fast rtps is not available. I could further imagine (without having precisely looked at the code) that some end-to-end tests might have to be disabled or compiled differently when the converters are not present in the current setup.

No problem, I will wait then. Thanks for the update!

[dirk-thomas]

Please revert unrelated (whitespace) changes.

[dirk-thomas]

Please run CI for this change with a non-FastRTPS RMW impl. If the build doesn't fail (which would be a big step forward) we will get this merged in the current state.

[Karsten1987]

I am requesting changes here because at least I would inform the user (the person who's compiling this lib) that the default converter plugins are not compiled in full. As it is right now, the user has no idea whether the plugins are compiled.
The user can then invoke the rosbag application as normal, but it will fail saying that the plugin is unavailable even though the library and such was compiled "correctly".

---

I believe the right change here is to make the plugins work with whatever RMW implementation available and try to avoid a hardcoded version of rmw_fastrtps_cpp.

[Karsten1987]

superfluous new line

[Karsten1987]

I would somehow inverse this logic and return if(NOT rmw_fastrtps_cpp_FOUND) with at least a notification that this library is not going to be compiled in full. This will also ease the diff a little bit as we don't have an extra layer of indentation.

[dirk-thomas]

I believe the right change here is to make the plugins work with whatever RMW implementation available and try to avoid a hardcoded version of rmw_fastrtps_cpp.

That might be a good goal but is outside of the scope of this PR.

[ivanpauno]

I've addressed comments and rebased with master.

CI (only opensplice, up to rosbag2 related packages):

• Linux (a build failure that I don't understand why it wasn't happening on master).

---

Note: this was run without 47d731a (which addressed the comments). I forgot that I have to push the changes to my fork (at that moment, I didn't have write access in this repo).

[ivanpauno]

Again:

• Linux

[ivanpauno]

Again:

• Linux

It builds correctly now (only opensplice). I solved the linter failures.
There are two other build failures, but they shouldn't happen if fastrtps is available (they don't appear locally, I have the three rmw implementations).

Double checking (fastrtps and connext, same packages as before):

• Linux

[ivanpauno]

Double checking (fastrtps and connext, same packages as before):

• Linux

Again, because I chose opensplice and connext by error 🤦‍♂️ :

• Linux

[dirk-thomas]

lgtm

[dirk-thomas]

@Karsten1987 can you please make sure that this gets backported into Dashing in time for the upcoming patch release.

[dirk-thomas]

can you please make sure that this gets backported into Dashing in time for the upcoming patch release.

Once the fix has been merged to the dashing branch the following job should start to pass (or at least become unstable): http://build.ros2.org/view/Dci/job/Dci__nightly-cyclonedds_ubuntu_bionic_amd64/