-
Notifications
You must be signed in to change notification settings - Fork 299
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
expose convert function #146
Conversation
putting this in review. |
CMakeLists.txt
Outdated
@@ -63,6 +63,8 @@ endif() | |||
ament_export_include_directories(include) | |||
ament_export_libraries(${PROJECT_NAME}) | |||
|
|||
ament_export_dependencies(roscpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't going to work the way intended. roscpp
is a ROS 1 package and should be found using the custom CMake function used above (find_ros1_package
which doesn't rely on CMake config files but pkg-config
instead). If you want this to be exposed in downstream package it would need to be done in an extra file by invoking find_ros1_package
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dirk-thomas, our problem is the following: we link against the ros1_bridge and on Linux everything works fine, whereas on Mac the linker cannot find roscpp
symbols.
We could solve this by finding roscpp
ourselves, as you said, but for us it is only a transitive dependency for the bridge, and therefore we thought that this would be the cleanest solution: with this change, in fact, the roscpp
dependency is exported and everything works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your use case but this is still the wrong approach. As soon as a roscpp
package is created in ROS 2 e.g. to provide an API shim the current approach is going to fail. The ros1_bridge
package needs to invoke find_ros1_package
in an extra file instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we will then use the find_ros1_package
macro in our package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not saying you have to call it in your package. The logic can be placed in a CMake extra file in ros2_bridge
in order to avoid having to repeat it in multiple downstream packages.
This reverts commit d8a1ccc.
@botteroa-si I reverted the last commit for the export dependency in here: b920e22 |
Do you still want to add the |
@dirk-thomas, that would of course be the cleanest solution, but currently we don't have time to look into it. Therefore, for the moment, we would just call the |
* expose convert function * expose convert2_to_1 function * export roscpp as dependency * Revert "export roscpp as dependency" This reverts commit d8a1ccc. Signed-off-by: Dhananjay Sathe <dhananjay.sathe@rapyuta-robotics.com>
In order to leverage the bridge to convert ros messages from ros1 to ros2, we would like to expose this function. This allows us to convert messages without using pub/sub.
cc @botteroa-si