Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup includes and dependencies #64

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

rjoomen
Copy link
Contributor

@rjoomen rjoomen commented Sep 6, 2023

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
Copy link
Contributor Author

rjoomen commented Sep 6, 2023

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
Copy link
Contributor Author

rjoomen commented Sep 6, 2023

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
Copy link
Contributor Author

rjoomen commented Sep 7, 2023

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

@marrts
Copy link
Contributor

marrts commented Sep 7, 2023

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
Copy link
Contributor

marip8 commented Sep 7, 2023

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
Copy link
Contributor Author

rjoomen commented Sep 8, 2023

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.

- Remove pluginlib dependency from tesseract_monitoring.
- Remove ament_target_dependencies() from tesseract_ros_examples
@rjoomen
Copy link
Contributor Author

rjoomen commented Sep 8, 2023

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
Copy link
Contributor Author

rjoomen commented Sep 12, 2023

Friendly ping

@marrts
Copy link
Contributor

marrts commented Sep 12, 2023

Looks good to me.

@marrts marrts merged commit 9ae8b53 into tesseract-robotics:master Sep 12, 2023
5 checks passed
@rjoomen rjoomen deleted the includecleanup branch September 15, 2023 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants