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 catkin_lint issues #1341

Merged
merged 4 commits into from Feb 7, 2019
Merged

Conversation

@rhaschke
Copy link
Collaborator

rhaschke commented Feb 7, 2019

Fix various catkin_lint issues and enable catkin_lint checker for Travis.

@rhaschke rhaschke force-pushed the ubi-agni:fix-catkin-lint branch from 39b6388 to f169bc2 Feb 7, 2019
rhaschke added 3 commits Feb 7, 2019
- package.xml format=2
- fix dependencies
- remove link_directories()
@rhaschke rhaschke force-pushed the ubi-agni:fix-catkin-lint branch from f169bc2 to 83e4854 Feb 7, 2019
@jonbinney

This comment has been minimized.

Copy link
Contributor

jonbinney commented Feb 7, 2019

Wow, a ton of good fixes here!

@rhaschke rhaschke requested a review from v4hn Feb 7, 2019
Copy link
Contributor

jonbinney left a comment

Looks good! I left a couple of small questions, just for my own understanding.

@@ -37,7 +37,7 @@

#include <gtest/gtest.h>
#include <memory>
#include <boost/thread/mutex.hpp>
#include <boost/bind.hpp>

This comment has been minimized.

Copy link
@jonbinney

jonbinney Feb 7, 2019

Contributor

Were we transitively getting this header through boost/thread/mutex.hpp before?

This comment has been minimized.

Copy link
@rhaschke

rhaschke Feb 7, 2019

Author Collaborator

I guess it came from somewhere else. Don't think it's in thread...

@@ -98,8 +94,7 @@ include_directories(rviz_plugin_render_tools/include

include_directories(SYSTEM
${EIGEN3_INCLUDE_DIRS}
${QT_INCLUDE_DIR}
${OGRE_INCLUDE_DIRS})

This comment has been minimized.

Copy link
@jonbinney

jonbinney Feb 7, 2019

Contributor

Is this getting removed because we get the OGRE_INCLUDE_DIRS from rviz already?

This comment has been minimized.

Copy link
@rhaschke

rhaschke Feb 7, 2019

Author Collaborator

Exactly.

@jonbinney

This comment has been minimized.

Copy link
Contributor

jonbinney commented Feb 7, 2019

I've tried to summarize the types of changes made in this PR, to convince myself its all fine (it is):

  • Change package.xml to format 2
  • Update run_depend to exec_depend
  • Add REQUIRED for some find_package calls in cmake.
  • Surpress warnings that raen't actuall a problem
  • Add missing find_package calls in cmake
  • Add missing catkin_depend's in cmake
  • Only run coverage tests if CATKIN_ENABLE_TESTING is set
  • Quote variable exapansions in cmake
  • Add missing test_depends in package.xml
  • Don't hardcode library install destination; use CATKIN_GLOBAL_LIB_DESTINATION
  • "" For include install paths
  • Install script that wasn't getting installed
  • Include the right file (boost/bind)
  • ompl_interface gets a system depend on OMPL in cmake catkin_package
  • Re-order cmakelists to have find_package calls before include_directories calls
  • Export plugin libraries via catkin_package in moveit_ros_control_interface
  • Simplify build/run depends into one depend in package.xmls
@rhaschke rhaschke merged commit 8075221 into ros-planning:melodic-devel Feb 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

rhaschke commented Feb 7, 2019

@jonbinney Thanks for the thorough review and nice summary!

@rhaschke rhaschke deleted the ubi-agni:fix-catkin-lint branch Feb 7, 2019
@rhaschke rhaschke mentioned this pull request Feb 12, 2019
rhaschke added a commit that referenced this pull request Mar 3, 2019
... that were removed in #1341.
MohmadAyman added a commit to MohmadAyman/moveit that referenced this pull request Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.