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 get_depends returning 1st run) none then on 2nd run) fewer pkgs #144

Conversation

130s
Copy link
Contributor

@130s 130s commented Jun 25, 2018

Issue: As described in #142.

Suggested solution

Note that I'm not adding a test case for this specific issue. To reproduce this issue requires traversing the dependency chain AFAICT, but looks like CI environment is kept plain so no ros pkgs are installed thus hard to replicate the environment. get_depends is tested in multiple test cases (e.g. test_rospkg_packages.py).

Output

  • Without this PR

See the output in #142 (comment)

  • With this PR
root@d5a64a312993:~# SRCDIR=~/cws/src && mkdir -p $SRCDIR && git clone https://github.com/plusone-robotics/rospkg -b fix_get_depends_issue142
root@d5a64a312993:~/cws/src/rospkg# export PYTHONPATH=$SRCDIR/rospkg/src:$PYTHONPATH
root@d5a64a312993:~/cws/src/rospkg# export PATH=$SRCDIR/rospkg/scripts:$PATH
root@d5a64a312993:~/cws/src/rospkg# ipython

In [1]: import rospkg
In [2]: rp = rospkg.RosPack()
In [3]: rp.get_depends("roscpp")
---------------------------------------------------------------------------
ResourceNotFound                          Traceback (most recent call last)
<ipython-input-3-b793dd583b6b> in <module>()
----> 1 rp.get_depends("roscpp")

/root/cws/src/rospkg/src/rospkg/rospack.py in get_depends(self, name, implicit)
    267                     ros_paths=self._ros_paths,
        268                     deps_sofar=s,
--> 269                     deps_unavailable=depends_unavailable)
    270             return s
        271

ResourceNotFound: Pkg(s) ['roslang', 'rosunit'] not available on your environment.
Defined dependency can be obtained in ResourceNotFound.get_depends: ['rosgraph_msgs', 'genlisp', 'genpy', 'genmsg', 'catkin', 'rosconsole', 'gencpp', 'xmlrpcpp', 'rostime', 'roslang', 'message_runtime', 'std_msgs', 'geneus', 'gennodejs', 'roscpp_traits', 'roscpp_serialization', 'rosbuild', 'message_generation', 'cpp_common', 'rosunit']
ROS path [0]=/opt/ros/melodic/share/ros
ROS path [1]=/opt/ros/melodic/share

In [4]: rp.get_depends("roscpp")
Out[4]:
['rosgraph_msgs',
 'genlisp',
 'genpy',
 'genmsg',
 'catkin',
 'rosconsole',
 'gencpp',
 'xmlrpcpp',
 'rostime',
 'roslang',
 'message_runtime',
 'std_msgs',
 'geneus',
 'gennodejs',
 'roscpp_traits',
 'roscpp_serialization',
 'rosbuild',
 'message_generation',
 'cpp_common',
 'rosunit']

**Issue**

When some dependencies are not available on the environment you run `RosPack.get_depends`, 2 issues:
- the error message doesn't mention all missing pkgs; in the example below, what missing are `roslang` and `rosunit`, but it only shows rosunit. After missing dep is somehow cleared then shows another single pkg missing on the next run.
- command fails without printing the dependency.
  - I might be misunderstanding the purpose of `get_depends`, but what I assume as a user from the method name is that it returns the list of defined dependency. So command failing to show any list (because of some depended pkgs are not available on your particular environment) is misleading.

**Solution**

- Returns the list of all dependency.
- Warn what's missing on your environment.

Also get_depends raises exception to not break API (see ros-infrastructure#129 (comment))
@130s 130s changed the title Fix get_depends returning dependent pkgs only partially under some condition Fix get_depends returning (1st run) none then (on 2nd run) fewer pkgs Aug 4, 2018
@130s 130s changed the title Fix get_depends returning (1st run) none then (on 2nd run) fewer pkgs Fix get_depends returning 1st run) none then on 2nd run) fewer pkgs Aug 4, 2018
@130s
Copy link
Contributor Author

130s commented Aug 4, 2018

Please review.

@130s
Copy link
Contributor Author

130s commented Dec 30, 2018

Please review. I'd call #142 a bug, and this PR is meant to amend the situation about it.

@dirk-thomas
Copy link
Member

Blocked by #141 due to the significant overlap of the patch.

@dirk-thomas
Copy link
Member

Due to the long time of inactivity I will go ahead and close the ticket. Please feel free to comment with updates and it can be reopened or feel free to open a new PR when you are ready referencing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants