Cleanup CMakeLists target dependencies #228

henningkayser · 2020-06-25T10:43:23Z

Started looking into dependency issues with the Foxy branch and ended up cleaning up most of the dependencies.

codecov · 2020-06-25T11:08:35Z

Codecov Report

Merging #228 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #228   +/-   ##
=======================================
  Coverage   47.35%   47.35%           
=======================================
  Files         143      143           
  Lines       13347    13347           
=======================================
  Hits         6321     6321           
  Misses       7026     7026

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9382ede...1525b39. Read the comment docs.

v4hn

just looked through it once, commenting what I didn't immediately understand.

moveit_core/CMakeLists.txt

v4hn · 2020-06-25T18:49:31Z

moveit_core/constraint_samplers/CMakeLists.txt

-    ${urdfdom_LIBRARIES}
-    ${urdfdom_headers_LIBRARIES}
-    ${rclcpp_headers_LIBRARIES}
-    ${geometric_shapes_LIBRARIES}


why are none of these needed anymore?

the test does use, e.g., moveit_constraint_samplers for all that I can see.

moveit_constraint_samplers is ${MOVEIT_LIB_NAME}. All other targets are provided with $MOVEIT_LIB_NAME so I didn't specify the same deps again for the test.

v4hn · 2020-06-25T18:58:51Z

moveit_core/planning_scene/CMakeLists.txt

+  urdfdom
+  urdfdom_headers
+  octomap_msgs
+  OCTOMAP
 )


This added public target includes for octomap before so that other libs linking with moveit_planning_scene automatically include the octomap headers. Does ament_target_dependencies automagically add the correct target includes as public/interface/private? How would it differentiate between private and non-private includes?

It's not PUBLIC in MoveIt 1, so I removed it. We can specify octomap PUBLIC with ament_target_dependencies though.

v4hn · 2020-06-25T19:00:36Z

moveit_core/planning_scene/CMakeLists.txt

+  ament_target_dependencies(test_planning_scene
+    ament_index_cpp
+    geometric_shapes
+    srdfdom


urdf_parser is included, but not listed here?

provided by ${MOVEIT_LIB_NAME}

v4hn · 2020-06-25T19:01:58Z

moveit_core/planning_scene/CMakeLists.txt

-    ${MOVEIT_LIB_NAME}
-    ${urdfdom_LIBRARIES}
-    ${urdfdom_headers_LIBRARIES})
+  target_link_libraries(test_multi_threaded moveit_test_utils ${MOVEIT_LIB_NAME})


robot_state, robot_model and collision_detection are included, but not listed here?

v4hn · 2020-06-25T19:05:27Z

moveit_core/trajectory_processing/CMakeLists.txt

    ${MOVEIT_LIB_NAME}
-    ${geometric_shapes_LIBRARIES}
-    resource_retriever::resource_retriever
  )


robot_state, robot_model, and robot_trajectory are included but not listed here.

henningkayser · 2020-06-26T07:45:13Z

@v4hn thanks for reviewing this. My goal was to remove all the very generic (and often redundant) target_link_libraries() deps and includes and use ament_target_dependencies() more consistently. A lot of the removed includes were added in Crystal/Dashing where this wouldn't work like it does now. The "missing" dependencies should only be with test targets that really get all dependencies from a library which is specified in the same CMakeLists file. I found that this just makes sense and is less redundant than specifying the same target dependencies again and again. Considering the circular dependencies in moveit_core, it wouldn't even be possible to always cover all included targets anyway.

v4hn · 2020-06-26T13:03:22Z

My goal was to remove all the very generic (and often redundant) target_link_libraries() deps

Now this is somewhat a matter of build-system philosophy (if such a term exists 😎). The consistent approach to header includes and linking definitions is to include and link what you use, whether or not you also depend on it transitively. This is because (1) you can syntactically identify all places depending on a package and (2) if the transitive dependency at some point vanishes, the code might still work. I tend to use this approach in my projects, but I do see the benefits of leaving them out.

I will leave it to someone else to accept this as I would not do it that way myself.

henningkayser · 2020-07-18T11:48:17Z

My goal was to remove all the very generic (and often redundant) target_link_libraries() deps

Now this is somewhat a matter of build-system philosophy (if such a term exists ). The consistent approach to header includes and linking definitions is to include and link what you use, whether or not you also depend on it transitively. This is because (1) you can syntactically identify all places depending on a package and (2) if the transitive dependency at some point vanishes, the code might still work. I tend to use this approach in my projects, but I do see the benefits of leaving them out.

I will leave it to someone else to accept this as I would not do it that way myself.

I totally see your point. But then again, I only applied this "cleanup" to test executables that don't have any downstream dependencies. All other exported targets also define and export their dependencies explicitly (instead of using global includes and library lists as before). So for me it's just a question of linking/including transitive dependencies to tests which are provided by a target in the same CMakeLists file. It probably is really a matter of style/convention at some point, similar to moveit/moveit#2170.

tylerjw

Out of curiosity I ran catkin_lint on this PR and these are the only cmake errors:

moveit_planners_chomp: CMakeLists.txt(16): error: duplicate find_package(catkin)
moveit_ros_robot_interaction: CMakeLists.txt(54): error: missing test_depend on 'rosunit'
moveit_setup_assistant: CMakeLists.txt(166): error: missing test_depend on 'rosunit'

I'm not sure if these matter, but they might be worth fixing while you are refactoring some of the cmake files. I looked through these changes and they seem to make sense to me. I'll approve it as I don't have anything against these changes.

henningkayser · 2020-07-18T15:01:18Z

Out of curiosity I ran catkin_lint on this PR and these are the only cmake errors:
moveit_planners_chomp: CMakeLists.txt(16): error: duplicate find_package(catkin)
moveit_ros_robot_interaction: CMakeLists.txt(54): error: missing test_depend on 'rosunit'
moveit_setup_assistant: CMakeLists.txt(166): error: missing test_depend on 'rosunit'
I'm not sure if these matter, but they might be worth fixing while you are refactoring some of the cmake files. I looked through these changes and they seem to make sense to me. I'll approve it as I don't have anything against these changes.

@tylerjw Thanks for reviewing and testing catkin_lint on this. The failing packages have not been ported yet and have therefore not been checked by ament_lint_cmake. The reported errors should be addressed with migration - fixes are automatically enforced by CI in that case. But maybe it would make sense to enable catkin_lint in MoveIt 1.

Cleanup CMakeLists target dependencies

f499e2d

henningkayser force-pushed the pr-cmakelist_cleanup branch from dab513c to f499e2d Compare June 25, 2020 11:09

henningkayser requested review from tylerjw and v4hn June 25, 2020 11:56

v4hn reviewed Jun 25, 2020

View reviewed changes

Remove global include dependencies in moveit_core

1525b39

henningkayser force-pushed the pr-cmakelist_cleanup branch from 1f2bf1d to 1525b39 Compare June 26, 2020 07:52

tylerjw approved these changes Jul 18, 2020

View reviewed changes

henningkayser merged commit 6477cd5 into moveit:master Jul 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup CMakeLists target dependencies #228

Cleanup CMakeLists target dependencies #228

henningkayser commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading

v4hn left a comment

v4hn Jun 25, 2020

henningkayser Jun 26, 2020

v4hn Jun 25, 2020

henningkayser Jun 26, 2020

v4hn Jun 25, 2020

henningkayser Jun 26, 2020

v4hn Jun 25, 2020

henningkayser Jun 26, 2020

v4hn Jun 25, 2020

henningkayser Jun 26, 2020

henningkayser commented Jun 26, 2020 •

edited

Loading

v4hn commented Jun 26, 2020

henningkayser commented Jul 18, 2020

tylerjw left a comment

henningkayser commented Jul 18, 2020

Cleanup CMakeLists target dependencies #228

Cleanup CMakeLists target dependencies #228

Conversation

henningkayser commented Jun 25, 2020 • edited Loading

codecov bot commented Jun 25, 2020 • edited Loading

Codecov Report

v4hn left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningkayser commented Jun 26, 2020 • edited Loading

v4hn commented Jun 26, 2020

henningkayser commented Jul 18, 2020

tylerjw left a comment

Choose a reason for hiding this comment

henningkayser commented Jul 18, 2020

henningkayser commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading

henningkayser commented Jun 26, 2020 •

edited

Loading