ManifestManager.get_depends_on should not depend on rosdep

[abencz]

Both of these queries can be implemented by examining the the list of packages in the ROS environment and filtering out non-ROS dependencies in the manifest based on this list. This is important because these methods are used in several places at runtime (e.g. to load plugins in roswtf) and runtime behaviour shouldn't depend on rosdeps being up-to-date.

[dirk-thomas]

How exactly should the question "filtering out non-ROS dependencies" be answered without rosdep in this case?

[abencz]

Actually, you're right, this is probably not possible for ManifestManager.get_depends, but get_depends_on relies on looking through the manifests of packages on the system. It shouldn't need to check whether the dependencies in those manifests are ROS dependencies since it's looking for a specific dependency name already.

[abencz]

Updated title.

[dirk-thomas]

That sounds reasonable. Since you seem to have a use case for this could you please consider providing a pull request for this enhancement.

[abencz]

Ok, will attempt do to so in the next few day.

[mikepurvis]

This was originally motivated by a plugin discovery use-case, but we have another one now— rosbag_migration uses get_depends_on, and therefore depends on rosdep being present and initialized: https://github.com/ros/ros_comm/blob/kinetic-devel/tools/rosbag/src/rosbag/migration.py#L529

FYI @efernandez @jjekircp

[efernandez]

If I understand well, we need to remove the dependency on rosdep2 re-implementing this without using it, right?
https://github.com/ros-infrastructure/rospkg/blob/master/src/rospkg/manifest.py#L389

I don't fully understand which other tools/libraries can be used instead. If any of you could help me with that, I could try to provide a PR.

[efernandez]

Essentially, these are the only two methods used from rosdep2 that we need to replace:
https://github.com/ros-infrastructure/rosdep/blob/master/src/rosdep2/rospack.py#L66
https://github.com/ros-infrastructure/rosdep/blob/master/src/rosdep2/rospack.py#L70

So the question is: how can we check if a dependency is a ros OR a system dependency w/o rosdep2?

@abencz Could you elaborate a little more on why we don't need to filter ros from system dependencies for get_depends_on?

Note that get_depends_on uses get_depends here: https://github.com/ros-infrastructure/rospkg/blob/master/src/rospkg/rospack.py#L285

[efernandez]

ping @abencz @dirk-thomas

[mikepurvis]

The proposal made above is that get_depends_on should be reimplemented so that rather than using get_depends under the hood (which requires checking what type of dep each one is), it can instead simply return a list of all known packages expressing a direct dependency on the given key.

This should be pretty easy— it's essentially popping open each package.xml (or manifest.xml) file pointed to by rospack list and checking whether it contains the specified dependency.

The main questions I'd have for @dirk-thomas about this is:

• Is there functionality in catkin_pkg that we should be leveraging for this, other than just the package.xml parsing?
• To maintain compatibility for rosbuild packages, would we want to do something like maintain the current/legacy implementation, but only call it when a rosbuild package is detected in the workspace? Is there some better way to handle this?

[dirk-thomas]

Is there functionality in catkin_pkg that we should be leveraging for this, other than just the package.xml parsing?

No, catkin_pkg only provides the parsing and some abstract for various dependency types.

To maintain compatibility for rosbuild packages, would we want to do something like maintain the current/legacy implementation, but only call it when a rosbuild package is detected in the workspace? Is there some better way to handle this?

I don't think a patch should call catkin_pkg directly. The existing method ManifestManager.get_manifest should be sufficient to use and already abstracts between catkin and rosbuild manifests.

Currently get_depends is being called with implicit=True which performs the recursion from top to bottom (which requires rosdep to distinguish the type of the rosdep key). get_depends_on needs to reimplement that recursion but from bottom to top - then rosdep isn't required since the "step upwards" in the dependency hierarchy is always from a ROS package to another ROS package.

[mikepurvis]

I've now gone down the rabbit hole a bit on this, and it's actually a little different than expressed above.

Both rosbag migration plugin discovery and roswtf plugin discovery call get_depends_on with implicit=False, so based on the implementation that's there, there's no reason for get_depends to ever get called under those circumstances— and changing the implicit implementation would have no effect:

    def get_depends_on(self, name, implicit=True):
        depends_on = []
        if not implicit:
            for r in self.list():
                if r == name:
                    continue
                try:
                    m = self.get_manifest(r)
                    if any(d for d in m.depends if d.name == name):
                        depends_on.append(r)
                except [snip]
        else:
            for r in self.list():
              # get recursive dependencies; uses depends_on with the rosdep2 dependency
        return depends_on

The real issue is that the sorting out of rosdeps and system package dependencies happens in the parse_manifest_file function:

rospkg/src/rospkg/manifest.py

Lines 387 to 410 in 60109b6

	# split ros and system dependencies (using rosdep)
	try:
	from rosdep2.rospack import init_rospack_interface, is_ros_package, is_system_dependency, is_view_empty
	global _static_rosdep_view
	# initialize rosdep view once
	if _static_rosdep_view is None:
	_static_rosdep_view = init_rospack_interface()
	if is_view_empty(_static_rosdep_view):
	sys.stderr.write("the rosdep view is empty: call 'sudo rosdep init' and 'rosdep update'\n")
	_static_rosdep_view = False
	if _static_rosdep_view:
	depends = set([])
	rosdeps = set([])
	for d in (p.buildtool_depends + p.build_depends + p.run_depends + p.test_depends):
	if (rospack and d.name in rospack.list()) or is_ros_package(_static_rosdep_view, d.name):
	depends.add(d.name)
	if is_system_dependency(_static_rosdep_view, d.name):
	rosdeps.add(d.name)
	for name in depends:
	manifest.depends.append(Depend(name, 'package'))
	for name in rosdeps:
	manifest.rosdeps.append(RosDep(name))
	except ImportError:
	pass

It's not at all clear to me that there's a good way forward here:

• Bypassing get_manifest is no good because then we're skipping over the caching it provides.
• Passing through an argument to skip the dep sorting is no good because then the cache will be inconsistent.
• Silencing the rosdep warning isn't ideal because there are legitimate circumstances where a user should see that warning.
• Adding an env var or some other way to explicitly skip this behaviour feels super clunky.

Maybe the rosdeps/depends attributes could be populated lazily rather than upfront, and the check could become if any(d for d in (m.buildtool_depends + m.build_depends + m.run_depends + m.test_depends) if d.name == name) or something? This would work, but will require some __getattr__ trickery which I'm not sure will be acceptable to @dirk-thomas.