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

solve catkin_lint errors #176

Open
wants to merge 1 commit into
base: kinetic-devel
Choose a base branch
from

Conversation

ipa-nhg
Copy link
Contributor

@ipa-nhg ipa-nhg commented Jul 7, 2020

Related to #26

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

A few comments in-line.

High-level comment/question: some packages may be missing a <exec_depend>message_runtime</exec_depend>, as I did see a <build_depend>message_generation</build_depend>.

@@ -2,7 +2,7 @@
<package format="2">
<name>robotiq_ft_sensor</name>
<version>1.0.0</version>
<description>Package for reading data from a Robotiq Force Torque Sensor</description>
<description><p2>Package for reading data from a Robotiq Force Torque Sensor</p2></description>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a <p> instead of a <p2>?

Also: did catkin_lint really complain about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes and no, catkin_lint gives a warning if the description of the package starts with "package for" or "package to" or "this package..." saying that this is the boilerplate informaion. I am not familiar with these packages and I don't feel comfortable changing their description, then I decided to add there a new format to makes catkin_lint happy..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

again, thanks! I will update this

Comment on lines +25 to +28
add_executable(${PROJECT_NAME}_node
include/${PROJECT_NAME}/${PROJECT_NAME}.h
src/${PROJECT_NAME}.cpp
src/${PROJECT_NAME}_node.cpp
)
Copy link
Member

Choose a reason for hiding this comment

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

I would have probably left these alone, but if catkin_lint likes this better ..

@@ -23,4 +23,6 @@
<depend>robotiq_2f_gripper_control</depend>
<depend>roscpp</depend>

<build_depend>message_generation</build_depend>
Copy link
Member

Choose a reason for hiding this comment

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

Should <exec_depend>message_runtime</exec_depend> also be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually even the message_generation dependency was not needed, this package is not generating messages. I will remove it here and for the CMakeLists

Thanks!

@@ -2,7 +2,7 @@
<package format="2">
<name>robotiq_2f_gripper_control</name>
<version>1.0.0</version>
<description>Package to control a 2-Finger Gripper from Robotiq inc.</description>
<description><p>Package to control a 2-Finger Gripper from Robotiq inc.</p></description>
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new thing for catkin_lint to complain about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(copy-paste) yes and no, catkin_lint gives a warning if the description of the package starts with "package for" or "package to" or "this package..." saying that this is the boilerplate informaion. I am not familiar with these packages and I don't feel comfortable changing their description, then I decided to add there a new format to makes catkin_lint happy..

@@ -27,6 +27,7 @@

<build_depend>angles</build_depend>
<build_depend>message_generation</build_depend>
<build_depend>gazebo</build_depend>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be gazebo_ros instead?

Copy link
Contributor Author

@ipa-nhg ipa-nhg Jul 7, 2020

Choose a reason for hiding this comment

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

no, gazebo_ros is already listed. Catkin_lint gave the following error:

robotiq_3f_gripper_articulated_gazebo_plugins: error: missing build_depend on 'gazebo'
catkin_lint: checked 17 packages and found 1 problems

@@ -2,7 +2,7 @@
<package format="2">
<name>robotiq_3f_gripper_control</name>
<version>1.0.0</version>
<description>Package to control a 3F gripper Gripper from Robotiq inc.</description>
<description><p2>Package to control a 3F gripper Gripper from Robotiq inc.</p2></description>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<description><p2>Package to control a 3F gripper Gripper from Robotiq inc.</p2></description>
<description><p>Package to control a 3F gripper Gripper from Robotiq inc.</p></description>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups! Thanks, I will modify this

@gavanderhoorn
Copy link
Member

Thanks for addressing my comments.

I'm going to leave a detailed review to @jproberge, as he is the current maintainer of these packages.

Given your result:

robotiq_3f_gripper_articulated_gazebo_plugins: error: missing build_depend on 'gazebo'
catkin_lint: checked 17 packages and found 1 problems

I suggest we mark this one as conditionally merged and move the card on the WRID20 project board to completed.

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.

2 participants