Migrate tools/licenses to null safety #35882

mit-mit · 2022-09-02T12:36:43Z

Migrate src/flutter/tools/licenses to null safety.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-09-02T12:36:44Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

zanderso · 2022-09-08T20:33:16Z

From PR triage: Adding the WIP label.

mit-mit · 2022-09-12T15:04:51Z

I believe this is ready for a review.

There is one test failure, but I'm a bit puzzled as I don't think I changed anything in the logic of _findLicenseBlocks.

And the test output actually looks legit; it's triggered by the file third_party/libjpeg-turbo/simd/jsimd_mips_dspr2.S that has several copyright statements... @Hixie @goderbauer looks you both worked on this tool, so wondering if you can comment if that failure is actually valid?

tools/licenses/lib/main.dart

tools/licenses/lib/licenses.dart

tools/licenses/lib/main.dart

Hixie · 2022-09-12T19:35:55Z

There is one test failure, but I'm a bit puzzled as I don't think I changed anything in the logic of _findLicenseBlocks.

Looking at the goldens, it seems we used to do this one correctly, so something in this patch must have affected it...

Hixie · 2022-09-12T19:39:31Z

tools/licenses/lib/main.dart

-      final _RepositorySourceFile main = parent.getChildByName('turbojpeg.c') as _RepositorySourceFile;
-      final _RepositoryDirectory simd = parent.getChildByName('simd') as _RepositoryDirectory;
+  List<License>? get licenses {
+    if (_licenses == null && parent != null) {


the && parent != null should not be necessary

Done.

I'm a bit puzzled why that works given parent is nullable. I guess there is an invariant that get isn't called when parent is null?

yeah, the root case is special in various ways such that the code never gets called when parent is null.

This code was designed for a nullable world. I would write it quite differently now with null safety.

tools/licenses/lib/main.dart

Hixie · 2022-09-12T19:41:39Z

tools/licenses/lib/licenses.dart

+      bool hadMoreLines;
+      while (hadMoreLines = lines.moveNext() && lines.current.value.startsWith(prefix)) {
        final String nextAuthor = lines.current.value.substring(prefix.length);
        if (nextAuthor == '' || nextAuthor[0] == ' ' || nextAuthor[0] == '\t') {
          throw 'unexpectedly ragged author list when looking for copyright';
        }
        end = lines.current.end;
      }
-      if (lines.current == null) {
+      if (!hadMoreLines) {


i wonder if this is somehow not 100% exactly semantically equivalent and is causing the bug you ran into

I was missing a set of parenthesis :-)

- while (hadMoreLines = lines.moveNext() && lines.current.value.startsWith(prefix)) { + while ((hadMoreLines = lines.moveNext()) && lines.current.value.startsWith(prefix)) {

aha, glad i pinned down the section if not the reason :-)

mit-mit · 2022-09-13T12:04:06Z

Test is now passing, but I need to update the goldens:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8803139310453266113/+/u/licenses_check/stdout

mit-mit · 2022-09-13T13:37:05Z

Golden updated:

mit@mit-gbuntu:~/dev/flutter_engine/src/flutter/tools/licenses$ rm -rf ../../../out/licenses
dart lib/main.dart --src ../../.. --out ../../../out/licenses --golden ../../ci/licenses_golden
Finding files...

Computing signature for license tool
    Detected changes to license tool. Forcing license collection for all components.
Collecting licenses for flutter
      2918 of 2918 ██████████ 100% (0 missing licenses)  Done.
Collecting licenses for fuchsia
      1831 of 1831 ██████████ 100% (0 missing licenses)  Done.
Collecting licenses for gpu
         2 of 2 ██████████ 100% (0 missing licenses)  Done.
Collecting licenses for third_party
     11611 of 11611 ██████████ 100% (0 missing licenses)  Done.
Collecting licenses for skia
      5296 of 5296 ██████████ 100% (0 missing licenses)  Done.
mit@mit-gbuntu:~/dev/flutter_engine/src/flutter/tools/licenses$
mit@mit-gbuntu:~/dev/flutter_engine/src/flutter/tools/licenses$ cp ../../../out/licenses/* ../../ci/licenses_golden
git diff
diff --git a/ci/licenses_golden/tool_signature b/ci/licenses_golden/tool_signature
index 74aacad982..6ae74cafe6 100644
--- a/ci/licenses_golden/tool_signature
+++ b/ci/licenses_golden/tool_signature
@@ -1,2 +1,2 @@
-Signature: d232ec47cb6d61b7385bf1ef936a93c4
+Signature: 647326c44237e255694c24deb9896c68

Hixie

Hixie · 2022-09-13T22:31:27Z

This PR reminds me that I have a big patch (which is going to conflict up the wazoo now, oops! oh well) that was waiting for https://dart-review.googlesource.com/c/sdk/+/250792 to get reviewed and landed, but I was trying to follow the non-Googler path to contributing to Dart and didn't figure out how to get a review and then forgot about it.

Hixie · 2022-09-13T22:32:09Z

test-exempt: code refactor with no semantic change

flutter-dashboard bot added the needs tests label Sep 2, 2022

mit-mit changed the title ~~Migrate tools/licenses to null safety~~ [WIP] Migrate tools/licenses to null safety Sep 2, 2022

zanderso added the Work in progress (WIP) Not ready (yet) for review! label Sep 8, 2022

mit-mit changed the title ~~[WIP] Migrate tools/licenses to null safety~~ Migrate tools/licenses to null safety Sep 12, 2022

mit-mit mentioned this pull request Sep 12, 2022

Address null safety exclusions in flutter/engine flutter/flutter#110020

Closed

11 tasks