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

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

Merged
merged 1 commit into from
Dec 8, 2021
Merged

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

merged 1 commit into from
Dec 8, 2021

Conversation

JafarAbdi
Copy link
Member

@JafarAbdi JafarAbdi commented Nov 1, 2021

Reopen #205, see my comment #205 (comment) :(

@JafarAbdi
Copy link
Member Author

@jlack1987
Copy link

@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
Copy link
Member Author

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

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

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

@JafarAbdi
Copy link
Member Author

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

jlack1987 commented Nov 1, 2021

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

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

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

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

@codecov
Copy link

codecov bot commented Dec 8, 2021

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.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

I've had to make this change in other places before. Thank you for doing this.

@tylerjw tylerjw merged commit 26099ec into moveit:ros2 Dec 8, 2021
@jlack1987
Copy link

jlack1987 commented Dec 8, 2021

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

@tylerjw
Copy link
Member

tylerjw commented Dec 8, 2021

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

jlack1987 pushed a commit to jlack1987/geometric_shapes that referenced this pull request Dec 8, 2021
Co-authored-by: Jordan Lack <jlack@houstonmechatronics.com>
tylerjw pushed a commit that referenced this pull request Dec 9, 2021
Co-authored-by: Jafar Abdi <cafer.abdi@gmail.com>
Co-authored-by: Jordan Lack <jlack@houstonmechatronics.com>
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.

4 participants