1562645 - Should have a commit hook and linter that stops people adding source (.cpp/.h/.js/.jsm) files without license headers

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task)

Description

[:Gijs (he/him)]

I just filed bug 1562642.

The VCS and presumably phabricator linters should be preventing this type of thing from happening (with an escape magic word for landing imported 3rd party code or something like that, and/or disregarding known 3rd party source dirs).

Comment 1

[Connor Sheehan [:sheehan]]

We could write a fairly simple hook for this on hg.mo, assuming all third party code lands in third_party/. That would at least stop things from being checked in to the repo. However the developer who makes this error won't know until they attempt to land with Lando and are unable to do so (ie they have to click land, notice their patch didn't land, go into Lando to see why, then fix the problem and wait for another r+).

I think it would make more sense to add this as a linting step, so the dev can fix their error during review. CC a few people who are working on our linters/formatters.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

An easy fix (not perfect) could be to create a regexp checking if the top of the file starts with a comment.
Like this one:
https://searchfox.org/mozilla-central/source/tools/lint/cpp-virtual-final.yml#25

A smarter way would be to run
https://packages.debian.org/sid/licensecheck
and trigger an error with licensecheck is failing.

And ignore stuff in https://searchfox.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt to avoid thirdparty libs to be checked.

gijs, how often do you see such mistakes?

Comment 3

[Andrew Halberstadt [:ahal]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #2)

An easy fix (not perfect) could be to create a regexp checking if the top of the file starts with a comment.
Like this one:
https://searchfox.org/mozilla-central/source/tools/lint/cpp-virtual-final.yml#25

A better naive way would be to fail the lint check if the string http(s)://mozilla.org/MPL/2.0/ doesn't appear in the file. We could obviously make it a bit smarter (e.g say it has to appear in the first 10 lines), or even define exactly what the whole string should be per file type. We should also be sure to support --fix as this should not be hard to do.

A smarter way would be to run
https://packages.debian.org/sid/licensecheck
and trigger an error with licensecheck is failing.

If this supports MPL, might be worth doing.

And ignore stuff in https://searchfox.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt to avoid thirdparty libs to be checked.

All paths in ThirdPartyPaths.txt are already implicitly ignored by all linters.

Comment 4

[Mark Banner (:standard8)]

In case it helps, there's a ESLint plugin that does js files (already included in the tree as devtools' debugger uses it):

https://github.com/Sekhmet/eslint-plugin-file-header
https://searchfox.org/mozilla-central/rev/11712bd3ce7454923e5931fa92eaf9c01ef35a0a/devtools/client/debugger/.eslintrc#379-388

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1562645 - Extend mozlint to have license check r?ahal

The goal is to have something simple and not relying on other tools.
I tried with licensecheck and licensee but, with if we use a different
wording to declare "public domain" (as example), they might not
catch it. Requiring to contribute upstream, etc

Instead, I just create a list of line of license to catch it.
From my trials, it works well enough and it is trivial python.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1562645 - Add an autofix to the license check r?ahal

Depends on D37082

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a817e3f2e0c9
Extend mozlint to have license check r=ahal

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a817e3f2e0c9

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7c3a3369339
Add an autofix to the license check r=ahal

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e7c3a3369339

Comment 12

[:Gijs (he/him)]

Looking at the commit, did we only do this for ['.cpp', '.c', '.cc', '.h', '.m', '.mm'], and is there a bug on file / what would it take to extend this check to frontend files as well as rust and python? ([, 'js', 'jsm', 'jsx', 'xml', 'xul', 'html', 'xhtml', 'py', 'rs'] are commented out, but also css and svg probably want this)

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #12)

([, 'js', 'jsm', 'jsx', 'xml', 'xul', 'html', 'xhtml', 'py', 'rs'] are commented out, but also css and svg probably want this)

Oh, and locale files ('properties', 'dtd', 'ftl')

Comment 14

[Sylvestre Ledru [:Sylvestre]]

This has been updated in bug 1562645
The current list:
- .c
- .cc
- .cpp
- .h
- .html
- .js
- .jsm
- .jsx
- .m
- .mm
- .py
- .rs
- .xhtml
- .xml
- .xul

I will have a look for the others