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

Factor out common cmake code to moveit_package macro #285

Merged
merged 10 commits into from
Oct 15, 2020

Conversation

lilustga
Copy link
Contributor

Description

Created the moveit_common package containing a moveit_package() macro. This is used to apply basic cmake code to all moveit packages.

This macro allows CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to be set for all moveit packages and allows for any new cmake code to be applied to all packages easily in the future.

Similar idea to this commit in nav2:
ms-iot/navigation2@3f65576

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

@lilustga lilustga mentioned this pull request Sep 22, 2020
6 tasks
@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #285 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #285   +/-   ##
=======================================
  Coverage   47.64%   47.64%           
=======================================
  Files         154      154           
  Lines       14844    14844           
=======================================
  Hits         7073     7073           
  Misses       7771     7771           

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 caab03c...c7f9281. Read the comment docs.

@lilustga
Copy link
Contributor Author

Looks like clang format and gcc build have passed however there's a warning from moveit_planners_ompl causing clang build to fail.

The warning is 'ompl_interface::StateValidityChecker::isValid' hides overloaded virtual function [-Woverloaded-virtual]

It could be suppressed from the moveit_planners_ompl CMakeLists.txt, or should this be fixed?

@henningkayser
Copy link
Member

Looks like clang format and gcc build have passed however there's a warning from moveit_planners_ompl causing clang build to fail.

The warning is 'ompl_interface::StateValidityChecker::isValid' hides overloaded virtual function [-Woverloaded-virtual]

It could be suppressed from the moveit_planners_ompl CMakeLists.txt, or should this be fixed?

@lilustga Well, this is strange. I don't know how this issue could have been introduced here, it should be checking for the same warnings as before. I think it should be fixed if possible.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please consider moving the moveit_common macros to the top with a short comment, I think it would be cleaner and more consistent.

moveit_core/CMakeLists.txt Outdated Show resolved Hide resolved
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 approve this approach. It is cleaner and a nice way to consolidate common cmake settings for the whole repo. Please remove the commented out code.

@lilustga
Copy link
Contributor Author

Travis clang build and code coverage look like they failed due to a transient issue. @henningkayser are you able to restart the build checks (I think it requires write access)?

@henningkayser
Copy link
Member

Travis clang build and code coverage look like they failed due to a transient issue. @henningkayser are you able to restart the build checks (I think it requires write access)?

It seems like the checks work now, but there are some unrelated CI fixes I'm currently working on

@henningkayser
Copy link
Member

CI issues are fixed with #294. After that, I would squash-merge this PR

@henningkayser henningkayser merged commit 9a479df into moveit:main Oct 15, 2020
@henningkayser henningkayser mentioned this pull request Oct 15, 2020
1 task
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.

3 participants