Find ROS1 message packages #67
Conversation
mikaelarguedas
added
in progress
CMakeLists.txt
Outdated
| @@ -34,6 +34,32 @@ endif() | |||
|
|
|||
| find_ros1_package(std_msgs REQUIRED) | |||
|
|
|||
| # find all known ROS1 message/service packages | |||
| find_ros1_package(rosmsg REQUIRED) | |||
There was a problem hiding this comment.
What is this line needed for?
CMakeLists.txt
Outdated
| # find all known ROS1 message/service packages | ||
| find_ros1_package(rosmsg REQUIRED) | ||
| execute_process( | ||
| COMMAND bash -c "rosmsg list" |
There was a problem hiding this comment.
Why use bash here? This should be platform agnostic.
Same below.
There was a problem hiding this comment.
I previously tried just the command and it resulted in an empty string that's why I added the bash -c here. Did investigate the why though
CMakeLists.txt
Outdated
| ) | ||
| set(ros1_interfaces_string "${rosmsg_output}\n${rossrv_output}") | ||
|
|
||
| string(REGEX REPLACE "\n" ";" ros1_interfaces_list "${ros1_interfaces_string}") |
There was a problem hiding this comment.
Since the pattern doesn't use a regex this should be calling string(REPLACE ...).
|
@Kukanani FYI |
CMakeLists.txt
Outdated
| ERROR_VARIABLE rossrv_error | ||
| ) | ||
| if(NOT rosmsg_error STREQUAL "" OR NOT rossrv_error STREQUAL "") | ||
| message(FATAL_ERROR "${rosmsg_error}\n${rossrv_error}\nFailed to find ROS 1 message packages\nPlease make sure to source your ROS2 workspace BEFORE sourcing your ROS1 workspace") |
There was a problem hiding this comment.
I am not sure I understand how this message is specific to the error here. I would expect that the commands fail when the user didn't source ROS 1. Sourcing (or not) ROS 2 as well as the order shouldn't make any difference.
And the CMake code got that far in the first place the user must have sourced something from ROS 1. So maybe the message should just mention that rosmsg / rossrv failed?
There was a problem hiding this comment.
The reason here is that ROS1 and ROS2 message packages have been found. But the ros2 ones are the first ones on the python path. So the invocation of rosmsg / rossrv will fail because these are python2 scripts trying to import python3 (ROS2) messages. I can make the error message more explicit to reflect what happens exactly.
I'm still not sure why the problem didnt appear when I was calling out bash explicitely though ...
|
When trying to build using this branch, I get CMake warnings. And get the following output: |
|
thanks @Kukanani for the report. looks like cmake is not happy about finding twice Can you confirm that despite the warning, this does fix the original issue and you can use the bridge for the custom message package? I'll try to reproduce the warning locally and comment here. |
|
tested this again in a fresh docker container using the same message package as @Kukanani and various configs and could not reproduce the Cmake warning. Without a reproducible example I'll move forward and ask for this to be reviewed. |
CMakeLists.txt
Outdated
| ERROR_VARIABLE rossrv_error | ||
| ) | ||
| if(NOT rosmsg_error STREQUAL "" OR NOT rossrv_error STREQUAL "") | ||
| message(FATAL_ERROR "${rosmsg_error}\n${rossrv_error}\nFailed to call rosmsg/rossrv\nPlease make sure to source your ROS2 workspace BEFORE sourcing your ROS1 workspace") |
There was a problem hiding this comment.
The current recommendation is sourcing ROS 2 after ROS 1. That is to find CMake config files of ROS 2 packages over ROS 1 packages in case of the same package name. With this inverted order how is that supposed to work?
There was a problem hiding this comment.
Could you clarify why this is necessary in the first place? (In the context of the bridge being built separately). Is it to find the mapping_rules? or is it for another reason that it's necessary to find the ROS2 cmake config files?
The problem I'm facing is that given that the message packages have the same names on both side, the ROS1 python scripts (rosmsg and rossrv) need the ros ones to be the first ones on the sys.path so that when they import std_msgs they get the ROS1 module. I'm happy to explore an alternative solution to the rosmsg/rossrv approach if you have a pointer to what would be the best way to handle this.
There was a problem hiding this comment.
ROS 2 packages can only be found via CMake, ROS 1 on the other hand can additionally be found via pkg-config. In order to be able to find both kinds. ROS 2 must be sourced last.
The problem with the Python code for messages is that both ROS 1 as well as 2 are using the exact same naming scheme. Maybe that is another reason why we should consider using a different naming in ROS 2 in order to distinguish them.
|
a266513 ensures that we can keep the current order of sourcing worskpaces. This addreses #67 (comment) allow us to keep the instructions as is and fixes the original problem of ros1 workspace overlays |
|
CI: |
CMakeLists.txt
Outdated
| @@ -34,6 +34,59 @@ endif() | |||
|
|
|||
| find_ros1_package(std_msgs REQUIRED) | |||
|
|
|||
| # ROS1 message packages need to be first in the PYTHONPATH | |||
| # So we remove every ROS2 path from the python path for now | |||
| set(__AMENT_PREFIX_PATH $ENV{AMENT_PREFIX_PATH}) | |||
There was a problem hiding this comment.
Why two underscores as a prefix? One should be sufficient.
CMakeLists.txt
Outdated
| string(REPLACE ":" ";" __PYTHONPATH ${__PYTHONPATH}) | ||
| foreach(ament_path ${__AMENT_PREFIX_PATH}) | ||
| foreach(py_path ${__PYTHONPATH}) | ||
| if(py_path MATCHES "^${ament_path}") |
There was a problem hiding this comment.
Since ament_path doesn't have a trailing slash this might match for unrelated paths. Therefore the pattern should end with a slash / backslash.
CMakeLists.txt
Outdated
| set(__PYTHONPATH_STRING "") | ||
| foreach(py_path ${__PYTHONPATH}) | ||
| string(CONCAT __PYTHONPATH_STRING "${__PYTHONPATH_STRING}" "${py_path}" ":") | ||
| endforeach() |
There was a problem hiding this comment.
Instead of building the Python path in a separate loop could this be populated in the loops above? Also I would suggest to use a CMake list and afterwards replace the semicolons with colons (but not on Windows!).
There was a problem hiding this comment.
oh I tried but couldn't figure out how to do that (Convert a list to string and replace semi-colon with colons).
I'm didn't put much thought about the windows case figuring that windows users won't have ROS1 installed so this cmakelists would return before hitting this section.
CMakeLists.txt
Outdated
| OUTPUT_VARIABLE rossrv_output | ||
| ERROR_VARIABLE rossrv_error | ||
| ) | ||
| set(ENV{PYTHONPATH} ${__ORIG_PYTHONPATH}) |
There was a problem hiding this comment.
Instead of changing the environment and resetting it later it would be cleaned to just set the environment variable for the two executed processes.
There was a problem hiding this comment.
oh good, I didn't know cmake had a shell + platform agnostic way to do that, I'll look into it then
There was a problem hiding this comment.
Couldn't find a way to do it from CMake so I kept the current logic of replacing and restoring it after calling the scripts
|
I addressed all review comments except #67 (comment). |
|
After some offline conversation I refactored the code a bit to hopefully make it work across all platforms and be little bit shorter / more readable. See 0157853 |
|
Looks like it fail, I think that the latest change leaves us with an empty pythonpath. Looking into it now |
mikaelarguedas
force-pushed
the
find_ros1_packages
branch
from
8d3fc0a
to
0f20667
Compare
|
all good: |
Currently the bridge doesnt compile if you have an overlay ROS1 workspace. The messages are found and added to the factories but then fail to compile because we never find_package them.
the first commits rename a variable to make it obvious it's referring to ros2 packages.
the second commit call rosmsg and rossrv to get the list of ROS1 message packages and find_package them
the third one makes the shared lib link against these packages
commit 3 and 4 are just fixups/ cleanup