Fix hard-coded Windows paths. #521

clalancette · 2022-04-29T20:24:36Z

These come about because of the use of the old-style CMake
variables like ${orocos_kdl_INCLUDE_DIRS}. Unfortunately,
there is a bug in the orocos_kdl CMake modern targets where
they forget to export a dependency on Eigen. To workaround
this, make an explicit dependency on Eigen (even though we
don't directly use it), and use the Eigen targets instead.

Note that on Windows, this still causes us to use a
hardcoded path on Eigen. However, this path is less problematic
because it will always be there if the user followed our
installation instructions (and won't be a "random" path like
C:\ci\ws\install\include). Also, once we fix
ros2/choco-packages#19 , this hard-coded
path will go away as well.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This is one part of a fix for the remaining issues on ros2/rclcpp#1688 . Opening as a draft for now, because while I think this will solve the issue I need to run the packaging jobs on Windows to know for sure.

These come about because of the use of the old-style CMake variables like ${orocos_kdl_INCLUDE_DIRS}. Unfortunately, there is a bug in the orocos_kdl CMake modern targets where they forget to export a dependency on Eigen. To workaround this, make an explicit dependency on Eigen (even though we don't directly use it), and use the Eigen targets instead. Note that on Windows, this *still* causes us to use a hardcoded path on Eigen. However, this path is less problematic because it will always be there if the user followed our installation instructions (and won't be a "random" path like C:\ci\ws\install\include). Also, once we fix ros2/choco-packages#19 , this hard-coded path will go away as well. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2022-04-29T20:28:39Z

Windows packaging CI before this change:

Windows packaging CI after this change:

(when these are done, I'll manually download their build artifacts and confirm that it actually fixes the issue)

ivanpauno · 2022-04-29T21:09:44Z

tf2_geometry_msgs/CMakeLists.txt

+find_package(Eigen3 QUIET NO_MODULE)
+# Work around broken find module in AlmaLinux/RHEL eigen3-devel from PowerTools repo
+if(NOT Eigen3_FOUND)
+  find_package(Eigen3 REQUIRED)
+endif()


Suggested change

find_package(Eigen3 QUIET NO_MODULE)

# Work around broken find module in AlmaLinux/RHEL eigen3-devel from PowerTools repo

if(NOT Eigen3_FOUND)

find_package(Eigen3 REQUIRED)

endif()

find_package(Eigen3 QUIET)

doesn't that just work?
Do we need to specify NO_MODULE?

Do we need to specify NO_MODULE?

As I understand it, we need that whole above sequence for it to work on RHEL because of the bug in the Eigen packaging there. But I didn't investigate this (I just copied it from elsewhere), maybe @sloretz can comment.

ivanpauno · 2022-04-29T21:11:54Z

tf2_geometry_msgs/CMakeLists.txt

 target_link_libraries(${PROJECT_NAME} INTERFACE
  ${geometry_msgs_TARGETS}
  orocos-kdl
  tf2::tf2
  tf2_ros::tf2_ros)
+if(TARGET Eigen3::Eigen)
+  # TODO(sloretz) require target to exist when https://github.com/ros2/choco-packages/issues/19 is addressed


based on this, it seems it won't actually fix the issue on Windows, right?

We could also create our own FindEigen.cmake module to work around the issue.

based on this, it seems it won't actually fix the issue on Windows, right?

Sorry, I see now that you wrote that in the PR description.
Having a custom find module for eigen is a possible workaround.

Yes, and we could possibly even use eigen_cmake_module for this. That said, we know what the issue is (ros2/choco-packages#19), we just need to package a newer version of Eigen for that. So I'm not sure it is worth it; maybe as part of these fixes, we can spend some time and repackage Eigen on Windows.

Yes, and we could possibly even use eigen_cmake_module for this

eigen_cmake_module turns Eigen's CMake targets into old-style CMake variables, so it's not much help now that we're using targets ourselves. Once we fix ros2/choco-packages#19 I'd like to remove both this workaround and eigen_cmake_module completely.

ivanpauno · 2022-04-29T21:13:30Z

tf2_kdl/CMakeLists.txt

@@ -12,6 +12,16 @@ endif()

 find_package(ament_cmake REQUIRED)
 find_package(builtin_interfaces REQUIRED)
+# TODO(clalancette): This package doesn't directly depend on Eigen, but orocos-kdl


same comments apply to this file

clalancette · 2022-04-29T21:37:55Z

I've downloaded both the before and after, and confirmed that this fixes the issue with the hardcoded paths for these two packages. As mentioned in the comments, we'll still have the hard-coded Eigen path until ros2/choco-packages#19 is fixed, but that shouldn't actually block this from going in.

Here's CI, including Windows Debug and RHEL:

Linux
Linux-aarch64
Windows
Windows Debug
RHEL

ivanpauno · 2022-05-02T13:01:12Z

The error in RHEL might be hapenning because we now need to ament_export_dependencies(Eigen3).
I'm not sure about that.

clalancette · 2022-05-02T13:10:55Z

The error in RHEL might be hapenning because we now need to ament_export_dependencies(Eigen3).
I'm not sure about that.

Oh, interesting thought. The failure is happening inside of https://github.com/ros-visualization/interactive_markers, and while looking it over I noticed that it was using an old-style ${tf2_geometry_msgs_TARGET} variable rather than tf2_geometry_msgs::tf2_geometry_msgs. I'm going to try correcting that and see if that makes a difference.

clalancette · 2022-05-02T15:49:38Z

I tried just adding in ament_export_dependencies(Eigen3), but that didn't work. @sloretz clued me in that the failure on RHEL is because of the broken find_package(Eigen REQUIRED) there. Essentially it doesn't set things up properly, so in order for dependent packages to find the package, we'd need to add an ament extras hook to do the correct dance for downstream:

find_package(Eigen3 QUIET NO_MODULE)
if(NOT Eigen3_FOUND)
  find_package(Eigen3 REQUIRED)
endif()

While we can do that, this is getting more ugly.

So we took a step back and tried to think of another way to do this. Fundamentally, the problem is that orocos_kdl doesn't properly export an Eigen dependency. On Ubuntu 22.04, the vendor package is empty because it finds the system package and uses that. On RHEL and Windows, however, we build it. Since the problem we are trying to solve here is that Windows needs the appropriate dependency, I think a better solution than this PR is to patch orocos_kdl_vendor with the appropriate dependency when we are building the vendor package. That will make this PR obsolete, fix all of the consumers of orocos_kdl_vendor at once, fix the Windows hard-coded path problem, and avoid the RHEL problem. So I'm going to convert this back to draft, and try to get that working instead. If I do get that fixed, I'll close this out.

ivanpauno · 2022-05-02T18:43:07Z

Essentially it doesn't set things up properly, so in order for dependent packages to find the package, we'd need to add an ament extras hook to do the correct dance for downstream:

Ah yes, that makes sense.

While we can do that, this is getting more ugly.

Agreed

So we took a step back and tried to think of another way to do this. Fundamentally, the problem is that orocos_kdl doesn't properly export an Eigen dependency. On Ubuntu 22.04, the vendor package is empty because it finds the system package and uses that. On RHEL and Windows, however, we build it. Since the problem we are trying to solve here is that Windows needs the appropriate dependency, I think a better solution than this PR is to patch orocos_kdl_vendor with the appropriate dependency when we are building the vendor package. That will make this PR obsolete, fix all of the consumers of orocos_kdl_vendor at once, fix the Windows hard-coded path problem, and avoid the RHEL problem. So I'm going to convert this back to draft, and try to get that working instead. If I do get that fixed, I'll close this out.

I don't have an opinion, if I think of a nice alternative I'll comment it here.

clalancette · 2022-07-08T15:38:16Z

I believe that this has been fixed separately in the orocos_kdl_vendor package and in #534. So closing this out.

clalancette requested a review from ivanpauno April 29, 2022 20:24

ivanpauno reviewed Apr 29, 2022

View reviewed changes

clalancette marked this pull request as ready for review April 29, 2022 21:34

clalancette marked this pull request as draft May 2, 2022 15:50

audrow changed the base branch from ros2 to rolling June 28, 2022 14:18

clalancette closed this Jul 8, 2022

clalancette deleted the clalancette/fix-hardcoded-windows-paths branch July 8, 2022 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hard-coded Windows paths. #521

Fix hard-coded Windows paths. #521

clalancette commented Apr 29, 2022

clalancette commented Apr 29, 2022

ivanpauno Apr 29, 2022

clalancette Apr 29, 2022

ivanpauno Apr 29, 2022

ivanpauno Apr 29, 2022

ivanpauno Apr 29, 2022

clalancette Apr 29, 2022 •

edited

Loading

sloretz Apr 29, 2022

sloretz Apr 29, 2022

ivanpauno Apr 29, 2022

clalancette commented Apr 29, 2022

ivanpauno commented May 2, 2022

clalancette commented May 2, 2022

clalancette commented May 2, 2022

ivanpauno commented May 2, 2022

clalancette commented Jul 8, 2022

Fix hard-coded Windows paths. #521

Fix hard-coded Windows paths. #521

Conversation

clalancette commented Apr 29, 2022

clalancette commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Apr 29, 2022

ivanpauno commented May 2, 2022

clalancette commented May 2, 2022

clalancette commented May 2, 2022

ivanpauno commented May 2, 2022

clalancette commented Jul 8, 2022

clalancette Apr 29, 2022 •

edited

Loading