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 boost linking errors for Windows #957

Merged
merged 6 commits into from
Jan 17, 2022
Merged

Fix boost linking errors for Windows #957

merged 6 commits into from
Jan 17, 2022

Conversation

Ace314159
Copy link
Contributor

Description

For Windows, #900 isn't enough to fix the boost linking errors and similar fixes were needed in background_processing and profiler. This also needs to be backported to foxy.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #957 (703e17e) into main (4804ffa) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
- Coverage   57.77%   57.77%   -0.00%     
==========================================
  Files         309      309              
  Lines       26120    26120              
==========================================
- Hits        15089    15087       -2     
- Misses      11031    11033       +2     
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 74.82% <0.00%> (-1.11%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 48.94% <0.00%> (+0.12%) ⬆️

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 4804ffa...703e17e. Read the comment docs.

Copy link
Contributor

@JafarAbdi JafarAbdi left a comment

Choose a reason for hiding this comment

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

We shouldn't need an explicit link against individual targets in Boost since we already have Boost in ament_target_dependencies

I'm on on my phone right now so I can't push the fix will do it later today

@Ace314159
Copy link
Contributor Author

Ace314159 commented Jan 5, 2022

The issue seems to arise because when doing find_package(geometric_shapes) it calls find_package(Boost ...) again with different and fewer components than what moveit_core initially calls it with, causing Boost_LIBRARIES to be overwritten. Then, ament_target_dependencies can't properly link all the Boost components.

I wasn't sure how to prevent Boost_LIBRARIES from being overwritten, so I chose to manually link. I was able to find this thread detailing a similar issue, but according to them, just maintaining the internal Boost variables should be sufficient to avoid Boost_LIBRARIES, but that doesn't seem to be the case anymore from what I observed.

If you are able to find a better fix, I'd be happy to use that instead.

@JafarAbdi
Copy link
Contributor

The issue seems to arise because when doing find_package(geometric_shapes) it calls find_package(Boost ...) again with different and fewer components than what moveit_core initially calls it with, causing Boost_LIBRARIES to be overwritten. Then, ament_target_dependencies can't properly link all the Boost components.

I wasn't sure how to prevent Boost_LIBRARIES from being overwritten, so I chose to manually link. I was able to find this thread detailing a similar issue, but according to them, just maintaining the internal Boost variables should be sufficient to avoid Boost_LIBRARIES, but that doesn't seem to be the case anymore from what I observed.

If you are able to find a better fix, I'd be happy to use that instead.

Thanks for the detailed explanation and sorry for the late response, we discussed this last moveit stand-up and we're not sure what the best solution is, we either need to
1- run include(ConfigExtras.cmake) after calling all find_packages(...) and rely on ament_* to link (we need to make sure no call to find_packages(...) depend on Boost)
2- Remove Boost from ament_target_dependencies calls and manually specify each boost component like what's done in this PR but we have to do it throughout the whole codebase

Personally, I prefer the first solution since it takes advantage of the ament functionalities

@Ace314159
Copy link
Contributor Author

Ace314159 commented Jan 11, 2022

The first solution sounds good to me too. I've made the necessary changes.

Comment on lines +52 to +54
# Finds Boost Components
include(ConfigExtras.cmake)

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you just moved this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to revert the previous commit as well. Is that what you were implying was missing?

Copy link
Member

Choose a reason for hiding this comment

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

Further up in this file you removed these two lines and then added them back here. I'm confused about what difference that would make.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per what Jafar said, the first solution involves moving the include(ConfigExtras.cmake) line after all the find_pakage calls. When we do this, the include(ConfigExtras.cmake) will be the last one to call find_package(Boost ...), so Boost_LIBRARIES will not be overwritten by any dependencies, allowing us to use ament_* to link.

That's what I did when I removed those two lines from above and added them back there.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explination. I misunderstood what this would do.

@tylerjw tylerjw added the backport-galactic Mergify label that triggers a PR backport to Galactic label Jan 13, 2022
@JafarAbdi
Copy link
Contributor

@Ace314159 Super sorry for the delay, do you mind testing this PR ? I'll merge then immediately

@vatanaksoytezer vatanaksoytezer added the backport-foxy Mergify label that triggers a PR backport to Foxy label Jan 17, 2022
@vatanaksoytezer
Copy link
Contributor

If we are reverting #900. We should also revert #925 as well.

@Ace314159
Copy link
Contributor Author

@JafarAbdi This looks good on Windows.

@JafarAbdi JafarAbdi merged commit 2efe1b2 into moveit:main Jan 17, 2022
mergify bot pushed a commit that referenced this pull request Jan 17, 2022
mergify bot pushed a commit that referenced this pull request Jan 17, 2022
bijoua29 pushed a commit to SchillingRobotics/moveit2 that referenced this pull request Jan 19, 2022
AndrejOrsula pushed a commit to AndrejOrsula/moveit2 that referenced this pull request Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-foxy Mergify label that triggers a PR backport to Foxy backport-galactic Mergify label that triggers a PR backport to Galactic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants