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 a lint for license files #21855

Closed
wants to merge 2 commits into from
Closed

Add a lint for license files #21855

wants to merge 2 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Feb 18, 2020

No description provided.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21855 February 18, 2020 14:17 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21855 May 14, 2020 09:31 Inactive
@foolip
Copy link
Member Author

foolip commented May 14, 2020

@jgraham do you want to give this another look now that I've updated lint.ignore? It makes it a lot clearer what the current state of the repo is, at least.

@jgraham
Copy link
Contributor

jgraham commented May 14, 2020

So I see the intent, but it seems like one effect might be that people add third party code without copying the license file in order to silence the lint. So instead of helping it will just cause a worse kind of problem.

Can we require any third_party sub-directory to contain a LICENSE file and also lint that directories other than third_party/<foo> and the root don't have LICENSE files?

If we want to specifically audit every license is acceptable and so require an explicit rule for each we could have two lint rules, one to check that the license exists and one for unaudited license. Then we should divide the allow list up into specific license types (MIT, BSD, etc.) so it's obvious what's acceptable.

@foolip
Copy link
Member Author

foolip commented May 14, 2020

So I see the intent, but it seems like one effect might be that people add third party code without copying the license file in order to silence the lint. So instead of helping it will just cause a worse kind of problem.

That would be bad, and does seem like a real risk.

Can we require any third_party sub-directory to contain a LICENSE file and also lint that directories other than third_party/<foo> and the root don't have LICENSE files?

I think that'd have to be an all_paths_lints like check_css_globally_unique, right? I can't see any other lint concerned with directories, but we could find all third_party directories given all paths, at least.

A problem I anticipate is that the existing *: */third_party/* line in lint.ignore will silence the error. I don't think we have a syntax for "only this lint", so should we add that, or hardcode special rules for the license lint?

If we want to specifically audit every license is acceptable and so require an explicit rule for each we could have two lint rules, one to check that the license exists and one for unaudited license. Then we should divide the allow list up into specific license types (MIT, BSD, etc.) so it's obvious what's acceptable.

Do you mean that every LICENSE file that does exist and we should accept should still be a lint error, so that we have to list it? Like, LICENSE-MIT, LICENSE-BSD and finally LICENSE-UNKNOWN?

foolip added a commit that referenced this pull request May 14, 2020
This is following the directory structure used for webrtc in
web-platform-tests/rfcs#46.

Spotted via #21855.
@jgraham
Copy link
Contributor

jgraham commented May 14, 2020

A problem I anticipate is that the existing *: /third_party/ line in lint.ignore will silence the error. I don't think we have a syntax for "only this lint", so should we add that, or hardcode special rules for the license lint?

Hmm, good point. I'm not sure. Might be easiest to hardcode special rules, unless you want to add a more generic mechanism.

Do you mean that every LICENSE file that does exist and we should accept should still be a lint error, so that we have to list it? Like, LICENSE-MIT, LICENSE-BSD and finally LICENSE-UNKNOWN?

I just meant in the ignore file having something like

# Verified MIT LICENSE
LICENSE-ALLOWED: tools/third_party/mit_project


# Verified BSD LICENSE
LICENSE-ALLOWED: tools/third_party/bsd_project

So it wouldn't actually do any verification that the license is the right kind, but a human adding a dep could see what precedent exists, and a reviewer could manually verify the claim.

LICENSE: compression/pako/LICENSE
LICENSE: css/css-color/LICENSE
LICENSE: css/css-ui/support/PTS/PngSuite.LICENSE
LICENSE: css/CSS2/LICENSE-BSD
Copy link
Member Author

Choose a reason for hiding this comment

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

I've sent #23593 to remove these 3 files.

# Various license files. Do not add to this list without going through the RFC
# process: https://github.com/web-platform-tests/rfcs
LICENSE: compression/pako/LICENSE
LICENSE: css/css-color/LICENSE
Copy link
Member Author

Choose a reason for hiding this comment

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


# Various license files. Do not add to this list without going through the RFC
# process: https://github.com/web-platform-tests/rfcs
LICENSE: compression/pako/LICENSE
Copy link
Member Author

Choose a reason for hiding this comment

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

I sent #23592 to move this into third_party

foolip added a commit that referenced this pull request May 14, 2020
This is following the directory structure used for webrtc in
web-platform-tests/rfcs#46.

Spotted via #21855.
@@ -86,6 +86,11 @@ class AhemCopy(Rule):
description = "Don't add extra copies of Ahem, use /fonts/Ahem.ttf"


class LicenseFiles(Rule):
name = "LICENSE"
description = "Don't add new license files"
Copy link
Member

Choose a reason for hiding this comment

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

I think we need more context here. Maybe include "Do not add new third-party code without going through the RFC process". Otherwise, people might just add dependencies without LICENSE.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 26, 2020
…ty directory, a=testonly

Automatic update from web-platform-tests
[compression] move pako into a third_party directory (#23592)

This is following the directory structure used for webrtc in
web-platform-tests/rfcs#46.

Spotted via web-platform-tests/wpt#21855.
--

wpt-commits: 66e875b5d6bbd71ab24af9ca27d341f242f7a7cb
wpt-pr: 23592
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request May 26, 2020
…ty directory, a=testonly

Automatic update from web-platform-tests
[compression] move pako into a third_party directory (#23592)

This is following the directory structure used for webrtc in
web-platform-tests/rfcs#46.

Spotted via web-platform-tests/wpt#21855.
--

wpt-commits: 66e875b5d6bbd71ab24af9ca27d341f242f7a7cb
wpt-pr: 23592
@foolip
Copy link
Member Author

foolip commented Jun 9, 2020

I've stopped working on this, because it turned into a lint that needs to determine which directories exist and which files in them don't, which is unlike any other lint. The ignore rule would also have to be for a directory, which is also something we don't have.

@foolip foolip closed this Jun 9, 2020
@foolip foolip deleted the foolip/license-lint branch June 9, 2020 11:06
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