-
Notifications
You must be signed in to change notification settings - Fork 493
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
Compile moveit_core #117
Compile moveit_core #117
Conversation
@JafarAbdi can you build this following the instructions in the Readme? |
d354e59
to
860cb96
Compare
@henningkayser - Yep, I was able to build it successfully following the README instructions, Thanks! Should I update the loggers for the following files and open a PR against this branch .? |
You mean changing the namespace or the name of the constant? I think we can leave it as is for now and revise once we actually decided on a fixed pattern. Please open an issue for this and I will address it in the following weeks (probably for the next sprint). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving forward, I'd prefer to see PRs more separated: for example we need a PR that changes logger across the whole code base, once and for all, rather than mixed with other changes per package. This will allow better use of find-replace, and a faster migration.
set(COVERAGE_EXCLUDES "*/test/*") | ||
add_code_coverage(NAME ${PROJECT_NAME}_coverage) | ||
endif() | ||
# TODO(henningkayser): enable once code_coverage is ported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeferguson want to port your code_coverage to ros2? ;-)
I totally agree. This PR was started as a simple fixup of #90. All the additional changes are necessary in order to get |
@henningkayser I think #119 only served as a test to the intermediate state of this PR, arm I right? If I would like to fix up the warnings, which branch I should submit PR to? |
@RoboticsYY You are right, the referenced PR was only a test. If your changes apply to this PR you could file it directly against the feature branch. It would also really help if you could take a look at the warnings from the package dependencies, like for instance |
51f9014
to
808afc7
Compare
fed7ffb
to
289d48d
Compare
Please push on creating a STYLE_GUIDE.md and document away all your preferences beyond what we have in https://moveit.ros.org/documentation/contributing/code/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few small comments/suggestions, but everything looks good.
289d48d
to
6417b16
Compare
* Fix compile warnings in moveit_core * Disable code_coverage * Add missing headers to fix '--symlink-install' * Fix include exports * Suppress CMake Warning Co-authored-by: vmayoral <v.mayoralv@gmail.com>, henningkayser <henningkayser@picknik.ai>
* collision_detection * collision_distance_field * moveit_utils * moveit_profiler * distance_field * robot_model * moveit_transforms
* Install libfcl-dev and ros-dashing-angles before CI
6417b16
to
362f983
Compare
This is a fixed-up version of #90.
I forked (and fixed) all pending dependencies, fixed build and linker errors and started to improve usage of ament. Currently, there are still several warnings to be fixed. Also, I would like to unify the use of ament in all package (each package should handle dependencies and exports itself, similar to ros1 versions).