License script improvements #38148

Hixie · 2022-12-09T01:01:51Z

This is a significant revamp of the license script that I've been slowly doing for the past 10+ months so it includes a whole host of things that I'm sure I've forgotten about, but most notably:

impellerc dependencies are now included so we are more confident that we're reporting the right things there.
There's now a file that lists all the excluded files, so that when we add new files to the repo we can be confident that they are included (by making sure they weren't added to the excluded files list).
The golden files lists all the origins for each license block.
A large number of unit tests and other such files are now excluded where previously they were not.
A number of licenses that were previously missed due to a bug in the script are now correctly caught and included. (As far as I can tell, none of the omissions were actually a problem, the list is just more comprehensive now.)
A bug involving trailing slashes being dropped in license text has been fixed.
We handle the ICU library in a more coherent manner.
The license reformatting has been tightened up a bit, and is now slightly tested.
I added a --verbose mode that reports on which RegExp instances were most expensive.
Some internal error handling is now more helpful.
I refactored the crawling logic so that skipping a directory is now just a matter of adding a line to a configuration list in paths.dart rather than a convoluted exercise involving creating a new subclass.
Similarly the obtuse massive regular expressions for files to skip are now largely replaced by lists of excluded file names that are easy to configure.
The MPL license for the certificates now includes a link to the actual source.
Removed some obsolete patterns that were no longer matching anything.

This does make the LICENSE file bigger overall (about 10KB compressed), but we may be able to skip shipping the file entirely soon (and download it on demand instead) so I'm not greatly worried about this.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

stuartmorgan-g

LGTM with some nits and suggestions! Mostly just comments about commenting to try to make the script a little less opaque; overall this looks great. The new directory pruning system is definitely a huge improvement, as is the built-in diagnostics (no more having to manually change things and re-run just to have any clue how a specific pattern was fitting in!)

I didn't audit all of the license regexes since I don't think there's any sane way to do all of them in the context of a review.

stuartmorgan-g · 2022-12-13T19:26:09Z

tools/licenses/lib/filesystem.dart

@@ -20,14 +21,14 @@ enum FileType {
  tar, // should be parsed as an archive and drilled into
  gz, // should be parsed as a single compressed file and exposed
  bzip2, // should be parsed as a single compressed file and exposed
-  metadata, // can be skipped entirely (e.g. Mac OS X ._foo files)
+  metadata, // can be skipped entirely (e.g. Mac OS X ._foo files, bash scripts)


Shouldn't bash scripts be their own type with their own handling? I don't think of bash scripts as metadata.

Or maybe this category should just have a different name, like notPartOfBuild?

stuartmorgan-g · 2022-12-13T19:29:34Z

tools/licenses/lib/filesystem.dart

@@ -390,13 +408,14 @@ mixin Latin1TextFile implements TextFile {
      if (isUTF8) {
        throw '$fullName contains valid UTF-8 and is probably not actually encoded as Win1252';
      }
-      return latin1.decode(bytes);
+      return _normalize(latin1.decode(bytes));


Doesn't line 402 need normalizing as well?

if we get past line 402 without throwing, then we discard the results, so there's no need to normalize it.

stuartmorgan-g · 2022-12-13T19:31:17Z

tools/licenses/lib/formatter.dart

+
+import 'patterns.dart';
+
+String reformat(String body) {


Given the generic name, a documentation comment explaining what exactly this does would be very helpful.

added this comment:

/// This function takes a block of text potentially decorated with leading /// prefix indents, horizontal lines, C-style comment blocks, blank lines, etc, /// and removes all such incidental material leaving only the significant text.

stuartmorgan-g · 2022-12-13T19:32:33Z

tools/licenses/lib/formatter.dart

+  while (true) {
+    // Strip full-line decorations, e.g. horizontal lines, and trailing whitespace.
+    for (int index = 0; index < lines.length; index += 1) {
+      if (lines[index].endsWith(' ')) {


Why only space and not other whitespace like tab? Couldn't we just unconditionally trimRight?

yeah, good point, that test is redundant. removed

stuartmorgan-g · 2022-12-13T19:37:16Z

tools/licenses/lib/formatter.dart

+    }
+    while (lines.isNotEmpty && lines.last == '') {
+      lines.removeLast();
+    }


Wouldn't it be way more efficient for large files to find the first and last non-blank line and then do a single sublist call?

i doubt it. in the vast majority of cases this won't find anything; in the few cases where it does it'll probably just remove one line. it'll only rarely remove from the top and bottom.

iirc when i was profiling this i found that basically all the time is spent in regular expression matching for the big patterns, reformat in general is a very minor aspect and the stripping of blank lines is basically not an issue at all.

(also iirc, List.removeLast is O(1) and basically free. I forget how much removeAt(0) costs.)

(also iirc, List.removeLast is O(1) and basically free. I forget how much removeAt(0) costs.)

I was assuming O(n) for head removal, which becomes O(n^2) in pathological cases in the loop, which is why it jumped out at me as a potential issue. But it being a rounding error in actual context makes sense.

stuartmorgan-g · 2022-12-13T20:36:07Z

tools/licenses/lib/main.dart

-      License.fromBodyAndType(_dewrap(match.group(14)!), LicenseType.mit, origin: io.fullName),
-    ];
-    return result;
+  static String _fixup(String license) {


Comment about why this is needed since it seems pretty random?

added.

and yes. ugh. that license.

stuartmorgan-g · 2022-12-13T20:43:15Z

tools/licenses/lib/paths.dart

+import 'regexp_debug.dart';
+
+final Set<String> skippedPaths = <String>{
+  // directories or files that are not included in the final binaries


These must be full paths relative to the repo root, right? If so that's worth mentioning explicitly.

added several comments to this file to explain these lists better

stuartmorgan-g · 2022-12-13T20:44:41Z

tools/licenses/lib/paths.dart

+};
+
+final Set<String> skippedCommonDirectories = <String>{
+  // directories that should be skipped anywhere we find them


Is there a reason you are commenting these inside instead of above? Usually I expect comments describing the whole variable to be the line above, and comments inside the collection to group specific elements.

no particular reason, switched to the other style

stuartmorgan-g · 2022-12-13T20:47:46Z

tools/licenses/lib/patterns.dart

-final RegExp copyrightMentionPattern = RegExp(r'©| \(c\) (?!{)|copy\s*right\b|copy\s*left', caseSensitive: false);
-final RegExp licenseMentionPattern = RegExp(r'license|warrant[iy]', caseSensitive: false);
+final RegExp copyrightSanityCheck = RegExp(r'©|copyright|\(c\)', caseSensitive: false);
+final RegExp copyrightMentionPattern = RegExp(r'©|copyright [0-9]|\(c\) [0-9]|copyright \(c\)', caseSensitive: false);


Maybe add comments to distinguish these two from each other?

added comments for these

stuartmorgan-g · 2022-12-13T20:56:17Z

tools/licenses/test/formatter_test.dart

+  });
+  test('Indenting blocks', () {
+    expect(reformat('  a\nb\n  c'), 'a\nb\n  c');
+    expect(reformat('  a\n b\n  c'), 'a\nb\nc');


Some of these test lines could use comments; I don't know why Adding one space before b is expected to remove two spaces before c for instance (same for similar jumps in other tests).

added comments to all the lines in "Indenting blocks" section.

stuartmorgan-g · 2022-12-13T21:02:02Z

that I've been slowly doing for the past 10+ months so it includes a whole host of things that I'm sure I've forgotten about

Feel free to send me any number of small incremental changes to this script any time (hint hint 😉 )

Hixie · 2022-12-13T21:10:21Z

Thanks for the review!

Feel free to send me any number of small incremental changes to this script any time

Yeah, that'd be preferable in general for sure. Unfortunately these changes were all sort of intertwined and I kept finding more problems that I had to resolve before I could land the previous step and, uh, it sort of got out of control. Sorry.

stuartmorgan-g · 2022-12-13T21:13:43Z

Yaks gonna yak :)

auto-submit · 2022-12-14T21:10:13Z

auto label is removed for flutter/engine, pr: 38148, due to - The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2022-12-14T21:32:11Z

auto label is removed for flutter/engine, pr: 38148, due to - The status or check suite Mac Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2022-12-15T00:26:52Z

auto label is removed for flutter/engine, pr: 38148, due to - This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

…117148) * df430c4fd Revert "[Impeller] Speculatively attempt to fix Metal PSO construction errors on host targets." (flutter/engine#38292) * 41dd4f5e1 Revert "Revert "[Impeller] Speculatively attempt to fix Metal PSO construction errors on host targets." (#38292)" (flutter/engine#38301) * 8ce9a3f71 Roll Skia from 3171deabd88a to b368746d696a (13 revisions) (flutter/engine#38294) * 4881fe25e Revert "Revert "reland "Migrate darwin common "framework_shared" target to ARC #37049" (#37219)" (#37320)" (flutter/engine#37883) * 3b2302c8d [Impeller] Remove validation log when the pipeline library is collected before pipeline is setup. (flutter/engine#38306) * a04997c81 [Impeller] Disable impeller_unittests. (flutter/engine#38307) * fc71faad0 License script improvements (flutter/engine#38148) * 6a2560c35 [Windows] Synthesize modifier keys events on pointer events (flutter/engine#38138) * b1d407563 Roll Skia from b368746d696a to 3f81f95176ce (11 revisions) (flutter/engine#38312) * b25fcf748 Roll Skia from 3f81f95176ce to 46e8f2a18a3d (3 revisions) (flutter/engine#38314) * 948699bba Collapse bounds calculations into DisplayListBuilder (flutter/engine#34365) * 38367de84 Roll Fuchsia Mac SDK from u-tC0QEGUT4xQ4KOo... to VEOIaacOA75U7PYyz... (flutter/engine#38316) * 29196519c Roll Skia from 46e8f2a18a3d to 9f728d78f10d (1 revision) (flutter/engine#38317)

…lutter#117148) * df430c4fd Revert "[Impeller] Speculatively attempt to fix Metal PSO construction errors on host targets." (flutter/engine#38292) * 41dd4f5e1 Revert "Revert "[Impeller] Speculatively attempt to fix Metal PSO construction errors on host targets." (flutter#38292)" (flutter/engine#38301) * 8ce9a3f71 Roll Skia from 3171deabd88a to b368746d696a (13 revisions) (flutter/engine#38294) * 4881fe25e Revert "Revert "reland "Migrate darwin common "framework_shared" target to ARC flutter#37049" (flutter#37219)" (flutter#37320)" (flutter/engine#37883) * 3b2302c8d [Impeller] Remove validation log when the pipeline library is collected before pipeline is setup. (flutter/engine#38306) * a04997c81 [Impeller] Disable impeller_unittests. (flutter/engine#38307) * fc71faad0 License script improvements (flutter/engine#38148) * 6a2560c35 [Windows] Synthesize modifier keys events on pointer events (flutter/engine#38138) * b1d407563 Roll Skia from b368746d696a to 3f81f95176ce (11 revisions) (flutter/engine#38312) * b25fcf748 Roll Skia from 3f81f95176ce to 46e8f2a18a3d (3 revisions) (flutter/engine#38314) * 948699bba Collapse bounds calculations into DisplayListBuilder (flutter/engine#34365) * 38367de84 Roll Fuchsia Mac SDK from u-tC0QEGUT4xQ4KOo... to VEOIaacOA75U7PYyz... (flutter/engine#38316) * 29196519c Roll Skia from 46e8f2a18a3d to 9f728d78f10d (1 revision) (flutter/engine#38317)

Hixie force-pushed the license branch 6 times, most recently from c8b15ac to 0616799 Compare December 11, 2022 07:22

stuartmorgan-g approved these changes Dec 13, 2022

View reviewed changes

Hixie force-pushed the license branch from 0616799 to 9278cd8 Compare December 14, 2022 20:50

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022

Hixie mentioned this pull request Dec 14, 2022

LUCI shards bury the results behind several clicks flutter/flutter#117106

Open

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2022

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 15, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 15, 2022

License script improvements

6b202b0

Hixie force-pushed the license branch from 9278cd8 to 6b202b0 Compare December 15, 2022 01:55

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 15, 2022

auto-submit bot merged commit fc71faa into flutter:main Dec 15, 2022

Hixie deleted the license branch December 15, 2022 05:00

engine-flutter-autoroll mentioned this pull request Dec 15, 2022

Roll Flutter Engine from db5605ea7115 to 29196519c124 (13 revisions) flutter/flutter#117148

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2022

fc71faad0 License script improvements (flutter/engine#38148)

7df7983

loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Dec 16, 2022

License script improvements (flutter#38148)

5f36e1e

jason-simmons mentioned this pull request Dec 16, 2022

Fuchsia SDK roll failing on licenses check flutter/flutter#117162

Closed

loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Jan 3, 2023

License script improvements (flutter#38148)

bf33ea9

rmistry mentioned this pull request Jan 18, 2023

Make the license script faster. flutter/flutter#118193

Open

License script improvements #38148

License script improvements #38148

Uh oh!

Conversation

Hixie commented Dec 9, 2022

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Dec 13, 2022

Uh oh!

Hixie commented Dec 13, 2022

Uh oh!

stuartmorgan-g commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auto-submit bot commented Dec 14, 2022

Uh oh!

auto-submit bot commented Dec 14, 2022

Uh oh!

auto-submit bot commented Dec 15, 2022

Uh oh!

Uh oh!

stuartmorgan-g commented Dec 13, 2022 •

edited

Loading