RobotLocomotion / ros-drake-vendor

Maintainer scripts that package Drake in the ROS build farm
Other
1 stars 1 forks source link

Collision with "drake" name for the ROS package #7

Closed j-rivero closed 8 months ago

j-rivero commented 10 months ago

The initial code submitted in #6 uses the name of "Drake" for the ROS package in this repository. While testing the installation I found that using the same "drake" name for the ROS package and the upstream code is going to generate conflicts when using CMake to resolve find_package(drake) since the ROS package is going to generate a cmake config file that has the exact name than the drake project: drake-config.cmake. Although they won't conflict in the installation file path, the name conflict and resolution using CMake and the different configurations of precedence is going to be a problem.

I just verified today with the OSRF infrastructure team that no other ROS "vendor-like" package is using the same name than original code that it ships and they have recommended to use drake_vendor or ros_drake_vendor.

j-rivero commented 10 months ago

Running an example of the name conflict using a colcon workspace: https://github.com/RobotLocomotion/ros-drake-vendor/actions/runs/7711370534/job/21016603900

The drake-config.cmake file is found using find_package(drake) but the one found is the one generated at:

CMake Error at src/CMakeLists.txt:7 (target_link_libraries): Target "particle_test" links to:

drake::drake

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.

-- Generating done (0.0s)

j-rivero commented 9 months ago

Going a bit further into having the ROS package with name just "drake" (same than upstream package), we could try to add CMake code to the ROS package so it can act as a transparent wrapper on the upstream CMake. The idea would be that the drakeConfig.cmake generated by ROS runs find_package(drake PATHS ${upstream_drake_in_ROS}) with that PATH set to the exact location of the upstream code installed by the ROS package.

Using a CONFIG_EXTRAS for injecting this code should be trivial. A quick pseudo-schema of where the files would be after build and installation:

(generated by the Bazel build called by the ROS package)
/opt/ros/$distro//opt/drake/lib/cmake/drake/drake-config.cmake 
(generated by the ROS Package)
/opt/ros/$distro/share/drake/cmake/drakeConfig.cmake # this file includes the drake-extras.cmake
cat /opt/ros/$distro/share/drake/cmake/drake-extras.cmake
...
find_package(drake PATHS  "${drake_DIR}/../../../opt/drake")
...

Problem of drake_DIR cache variable While testing this I found that we have a problem with the drake_DIR variable set by CMake and stored in the CMakeCache.txt. After the first find_package(drake) that founds the ROS drake package the path (/opt/ros/$distro/share/drake/cmake/drakeConfig.cmake) is stored in the CMake cache and when the /opt/ros/$distro/share/drake/cmake/drake-extras.cmake calls find_package(drake PATHS "${drake_DIR}/../../../opt/drake") the cache result is resolved pointing to the ROS package instead of the desired upstream (/opt/ros/$distro/share/drake/cmake/drake-extras.cmake).

For resolving this problem we can add an unset(drake_DIR)variable before running the find_package for the Drake upstream. So code in extras could be something like:

unset(drake_DIR)
find_package(drake PATHS  "${drake_DIR}/../../../opt/drake")

Problem of overriding drake_DIR Running the unset things work as expected in the first pass of the whole setup:

ROS package: drake-config.cmake >> ROS package: extras.cmake >> unset + find_package(drake PATHS ...) >> Upstream package: drake-config.cmake

Problem is that setting that drake_DIRto point to the upstream package invalidate any later use of drake_DIR by ROS. So having an standard vendor package we found that this is a problem since the ROS Package drake-config.cmake is running the following:

set(_extras "drake-extras.cmake;vendor_package_cmake_prefix.cmake")
foreach(_extra ${_extras})
include("${drake_DIR}/${_extra}")
endforeach()

After the first include of drake-extras.cmake, thedrake_DIRvariable changes to point to upstream Drake, so the second pass forvendor_package_cmake_prefix.cmake` is broken.

Fragile workaround: use GLOBAL_HOOK Looing at the CMake vendor package code, I found that we can avoid to have more than the "drake-extras.cmake" file in the loop before (and thus avoid the problem of needed the drake_DIR set to ROS package) by declaring in the vendor package the parameter GLOBAL_HOOK. This is used to inject in CMAKE_PREFIX_PATH the location of vendor upstream code but seems like when I combine it with the EXTRA_CONFIG the result is no change and the ROS package is found without problems. Conceptually even if the drake upstream is located first that is what we want. With this I'm able to build ROS drake package and examples on top of it that requires upstream Drake. Workaround seems working although it is not a robust solution since it relies on code behavior that could perfectly change.

j-rivero commented 8 months ago

After the merge of #6 where this solution was implemented we can close this.