Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable plugins/tests which need rmw_fastrtps_cpp if unavailable #137

Merged

Conversation

ivanpauno
Copy link
Member

Fixes #136.

@ivanpauno
Copy link
Member Author

ivanpauno commented Jun 25, 2019

CI (only opensplice):

  • Linux Build Status

@ivanpauno
Copy link
Member Author

I think this is ready to merge

@Karsten1987
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member Author

@Karsten1987 Friendly ping

@Karsten1987
Copy link
Collaborator

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
Copy link
Member Author

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!

<depend>rosbag2</depend>


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert unrelated (whitespace) changes.

@dirk-thomas
Copy link
Member

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.

Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<depend>rosbag2</depend>


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superfluous new line


add_library(${PROJECT_NAME} SHARED
src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp)
if(rmw_fastrtps_cpp_FOUND)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

dirk-thomas commented Jul 31, 2019

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.

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

ivanpauno commented Jul 31, 2019

I've addressed comments and rebased with master.

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

  • Linux Build Status (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).

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/find-rmw-fastrtps-cpp-quietly branch from bb38b7a to ef76174 Compare July 31, 2019 18:19
@ivanpauno
Copy link
Member Author

Again:

  • Linux Build Status

@dirk-thomas dirk-thomas changed the title Find rmw_fastrtps_cpp quietly disable plugins/tests which need rmw_fastrtps_cpp if unavailable Jul 31, 2019
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

ivanpauno commented Jul 31, 2019

Again:

  • Linux Build Status

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 Build Status

@ivanpauno
Copy link
Member Author

ivanpauno commented Jul 31, 2019

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

  • Linux Build Status

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

  • Linux Build Status

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

rosbag2_transport/package.xml Outdated Show resolved Hide resolved
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno merged commit fd65963 into ros2:master Jul 31, 2019
@dirk-thomas
Copy link
Member

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

@dirk-thomas
Copy link
Member

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/

Karsten1987 pushed a commit that referenced this pull request Aug 19, 2019
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Karsten1987 added a commit that referenced this pull request Aug 29, 2019
… (#148)

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build fails if you don't have fastrtps
3 participants