Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

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

Closed
j-rivero opened this issue Jan 22, 2024 · 3 comments
Closed

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

j-rivero opened this issue Jan 22, 2024 · 3 comments

Comments

@j-rivero
Copy link
Collaborator

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 added a commit that referenced this issue Jan 25, 2024
Name change is waiting to resolve #7 but this name change was
required in order to make the examples to work when using the
drake-extras.cmake.

Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Copy link
Collaborator Author

j-rivero commented Jan 30, 2024

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:

  • ros_ws/build/drake/drake-config.cmake
    and not the expected in:
  • ros_ws/install/opt/drake/lib/cmake/drake/drake-config.cmake
    leading to the error:
-- Found drake: 0.0.1 (/home/runner/work/ros-drake-vendor/ros-drake-vendor/ros_ws/install/share/drake/cmake)
-- Configuring done (1.1s)
CMake Error at src/CMakeLists.txt:4 (target_link_libraries):
  Target "particle" 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.
    * An ALIAS target is missing.



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)
    * An ALIAS target is missing.

j-rivero added a commit to j-rivero/ros-drake-vendor that referenced this issue Mar 1, 2024
Name change is waiting to resolve RobotLocomotion#7 but this name change was
required in order to make the examples to work when using the
drake-extras.cmake.

Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Copy link
Collaborator Author

j-rivero commented Mar 5, 2024

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, the drake_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 added a commit that referenced this issue Mar 6, 2024
@j-rivero
Copy link
Collaborator Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant