Add pre-commit hook for including the MPLv2 in all source code files #10617

stevejalim · 2021-10-20T20:16:23Z

Adds to Python, JS, SCSS, Jinja HTML, Fluent files and shell scripts

Description

This changeset adds a pre-commit hook that can automatically insert a licence in a file. It is configured to insert the MPLv2 into Python, JS, SCSS, Jinja HTML and shell files.

Issue / Bugzilla link

Resolves #10614

Testing

pull this branch
Pick a Python, JS, SCSS, .sh, Fluent, Jinja HTML file and remove the MPLv2 text from it
Stage the file(s)
git commit to trigger the pre-commit hooks
Confirm that all the files fail pre-commit checks and have the MPLv2 text re added to them in a way that staging the changes removes any difference
run pre-commit run --all-files and confirm there are no changes
Test drive around the site and confirm there are no unexpected markup bombs or JS errors

stevejalim · 2021-10-21T13:50:01Z

.eslintrc.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+


Given that the webpack config already had the MPL in it (see the final file in this changeset), I'm assuming that literally everything in the repo needs the MPL, including configuration files like this. Makes sense that it would, I think.

stevejalim · 2021-10-21T14:15:07Z

bedrock/base/templates/404.html

+ This Source Code Form is subject to the terms of the Mozilla Public
+ License, v. 2.0. If a copy of the MPL was not distributed with this
+ file, You can obtain one at https://mozilla.org/MPL/2.0/.
+#}


This might look like an uncessary change, but we needed to make sure the existing comments matche the format that the pre-commit hook was expecting and generating, else we'd end up with the MPL double-included

stevejalim · 2021-10-21T14:19:34Z

bedrock/base/templates/includes/careers-teaser.html

+ License, v. 2.0. If a copy of the MPL was not distributed with this
+ file, You can obtain one at https://mozilla.org/MPL/2.0/.
+#}
+


This shoudn't mess with the beast, I think

stevejalim · 2021-10-21T14:39:44Z

(Self-reviewing from the top-down)

alexgibson

I'm still test driving this but here's a few issues I encountered so far:

For JS files, the license block inserted by pre-commit differs from the comment formatting expected by Prettier. When I delete a comment and then try to make a commit, the hook fails twice (once for this check and a second time for Prettier) before it will accept the comment formatting.
When I run pre-commit run --all-files it still finds some changes:

Some sources were modified by the hook ['lib/l10n_utils/tests/test_files/l10n/fr/brands.ftl'] 
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit


Some sources were modified by the hook ['lib/l10n_utils/tests/test_files/l10n/fr/mozorg/fluent.ftl'] 
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit


Some sources were modified by the hook ['lib/l10n_utils/tests/test_files/l10n/en/brands.ftl'] 
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit


Some sources were modified by the hook ['lib/l10n_utils/tests/test_files/l10n/en-US/mozorg/fluent.ftl'] 
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit


Some sources were modified by the hook ['lib/l10n_utils/tests/test_files/l10n/en/mozorg/fluent.ftl'] 
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit


Some sources were modified by the hook ['lib/l10n_utils/tests/test_files/l10n/de/mozorg/fluent.ftl'] 
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit

alexgibson · 2021-10-21T16:04:55Z

We should also make sure this commit gets added to .git-blame-ignore-revs

stevejalim · 2021-10-21T16:10:01Z

FTL omissions now included/fixed @alexgibson. Will DM re prettier vs this hook

stevejalim · 2021-10-21T16:29:43Z

@alexgibson Thanks for the feedback. Fixed the prettier issue by re-ordering the execution of the hooks - works fine that we add the missing text before running the style checkers, so that any styling mismatches (eg prettier's take on comments formatting) happens in the same single pre-commit pass. Basically, if someone forgets a licence, they will have to re-stage the relevant file(s), but now at least only once, including any formatting fixes

(bedrock) [steve] ~/Code/bedrock $ gc                                                                                 [10614--mpl-pre-commit]
Insert license in comments...........................(no files to check)Skipped
Insert license in comments...............................................Failed
- hook id: insert-license
- exit code: 1
- files were modified by this hook

Some sources were modified by the hook ['media/index.js']
Now aborting the commit.
You should check the changes made. Then simply "git add --update ." and re-commit

Insert license in comments...........................(no files to check)Skipped
Insert license in comments...........................(no files to check)Skipped
Insert license in comments...........................(no files to check)Skipped
Insert license in comments...........................(no files to check)Skipped
black................................................(no files to check)Skipped
isort................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
prettier.................................................................Failed
- hook id: prettier
- files were modified by this hook

media/index.js

stylelint............................................(no files to check)Skipped
eslint...................................................................Passed

stevejalim · 2021-10-21T16:47:13Z

bedrock/mozorg/templates/mozorg/mpl/headers/index.html

@@ -1,6 +1,8 @@
-{# This Source Code Form is subject to the terms of the Mozilla Public


This is another special file because it contains the MPL as a comment and as content/examples for people to use

stevejalim · 2021-10-21T16:48:59Z

bedrock/mozorg/templates/mozorg/mpl/2.0/combining-mpl-and-gpl.html

+ This Source Code Form is subject to the terms of the Mozilla Public
+ License, v. 2.0. If a copy of the MPL was not distributed with this
+ file, You can obtain one at https://mozilla.org/MPL/2.0/.
+#}


This is another special file which uses the licence text in different ways - for the file itself, and for its content

stevejalim · 2021-10-21T20:15:36Z

We should also make sure this commit gets added to .git-blame-ignore-revs

Thinking about this @alexgibson -- maybe it's a change we do want to reflect in the changelog, as it's basically additive or corrective in very specific places, that won't obliterate significant history, rather than - say - black which would make it harder to spot the relevant history of a line of code that was amended purely for formatting.

pmac · 2021-10-21T20:19:14Z

Good point. This does seem like an okay thing to keep in the history.

alexgibson

r+ (reviewing this PR has caused my tab in Firefox to freeze twice!) 😬

stevejalim · 2021-10-22T09:53:36Z

lib/l10n_utils/tests/test_commands.py

@@ -2,7 +2,7 @@



Some test fixups were needed here for things which parsed templates

stevejalim · 2021-10-22T11:27:58Z

@alexgibson Following our DMs, I've removed the MPL added in error to the four JS lib files we have and also amended the pre-commit hook to not do that in the future. The isolated commit for that is: dd5d766

I looked into adding RST support but a) we don't have any gaps at the mo, so the manual process is decent and b) the hook doesn't seem to pick up changes to .rst, so am drawing a line under this

The only other file type that we could include because we have the MPL in some files but not all is .svg files, but I would prefer that to be split into a separate ticket that maybe somoene more frontendy could run with, as they would might have a faster/better way to test for potential SVG breakage etc

I've eyeballed literally all of the diff, so if you're happy with the commit above, I think we're good.

(Will quickly rebase on master now)

Adds to Python, JS, SCSS, Jinja HTML, Fluent templates and shell scripts Note that the order of application of the hooks is important - we want to add a missing license before we check the formatting of files

…t produced by pre-commit hook * Updates existing MPLv2 text to use a https URL * Amend a handful of Fluent templates that used a token instead of the string "Mozilla" - this standardised things; translation was not used or needed * Add missing MPLv2 where needed * Update three tests that regressed with these changes, above

…e MPL from the four files which should not have had it

alexgibson · 2021-10-22T12:04:50Z

@stevejalim thanks! I think this looks great overall. As to adding comments to svg files, that's something we certainly could do, although I'm also a bit concious as to how far it really makes sense to go with this, as doing this with assets loaded as images likely also has a (small but incremental) performance impact, unless we then go to the effort of optimising them again in the build process.

For now, I think this is fine to merge and iterate on in the future, if we think it makes sense. r+ 🍡

stevejalim force-pushed the 10614--mpl-pre-commit branch 2 times, most recently from 3c57485 to aa1cc65 Compare October 21, 2021 13:10

stevejalim marked this pull request as ready for review October 21, 2021 13:42

stevejalim added Needs Review Awaiting code review Review: M Code review time: 1-2 hours labels Oct 21, 2021

stevejalim requested review from pmac and alexgibson October 21, 2021 13:42

stevejalim commented Oct 21, 2021

View reviewed changes

alexgibson requested changes Oct 21, 2021

View reviewed changes

stevejalim force-pushed the 10614--mpl-pre-commit branch from aa1cc65 to a1c8270 Compare October 21, 2021 16:07

stevejalim force-pushed the 10614--mpl-pre-commit branch from a1c8270 to 8a18a40 Compare October 21, 2021 16:27

stevejalim requested a review from alexgibson October 21, 2021 16:29

stevejalim force-pushed the 10614--mpl-pre-commit branch from 8a18a40 to 47a26a0 Compare October 21, 2021 16:32

stevejalim commented Oct 21, 2021

View reviewed changes

stevejalim force-pushed the 10614--mpl-pre-commit branch 2 times, most recently from 6eb48fc to b13efc0 Compare October 21, 2021 22:12

alexgibson approved these changes Oct 22, 2021

View reviewed changes

stevejalim commented Oct 22, 2021

View reviewed changes

stevejalim added 2 commits October 22, 2021 12:32

10614: Add pre-commit hook for including the MPLv2

fafcfcd

Adds to Python, JS, SCSS, Jinja HTML, Fluent templates and shell scripts Note that the order of application of the hooks is important - we want to add a missing license before we check the formatting of files

10614: Update pre-commit config to not add MPL to JS libraries; Remov…

a5e48d3

…e MPL from the four files which should not have had it

stevejalim force-pushed the 10614--mpl-pre-commit branch from dd5d766 to a5e48d3 Compare October 22, 2021 11:35

stevejalim merged commit a8ed6ae into mozilla:master Oct 22, 2021

stevejalim deleted the 10614--mpl-pre-commit branch October 22, 2021 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre-commit hook for including the MPLv2 in all source code files #10617

Add pre-commit hook for including the MPLv2 in all source code files #10617

stevejalim commented Oct 20, 2021 •

edited

Loading

stevejalim Oct 21, 2021 •

edited

Loading

stevejalim Oct 21, 2021

stevejalim Oct 21, 2021

stevejalim commented Oct 21, 2021

alexgibson left a comment •

edited

Loading

alexgibson commented Oct 21, 2021

stevejalim commented Oct 21, 2021 •

edited

Loading

stevejalim commented Oct 21, 2021

stevejalim Oct 21, 2021 •

edited

Loading

stevejalim Oct 21, 2021

stevejalim commented Oct 21, 2021

pmac commented Oct 21, 2021

alexgibson left a comment

stevejalim Oct 22, 2021

stevejalim commented Oct 22, 2021

alexgibson commented Oct 22, 2021

		@@ -1,6 +1,8 @@
		{# This Source Code Form is subject to the terms of the Mozilla Public

Add pre-commit hook for including the MPLv2 in all source code files #10617

Add pre-commit hook for including the MPLv2 in all source code files #10617

Conversation

stevejalim commented Oct 20, 2021 • edited Loading

Description

Issue / Bugzilla link

Testing

stevejalim Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

stevejalim Oct 21, 2021

Choose a reason for hiding this comment

stevejalim Oct 21, 2021

Choose a reason for hiding this comment

stevejalim commented Oct 21, 2021

alexgibson left a comment • edited Loading

Choose a reason for hiding this comment

alexgibson commented Oct 21, 2021

stevejalim commented Oct 21, 2021 • edited Loading

stevejalim commented Oct 21, 2021

stevejalim Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

stevejalim Oct 21, 2021

Choose a reason for hiding this comment

stevejalim commented Oct 21, 2021

pmac commented Oct 21, 2021

alexgibson left a comment

Choose a reason for hiding this comment

stevejalim Oct 22, 2021

Choose a reason for hiding this comment

stevejalim commented Oct 22, 2021

alexgibson commented Oct 22, 2021

stevejalim commented Oct 20, 2021 •

edited

Loading

stevejalim Oct 21, 2021 •

edited

Loading

alexgibson left a comment •

edited

Loading

stevejalim commented Oct 21, 2021 •

edited

Loading

stevejalim Oct 21, 2021 •

edited

Loading