tesseract-robotics / tesseract_ros2

22 stars 24 forks source link

Cleanup includes and dependencies #64

Closed rjoomen closed 1 year ago

rjoomen commented 1 year ago

I cleaned up quite a number of includes and dependencies.

Something to keep in mind is that it seems that ament_export_dependencies() overwrites the (public) dependencies exported by target_link_libraries(). And for ROS2 dependencies like visualization_msgs that do not produce proper CMake targets (no visualization_msgs::visualization_msgs ) one is dependent on ament_target_dependencies() to link them, and hence on ament_export_dependencies() to export them, and then also export the libraries from target_link_libraries().

rjoomen commented 1 year ago

ROS2 Foxy has a tf2_eigen version (0.13) that does not have the modern CMake target tf2_eigen::tf2_eigen yet (introduced in 0.25).

rjoomen commented 1 year ago

A pity, really, because I just found out that I could replace this:

ament_target_dependencies(${PROJECT_NAME} PUBLIC
  tf2_eigen  
  sensor_msgs
  visualization_msgs
  tesseract_msgs
)

by this:

target_link_libraries(${PROJECT_NAME} PUBLIC
  tf2_eigen::tf2_eigen
  ${sensor_msgs_TARGETS}
  ${visualization_msgs_TARGETS}
  ${tesseract_msgs_TARGETS}
)

Allowing me to drop the usage of ament_target_dependencies() (and ament_export_dependencies()) entirely. But not on foxy, apparently. BTW, do we still need to support EOL foxy?

rjoomen commented 1 year ago

@marrts, @Levi-Armstrong could you please review?

marrts commented 1 year ago

BTW, do we still need to support EOL foxy?

I think it is a reasonable approach at this point to make a separate branch called foxy-eol that holds the state of things as is and continue forward dropping foxy support on the master branch. @marip8, do you have an opinion on this? I'd say we merge this branch in first though, these seem like some nice cleanups.

marip8 commented 1 year ago

ROS2 Foxy has a tf2_eigen version (0.13) that does not have the modern CMake target tf2_eigen::tf2_eigen yet (introduced in 0.25).

You could potentially create a target for tf2_eigen if it doesn't exist like we do in tesseract for a variety of libraries

Regarding supporting foxy, if it's not too cumbersome or hacky to continue supporting it, I think we should. Just because it is technically EOL doesn't mean everyone has moved on from that distro. Especially since 20.04 is that last OS distro that officially supports ROS1, I imagine people might linger on 20.04 for a while to be able to support both ROS1 and ROS2 systems (or maybe I'm biased because that's what I'm doing now...). It seems like what you have here works and is not overly complicated or hacky, so I would vote to continue supporting foxy in the main branch for now

rjoomen commented 1 year ago

I imagine people might linger on 20.04 for a while to be able to support both ROS1 and ROS2 systems I would vote to continue supporting foxy in the main branch for now

Seems reasonable to me.

rjoomen commented 1 year ago

I have to correct myself: removing ament_export_dependencies() is not possible, it's always needed for correctly exporting dependencies in an ament package.

You could potentially create a target for tf2_eigen if it doesn't exist like we do in tesseract for a variety of libraries

Thanks for the tip, I just found out linking to ${tf2_eigen_TARGETS} also works. The PR is ready to merge now.

rjoomen commented 1 year ago

Friendly ping

marrts commented 1 year ago

Looks good to me.