do not fail if other package.xml provides package required by k-okada · Pull Request #730 · ros/ros_comm

k-okada · 2016-01-19T10:54:39Z

roslaunch-check fails for heqtor_quadrotor_gazebo

$ rosrun roslaunch roslaunch-check  /opt/ros/indigo/share/hector_quadrotor_gazebo/launch/quadrotor_empty_world.launch 
checking /opt/ros/indigo/share/hector_quadrotor_gazebo/launch/quadrotor_empty_world.launch
...writing test results to /home/k-okada/.ros/test_results/hector_quadrotor_gazebo/rosunit-roslaunch_check_quadrotor_empty_world_launch.xml
FAILURE:
[/opt/ros/indigo/share/hector_quadrotor_gazebo/launch/quadrotor_empty_world.launch]:
    Missing package dependencies: hector_quadrotor_gazebo/package.xml: gazebo_ros, hector_quadrotor_controller
Missing package dependencies: hector_quadrotor_controller/package.xml: controller_manager

but it has dependency like

$ rospack depends-why --target gazebo_ros hector_quadrotor_gazebo 
Dependency chains from hector_quadrotor_gazebo to gazebo_ros:
* hector_quadrotor_gazebo -> gazebo_plugins -> gazebo_ros 
* hector_quadrotor_gazebo -> hector_gazebo_plugins -> gazebo_ros 
* hector_quadrotor_gazebo -> hector_sensors_gazebo -> gazebo_plugins -> gazebo_ros 
* hector_quadrotor_gazebo -> hector_sensors_gazebo -> hector_gazebo_plugins -> gazebo_ros 
* hector_quadrotor_gazebo -> hector_quadrotor_gazebo_plugins -> gazebo_ros 
* hector_quadrotor_gazebo -> hector_quadrotor_gazebo_plugins -> hector_gazebo_plugins -> gazebo_ros 
* hector_quadrotor_gazebo -> hector_quadrotor_controller_gazebo -> gazebo_ros_control -> gazebo_ros

tfoote · 2016-01-21T00:14:46Z

Can you clarify which cases this is covering? It sounds like your're allowing launch files with transitive dependencies. If the files are referenced locally I would consider that correct behavior, since if the transitive dependency gets removed then the launch file will break. If you're transitively using the launch files through other launch files it should be ok.

k-okada · 2016-01-21T02:13:30Z

to reproduce this

create package

catkin_create_pkg hoge gazebo_ros hector_quadrotor_gazebo gazebo_ros hector_quadrotor_controller

put launch file

$ cat hoge/hoge.launch 
<launch>
  <include file='$(find hector_quadrotor_gazebo)/launch/quadrotor_empty_world.launch' />
</launch>

run roslaunch-check

k-okada@kokada-t440s:/tmp/aaa/src$ rosrun roslaunch roslaunch-check hoge/hoge.launch
checking hoge/hoge.launch
^P...writing test results to /home/k-okada/.ros/test_results/hoge/rosunit-roslaunch_check_hoge_launch.xml
FAILURE:
[hoge/hoge.launch]:
    Missing package dependencies: hector_quadrotor_gazebo/package.xml: gazebo_ros, hector_quadrotor_controller
Missing package dependencies: hector_quadrotor_controller/package.xml: controller_manager
wrote test file to [/home/k-okada/.ros/test_results/hoge/rosunit-roslaunch_check_hoge_launch.xml]

it is true that hector_quadrotor_gazebo does not have correct dependency for gazebo_ros, but if you look this from hoge package, it has dependency to the gazebo_ros so hoge package itself works. It is very painful to track all upstream package to correct dependency.

k-okada@kokada-t440s:/tmp/aaa/src$ rospack depends hoge | grep gazebo
gazebo_msgs
gazebo_ros
gazebo_plugins
hector_gazebo_plugins
hector_sensors_gazebo
hector_quadrotor_gazebo_plugins
gazebo_ros_control
hector_quadrotor_controller_gazebo
hector_quadrotor_gazebo

dirk-thomas · 2016-03-01T20:56:59Z

tools/roslaunch/src/roslaunch/rlutil.py

        file_deps = {}
    for pkg, miss in missing.items():
        if miss:
+            print("Missing package dependencies: %s/package.xml: %s"%(pkg, ', '.join(miss)))


Why should this message be still printed? I thought the point was that this is not actually a problem?

I think missing package dependencies should be fixed, the point is upstream package dependencies can not be fixed by downstream maintainers, so test should not be fail. But printing this is useful for downstream maintainers to notify upstream maintainers.

print("Missing package dependencies: %s/package.xml: %s (notify upstream maintainer)"%(pkg, ', '.join(miss)))

added message.

dirk-thomas · 2016-03-01T20:58:07Z

The CI build for this PR reports numerous failing tests. Please look into them and resolve them.

k-okada · 2016-03-05T11:58:20Z

OK fixed the patch and now test passed, please review.

dirk-thomas · 2016-03-08T19:28:51Z

I don't think this patch achieve the goal at all. From my point of view that check for one launch file should ensure that the containing package contains the necessary dependencies to everything which is being directly using within this launch file. Recursively included launch files should not be checked since they should be covered by the dependencies of the package containing that launch file.

Currently file_deps returned by roslaunch.depends.roslaunch_deps already contains recursive information. The patched code has no way to distinguish between direct file dependencies and transitive file dependencies. Hence it can't warn about the first case while being quiet about the second.

k-okada · 2016-03-09T00:26:28Z

Current implementation fails if upstream launch file did not contains the
necessary dependencies, even if my package depends on that package, for
example current fetch_navigation fails to include necessary dependencies
(see https://github.com/fetchrobotics/fetch_ros/pull/35)
and if you have package that depends on fetch_navigation it fails as

$ rosrun roslaunch roslaunch-check  fetch_bringup.launch
checking fetch_bringup.launch
...writing test results to
/home/k-okada/.ros/test_results/jsk_fetch_startup/rosunit-roslaunch_check_fetch_bringup_launch.xml
FAILURE:
[fetch_bringup.launch]:
Missing package dependencies: fetch_navigation/package.xml: move_base,
map_server, amcl
wrote test file to
[/home/k-okada/.ros/test_results/jsk_fetch_startup/rosunit-roslaunch_check_fetch_bringup_launch.xml]

This mean if we want to add roslaunch_add_file_check to our CMakeLists.txt,
we have to wait upstream maintainer for fixing this problem.

with this PR,

k-okada@kokada-t440s:~/catkin_ws/ws_fetch/src/jsk_robot/jsk_fetch_robot/jsk_fetch_startup/launch$
rosrun roslaunch roslaunch-check  fetch_bringup.launch
checking fetch_bringup.launch
Missing package dependencies: fetch_navigation/package.xml: move_base,
map_server, amcl (notify upstream maintainer)
...writing test results to
/home/k-okada/.ros/test_results/jsk_fetch_startup/rosunit-roslaunch_check_fetch_bringup_launch.xml
passed
k-okada@kokada-t440s:~/catkin_ws/ws_fetch/src/jsk_robot/jsk_fetch_robot/jsk_fetch_startup/launch$
cat ../package.xml
<package>
  <name>jsk_fetch_startup</name>
  <version>1.0.3</version>
  <description>
     jsk_fetch_startup

     data/boost_sound.wav is downloaded from
https://www.youtube.com/watch?v=HM-xG0qXaeA
  </description>
  <maintainer email="k-okada@jsk.t.u-tokyo.ac.jp">Kei Okada</maintainer>
  <license>BSD</license>
  <url>http://ros.org/wiki/jsk_fetch_startup</url>
  <buildtool_depend>catkin</buildtool_depend>

  <run_depend>jsk_robot_startup</run_depend>
  <run_depend>jsk_maps</run_depend>
  <run_depend>jsk_pr2_startup</run_depend>
  <run_depend>fetch_navigation</run_depend>
  <run_depend>rviz</run_depend>
  <run_depend>tf2_ros</run_depend>

  <!-- for fetch_naviation -->
  <run_depend>amcl</run_depend>
  <run_depend>map_server</run_depend>
  <run_depend>move_base</run_depend>

  <test_depend>rostest</test_depend>

  <export>
  </export>
</package>

◉ Kei Okada

On Wed, Mar 9, 2016 at 4:28 AM, Dirk Thomas notifications@github.com
wrote:

I don't think this patch achieve the goal at all. From my point of view
that check for one launch file should ensure that the containing package
contains the necessary dependencies to everything which is being directly
using within this launch file. Recursively included launch files should not
be checked since they should be covered by the dependencies of the package
containing that launch file.

Currently file_deps returned by roslaunch.depends.roslaunch_deps already
contains recursive information. The patched code has no way to distinguish
between direct file dependencies and transitive file dependencies. Hence it
can't warn about the first case while being quiet about the second.

—
Reply to this email directly or view it on GitHub
#730 (comment).

dirk-thomas · 2016-03-09T01:04:58Z

So your test would pass but it wouldn't actually work? I am not sure if that is a useful test.

k-okada · 2016-03-09T01:15:50Z

No

both an upstream and my package has correct dependency
an upstream package does not have correct dependency and my package does not include missing dependency
an upstream package does not have correct dependency , but my package covers missing dependency

In current implementation, 1 passes the test, but 2 and 3 fails
this PR, both.1,3 pass the test, and only 2 fails

◉ Kei Okada

2016/03/09 10:04、Dirk Thomas notifications@github.com のメッセージ:

So your test would pass but it wouldn't actually work? I am not sure if that is a useful test.

—
Reply to this email directly or view it on GitHub.

dirk-thomas · 2016-03-09T01:25:55Z

tools/roslaunch/src/roslaunch/rlutil.py

+                all_pkgs.extend(rospack.get_depends(file_pkg,implicit=False))
+                miss_all = list(set(miss) - set(all_pkgs))
+        except Exception as e:
+            print(e)


Since this is an error message please print it to stderr using:

print(e, file=sys.stderr)

dirk-thomas · 2016-03-09T01:26:25Z

Without a clear example to reproduce and follow what you are running it is really difficult to understand current and new expected behavior. I just had a very hard time following this.

Anyway I have tried the cases you mentioned and the behavior with this patch is for sure better then without. I have only small comment left regarding `print. If you could address that one and squash I am happy to merge this contribution.

roslaunch-check fails for heqtor_quadrotor_gazebo ``` $ rosrun roslaunch roslaunch-check /opt/ros/indigo/share/hector_quadrotor_gazebo/launch/quadrotor_empty_world.launch checking /opt/ros/indigo/share/hector_quadrotor_gazebo/launch/quadrotor_empty_world.launch ...writing test results to /home/k-okada/.ros/test_results/hector_quadrotor_gazebo/rosunit-roslaunch_check_quadrotor_empty_world_launch.xml FAILURE: [/opt/ros/indigo/share/hector_quadrotor_gazebo/launch/quadrotor_empty_world.launch]: Missing package dependencies: hector_quadrotor_gazebo/package.xml: gazebo_ros, hector_quadrotor_controller Missing package dependencies: hector_quadrotor_controller/package.xml: controller_manager ``` but it has dependency like ``` $ rospack depends-why --target gazebo_ros hector_quadrotor_gazebo Dependency chains from hector_quadrotor_gazebo to gazebo_ros: * hector_quadrotor_gazebo -> gazebo_plugins -> gazebo_ros * hector_quadrotor_gazebo -> hector_gazebo_plugins -> gazebo_ros * hector_quadrotor_gazebo -> hector_sensors_gazebo -> gazebo_plugins -> gazebo_ros * hector_quadrotor_gazebo -> hector_sensors_gazebo -> hector_gazebo_plugins -> gazebo_ros * hector_quadrotor_gazebo -> hector_quadrotor_gazebo_plugins -> gazebo_ros * hector_quadrotor_gazebo -> hector_quadrotor_gazebo_plugins -> hector_gazebo_plugins -> gazebo_ros * hector_quadrotor_gazebo -> hector_quadrotor_controller_gazebo -> gazebo_ros_control -> gazebo_ros ```

k-okada · 2016-03-09T01:53:15Z

ok fixed the patch and squashed,

and added example https://github.com/k-okada/roslaunch_730, which is basically follow the instructions in -> #730 (comment)
if you run catkin run_test and catkin_test_results you'll see both package fails without this PR

dirk-thomas · 2016-03-09T06:38:23Z

Thank you for the patch and iterating on it.

do not fail if other package.xml provides package required

k-okada added a commit to k-okada/jsk_mbzirc that referenced this pull request Jan 19, 2016

use ros/ros_comm#730 version of roslaunch-check

601eddb

k-okada added a commit to k-okada/jsk_mbzirc that referenced this pull request Jan 21, 2016

jsk_mbzirc_task2.launch is also needs ros/ros_comm#730 due to husky

be29018

dirk-thomas reviewed Mar 1, 2016
View reviewed changes

dirk-thomas added the requires-changes label Mar 3, 2016

k-okada force-pushed the patch-4 branch 2 times, most recently from cc50a0a to f9fb7d6 Compare March 5, 2016 07:38

dirk-thomas reviewed Mar 9, 2016
View reviewed changes

k-okada force-pushed the patch-4 branch from f9fb7d6 to b76a7f3 Compare March 9, 2016 01:37

dirk-thomas added a commit that referenced this pull request Mar 9, 2016

Merge pull request #730 from k-okada/patch-4

bee9d5e

do not fail if other package.xml provides package required

dirk-thomas merged commit bee9d5e into ros:indigo-devel Mar 9, 2016

dirk-thomas added enhancement and removed requires-changes labels Mar 9, 2016

k-okada deleted the patch-4 branch March 9, 2016 09:15

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017

Merge pull request ros#730 from k-okada/patch-4

d1a9533

do not fail if other package.xml provides package required

Conversation

k-okada commented Jan 19, 2016

Uh oh!

tfoote commented Jan 21, 2016

Uh oh!

k-okada commented Jan 21, 2016

Uh oh!

dirk-thomas Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

k-okada Mar 5, 2016

Choose a reason for hiding this comment

Uh oh!

k-okada Mar 5, 2016

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Mar 1, 2016

Uh oh!

k-okada commented Mar 5, 2016

Uh oh!

dirk-thomas commented Mar 8, 2016

Uh oh!

k-okada commented Mar 9, 2016

Uh oh!

dirk-thomas commented Mar 9, 2016

Uh oh!

k-okada commented Mar 9, 2016

Uh oh!

dirk-thomas Mar 9, 2016

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Mar 9, 2016

Uh oh!

k-okada commented Mar 9, 2016

Uh oh!

dirk-thomas commented Mar 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants