-
Notifications
You must be signed in to change notification settings - Fork 97
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" feature (reopen #133) #201
base: master
Are you sure you want to change the base?
Add "get_licenses" feature (reopen #133) #201
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dirk-thomas Not all review comments from #133 are addressed at the time of opening this PR, but in an attempt to ease the review I linked to those unresolved comments at the corresponding lines and am asking your thoughts. Thanks in advance.
src/rospkg/rospack.py
Outdated
# Traverse for Non-ROS, system packages | ||
pkgnames_rosdep = self.get_rosdeps(package=pkg_name, implicit=implicit) | ||
for pkgname_rosdep in pkgnames_rosdep: | ||
license_dict[None].append(pkgname_rosdep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #133 (comment)
Why should this function combine both - the results from
get_depends
as well asget_rosdeps
- into a single result? That seems very use case specific.
The method get_licenses
in this PR returns the dictionary of licenses and the packages. I found that useful that the output is not just a list of licenses but also the name of all packages in the dependency tree.
I found:
get_depends
returns the list of upstream catkin packages.get_rosdeps
returns the list of upstream non-catkin packages.
In my usecase there's no reason to limit the scope of license introspection to catkin package (our customers want to know all the software installed on the delivered system). So including catking and non-catking packages actually important to me.
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?
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. Since in rospkg
there's already get_depends
, which returns a set of pkgs in the dependency tree, what get_licenses
adds is essentially a license layer to the get_depends
's output.
With the current implementation of this PR, capturing the licenses from the non-catkin packages is not included (e.g. what if the upstream implementation changes so that license of non-catkin package can be obtained?). I have a working code to capture licenses from system package (on Ubuntu) but due to the conversation with Dirk on previous PRs, I'm going with more granular PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to answer from a different perspective, comparing get_depends
(already a part of rospkg
and get_licenses
method that is being added in this PR,
get_depends
returns a list ofname
elements in the manifest.get_licenses
returns a list ofname
elements grouped bylicense
in the manifest.
Because both only return elements in the manifest (package.xml
) without adding any info external from the manifest file. So it makes sense to me to add get_licenses
capability to rospkg
.
c3c1af0
to
c499311
Compare
@@ -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> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these dependencies removed in 32e6580?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the commit message, unit test fails as the dependency genmsg
doesn't get installed for rospkg
itself nor for the unit test. If there's a non-hacky way to install the test dependency I'm happy to add that as well as getting these lines back in.
src/rospkg/rospack.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this documentation line is out of date since None
is now used right?
for the given package. Special value 'ERR' is used as the license for the | |
for the given package. ``None`` is used as the license for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR; I now set the string "license_not_found
" for the packages that any license wasn't detected of.
Follow-up for #133 (comment), yes I followed the review comment and changed to 'None'. Today (on Ubuntu 20.04 with Python 3.8.5) I found it causes "TypeError ('<' not supported between instances of 'NoneType' and 'License
"), and found that having None
as the dict key caused that. Thus I switched back to using string. This time I'm trying to make the variable name more descriptive so hope it got better.
[get_licenses] Delegate traversing all dependency to existing method (not exactly but hopefully closer to what's suggested in ros-infrastructure#133 (comment)) [get_licenses] Add system dependency to the output. [get_licenses] Sort licenses and pkgs. [get_licenses] Conform to variable name convention.
…packages with multiple licenses. Add a `licenses` field to Manifest class (in addition to `license`. Although {manifest, package}.xml format allows to define separate xml elements, rospkg handles multiple licenses as a single string joined by a comma, which is inconvenient depending on the consumers' usecase. Pointed out at https://github.com/ros-infrastructure/rospkg/pull/133/files#r253230420 If we want to maintain backward compatibility, `license` str field shouldn't change. So without making any change to it, this PR only adds a new field. This could cause a confusion. I thought to add printing a warning for deprecation for license, but because it looks like `license` is (and other fields are) intended to be accessed directly (without getter method/structure), I'm not sure how to notify the users during runtime. ``` $ ipython Python 2.7.12 (default, Nov 12 2018, 14:36:49) Type "copyright", "credits" or "license" for more information. IPython 2.4.1 -- An enhanced Interactive Python. ? -> Introduction and overview of IPython's features. %quickref -> Quick reference. help -> Python's own help system. object? -> Details about 'object', use 'object??' for extra details. In [1]: import rospkg In [2]: rp = rospkg.RosPack() In [4]: p = rp.get_manifest("rviz") In [5]: p.license Out[5]: 'BSD, Creative Commons' In [6]: p.licenses Out[6]: ['BSD', 'Creative Commons'] ``` - [x] Requester provides reproducible commands and sample test result. - [ ] CI should pass. - [ ] Code review. [get_licenses] Use Manifest.licenses, not Manifest.license in order to handle pkgs with multiple licenses better.
…be usecase, and if there is, users can easily do by themselves. [get_licenses] Call methods with kw arguments to better clarify args. Rename an arg for better clarity. [get_licenses] Clean up.
…gs license could not be detected (address https://github.com/ros-infrastructure/rospkg/pull/133/files#r253230933).
[test][catkin] Removing a ROS package from a dependency list as it is unresolvable during the set of nosetests in this package.
Setting up apt-get for python-rosdep might be complicated. For CI pip seems to be an acceptable solution. Enabling sudo in Travis level might be needed to run rosdep init. Copying from bloom project how to run rosdep on Travis CI with pip.
4755c5c
to
584c861
Compare
…ne' causes TypeError ('<' not supported between instances of 'NoneType' and 'License) so defining non None.
584c861
to
2b64f55
Compare
Struggling to understand why the test fails on Locally (on a Docker container) the specific test passes (you can see the num of tests increased from 102 to 103, so the new test is run).
Or to verify the specific test is executed,
|
I would love to see some community reviewers of this change. Especially by others who are interested in its utility. |
src/rospkg/rospack.py
Outdated
# Traverse for Non-ROS, system packages | ||
pkgnames_rosdep = self.get_rosdeps(package=pkg_name, implicit=implicit) | ||
for pkgname_rosdep in pkgnames_rosdep: | ||
license_dict[None].append(pkgname_rosdep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #133 (comment)
Why should this function combine both - the results from
get_depends
as well asget_rosdeps
- into a single result? That seems very use case specific.
The method get_licenses
in this PR returns the dictionary of licenses and the packages. I found that useful that the output is not just a list of licenses but also the name of all packages in the dependency tree.
I found:
get_depends
returns the list of upstream catkin packages.get_rosdeps
returns the list of upstream non-catkin packages.
In my usecase there's no reason to limit the scope of license introspection to catkin package (our customers want to know all the software installed on the delivered system). So including catking and non-catking packages actually important to me.
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?
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. Since in rospkg
there's already get_depends
, which returns a set of pkgs in the dependency tree, what get_licenses
adds is essentially a license layer to the get_depends
's output.
With the current implementation of this PR, capturing the licenses from the non-catkin packages is not included (e.g. what if the upstream implementation changes so that license of non-catkin package can be obtained?). I have a working code to capture licenses from system package (on Ubuntu) but due to the conversation with Dirk on previous PRs, I'm going with more granular PR.
Re-opening #133 due to #133 (comment). Following description is copied from #133 with some updates.
Changes added
rospkg
can access its licenses to.Output example
Dependency on other PRs/Issues
Review item list
L-GPL
). -> Delegated to [get_depends] Add option to specify the type of dependency. #132