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 "get_licenses" method #133

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ python:
install:
# develop seems to be required by travis since 02/2013
- python setup.py build develop
- pip install nose coverage
- pip install nose coverage rosdep
- sudo `which rosdep` init
Copy link
Member

Choose a reason for hiding this comment

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

Why is which needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I took this portion from ros-infrastructure/bloom/.travis.yml#L17, which seems to have been added in ros-infrastructure/bloom#221. I can only guess that it is needed as sudo doesn't know where the executable of rosdep is, which is installed into user's space by pip. Not yet super sure why sudo was/is needed on Travis CI though.

- rosdep update
# command to run tests
script:
- nosetests --with-coverage --cover-package=rospkg --with-xunit test
Expand Down
7 changes: 5 additions & 2 deletions src/rospkg/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
from .common import MANIFEST_FILE, PACKAGE_FILE, STACK_FILE

# stack.xml and manifest.xml have the same internal tags right now
REQUIRED = ['license']
REQUIRED = ['license', 'name']
ALLOWXHTML = ['description']
OPTIONAL = ['author', 'logo', 'url', 'brief', 'description', 'status',
'notes', 'depend', 'rosdep', 'export', 'review',
Expand Down Expand Up @@ -318,7 +318,7 @@ class Manifest(object):
'author', 'license', 'licenses', 'license_url', 'url',
'depends', 'rosdeps', 'platforms',
'exports', 'version',
'status', 'notes',
'status', 'name', 'notes',
Copy link
Member

Choose a reason for hiding this comment

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

Please move name to the beginning of __slots__ since it is also the first tag in the XML.

'unknown_tags', 'type', 'filename',
'is_catkin']

Expand All @@ -334,6 +334,7 @@ def __init__(self, type_='package', filename=None, is_catkin=False):
self.version = self.notes = ''
self.licenses = []
self.depends = []
self.licenses = []
self.rosdeps = []
self.exports = []
self.platforms = []
Expand Down Expand Up @@ -396,6 +397,7 @@ def parse_manifest_file(dirpath, manifest_name, rospack=None):
p = parse_package(package_filename)
# put these into manifest
manifest.description = p.description
manifest.name = p.name
manifest.author = ', '.join([('Maintainer: %s' % str(m)) for m in p.maintainers] + [str(a) for a in p.authors])
manifest.license = ', '.join(p.licenses)
manifest.licenses = p.licenses
Expand Down Expand Up @@ -499,6 +501,7 @@ def parse_manifest(manifest_name, string, filename='string'):
pass # manifest is missing optional 'review notes' tag

m.author = _check('author', True)(p, filename)
m.name = _check('name')(p, filename)
m.url = _check('url')(p, filename)
m.version = _check('version')(p, filename)

Expand Down
33 changes: 33 additions & 0 deletions src/rospkg/rospack.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
# ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
# POSSIBILITY OF SUCH DAMAGE.

from collections import defaultdict, OrderedDict
import os
from threading import Lock
from xml.etree.cElementTree import ElementTree
Expand Down Expand Up @@ -387,6 +388,38 @@ def stack_of(self, package):
else:
d = os.path.dirname(d)

def get_licenses(self, pkg_name, implicit=True):
"""
@summary: Return a list of licenses and the packages in the dependency tree
for the given package. Special value 'ERR' is used as the license for the
packages that license was not detected for.
@param pkg_name: Name of the package the dependency tree begins from.
@return Dictionary of license name and a list of packages.
@rtype { k, [d] }
@raise ResourceNotFound
"""
MSG_LICENSE_NOTFOUND_SYSPKG = "ERR"
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a pretty awkward choice for the API. Maybe use the key None for package names where no license information is available.

license_dict = defaultdict(list)

self.get_depends(name=pkg_name, implicit=implicit)

for p_name, manifest in self._manifests.items():
for license in manifest.licenses:
license_dict[license].append(p_name)

# Traverse for Non-ROS, system packages
pkgnames_rosdep = self.get_rosdeps(package=pkg_name, implicit=implicit)
for pkgname_rosdep in pkgnames_rosdep:
license_dict[MSG_LICENSE_NOTFOUND_SYSPKG].append(pkgname_rosdep)
Copy link
Member

Choose a reason for hiding this comment

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

Why should this function combine both - the results from get_depends as well as get_rosdeps - into a single result? That seems very use case specific.

In general since the function only converts the results into a reverse mapping from license names to package names I am wondering how useful this is as part of the generic API of rospkg. Where do you plan to use the new API? Depending on the answer why can't the logic be implemented there instead?

Copy link
Contributor Author

@130s 130s May 5, 2020

Choose a reason for hiding this comment

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

I couldn't come up with a better alternative direction, so I just share my usecase below.

I found this function convenient as a one-off to get all the list of licenses and all relevant packages (catkin and non-catkin packages), so that no other command is needed to get a complete list. Indeed, for those packages where license declaration wasn't found, I need additional steps (which is not part of this PR), but getting all the package names whose licenses couldn't be determined is a useful information IMO.

That said, with the current implementation in this PR, there's no room for capturing the license from the non-catkin packages (e.g. what if the upstream implementation changes so that license of non-catkin package can be obtained?). I'll think about updating this PR so that the code doesn't rule out the chance of getting license for non-catkin pkgs.


# Sort pkg names in each license
for list_key in license_dict.values():
list_key.sort()
# Sort license names
licenses = OrderedDict(sorted(license_dict.items()))
# Convert to dict for user friendlier output.
return dict(licenses)


class RosStack(ManifestManager):
"""
Expand Down
21 changes: 21 additions & 0 deletions test/catkin_package_tests/p1/bar/package.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?xml version="1.0"?>
<package format="2">
<name>bar</name>
<version>1.2.3</version>
<description>
I AM THE VERY MODEL OF A MODERN MAJOR GENERAL
</description>
<maintainer email="[email protected]">Someone</maintainer>

<license>MIT</license>

<url type="website">http://wiki.ros.org/bar</url>
<url type="bugtracker">http://www.github.com/my_org/bar/issues</url>
<author>Jane Doe</author>
<author email="[email protected]">Jane Doe</author>

<depend>liburdfdom-dev</depend>
<exec_depend>foo</exec_depend>
<test_depend>gtest</test_depend>
<export />
</package>
3 changes: 0 additions & 3 deletions test/catkin_package_tests/p1/foo/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@
<author>John Doe</author>
<author email="[email protected]">Jane Doe</author>

<build_depend>catkin</build_depend>
<build_depend version_gte="1.1" version_lt="2.0">genmsg</build_depend>
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'm afraid these packages would not be available in non-ROS context, which nosetest in CI runs in. So getting rid of these. I looked around and couldn't find test cases that these removal directly affects (although there may be test cases that implicitly depend on them?).


<build_depend>libboost-thread-dev</build_depend>
<run_depend>libboost-thread</run_depend>

Expand Down
16 changes: 13 additions & 3 deletions test/test_rospkg_catkin_packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,18 @@


def test_find_packages():
PKGS_EXIST = ['foo', 'bar']
manager = rospkg.rospack.ManifestManager(rospkg.common.MANIFEST_FILE, ros_paths=[search_path])
# for backward compatibility a wet package which is not a metapackage is found when searching for MANIFEST_FILE
assert(len(manager.list()) == 1)
assert(len(manager.list()) == 2)
manager = rospkg.rospack.ManifestManager(rospkg.common.STACK_FILE, ros_paths=[search_path])
assert(len(manager.list()) == 0)
manager = rospkg.rospack.ManifestManager(rospkg.common.PACKAGE_FILE, ros_paths=[search_path])

for pkg_name in manager.list():
assert(pkg_name == 'foo')
assert(pkg_name in PKGS_EXIST)
path = manager.get_path(pkg_name)
assert(path == os.path.join(search_path, 'p1', 'foo'))
assert(path == os.path.join(search_path, 'p1', PKGS_EXIST[PKGS_EXIST.index(pkg_name)]))


def test_get_manifest():
Expand All @@ -67,3 +68,12 @@ def test_licenses():
assert(len(manif.licenses) == 2)
for l in manif.licenses:
assert(l in licenses_list)


def test_get_licenses():
"""Check licenses from all packages in the dependency chain"""
rospack = rospkg.rospack.RosPack(ros_paths=[search_path])
licenses = rospack.get_licenses("bar", implicit=True)
assert("MIT" in licenses)
assert("BSD" in licenses)
assert("LGPL" in licenses)
Copy link
Member

Choose a reason for hiding this comment

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

Please also test for the expected values.