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

Remove false octomap from catkin_packages LIBRARIES entries #2700

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

0Nel
Copy link
Contributor

@0Nel 0Nel commented May 31, 2021

Description

Octomap is a dependency of moveit_core, but in the catkin_package description it is listed under LIBRARIES, which suggests it is a library that originates from moveit_core.

This PR is opened in order to fix this false entry, pointed out by @v4hn on another PR #2671

  Conceptually this line is probably wrong, because LIBRARIES should only list "the exported libraries from the project".
  In theory the entry OCTOMAP in DEPENDS below should do the same the right way.

  The corresponding line in moveit_core that you refer to is also 9 years old apparently and I guess nobody ever looked into it. 

Tested on a clean noetic build.

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

@0Nel 0Nel requested review from rhaschke and v4hn as code owners May 31, 2021 13:21
@codecov
Copy link

codecov bot commented May 31, 2021

Codecov Report

Merging #2700 (22cfdaa) into master (506bad2) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2700      +/-   ##
==========================================
- Coverage   60.57%   60.54%   -0.03%     
==========================================
  Files         402      402              
  Lines       29604    29615      +11     
==========================================
- Hits        17931    17927       -4     
- Misses      11673    11688      +15     
Impacted Files Coverage Δ
.../ompl_interface/src/detail/constrained_sampler.cpp 43.25% <0.00%> (-16.21%) ⬇️
...bot_model/include/moveit/robot_model/robot_model.h 72.42% <0.00%> (-11.58%) ⬇️
...eit_ros/manipulation/pick_place/src/pick_place.cpp 89.59% <0.00%> (-3.12%) ⬇️
..._motion_planner/src/move_group_sequence_action.cpp 95.62% <0.00%> (-1.65%) ⬇️
...ipulation/pick_place/src/manipulation_pipeline.cpp 73.79% <0.00%> (-0.97%) ⬇️
...e/collision_detection_fcl/src/collision_common.cpp 77.67% <0.00%> (-0.94%) ⬇️
.../collision_detection_fcl/src/collision_env_fcl.cpp 89.64% <0.00%> (-0.25%) ⬇️
...ove_group_interface/src/wrap_python_move_group.cpp 37.82% <0.00%> (-0.22%) ⬇️
moveit_core/robot_model/src/robot_model.cpp 78.84% <0.00%> (-0.13%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 50.64% <0.00%> (-0.13%) ⬇️
... and 4 more

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 506bad2...22cfdaa. 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.

Good catch!

@v4hn v4hn merged commit 45dec5b into moveit:master Jun 2, 2021
@v4hn
Copy link
Contributor

v4hn commented Jun 2, 2021

Thanks.

@0Nel
Copy link
Contributor Author

0Nel commented Jun 2, 2021

Thanks for the quick review and merge!

sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Which now also supports save-always, but is maintained much better.
See pat-s/always-upload-cache#2
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

4 participants