Conversation
130s
left a comment
130s
left a comment
There was a problem hiding this comment.
@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.
| # 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.
From #133 (comment)
Why should this function combine both - the results from
get_dependsas 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_dependsreturns the list of upstream catkin packages.get_rosdepsreturns 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.
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_dependsreturns a list ofnameelements in the manifest.get_licensesreturns a list ofnameelements grouped bylicensein 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
|
|
||
| <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.
Why were these dependencies removed in 32e6580?
There was a problem hiding this comment.
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.
| 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.
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.
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. |
| # 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.
From #133 (comment)
Why should this function combine both - the results from
get_dependsas 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_dependsreturns the list of upstream catkin packages.get_rosdepsreturns 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.
3 participants
Re-opening #133 due to #133 (comment). Following description is copied from #133 with some updates.
Changes added
rospkgcan 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