Bsd2 license #73

clalancette · 2017-04-26T17:09:40Z

This PR adds support for the BSD license into ament_lint. This is necessary for turning on all linting in sensor_msgs, since some of that code is under the BSD 2 license.

fixes #72

CI:
windows:
linux:
linux aarch64:
osx:

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

This is what is recommended by the BSD license. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

It is only ever called there, and operates on member variables in that subclass, so it should be located there. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

clalancette · 2017-04-26T17:12:24Z

In terms of review, this is probably easiest to review patch-by-patch, rather than the combined diff (particularly for the regex changes).

dirk-thomas · 2017-04-26T17:36:28Z

ament_copyright/ament_copyright/parser.py

@@ -166,7 +166,7 @@ def search_copyright_information(content):
    year = '\d{4}'
    year_range = '%s-%s' % (year, year)
    year_or_year_range = '(?:%s|%s)' % (year, year_range)
-    pattern = '^[^\n\r]?\s*Copyright\s+(%s(?:,\s*%s)*)\s+([^\n\r]+)$' % \
+    pattern = '^[^\n\r]?\s*Copyright\s+(%s(?:,\s*%s)*),?\s+([^\n\r]+)$' % \


Can you please give an example what wasn't accepted before and is accepted with this change.

Sure. Previous to this regex change, this was accepted:

Copyright 2017 Open Source Robotics Foundation, Inc.

But this wasn't:

Copyright 2017, Open Source Robotics Foundation, Inc.

(note the comma after the year). It is fairly minor, but I think it makes this regex a little more forgiving of the format.

mikaelarguedas · 2017-04-26T17:38:31Z

Do these CI jobs test any code that has a BSD licence ? e.g https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/include/sensor_msgs/distortion_models.hpp

dirk-thomas · 2017-04-26T17:39:14Z

ament_copyright/ament_copyright/parser.py

@@ -166,7 +166,7 @@ def search_copyright_information(content):
    year = '\d{4}'
    year_range = '%s-%s' % (year, year)
    year_or_year_range = '(?:%s|%s)' % (year, year_range)
-    pattern = '^[^\n\r]?\s*Copyright\s+(%s(?:,\s*%s)*),?\s+([^\n\r]+)$' % \
+    pattern = '^[^\n\r]?\s*Copyright(?:\s+\(c\))?\s+(%s(?:,\s*%s)*),?\s+([^\n\r]+)$' % \


Do we have to support this case or could we update the code using that.

So, this regex change is to allow a (c) after the word "Copyright". While we could change the code that has this, the BSD license recommends this format:

Copyright (c) <year>, <rights-holder>

(https://en.wikipedia.org/wiki/BSD_licenses#3-clause_license_.28.22BSD_License_2.0.22.2C_.22Revised_BSD_License.22.2C_.22New_BSD_License.22.2C_or_.22Modified_BSD_License.22.29).
Thus, any BSD code we come across is likely to be in that format, so I thought it would be better to support that.

dirk-thomas · 2017-04-26T17:48:24Z

ament_copyright/ament_copyright/parser.py

-                break
-        else:
-            self.license_identifier = UNKNOWN_IDENTIFIER
-


Why is this function being removed and the logic being duplicated into multiple locations?

So, I wasn't entirely sure what to do about this. We need to do something slightly different in this method in the subclass SourceDescriptor than we do in ContributingDescriptor and LicenseDescriptor. Thus, my options seemed to be:

Override it in SourceDescriptor

Add in some special-case flag for when we wanted to do additional substitutions

Copy the code

Given that it's really only 4 lines of code, I opted for the last one. However, I'm open to other suggestions.

I am not sure what the override is needed for (since the Apache license has the same structure already) but I would certainly prefer the first or second option.

I need to override identify_license() because the SourceDescriptor version of it wants to iterate over the self.copyrights list, which only exists in the SourceDescriptor subclass. I can restore the generic version to FileDescriptor and override it in SourceDescriptor, but it seems like overkill since it is such a simple function.

clalancette · 2017-04-26T17:58:32Z

@mikaelarguedas Good point, no, that CI doesn't have any BSD code. I've been testing locally by adding the ament_lint_auto back into sensor_msgs; I can do a CI with that instead to show this actually working.

Then override it in SourceDescriptor, since we need to do something different there. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

clalancette · 2017-04-26T19:53:35Z

New CI, which should test the BSD license in common_interfaces/sensor_msgs now:

windows:
linux:
linux aarch64:
osx:

dirk-thomas · 2017-04-26T20:54:25Z

The replacement of {copyright} in the source file template is not necessary. The Apache header also contains {copyright} and doesn't require the expansion.

Regarding the expansion of {company} I would rather suggest to use a "generic" snippet (see https://opensource.org/licenses/BSD-3-Clause) which refers to "Neither the name of the copyright holder nor the names of its contributors" rather than filling in the company name. (The template also lacks a trailing newline.)

The changes to accept (c) and the extra comma in the copyright line are fine.

All other changes are not necessary and I would rather avoid them if there is no good reason for them. E.g. the FileDescriptor doesn't have the member license_identifier because it never has a license. Only some of the subclasses have that attribute.

clalancette · 2017-04-27T13:06:47Z

Regarding the expansion of {company} I would rather suggest to use a "generic" snippet (see https://opensource.org/licenses/BSD-3-Clause) which refers to "Neither the name of the copyright holder nor the names of its contributors" rather than filling in the company name. (The template also lacks a trailing newline.)

This is the key point, I think. If we are OK with converting all incoming BSD licenses to the generic version, then we can do away with most of the code changes here. Are we OK with doing that?

tfoote · 2017-04-27T18:05:17Z

We shouldn't require changing license text contents to pass the linter beyond possibly stripping trailing whitespace. We can do that for our own copyright, but should not apply changes to anything for which we don't hold the copyright.

I would recommend that we allow basically arbitrary strings in the {company} section.

dirk-thomas · 2017-04-27T18:10:44Z

I would recommend that we allow basically arbitrary strings in the {company} section.

Sounds good to me.

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

clalancette · 2017-04-28T15:00:30Z

I just pushed a different approach to this. I changed the BSD2 template to be the generic version, and then I used difflib to compare a ratio between the incoming text and the template. If that is > 95%, we accept that as the license (I'm not wedded to that ratio, it just seemed somewhat reasonable).

I left the move of the identify_copyright() function, and the consolidation of self.license_identifier into FileDescriptor(). Here are my reasons:

identify_copyright() operates on data only present in the SourceDescriptor subclass. Thus, it doesn't belong in a generic super class.
self.license_identifier is set by all of its subclasses. This suggests that it is a property that they all share, and thus it makes sense to me to hoist it up to the super class.

CI:

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2017-04-28T16:08:52Z

ament_copyright/ament_copyright/parser.py

        for name, license in get_licenses().items():
-            if content is not None and getattr(license, license_part) == content:
+            template = getattr(license, license_part).replace('\n', ' ')
+            tomatch = content.replace('\n', ' ')


Unused variable?

Oh yeah, that's a bug. I should be using that in the difflib call.

(it's also the cause of the test failure in the CI)

dirk-thomas · 2017-04-28T16:09:20Z

ament_copyright/ament_copyright/parser.py

@@ -44,6 +45,7 @@ def __init__(self, filetype, path):
        self.path = path
        self.exists = os.path.exists(path)
        self.content = None
+        self.license_identifier = UNKNOWN_IDENTIFIER


See previous comment why I don't think the base class should have that attribute.

Right, I guess that is the point I disagree with :). While that base class itself doesn't have a license, all of its sub-classes do (they all call identify_license()). Thus it seems to be something common to all of them, and seems to make sense in the base class. It's similar in many respects to the identify_license() method, in that it is a convenient place to not duplicate the code, rather than it being somehow intrinsic to the base class.

dirk-thomas · 2017-04-28T16:10:01Z

ament_copyright/ament_copyright/parser.py

+            template = getattr(license, license_part).replace('\n', ' ')
+            tomatch = content.replace('\n', ' ')
+            x = difflib.SequenceMatcher(a=template, b=content)
+            if x.ratio() > 0.95:


I am not sure this is "good enough". This could result in a wrong detection if he license is just similar enough.

I guess, though a license that is 95% the same as this would probably be considered the same.

So the real question is whether we should just up this value (98%, say), or do you disagree with the approach?

dirk-thomas · 2017-04-28T16:11:37Z

ament_copyright/ament_copyright/template/bsd2_header.txt

+CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+POSSIBILITY OF SUCH DAMAGE.


Please see previous comment. Still lacking a newline at the end.

Will fix that.

"Cutting out" the company name and checking the string before and after should be straight forward. So I am not sure what the diff approach is gaining us.

Basically, what this buys us is the ability to be a lot more forgiving with the license.

In the case of the BSD license, there are at least 3 different variations I've seen, just between looking at the common_interfaces code, the license you linked to at opensource.org, and the license on wikipedia. In the one in the code, it has the name of the company in it, and uses * for each point made. In the one at opensource.org, it says "copyright holder" instead of the company name, and uses numerals instead of stars for each point. In the one on wikipedia, it has the name of the company in it, and uses * for each point.

While I could write some custom parsing code to deal with all of those differences, it would necessarily be BSD-specific, which doesn't seem like an ideal path to go down. The difflib approach is more flexible, and should work for other licenses as well.

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

tfoote · 2017-04-28T20:12:35Z

I don't think that this new approach is appropriate for this use case. Comparing the fraction of match is a great way to get yourself into a lot of trouble. If I add a "not" in any clause, it completely changes the meaning and compliance but does not significantly change the diff. If there's any change outside the standard parameters it's no longer the named license but a variant which needs to be interpreted as it's own entity.

clalancette · 2017-05-02T17:27:31Z

@tfoote I thought it might be interesting to try this approach, but I agree with your assessment. So my plan will be to split the templated license on the "{company}" string, then look for those substrings (in that order) against the incoming text to test against. Does that sound like a better approach?

This allows us to deal with the somewhat problematic BSD2 license, where a company name can be embedded in the middle. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

tfoote · 2017-05-02T18:51:57Z

That sounds like a reasonable approach.

clalancette · 2017-05-02T21:14:25Z

CI for the new approach:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2017-05-10T18:59:59Z

once this is merged ros2/common_interfaces@a0234f2 and ros2/common_interfaces#33 need to be reverted

clalancette · 2017-05-11T15:40:53Z

Thanks for the review. I know we usually squash changes together, but in this one, I'd actually prefer for it to go in as 3 different commits: one to change the regex to allow the (c), one to change the regex to allow the comma, and one to add the BSD license. I think this will make following the regex a little easier later on. Does that seem OK?

dirk-thomas · 2017-05-11T15:44:00Z

Sounds good to me.

clalancette · 2017-05-11T15:58:16Z

I've now merged this (by hand, to split the commits up). Thanks!

dirk-thomas · 2017-05-11T16:10:20Z

It would be better to rebase the commits on the branch of the PR. That way the actually merged changes are still visible from the PR. Currently the PR doesn't reference the actual changes done on master.

clalancette · 2017-05-11T16:13:51Z

Oh, right, that would be better. Should I go back and do that (force pushing master to remove the commits), or is that advice for the future?

dirk-thomas · 2017-05-11T16:15:13Z

In this case it I would say it is too late. But you could reference the actual commit hashes which have been applied to master so that future readers find them easily.

clalancette · 2017-05-11T16:18:30Z

This PR ended up applying these commits:

3f65895
77061fd
767257f

* don't ignore copyright test * make copyright comment comply with ament/ament_lint#73 implementation * OSRF is the copyright holder

clalancette added 4 commits April 25, 2017 16:00

Change the copyright regex to allow a comma after the year(s).

3f65895

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

Change the copyright regex to allow a (c) after the "Copyright" word.

77061fd

This is what is recommended by the BSD license. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

Move the identify_copyright method into SourceDescriptor.

0eb7b5d

It is only ever called there, and operates on member variables in that subclass, so it should be located there. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

Add in support for the BSD 2.0 license.

9f97869

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

clalancette added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 26, 2017

dirk-thomas reviewed Apr 26, 2017

View reviewed changes

Make identify_license() a method of FileDescriptor again.

ed04f19

Then override it in SourceDescriptor, since we need to do something different there. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

Switch to using difflib for the comparisons.

b7567a7

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

dirk-thomas reviewed Apr 28, 2017

View reviewed changes

Fix review comments.

868fb69

Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

mikaelarguedas mentioned this pull request May 1, 2017

Reenable linting in sensor_msgs. ros2/common_interfaces#33

Merged

clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 2, 2017

Split licenses up by {company}, and match substrings.

21661e7

This allows us to deal with the somewhat problematic BSD2 license, where a company name can be embedded in the middle. Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>

clalancette added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 2, 2017

dirk-thomas approved these changes May 10, 2017

View reviewed changes

clalancette closed this May 11, 2017

clalancette removed the in review Waiting for review (Kanban column) label May 11, 2017

clalancette deleted the bsd2-license branch May 11, 2017 15:58

This was referenced May 11, 2017

Re-enable copyright linting in sensor_msgs ros2/common_interfaces#35

Closed

Add in support for the BSD license in the copyright linting #72

Closed

mikaelarguedas added a commit to ros2/common_interfaces that referenced this pull request May 16, 2017

make copyright comment comply with ament/ament_lint#73 implementation

0caca60

mikaelarguedas mentioned this pull request May 16, 2017

[sensor_msgs] Reenable copyright test ros2/common_interfaces#36

Merged

mikaelarguedas added a commit to ros2/common_interfaces that referenced this pull request May 16, 2017

[sensor_msgs] Reenable copyright test (#36)

fdadc05

* don't ignore copyright test * make copyright comment comply with ament/ament_lint#73 implementation * OSRF is the copyright holder

Bsd2 license #73

Bsd2 license #73

Conversation

clalancette commented Apr 26, 2017 • edited Loading

clalancette commented Apr 26, 2017

dirk-thomas Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Apr 26, 2017

Choose a reason for hiding this comment

clalancette Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Apr 26, 2017

clalancette commented Apr 26, 2017

dirk-thomas commented Apr 26, 2017 • edited Loading

clalancette commented Apr 27, 2017

tfoote commented Apr 27, 2017

dirk-thomas commented Apr 27, 2017

clalancette commented Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfoote commented Apr 28, 2017

clalancette commented May 2, 2017

tfoote commented May 2, 2017

clalancette commented May 2, 2017 • edited Loading

mikaelarguedas commented May 10, 2017

clalancette commented May 11, 2017

dirk-thomas commented May 11, 2017

clalancette commented May 11, 2017

dirk-thomas commented May 11, 2017

clalancette commented May 11, 2017

dirk-thomas commented May 11, 2017

clalancette commented May 11, 2017

clalancette commented Apr 26, 2017 •

edited

Loading

dirk-thomas Apr 26, 2017 •

edited

Loading

clalancette Apr 26, 2017 •

edited

Loading

dirk-thomas commented Apr 26, 2017 •

edited

Loading

clalancette commented Apr 28, 2017 •

edited

Loading

clalancette commented May 2, 2017 •

edited

Loading