Replace poco dependency by rcutils

[ahcorde]

As part of the effort to remove Poco dependency described in this issue ros2/ros2#867 this PR makes use of the recent changes in rcutils ros2/rcutils#215 to load, unload and get symbols from shared libraries instead of using Poco.

Signed-off-by: ahcorde ahcorde@gmail.com

[piraka9011]

Have you properly linked rcutils in rosbag2_cpp's CMakeLists.txt?

/home/runner/work/rosbag2/rosbag2/ros_ws/src/rosbag2/rosbag2_cpp/src/rosbag2_cpp/typesupport_helpers.cpp:26:10: fatal error: rcutils/shared_library.h: No such file or directory
   #include "rcutils/shared_library.h"

[ahcorde]

@piraka9011 the PR is not merged yet ros2/rcutils#215

[Karsten1987]

a few comments. I would like to refactor the library initialization into its own function to avoid code duplication.

[ahcorde]

I have created an abstraction in rcpputils for rcutils shared library in this branch and I made the changes in rosbag2 here. There is no leak with this solution.

@emersonknapp @piraka9011 @Karsten1987 @wjwwood Can you have a look? Maybe we can decline this PR and I can open these two others which is more modern C++ oriented.

[emersonknapp]

Took a quick look through it - over all i like that approach, I think it's a useful abstraction to have for C++. My personal vote is yes, we should split this out into these two PRs

[wjwwood]

@ahcorde can you make ahcorde/rcpputils@efc9963 into a pull request? I have some comments but it's better if it is a pull request first.

[ahcorde]

Sure @wjwwood ros2/rcpputils#49

[ahcorde]

rcpputils PR is merged, can you review this ? @emersonknapp @piraka9011 @Karsten1987

[piraka9011]

Why catch and just let it throw?

[piraka9011]

This looks much cleaner than before, thanks @ahcorde 👍

[ahcorde]

• Linux
• Linux-aarch64
• macOS
• Windows

Warnings in Windows are in rmw_dds_common

17:03:59 C:\ci\ws\src\ros2\rmw_dds_common\rmw_dds_common\test\test_graph_cache.cpp(195,8): warning C4996: 'strncpy': This function or variable may be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\ci\ws\build\rmw_dds_common\test_graph_cache.vcxproj]

17:03:59 C:\ci\ws\src\ros2\rmw_dds_common\rmw_dds_common\test\test_graph_cache.cpp(388,8): warning C4996: 'strncpy': This function or variable may be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\ci\ws\build\rmw_dds_common\test_graph_cache.vcxproj]

[ahcorde]

Feel free to merge it

[dirk-thomas]

Warnings in Windows are in rmw_dds_common

@ahcorde please make sure to identify the author of the causing change and notify them on that PR.

[Karsten1987]

rant on
I just lost a day of my life to find out that this PR broke the rosbag2_bag_v2_plugins. Essentially, the converter functionality.
rant off

I haven't paid enough attention while reviewing this PR, but I think it's really bad practice to pass in a shared_pointer by reference (std::shared_pointer<rcpputils::SharedLibrary> & library) and somewhat blindly overwrite it by calling std::make_shared on it. Also, this assumes that the pointer is empty, which isn't checked here either.

The reason why it fails in the end is that the same pointer is being passed in to load multiple typesupports, which are stored in add_topics. Later on, when using these typesupport symbols to create a new introspection message, these pointers have dangled.

We definitely have to come up with better tests for the converter.

[wjwwood]

but I think it's really bad practice to pass in a shared_pointer by reference (std::shared_pointerrcpputils::SharedLibrary & library) and somewhat blindly overwrite it by calling std::make_shared on it.

So, I know there's some kind of taboo in our team about shared pointers and references, but what you describe is exactly what the CppCoreGuidelines suggest you do if you might "reseat" (their term meaning point it to a different object) the shared pointer:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-sharedptrparam

[Karsten1987]

fair enough. I take that argument when you transfer ownership.

I still think though that in this case it's not necessarily applied correctly. The current get_typesupport function does two things: It transfers ownership of the passed in shared pointer and returns a raw pointer from a newly created object. I would agree if that function would be split in two, where the first one transfers ownership and instantiates a new shared_ptr to a rcpputils::SharedLibrary. The getter function should then take a correctly initialized shared pointer - const if possible - and return the typesupport handle. IMO, this would make things more explicit.

The moment you call the current implementation, you have to be aware that not only will the shared pointer you pass in be reset, but also that all pointers you've received from previous calls to this function are invalid. And that's the problem here.