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.

Sign up for GitHub

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

Jump to bottom

Fix moveit_core building #1553

Merged
merged 5 commits into from
Sep 7, 2022
Merged

Fix moveit_core building #1553

merged 5 commits into from
Sep 7, 2022

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Sep 6, 2022 •
edited
Loading

Trying to build branch main on Galactic, I ran into the following compiler error:

collision_matrix.cpp: In constructor ‘collision_detection::AllowedCollisionMatrix::AllowedCollisionMatrix(const srdf::Model&)’:
collision_matrix.cpp:62:39: error: ‘const class srdf::Model’ has no member named ‘getNoDefaultCollisionLinks’

There were actually two issues (having srdfdom part of the current workspace):

  • srdfdom was missing as an explicit dependency. Its includes were only found accidentally in /opt/ros
  • The include order was screwed because moveit_planning_scene's include directories (not comprising srdfdom but /opt/ros) were included before all other includes. The latter are pulled in via ament_target_dependencies(), which handles correct ordering for the overlay. Thus srdfdom was found in /opt/ros instead of the local overlay.

Skimming through the cmake files, I noticed inconsistent naming of export targets: Most packages used export_${PROJECT_NAME}, while some also used the standard (by convention) ${PROJECT_NAME}Targets. I changed all to the latter.

@codecov
Copy link

codecov bot commented Sep 6, 2022 •
edited
Loading

Codecov Report

Merging #1553 (52acc36) into main (0faa583) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 52acc36 differs from pull request most recent head ba62fe3. Consider uploading reports for the commit ba62fe3 to get more accurate results

@@            Coverage Diff             @@
##             main    #1553      +/-   ##
==========================================
+ Coverage   51.10%   51.10%   +0.01%     
==========================================
  Files         380      380              
  Lines       31802    31802              
==========================================
+ Hits        16248    16249       +1     
+ Misses      15554    15553       -1
Impacted Files Coverage Δ
moveit_core/robot_state/src/robot_state.cpp 47.91% <0.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rhaschke rhaschke marked this pull request as draft September 6, 2022 07:35
@henningkayser
Copy link
Member

Why would you re-enable Galactic support on main? We dropped it a while ago because of API breakages, Focal, etc...

@rhaschke
Copy link
Contributor Author

rhaschke commented Sep 6, 2022

Why would you re-enable Galactic support on main? We dropped it a while ago because of API breakages, Focal, etc...

I'm running Focal. However, all fixes suggested here are independent of Galactic. Galactic-specific patches I will keep locally only.

@rhaschke rhaschke marked this pull request as ready for review September 6, 2022 12:38
rhaschke
rhaschke commented Sep 6, 2022
View reviewed changes
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
@@ -185,8 +185,7 @@ class MoveGroupInterface::MoveGroupInterfaceImpl
if (constraints_init_thread_)
constraints_init_thread_->join();

if (callback_executor_.is_spinning())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the need to test for spinning. cancel() should also work when the executor isn't spinning (anymore).
That's the only Galactic-specific change I propose here.

@mergify
Copy link

mergify bot commented Sep 6, 2022

This pull request is in conflict. Could you fix it @rhaschke?

tylerjw
tylerjw approved these changes Sep 6, 2022
View reviewed changes
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.

These all look straightforward and are a nice cleanup of the export name.

@rhaschke
Add missing srdfdom dependency
a36607f
@rhaschke
moveit_core/collision_detection: fix include order
072482c 
moveit_planning_scene's include directories have to be appended
to the include directories found by ament_target_dependencies().
@rhaschke
Copy link
Contributor Author

rhaschke commented Sep 6, 2022

@AndyZe, looks like test_servo_singularity is flaky!

@AndyZe
Copy link
Member

AndyZe commented Sep 6, 2022

Sorry. I thought it was fixed recently in #1543 (which passed CI 5x successfully) but I guess we'll have to disable it again, for now.

@rhaschke
Use standard exported targets: export_${PROJECT_NAME} -> ${PROJECT_NA…
1e6af35 
…ME}Targets
@rhaschke
Cleanup cmake files
7b01290 
- Replace ament_export_libraries() -> ament_export_targets(HAS_LIBRARY_TARGET)
- Replace ament_export_include_directories() -> INCLUDES DESTINATION include

See https://docs.ros.org/en/foxy/How-To-Guides/Ament-CMake-Documentation.html#building-a-library
@rhaschke
Remove callback_executor_.is_spinning()
ba62fe3
@rhaschke rhaschke merged commit ed197cc into moveit:main Sep 7, 2022
@rhaschke rhaschke deleted the fix-build branch September 7, 2022 06:23
@bijoua29 bijoua29 mentioned this pull request Oct 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerjw tylerjw tylerjw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rhaschke @henningkayser @AndyZe @tylerjw