Skip to content

CI: check for dependency licenses#1779

Merged
ChrisLovering merged 5 commits into
mainfrom
experiments/akarys/check-licenses
Aug 31, 2021
Merged

CI: check for dependency licenses#1779
ChrisLovering merged 5 commits into
mainfrom
experiments/akarys/check-licenses

Conversation

@Akarys42
Copy link
Copy Markdown
Contributor

@Akarys42 Akarys42 commented Aug 25, 2021

Since our project is licensed under the MIT License, we can't be using any dependencies in our project.

This PR adds a step to the CI lint phase that will use pip-licenses to verify that all the installed packages are part of an ALLOWED_LICENSE variable. This variable is currently set to be every license we currently use.

We opted to use an allowlist instead of a denylist to make sure that new licenses are reviewed by hand and added to that variable.

Since our project is licensed under the MIT License, we can't be using any dependencies in our project. This commit adds a step to the CI lint phase that will use pip-licenses to verify that all the installed packages are part of an ALLOWED_LICENSE variable. This variable is currently set to be every license we currently use. We opted to use an allowlist instead of a denylist to make sure that new licenses are reviewed by hand and added to that variable.
@Akarys42 Akarys42 force-pushed the experiments/akarys/check-licenses branch from 4b03a94 to b3b9d3f Compare August 25, 2021 14:39
@Xithrius Xithrius added a: CI Related to continuous intergration and deployment a: dependencies Related to package dependencies and management p: 2 - normal Normal Priority t: feature New feature or request labels Aug 26, 2021
Thanks to raimon49/pip-licenses#109, we are now able to ignore spaces around the allow-only parameter. Rejoice!
@Akarys42 Akarys42 marked this pull request as ready for review August 27, 2021 22:17
@Akarys42
Copy link
Copy Markdown
Contributor Author

This is now ready for review! Once merged, we should probably port that to our other major projects.

Copy link
Copy Markdown

@ventaquil ventaquil left a comment

Choose a reason for hiding this comment

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

Looks okay for my amateur eye.

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 27, 2021

How was it determined which licences are compatible with MIT?

@Akarys42
Copy link
Copy Markdown
Contributor Author

How was it determined which licences are compatible with MIT?

I've looked at choose-a-license.org to see what their requirements are, and they all seemed to be okay with being used with MIT. All of those are very permissive.

Note that I'm not a lawyer and this is not a legal advice, you know the drill

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 28, 2021

I looked at LGPL and even if it might be compatible with MIT, it does have some extra requirements. See section 4, which states that copies of licence must be included among other things.

@Akarys42
Copy link
Copy Markdown
Contributor Author

@MarkKoz my understanding here is we are only a consumer of the original library and never modify it, meaning it isn't a derived work and aren't subject to those rules. Although I'm honestly not sure, should we ask anyone about this?

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 28, 2021

@MarkKoz my understanding here is we are only a consumer of the original library and never modify it, meaning it isn't a derived work and aren't subject to those rules. Although I'm honestly not sure, should we ask anyone about this?

It is a combined work. What I've heard is importing a library is considered to be linking to it. So, see the licence's definition of a combined work:

A “Combined Work” is a work produced by combining or linking an Application with the Library. The particular version of the Library with which the Combined Work was made is also called the “Linked Version”.

@onerandomusername
Copy link
Copy Markdown
Contributor

As this project uses pre-commit, I feel like it would make sense to add this as a pre-commit hook, using the local configuration options, and setting it to only run on edits to pyproject.toml/poetry.lock. This would help ensure that regressions are discovered before they're ever commited.

@onerandomusername
Copy link
Copy Markdown
Contributor

I must be doing something wrong, as I tried this locally and added fuzzywuzzy as a dependency and this did not flag it at all.

@Akarys42
Copy link
Copy Markdown
Contributor Author

@MarkKoz alright, so we have two packages that uses LGPL. One of them is flake8-import-order which is quite easy to swap for Isort and would be quite an improvement.

The other is chardet, a dependency of aiohttp. It is a bit more problematic, but we could dodge it using the accelator lib instead cChardet, which is under the Mozilla license.

Maybe we could explore that if we want to get rid of them? Another solution would be to simply include a LICENSE-THIRD-PARTY file. I honestly surprised LGPL would require that, I don't see it being mentioned anywhere on the Internet, neither do I see any project which include the license. Maybe people are just not following the license.

@onerandomusername we opted to run it only in the CI to facilitate things, as running custom hooks on both Windows and Linux can be a pain. I don't think it matters when it is detected, as the work will have to be re-done either way. Also I don't know how you managed to not make it work, here is an (accidental) working example: 384d5b4. Updating pip-licenses added a new dep that weren't part of our set of allowed licenses and the CI correctly rejected it.

Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering left a comment

Choose a reason for hiding this comment

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

This PR is good to catch the licenses that are obviously against our MIT license.

I think we should create an issue to discuss what to do about the grey area licenses, rather than blocking this feature.

@ChrisLovering ChrisLovering merged commit e94931b into main Aug 31, 2021
@ChrisLovering ChrisLovering deleted the experiments/akarys/check-licenses branch August 31, 2021 15:38
Akarys42 added a commit to python-discord/sir-lancebot that referenced this pull request Sep 6, 2021
Port of python-discord/bot#1779

---

Since our project is licensed under the MIT License, we can't be using any incompatible license-wise dependencies in our project, such as GPL.

This commit adds a step to the CI lint phase that will use pip-licenses to verify that all the installed packages are part of an ALLOWED_LICENSE variable. This variable is currently set to be every license we currently use.

We opted to use an allowlist instead of a denylist to make sure that new licenses are reviewed by hand and added to that variable.
Akarys42 added a commit to python-discord/sir-lancebot that referenced this pull request Sep 6, 2021
Port of python-discord/bot#1779

---

Since our project is licensed under the MIT License, we can't be using any incompatible license-wise dependencies in our project, such as GPL.

This commit adds a step to the CI lint phase that will use pip-licenses to verify that all the installed packages are part of an ALLOWED_LICENSE variable. This variable is currently set to be every license we currently use.

We opted to use an allowlist instead of a denylist to make sure that new licenses are reviewed by hand and added to that variable.
Xithrius added a commit to python-discord/sir-lancebot that referenced this pull request Sep 6, 2021
* CI: check for license compatibility

Port of python-discord/bot#1779

---

Since our project is licensed under the MIT License, we can't be using any incompatible license-wise dependencies in our project, such as GPL.

This commit adds a step to the CI lint phase that will use pip-licenses to verify that all the installed packages are part of an ALLOWED_LICENSE variable. This variable is currently set to be every license we currently use.

We opted to use an allowlist instead of a denylist to make sure that new licenses are reviewed by hand and added to that variable.

Co-authored-by: ChrisJL <ChrisLovering@users.noreply.github.com>
Co-authored-by: Xithrius <15021300+Xithrius@users.noreply.github.com>
Co-authored-by: Vivaan Verma <54081925+doublevcodes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: CI Related to continuous intergration and deployment a: dependencies Related to package dependencies and management p: 2 - normal Normal Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants