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

Add missing package dependency #123

Merged
merged 1 commit into from Mar 5, 2020

Conversation

mintar
Copy link
Contributor

@mintar mintar commented Feb 26, 2020

No description provided.

@gavanderhoorn
Copy link
Contributor

This is indeed used by ur_robot_driver, but seeing as it's hosted in the same repository, I'm curious as to when not having this declaration there could lead to problems.

@mintar: could you provide some more information on what made you submit this PR?

@fmauch
Copy link
Collaborator

fmauch commented Feb 26, 2020

It seems to be right to me. Isn't it common practice to also include those from the same repository. As soon as we will have a binary release this will matter for sure, right?

@mintar
Copy link
Contributor Author

mintar commented Feb 26, 2020

@mintar: could you provide some more information on what made you submit this PR?

I like to use roslaunch_add_file_check in my CMakeLists. When I include one of the launch files from ur_robot_driver in mine, it leads to a test failure.

Here's a minimal working example:

temp.launch

<launch>
  <include file="$(find ur_robot_driver)/launch/ur5_bringup.launch">
    <arg name ="robot_ip" value="foo" />
  </include>
</launch>
$ rosrun roslaunch roslaunch-check temp.launch                                                                  
checking temp.launch
Missing package dependencies: ur_robot_driver/package.xml: controller_stopper
...writing test results to /home/martin/.ros/test_results/ur_robot_driver/rosunit-roslaunch_check_launch_temp_launch.xml
FAILURE:
[temp.launch]:
	Missing package dependencies: ur_robot_driver/package.xml: controller_stopper
wrote test file to [/home/martin/.ros/test_results/ur_robot_driver/rosunit-roslaunch_check_launch_temp_launch.xml]

I agree that since controller_stopper is hosted in the same repo, the missing dependency usually shouldn't cause any problems. Unless you plan to release it later: then you need the package.xml dependency in order to generate a debian package dependency.

@gavanderhoorn
Copy link
Contributor

As I wrote earlier: the suggested change is the right one, as the dependency declaration is indeed missing.

I just wanted to understand better in which context you ran into an error or problem caused by this.

Thanks for clarifying.

PS: catkin_lint also checks for these kinds of things with the recent release. It may be something to add to the CI configuration @fmauch.

@fmauch fmauch merged commit 2ee8f2b into UniversalRobots:master Mar 5, 2020
@fmauch fmauch mentioned this pull request Mar 5, 2020
@mintar mintar deleted the fix_package_deps branch June 15, 2020 08:22
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.

None yet

3 participants