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

[infra] license cleanup, refs #279 #294

Merged
merged 1 commit into from
Mar 11, 2018
Merged

Conversation

stonier
Copy link
Collaborator

@stonier stonier commented Mar 8, 2018

Keeping it simple stupid.

Rather wholesale cleanout, including some drake licenses (which will get fixed when we send them back to drake) and an odd apache license for backend/system.h?

I made sure to leave the gtest source licenses in.

Copy link

@basicNew basicNew left a comment

Choose a reason for hiding this comment

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

Just one question about the licence date.

LICENSE Outdated
@@ -1,27 +1,27 @@
Copyright 2017 Open Source Robotics Foundation
Copyright (c) 2018, Toyota Research Institute.
Copy link

Choose a reason for hiding this comment

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

Not sure about this, but shouldn't the date be 2017, as that is when the work on this codebase started?

Copy link
Collaborator Author

@stonier stonier Mar 8, 2018

Choose a reason for hiding this comment

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

Yes, should at least cover the starting date of contributions and I expect 2017-2018 is more accurate, though that doesn't seem to be a large bone of contention. Since the sensitivity to the last date is in the order of a lifetime I'm not inclined to worry about updating that every year.

@clalancette
Copy link
Contributor

and an odd apache license for backend/system.h?

I left that mostly because I copied it rather wholesale from ignition libraries. But it is fairly minor, so I think it would be OK to make it like the rest.

Of course, CI is unhappy because cpplint expects a copyright line in every file. We could disable that warning in our cpplint, but like I've said before, I would actually prefer to keep the license header at the top of the files, since that seems to be best practice. A change of just the copyright from OSRF -> TRI would also be a much smaller PR :).

@stonier
Copy link
Collaborator Author

stonier commented Mar 8, 2018

Just found some recommendations about file-scope and centralised conventions I wasn't aware of here: http://softwarefreedom.org/resources/2012/ManagingCopyrightInformation.html

Perhaps following the minimum effort conventions here should guide this, with a secondary thought to keeping tools like cpplint happy so we don't drift into a custom universe.

@stonier stonier force-pushed the stonier/license branch 3 times, most recently from c77495b to 58c65f7 Compare March 8, 2018 23:31
@stonier
Copy link
Collaborator Author

stonier commented Mar 9, 2018

@clalancette made some final changes. Per file scope to match TRI driving and keep cpplint happy and centralised for the LICENSE.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Overall, looks OK to me. No copyright specified at all on the scripts and protobuf files looks a little weird to me, but I'll let @stonier be the final arbiter on whether those need it or not.

#
# 1. Redistributions of source code must retain the above copyright notice,
# this list of conditions and the following disclaimer.
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but we are specifying /usr/bin/env python2.7 everywhere else. Probably stick with that to be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was wondering about that. Saw a mix in the couple of files I saw. @basicNew are we locked onto 2.7 due to pybind11 being linked against 2.7?

Copy link
Contributor

Choose a reason for hiding this comment

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

For a while we were locked into 2.7 due to the python LCM bindings that were coming from drake. Now that we aren't using LCM anymore, there is some possibility that we might be able to move to Python 3. But @basicNew will know better.

Choose a reason for hiding this comment

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

Yes, #158 was created to track that. I'll follow up on that issue.

@stonier
Copy link
Collaborator Author

stonier commented Mar 11, 2018

  • python -> python2.7 (thanks for catching that)
  • copyright -> protobuf scripts

Left off of the build files / CI scripts. Code is automagically covered by copyright, so the comments mostly serve as a convenience. No rocket science in these, nor do we distribute them.

@stonier stonier merged commit c0c4334 into maliput:master Mar 11, 2018
@stonier stonier deleted the stonier/license branch March 11, 2018 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants