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 license to every file and add license check to Travis tests #303

Open
wants to merge 2 commits into
base: master
from

Conversation

@blag
Copy link

@blag blag commented Sep 26, 2016

Supersedes #130; closes #125.

Completes milestone 1.

Edit: Milestone 1 was updated with another task.

@blag blag force-pushed the blag:licensecheck branch from f6ffc99 to be73d33 Sep 27, 2016
@blag
Copy link
Author

@blag blag commented Sep 27, 2016

Added ./licensecheck.pl to the project, and updated .travis.yml to run that after other CI tests. The 56 other changes are simply license additions.

@blag
Copy link
Author

@blag blag commented Oct 13, 2016

Bump.

@blag
Copy link
Author

@blag blag commented Oct 18, 2016

Bump again. If you add me as a maintainer on GitHub and PyPI I can merge it in and release version 1.0 myself. I'm already a GitHub and/or PyPI maintainer for:

  • django-admin-row-actions
  • django-multiselectfield
  • django-smart-selects
  • django-world-languages
  • humanhash3
  • markdown-icons
  • django-cities
  • django-osm-field
@ambv
Copy link
Member

@ambv ambv commented Oct 20, 2016

Why is this necessary? Many projects clutter their files with licensing information, many don't. The only argument in favor for it I found is that it makes it "more obvious" that there's no warranty in case somebody "separates the file" from the project. Is that a worry that has ever been confirmed as being a real issue?

The reason I'm picky here is that there is a non-zero cost from putting this in. It poisons history, it must be maintained forward when new files are created, it wastes disk space and screen real estate when opening the file. Is it a big deal? No. But is it necessary? I also doubt it.

@blag
Copy link
Author

@blag blag commented Oct 20, 2016

Since @gsnedders was the author of #130 he may be in a better position to answer to why this is important than me.

According to @kenrussell in #125:

In order for Chromium to be pulled in to various Linux source distributions there's a requirement that all of the third party files pass the Linux licensecheck utility.

I have asked him in that PR to be more specific about exactly which Linux distributions carry that requirement.

@ambv Here's some point-by-point rebuttals:

It poisons history

Can you explain what you mean by this more? It seems like you're saying this is a lot of files that people would have to rebase onto, but that's the case for any PR that makes changes to a lot of files, and because the changes to all files (ignoring adding licensecheck.pl and the travis test) are at the top, the number of merge conflicts that can't be automatically resolved should theoretically be minimal.

it must be maintained forward when new files are created

That's a legitimate complaint. I have added the license check to the travis.yml as you can see in this line, so once this is merged in tests with new files should fail until they explicitly carry a license. Note that this travis test rather intentionally doesn't check which license a file has, just that it has a recognized license.

it wastes disk space

It's 2016, is this really an issue anymore? It adds a few KB at most. Loading this webpage probably caused your computer to swap more memory to disk than the entirety of this size change. If it is legitimately that big of an issue for you or any other contributor I would be happy to ship you a few extra low-capacity (<100 GB) hard drives I have lying around free of charge (maybe not international).

it wastes...screen real estate

Yes...until you scroll past it. Which I'll admit is certainly an annoyance, but as you note, hardly a big deal.

@blag
Copy link
Author

@blag blag commented Oct 20, 2016

Also, I'm happy to have this not merged in - that's totally fine. Just, in that case though, please close #125 as WONTFIX, remove it from milestone 1 and mark the milestone as complete. The milestone is actually what the bleach project is waiting on and I'm waiting on them. 😄

@gsnedders
Copy link
Member

@gsnedders gsnedders commented Oct 24, 2016

@blag uh, that shouldn't be the only thing on that milestone… oh, there's also https://github.com/html5lib/html5lib-python/milestone/8 with another useless number, which appears to be what I expected to put out as a de-facto 1.0rc (whether or not it gets labelled as such depends on whether we want pip users to get updated to it, etc.).

@willkg willkg added this to the 1.0 milestone Oct 3, 2017
@@ -0,0 +1,551 @@
#!/usr/bin/perl -w

This comment has been minimized.

@willkg

willkg Oct 31, 2017
Contributor

It looks like this was copy-and-pasted and has curious indentation. I have no way of determining whether this is maintained somewhere and making sure our version is up-to-date. Adding this file to this repository means we have to maintain it going forward manually.

With other linting/testing things, we install a package/library and then use that. Can we do that with license checking, too?

This comment has been minimized.

@jayvdb

jayvdb Nov 1, 2017
Contributor

licencecheck.pl is now available as a separate package in most distributions. e.g. https://packages.debian.org/sid/licensecheck . iirc it was part of devscripts on Debian in jessie and earlier, but jessie-backports now has the licensecheck package and updated scripts. Unfortunately (from a Travis perspective) it is only a separate package in Ubuntu zesty and onwards.

This comment has been minimized.

@willkg

willkg Nov 1, 2017
Contributor

@bllag Can you install the package rather than include the file? This might help:

https://docs.travis-ci.com/user/installing-dependencies/#Installing-Packages-on-Standard-Infrastructure

This comment has been minimized.

@blag

blag Nov 3, 2017
Author

I believe what @jayvdb is saying is that I cannot simply install the licensecheck package in the Travis tests, because Travis doesn't yet support Ubuntu Zesty or newer. @jayvdb can you confirm that?

If this PR gets merged, once Travis supports Ubuntu Zesty+, I would be more happy to rip out the bundled licensecheck.pl script and modify the .travis.yml file to install it from the package repository.

This comment has been minimized.

@jayvdb

jayvdb Nov 3, 2017
Contributor

I am pretty sure you can use apt to install the jessie-backport onto a trusty node.
If not, it is easy to backport into a ppa: (Or maybe openSUSE's Open Build Service?) , or install from source; maybe even a single wget could do it. It is a tiny chunk of code without many dependencies. Definitely, no need to put the source into this repository.

@willkg willkg removed this from the 1.0 milestone Oct 31, 2017
@willkg
Copy link
Contributor

@willkg willkg commented Oct 31, 2017

I'm taking this out of the 1.0 milestone. I'm not wild about including the pl file in the repository for the reasons listed. I'm happy for this to land in 1.0, but I don't think it should block.

@blag
Copy link
Author

@blag blag commented Nov 3, 2017

That's understandable.

The only reason I added a Perl file to this Python project is because it's a standard tool copied from other projects. If there was an acceptable Python version I would have used that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.