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

Handle multiple link libraries for FCL #2325

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Conversation

rhaschke
Copy link
Contributor

This is in preparation to use fcl 0.6.1 in Noetic. See ros/rosdistro#26527 for details.
Our cmake code to find absolute library paths of FCL libs was only handling a single lib (namely libfcl). However, future releases of libfcl declare several link libraries. To correctly link to those, we need to find absolute paths for all of them.

@codecov
Copy link

codecov bot commented Sep 26, 2020

Codecov Report

Merging #2325 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2325      +/-   ##
==========================================
- Coverage   57.93%   57.75%   -0.17%     
==========================================
  Files         327      327              
  Lines       25633    25633              
==========================================
- Hits        14848    14802      -46     
- Misses      10785    10831      +46     
Impacted Files Coverage Δ
.../move_group_interface/src/move_group_interface.cpp 44.45% <0.00%> (-3.63%) ⬇️
...e/src/parameterization/model_based_state_space.cpp 72.23% <0.00%> (-2.77%) ⬇️
moveit_core/robot_model/src/joint_model_group.cpp 59.34% <0.00%> (-2.71%) ⬇️
...ipulation/pick_place/src/manipulation_pipeline.cpp 76.42% <0.00%> (-0.94%) ⬇️
...meterization/work_space/pose_model_state_space.cpp 82.32% <0.00%> (-0.68%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 47.40% <0.00%> (+0.10%) ⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 79.38% <0.00%> (+0.45%) ⬆️
...on/pick_place/src/approach_and_translate_stage.cpp 73.60% <0.00%> (+0.80%) ⬆️

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 1d8d51e...ed502c6. Read the comment docs.

find_path(LIBFCL_INCLUDE_DIRS fcl/config.h HINTS ${LIBFCL_PC_INCLUDE_DIR} ${LIBFCL_PC_INCLUDE_DIRS})
find_library(LIBFCL_LIBRARIES fcl HINTS ${LIBFCL_PC_LIBRARY_DIRS})

set(LIBFCL_INCLUDE_DIRS ${LIBFCL_PC_INCLUDE_DIRS})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newer versions of fcl (ie not the system version in noetic ... what a mess ...) also provide a cmake config. So maybe

find_package(fcl REQUIRED)

already does the trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I tried this before. It works here, locally. But exporting the required fcl libs to downstream packages didn't work:
catkin provides two mechanisms to do this:

  1. For catkin packages via CATKIN_DEPENDS and
  2. for 3rd-party packages via DEPENDS in catkin_package().

The latter - required here - requires to declare LIBFCL_INCLUDE_DIRS and LIBFCL_LIBRARIES. But, I failed to assemble LIBFCL_LIBRARIES because fcl's cmake config declares an imported library target. Manually accessing IMPORTED_LOCATION and INTERFACE_LINK_LIBRARIES of that target was a nightmare and I gave up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, who besides collision_detection_fcl needs that dependency? But looking at the documentation of catkin_package I see your problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who besides collision_detection_fcl needs that dependency?

moveit_compare_collision_checking_speed_fcl_bullet was failing to link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the clean and lean find_package could be moved to these two directly? Normally the fcl dependency should be abstracted away right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. I'm not sure moveit_compare_collision_checking_speed_fcl_bullet explicitly uses fcl functionality. If so, it should find_package(fcl) itself, I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the fact that we need to handle FCL 0.5 for Melodic, we are stuck to pkgconfig for now.
Hence, I would like to get this basic fix merged as is.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Makes sense to me.

@v4hn v4hn merged commit 76ea533 into moveit:master Sep 30, 2020
@rhaschke rhaschke deleted the fix-fcl branch September 30, 2020 10:20
@tylerjw tylerjw mentioned this pull request Oct 23, 2020
57 tasks
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request Oct 23, 2020
tylerjw pushed a commit to tylerjw/moveit that referenced this pull request Oct 27, 2020
tylerjw pushed a commit that referenced this pull request Nov 19, 2020
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.

None yet

3 participants