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 EXPORT install in CMake #372

Merged
merged 4 commits into from
Mar 9, 2021
Merged

Conversation

tylerjw
Copy link
Member

@tylerjw tylerjw commented Feb 24, 2021

Description

This should fix all EXPORTs in the install commands in cmake. This makes so --symlink-install works correctly and so you can run colcon test after colcon build without sourcing the local workspace.

One thing nice about this is it net reduces the cmake code and I think is simpler to read and replicate than what we had before that didn't work.

References

Reason for the ConfigExtras.cmake files with the Boost find_package commands: https://answers.ros.org/question/331089/ament_export_dependenciesboost-not-working/?answer=332460#post-id-332460

Code reference for the CONFIG_EXTRAS parameter to ament_package: https://github.com/ament/ament_cmake/blob/95cab1de1cd6bd7caad34d0122e11b2e86310ee2/ament_cmake_core/cmake/core/ament_package.cmake#L24-L32

The issue I created against ament_cmake documenting how I think the documentation should read about cmake install of libraries: ament/ament_cmake#317

The official documentation: https://index.ros.org/doc/ros2/Tutorials/Ament-CMake-Documentation/#building-a-library

Checklist

@tylerjw tylerjw added this to In progress in Sprint 00 via automation Feb 24, 2021
Signed-off-by: Tyler Weaver <tylerjw@gmail.com>
@tylerjw
Copy link
Member Author

tylerjw commented Feb 24, 2021

I force-pushed to this after fixing a couple of issues I discovered that this caused (ament_lint_cmake and an issue with the install of binaries in moveit_servo). Those issues should be resolved and travis should pass.

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #372 (066b86c) into main (45b9622) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   53.22%   53.25%   +0.04%     
==========================================
  Files         209      209              
  Lines       18849    18849              
==========================================
+ Hits        10030    10036       +6     
+ Misses       8819     8813       -6     
Impacted Files Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp 56.82% <0.00%> (+0.61%) ⬆️
...meterization/work_space/pose_model_state_space.cpp 84.56% <0.00%> (+1.48%) ⬆️

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 45b9622...3dce858. Read the comment docs.

Sprint 00 automation moved this from In progress to Reviewer approved Feb 24, 2021
@tylerjw
Copy link
Member Author

tylerjw commented Feb 25, 2021

I created this gist to explain the Boost stuff: https://gist.github.com/tylerjw/ce3bcb94b3df7996e450dec18ee54cde

I plan on doing a tiny lightning talk for WMD on this. It was confusing for me to figure out, hopefully, it'll help others.

moveit_core/robot_model/CMakeLists.txt Show resolved Hide resolved
moveit_kinematics/CMakeLists.txt Show resolved Hide resolved
moveit_kinematics/CMakeLists.txt Show resolved Hide resolved
find_package(eigen3_cmake_module REQUIRED)
find_package(Eigen3 REQUIRED)

# 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.

This is a really neat solution!

moveit_trajectory_rviz_plugin_core
)

set(THIS_PACKAGE_INCLUDE_DEPENDS
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be missing moveit_ros_warehouse. It wasn't exported before, but for some reason MTC failed to compile when not using a symlink ws.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@tylerjw
Copy link
Member Author

tylerjw commented Mar 8, 2021

@henningkayser @JafarAbdi I believe this PR is now ready to be merged.

@tylerjw
Copy link
Member Author

tylerjw commented Mar 8, 2021

When this is merged, please just do the squash and merge.

@henningkayser henningkayser merged commit 44e9487 into moveit:main Mar 9, 2021
Sprint 00 automation moved this from Reviewer approved to Done Mar 9, 2021
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants