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

Selection of dependency types with cmd line option [#727 continuation] #789

Merged
merged 18 commits into from
Jun 25, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/rosdep2/lookup.py
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ def get_resources_that_need(self, rosdep_name):
@staticmethod
def create_from_rospkg(rospack=None, rosstack=None,
sources_loader=None,
verbose=False):
verbose=False, dependency_types=[]):
wjwwood marked this conversation as resolved.
Show resolved Hide resolved
"""
Create :class:`RosdepLookup` based on current ROS package
environment.
Expand All @@ -339,6 +339,8 @@ def create_from_rospkg(rospack=None, rosstack=None,
instance used to crawl ROS stacks.
:param sources_loader: (optional) Override SourcesLoader used
for managing sources.list data sources.
:param dependency_types: (optional) List of dependency types.
Allowed: "build", "buildtool", "build_export", "run", "test", "exec", "doc"
"""
# initialize the loader
if rospack is None:
Expand All @@ -358,7 +360,7 @@ def create_from_rospkg(rospack=None, rosstack=None,

# Create the rospkg loader on top of the underlay
loader = RosPkgLoader(rospack=rospack, rosstack=rosstack,
underlay_key=underlay_key)
underlay_key=underlay_key, dependency_types=dependency_types)

# create our actual instance
lookup = RosdepLookup(rosdep_db, loader)
Expand Down
9 changes: 7 additions & 2 deletions src/rosdep2/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
from .installers import RosdepInstaller
from .lookup import RosdepLookup, ResolutionError, prune_catkin_packages
from .meta import MetaDatabase
from .rospkg_loader import DEFAULT_VIEW_KEY
from .rospkg_loader import DEFAULT_VIEW_KEY, all_dep_types
from .sources_list import update_sources_list, get_sources_cache_dir,\
download_default_sources_list, SourcesListLoader, CACHE_INDEX,\
get_sources_list_dir, get_default_sources_list_file,\
Expand Down Expand Up @@ -132,7 +132,7 @@ def _get_default_RosdepLookup(options):
sources_loader = SourcesListLoader.create_default(sources_cache_dir=options.sources_cache_dir,
os_override=os_override,
verbose=options.verbose)
lookup = RosdepLookup.create_from_rospkg(sources_loader=sources_loader)
lookup = RosdepLookup.create_from_rospkg(sources_loader=sources_loader, dependency_types=options.dependency_types)
lookup.verbose = options.verbose
return lookup

Expand Down Expand Up @@ -368,6 +368,11 @@ def _rosdep_main(args):
help="Affects the 'update' verb. "
'If specified end-of-life distros are being '
'fetched too.')
parser.add_option('-t', '--dependency-types', dest='dependency_types',
type="choice", choices=list(all_dep_types()),
default=[], action='append',
help='Dependency types to install, can be given multiple times. '
'Choose from {}. Default: all except doc.'.format(all_dep_types()))
wjwwood marked this conversation as resolved.
Show resolved Hide resolved

options, args = parser.parse_args(args)
if options.print_version or options.print_all_versions:
Expand Down
1 change: 1 addition & 0 deletions src/rosdep2/rospack.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ def __init__(self):
self.os_override = None
self.sources_cache_dir = get_sources_cache_dir()
self.verbose = False
self.dependency_types = []
lookup = _get_default_RosdepLookup(Options())
return lookup.get_rosdep_view(DEFAULT_VIEW_KEY)

Expand Down
10 changes: 8 additions & 2 deletions src/rosdep2/rospkg_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,13 @@
# resources and SourcesListLoader would build a *single* view that was
# no longer resource-dependent.

def all_dep_types():
# NOTE: 'group' is excluded
return {x[:-len('_depends')] for x in catkin_pkg.package.Package.__slots__ if x.endswith('_depends')} - {'group'}

class RosPkgLoader(RosdepLoader):

def __init__(self, rospack=None, rosstack=None, underlay_key=None):
def __init__(self, rospack=None, rosstack=None, underlay_key=None, dependency_types=[]):
"""
:param underlay_key: If set, all views loaded by this loader
will depend on this key.
Expand All @@ -78,6 +81,9 @@ def __init__(self, rospack=None, rosstack=None, underlay_key=None):
self._loadable_resource_cache = None
self._catkin_packages_cache = None

default_dep_types = all_dep_types() - {'doc'}
self.include_dep_types = all_dep_types().intersection(set(dependency_types)) if dependency_types else default_dep_types

def load_view(self, view_name, rosdep_db, verbose=False):
"""
Load view data into *rosdep_db*. If the view has already
Expand Down Expand Up @@ -140,7 +146,7 @@ def get_rosdeps(self, resource_name, implicit=True):
if resource_name in self.get_catkin_paths():
pkg = catkin_pkg.package.parse_package(self.get_catkin_paths()[resource_name])
pkg.evaluate_conditions(os.environ)
deps = pkg.build_depends + pkg.buildtool_depends + pkg.run_depends + pkg.test_depends + pkg.buildtool_export_depends
deps = sum((getattr(pkg, '{}_depends'.format(d)) for d in self.include_dep_types), [])
return [d.name for d in deps if d.evaluated_condition]
elif resource_name in self.get_loadable_resources():
rosdeps = set(self._rospack.get_rosdeps(resource_name, implicit=False))
Expand Down
41 changes: 41 additions & 0 deletions test/test_rosdep_lookup.py
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,47 @@ def test_RosdepLookup_get_rosdeps():
assert set(lookup.get_rosdeps('metapackage_with_deps', implicit=False)) == set(['catkin', 'simple_catkin_package', 'another_catkin_package'])


def test_RosdepLookup_dependency_types():
from rosdep2.loader import RosdepLoader
from rosdep2.lookup import RosdepLookup
rospack, rosstack = get_test_rospkgs()

sources_loader = create_test_SourcesListLoader()
default_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader)
buildtool_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['buildtool'])
build_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['build'])
build_export_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['build_export'])
exec_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['exec'])
test_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['test'])
doc_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['doc'])
mix_lookup = RosdepLookup.create_from_rospkg(rospack=rospack, rosstack=rosstack,
sources_loader=sources_loader, dependency_types=['build', 'build_export'])

buildtool_deps = ['catkin']
build_deps = ['testboost', 'eigen']
build_export_deps = ['eigen', 'testtinyxml']
exec_deps = ['eigen', 'testlibtool']
test_deps = ['curl']
doc_deps = ['epydoc']
default_deps = buildtool_deps + build_deps + build_export_deps + exec_deps + test_deps

assert set(buildtool_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(buildtool_deps)
assert set(build_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(build_deps)
assert set(build_export_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(build_export_deps)
assert set(exec_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(exec_deps)
assert set(test_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(test_deps)
assert set(mix_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(build_deps + build_export_deps)
assert set(default_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(default_deps)
assert set(doc_lookup.get_rosdeps('multi_dep_type_catkin_package')) == set(doc_deps)


def test_RosdepLookup_get_resources_that_need():
from rosdep2.lookup import RosdepLookup
rospack, rosstack = get_test_rospkgs()
Expand Down
13 changes: 12 additions & 1 deletion test/test_rosdep_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,12 @@ def read_stdout(cmd, capture_stderr=False):
expected = [
'#[apt] Installation commands:',
' sudo -H apt-get install ros-fuerte-catkin',
' sudo -H apt-get install libboost1.40-all-dev'
' sudo -H apt-get install libboost1.40-all-dev',
' sudo -H apt-get install libeigen3-dev',
' sudo -H apt-get install libtinyxml-dev',
' sudo -H apt-get install libltdl-dev',
' sudo -H apt-get install libtool',
' sudo -H apt-get install libcurl4-openssl-dev',
]
lines = stdout.getvalue().splitlines()
assert set(lines) == set(expected), lines
Expand Down Expand Up @@ -269,6 +274,12 @@ def test_keys(self):
rosdep_main(['keys', 'another_catkin_package'] + cmd_extras + ['-i'])
stdout, stderr = b
assert stdout.getvalue().strip() == 'catkin', stdout.getvalue()
with fakeout() as b:
rosdep_main(['keys', 'multi_dep_type_catkin_package', '-t', 'test', '-t', 'doc'] + cmd_extras)
stdout, stderr = b
output_keys = set(stdout.getvalue().split())
expected_keys = set(['curl', 'epydoc'])
assert output_keys == expected_keys, stdout.getvalue()
except SystemExit:
assert False, 'system exit occurred'
try:
Expand Down
4 changes: 4 additions & 0 deletions test/test_rosdep_rospkg_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,7 @@ def test_RosPkgLoader_get_loadable():
keys = loader.get_loadable_views()
for s in ['ros', 'empty', 'invalid', 'stack1']:
assert s in keys

def test_rospkg_loader_all_dep_types():
from rosdep2.rospkg_loader import all_dep_types
assert all_dep_types() == {'build', 'buildtool', 'build_export', 'buildtool_export', 'exec', 'test', 'doc'}, all_dep_types()
Copy link

Choose a reason for hiding this comment

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

This is a test file so fixed values may make more sense, but I still want to raise this thoughts: Should the types be taken from the upstream catkin_pkg.package.Package like done in the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this strategy isn't adequate because it would use the same principle the function being tested uses as if I was doing asssert all_dep_types() == all_dep_types(), which would be always true. Imagine if a new dependency type is added to Package and we don't want to consider it just yet. The tests would pass without our intention.

On the other hand, you might argue that this list could be constant in the source code and the test could check the list against all the dep types in Package. I think this is valid too and maybe it is in better agreement with my dissatisfaction with this function. I think the maintainers could help us with this decision.

Copy link

Choose a reason for hiding this comment

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

On the other hand, you might argue that this list could be constant in the source code and the test could check the list against all the dep types in Package.

+1 for that (I'm not a maintainer).

19 changes: 19 additions & 0 deletions test/tree/catkin/multi_dep_type_catkin_package/package.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<package format="2">
Copy link

Choose a reason for hiding this comment

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

Suggested change
<package format="2">
<package format="3">

Maybe nit, and open to counter suggestion; my personal preference is new development to happen in format 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the dependency types considered in the new feature were introduced in format 2. It is the minimum format required for this feature. Format 3 is compatible with it so it should work too (we could change it without breaking the tests). However, it is important to highlight that I'm ignoring the new dependency type group_depend, introduced in format 3. I was not sure how I would handle it, so I excluded it from this update.

If the current behavior is considered adequate for format 3 given the group_depend ignoring, I'm ok with changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to leave group dependencies out of this feature for the time being since the current implementation only has enough information to evaluate them in a source workspace context and it's not clear how rosdep should resolve them generally.

For manifest format I think the common convention is to use the earliest format which contains the features that are needed for the package and for the sake of making sure we maintain format 2 support I think that makes sense for these tests if not for new packages in general.

<name>multi_dep_type_catkin_package</name>
<version>0.0.0</version>
<description>
This is a package with depencencies of multiple types
</description>
<maintainer email="[email protected]">Nobody</maintainer>
<license>BSD</license>

<buildtool_depend>catkin</buildtool_depend>

<depend>eigen</depend>
<build_depend>testboost</build_depend>
<build_export_depend>testtinyxml</build_export_depend>
<exec_depend>testlibtool</exec_depend>
<test_depend>curl</test_depend>
<doc_depend>epydoc</doc_depend>

</package>