moveit / geometric_shapes

Representation of geometric shapes
57 stars 92 forks source link

Fix cmake such that Boost::filesystem is exported properly #206

Closed JafarAbdi closed 2 years ago

JafarAbdi commented 2 years ago

Reopen https://github.com/ros-planning/geometric_shapes/pull/205, see my comment https://github.com/ros-planning/geometric_shapes/pull/205#issuecomment-956245216 :(

JafarAbdi commented 2 years ago

@jlack1987 please review https://github.com/jlack1987/geometric_shapes/pull/1

jlack1987 commented 2 years ago

@JafarAbdi your change there doesn't fix the issue. The problem is that packages depending on geometric_shapes can't properly link against it because the boost components aren't properly exported

JafarAbdi commented 2 years ago

@JafarAbdi your change there doesn't fix the issue. The problem is that packages depending on geometric_shapes can't properly link against it because the boost components aren't properly exported

I don't think this should be the case, we have been using this method with moveit2 and linking against it without any problem, do you mind trying a clean build .?

jlack1987 commented 2 years ago

I'm betting you're picking up Boost::filesystem from some other package you depend on then and that's how it's working. If you go into one of your packages that depends on geometric_shapes and do message(STATUS ${geometric_shapes_LIBRARIES}) you'll see that it does not include Boost::filesystem. That's how I discovered that geometric_shapes was where the problem was.

JafarAbdi commented 2 years ago

Out of curiosity, how are you linking your code against geometric_shapes, is it possible to share your CMakeLists .? thanks!

JafarAbdi commented 2 years ago

I'm betting you're picking up Boost::filesystem from some other package you depend on then and that's how it's working. If you go into one of your packages that depends on geometric_shapes and do message(STATUS ${geometric_shapes_LIBRARIES}) you'll see that it does not include Boost::filesystem. That's how I discovered that geometric_shapes was where the problem was.

Actually, I do see it in ${geometric_shapes_LIBRARIES} tested on rolling

jlack1987 commented 2 years ago

I can't really share the entire CMakeLists but it's nothing special, just

add_library(${PROJECT_NAME}_collision SHARED src/collision_monitor.cpp)
ament_target_dependencies(${PROJECT_NAME}_collision geometric_shapes)

I have more libraries in the ament_target_dependencies line in practice, but the way I found out that geometric_shapes was the problem was I commented out all the code from the library and started removing dependencies from that line and it failed to build until I removed geometric_shapes so then I went digging and have addressed this issue with other libraries so I knew what to look for

henningkayser commented 2 years ago

From previous PR discussion: I think we should include the cmake config instead of the Boost include like in moveit_core. That way, all Boost components are defined in one place which is easier to maintain and less error-prone.

jlack1987 commented 2 years ago

Yeah i'll leave that up to you all. Just pointing out that the way it exists right now unless that Boost::filesystem lib is picked up by some other dependency you can't depend on this package.

jlack1987 commented 2 years ago

Made recommended changes. Let me know if there's anything else I need to do. Thanks

codecov[bot] commented 2 years ago

Codecov Report

Merging #206 (c7c2c34) into ros2 (44d2b7f) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             ros2     #206   +/-   ##
=======================================
  Coverage   54.66%   54.66%           
=======================================
  Files          11       11           
  Lines        2031     2031           
=======================================
  Hits         1110     1110           
  Misses        921      921           

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 44d2b7f...c7c2c34. Read the comment docs.

jlack1987 commented 2 years ago

Yeah np, any chance we could cherry-pick this back into foxy?

tylerjw commented 2 years ago

I don't see why not. Please just open this change to the foxy branch and I'll happily merge it.