Font subset in the tool by dnfield · Pull Request #49737 · flutter/flutter

dnfield · 2020-01-29T22:07:56Z

Description

Shakes out unused icon fonts from Flutter applications. This reduces the Gallery by 100kb.

Wires in the font subetting logic to the assets target, defaulted to off, only available in release or profile mode
Updates all commands to take flag.
Defaults flag to false
Stubs out implementation for other platforms that aren't yet supported.

Not available in debug mode because incremental kernels in there would make this really tricky, slow down hot restart, and you generally don't care about font file size for local debugging.

Needs tests.

/cc @willlarche
/cc @chingjun - we will want to wire up similar logic for Google builds

Related Issues

Fixes #43644
Fixes #16311
Addresses two platforms for #49730

Tests

I added the following tests:

TODO

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

jonahwilliams · 2020-01-29T22:10:35Z

packages/flutter_tools/lib/src/build_info.dart

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+import 'package:flutter_tools/src/build_system/targets/assets.dart';


nit: import

jonahwilliams · 2020-01-29T22:12:10Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

+      codePoints: iconData[entry.key],
+    );
+  }
+  globals.printTrace('Found iconData: $result');


This might be a lot of data to see in devicelab logs

jonahwilliams · 2020-01-29T22:13:02Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

+    outputPath,
+    inputPath,
+  ];
+  globals.printTrace('Starting font-subset: ${cmd.join(' ')}...');


I would printTrace less here since the command runs in a pool with concurrency they'll be stacked up in ways that don't make sense

jonahwilliams · 2020-01-29T22:15:36Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

 }

+/// Returns a map of [FontSubsetData] keyed by relative path.
+Future<Map<String, FontSubsetData>> _getIconData(Environment environment, DevFSStringContent fontManifest) async {


Can we collect the various private methods here into a single class? Once this is done, it would be better if injected values like the filesystem, artifacts, or logger came from the class instead. For example, see https://github.com/flutter/flutter/pull/48444/files

zanderso

+1 to creating (a) class(es), probably in a new file, for the font subsetting logic.

zanderso · 2020-01-29T22:26:07Z

packages/flutter_tools/gradle/flutter.gradle

            }
+            print(fontSubset);
+            if (fontSubset == true) {
+


nit: extra newline

zanderso · 2020-01-29T22:28:31Z

packages/flutter_tools/lib/src/artifacts.dart

      case Artifact.fontSubset:
      case Artifact.constFinder:
-        return _cache.getArtifactDirectory('font-subset').childFile(_artifactToFileName(artifact, platform, mode)).path;
+        return _cache.getArtifactDirectory('engine').childDirectory(getNameForTargetPlatform(platform)).childFile(_artifactToFileName(artifact, platform, mode)).path;


nit: long line:

return _cache.getArtifactDirectory('engine') .childDirectory(getNameForTargetPlatform(platform)) .childFile(_artifactToFileName(artifact, platform, mode)) .path;

zanderso · 2020-01-29T22:31:32Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

  final List<File> outputs = <File>[];
+
+  final Map<String, FontSubsetData> iconData = useFontSubset
+      ? await _getIconData(


nit: 2 space indent on a continued line.

zanderso · 2020-01-29T22:39:15Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

          inputs.add(globals.fs.file(content.file.path));
-          await (content.file as File).copy(file.path);
+          final FontSubsetData fontSubsetData = iconData[entry.key];
+          if (fontSubsetData != null) {


This is getting a bit too nested. Maybe:

if (content is! DevFSFileContent || content.file is! File) { await file.writeAsBytes(await entry.value.contentAsBytes()); return; } inputs.add(globals.fs.file(content.file.path)); final FontSubsetData fontSubsetData = iconData[entry.key]; if (fontSubsetData == null) { await (content.file as File).copy(file.path); return; } assert(useFontSubset); await _subsetFont( environment: environment, inputPath: content.file.path, outputPath: file.path, fontSubsetData: fontSubsetData, );

This fails analysis - analyzer isn't smart enough to see that we've said content is! ... || and then know that otherwise it is.

Refactored to a class and made it a bit easier to use - now subsetFont returns a bool to tell you whether it actually did the thing, and that eliminates a level of nesting and a few lines of code in this file.

zanderso · 2020-01-29T22:47:35Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

+  final Process fontSubsetProcess = await globals.processManager.start(cmd);
+  final String codePoints = fontSubsetData.codePoints.join(' ');
+  globals.printTrace('Started, writing $codePoints...');
+  fontSubsetProcess.stdin.writeln(codePoints);


Writing to the subprocess' stdin can throw an exception if the subprocess exits early for some reason. I'd suggest wrapping these three lines in a try {} catch {}, and letting the exit code test handle that case.

zanderso · 2020-01-29T22:56:26Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

+  final Map<String, String> result = <String, String>{};
+  final List<Map<String, dynamic>> fontList = _getList(json.decode(fontManifestData));
+  for (final Map<String, dynamic> map in fontList) {
+    final String familyKey = map['family'] as String;


Do we need to validate that this is a string?

zanderso · 2020-01-29T22:56:59Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

+      if (fonts.length != 1) {
+        throwToolExit('This tool cannot process icon fonts with multiple fonts in a single family.');
+      }
+      result[familyKey] = fonts.first['asset'] as String;


zanderso · 2020-01-29T22:59:21Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

+  if (constFinderProcessResult.exitCode != 0) {
+    throwToolExit('ConstFinder failure: ${constFinderProcessResult.stderr}');
+  }
+  final Map<String, dynamic> constFinderMap = json.decode(constFinderProcessResult.stdout as String) as Map<String, dynamic>;


I'd recommend an explicit check here that the json matches the schema your expecting.

zanderso · 2020-01-29T23:00:55Z

packages/flutter_tools/lib/src/commands/assemble.dart

  Map<String, String> _parseDefines(List<String> values) {
    final Map<String, String> results = <String, String>{};
    for (final String chunk in values) {
+      print(chunk);


leftover debug print?

zanderso · 2020-01-29T23:02:50Z

packages/flutter_tools/lib/src/web/compile.dart

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+import 'package:flutter_tools/src/build_system/targets/assets.dart';


import '../build_system/targets/assets.dart';

but it looks like it's unused?

Yeah I started web and it won't quite work yet - reverted this part.

blasten · 2020-01-30T01:02:50Z

packages/flutter_tools/test/general.shard/android/gradle_test.dart

    test('assemble release', () {
      expect(
-        getAssembleTaskFor(const BuildInfo(BuildMode.release, null)),
+        getAssembleTaskFor(const BuildInfo(BuildMode.release, null, fontSubset: false)),


nit: fontSubset default could be false.

I didn't want to miss any invocations of it anywhere. I think there's also a slight preference for requiring parameters to be set in the tool so that we avoid surprise defaults

blasten · 2020-01-30T01:07:36Z

packages/flutter_tools/lib/src/runner/flutter_command.dart

  }

+  void addFontSubsetFlag() {
+    argParser.addFlag('font-subset',


maybe tree-shake-fonts?

I'm not sure if folks would be able to determinate what this flag does from the current name and help text.

dnfield · 2020-01-30T05:39:52Z

I think I captured all the review, will look to add tests soon.

zanderso · 2020-01-30T15:36:36Z

packages/flutter_tools/lib/src/build_system/targets/assets.dart

 ///
 /// Returns a [Depfile] containing all assets used in the build.
 Future<Depfile> copyAssets(Environment environment, Directory outputDirectory) async {
+


nit: extra newline

zanderso · 2020-01-30T15:42:48Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+  ///
+  /// The constructor will validate the environment and print a warning if
+  /// font subsetting has been requested in a debug build mode.
+  FontSubset(this._environment, DevFSStringContent fontManifest) : assert(_environment != null) {


assert(fontManifest != null) in the initializer list to match the comment.

Done.

One thing I'm worried about here is that this ctor can actually launcha process. This is probably surprising to callers.

I considered putting that logic into the first call of subsetFont (e.g. if the _iconData is null, run that logic.

WDYT?

Oh yeah, I missed that. The analyzer should be warning you about the dropped Future at the _getIconData() call site in the constructor. It should also be warning that _getIconData() is returning a Map but is declared to return Future<void>.

Putting the _getIconData() call in the first call to subsetFont sounds better to me

Yeah - I was going to have to rework it to set a completer somewhere. Super weird, and probably never good to have a ctor start a process.

zanderso · 2020-01-30T15:46:08Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+    final Map<String, String> fonts = await _parseFontJson(fontManifest.string, familyKeys);
+
+    if (fonts.length != iconData.length) {
+      throwToolExit('Expected to find fonts for ${iconData.keys}, but found ${fonts.keys}.');


If we throw a ToolExit we should be giving the user some guidance about how to fix the problem. If this isn't a problem the user can fix, and is instead a bug, then you can just throw an exception. Maybe a custom exception type like FontSubsetException so that it will be more visible in crash logging.

Adding some guidance. This should be a user error.

zanderso · 2020-01-30T15:47:01Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+    return result;
+  }
+
+


nit: extra newline

zanderso · 2020-01-30T15:51:44Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+      return false;
+    }
+
+    final File fontSubset = globals.fs.file(globals.artifacts.getArtifactPath(Artifact.fontSubset));


Check fontSubset.existsSync(). If it doesn't exist you can throwToolExit() with guidance to run the doctor.

zanderso · 2020-01-30T15:55:21Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+        throwToolExit('FontManifest.json invalid: expected the family value to be a string, got: ${map['family']}.');
+      }
+      final String familyKey = map['family'] as String;
+      if (families.contains(familyKey)) {


Flip the sense, and unindent the bigger part.

if (!families.contains(familyKey)) { continue; } ...

zanderso · 2020-01-30T15:57:31Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+    }
+    final dynamic jsonDecode = json.decode(constFinderProcessResult.stdout as String);
+    if (jsonDecode is! Map<String, dynamic>) {
+      throwToolExit('Invalid ConstFinder output: expected a top level JavaScript object, got $jsonDecode.');


This one seems like an internal error that the user can't do anything about, so probably an exception is better than a ToolExit.

zanderso · 2020-01-30T15:58:03Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+    final ProcessResult constFinderProcessResult = await globals.processManager.run(cmd);
+
+    if (constFinderProcessResult.exitCode != 0) {
+      throwToolExit('ConstFinder failure: ${constFinderProcessResult.stderr}');


A ToolExit is fine here if the stderr will tell the user how to fix the problem.

zanderso · 2020-01-30T16:02:02Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+  const _ConstFinderResult(this.result);
+
+  final Map<String, dynamic> result;
+  List<Map<String, dynamic>> get constantInstances => _getList(result['constantInstances'], 'Invalid ConstFinder output: Expected "constInstances" to be a list of objects.');


Cache the results of json parsing to avoid recomputing every time the getter is touched.

zanderso · 2020-01-30T16:05:58Z

packages/flutter_tools/lib/src/build_system/targets/font_subset.dart

+    final Map<String, List<int>> result = <String, List<int>>{};
+    for (final Map<String, dynamic> iconDataMap in consts.constantInstances) {
+      if (iconDataMap['fontPackage'] is! String || iconDataMap['fontFamily'] is! String || iconDataMap['codePoint'] is! int) {
+        throwToolExit('Invalid ConstFinder result. Expected "fontPackage" to be a String, "fontFamily" to be a String, and "codePoint" to be an int, got: $iconDataMap.');


jonahwilliams · 2020-02-03T19:20:21Z

packages/flutter_tools/gradle/flutter.gradle

            if (extraGenSnapshotOptions != null) {
                args "--ExtraGenSnapshotOptions=${extraGenSnapshotOptions}"
            }
+            if (treeShakeIcons == true) {


nit: the ExtraGenSnapshotOptions is a bit weird, so place the -d above it so it aligned within the other define options

jonahwilliams · 2020-02-03T19:22:08Z

packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart

+    DevFSStringContent fontManifest, {
+    @required ProcessManager processManager,
+    @required Logger logger,
+    @required FileSystem fs,


nit: name this fileSystem and not fs to avoid confusion with the global

jonahwilliams · 2020-02-03T19:23:29Z

packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart

+      _artifacts.getArtifactPath(Artifact.fontSubset),
+    );
+    if (!fontSubset.existsSync()) {
+      throwToolExit('The font-subset utility is missing. Run "flutter doctor".');


This would indicate a programming error in the flutter tool. I think we should just let this one crash if we hit it so we can see it crash logging instead of covering it with a tool exit

I was thinking a user could mess this up on us but I guess we should re-download if they do. Will change.

jonahwilliams · 2020-02-03T19:24:11Z

packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart

+      fontSubsetProcess.stdin.writeln(codePoints);
+      await fontSubsetProcess.stdin.flush();
+      await fontSubsetProcess.stdin.close();
+    } catch (e) {


nit: catch the specific error/exception types you expect. As written this will catch NoSuchMethodError or TypeError

It's not clear to me which exceptions these methods can throw. The idea is that exitCode will handle it. @zanderso is there some exception or set of exceptions to do? OSError and some others?

Catching NoSuchMethodError or TypeError is bad because those are programming errors. There are several errors that could happen writing to stdin. They'll all be derived from Exception, which doesn't include the Error types like TypeError etc.

Ok. I have a block for on Exception and a block for on OSError that I think shoudl cover us.

jonahwilliams · 2020-02-03T19:26:01Z

packages/flutter_tools/test/general.shard/build_system/targets/icon_tree_shaker_test.dart

+  });
+
+  Environment _createEnvironment(Map<String, String> defines) {
+    return Environment(


Use Environment.test

Environment.test wants a test directory, which I don't have, right?

It allows you to set a single directory that everything is based on, if the directories themselves don't matter much

Ah, I get it now, done.

jonahwilliams · 2020-02-03T19:29:16Z

packages/flutter_tools/lib/src/ios/mac.dart

    'xcrun',
    'xcodebuild',
    '-configuration', configuration,
+    if (buildInfo.treeShakeIcons)


Add this with the reset of the environment variables in buildCommands

Did you have somewhere specific in mind? They seem sprinked throughout the rest of this function.

in buildCommands, those get written to an xcconfig so builds through the xcode ui reference the variables correctly. You're adding it to the invocation which only takes effect when running through the flutter tool

Moved this to the xcodeproject file where it writes to the generated xcconfig

zanderso · 2020-02-04T16:47:34Z

packages/flutter_tools/test/general.shard/build_system/targets/assets_test.dart

    expect(globals.fs.file(globals.fs.path.join(environment.buildDir.path, 'flutter_assets', 'assets/wildcard/%23bar.png')).existsSync(), true);
  }));

-  test('Does not leave stale files in build directory', () => testbed.run(() async {


This seems like a good property to have. Why does the test need to be removed?

This test requires the whole build system to work. With assets now having new dependencies, it's much more complicated to make it work. @jonahwilliams says its covered elsewhere anyway, but that he'll rework this specific case into a build system check - #49914 was filed to track that.

zanderso

lgtm if all of @jonahwilliams' comments are addressed

jonahwilliams

LGTM

fluttergithubbot · 2020-02-04T23:43:01Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite hostonly_devicelab_tests-2-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite build_tests-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.

dnfield · 2020-02-05T04:34:13Z

Cirrus says the build is not currently red - status isn't up to date. landing.

dnfield added 2 commits January 29, 2020 14:03

Wire in font subset

23a4e3c

unrelated test updates

8d6139e

dnfield requested review from blasten, jonahwilliams and zanderso January 29, 2020 22:07

fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Jan 29, 2020

googlebot added the cla: yes label Jan 29, 2020

dnfield added the a: size Reducing IPA/APK/JS sizes label Jan 29, 2020

jonahwilliams reviewed Jan 29, 2020

View reviewed changes

easy parts of review

39265c3

zanderso reviewed Jan 29, 2020

View reviewed changes

..

ebe8927

blasten reviewed Jan 30, 2020

View reviewed changes

dnfield force-pushed the font_subset branch from f7f8bc7 to 473e384 Compare January 30, 2020 05:26

review

d7945f8

dnfield force-pushed the font_subset branch from 473e384 to d7945f8 Compare January 30, 2020 05:26

catch cast errors

1905c64

zanderso reviewed Jan 30, 2020

View reviewed changes

dnfield added 3 commits January 30, 2020 14:42

Start tests

fa2862f

..

227d300

actual test

0e6b02d

dnfield force-pushed the font_subset branch from d1a9983 to 0e6b02d Compare January 31, 2020 05:06

integration test

a376932

dnfield marked this pull request as ready for review January 31, 2020 06:12

dnfield mentioned this pull request Jan 31, 2020

Move regression test for assets to build system #49914

Closed

dnfield added 2 commits January 31, 2020 15:42

Fix bugs, jonah review

40bc648

analyzer

3757b1f

dnfield requested review from jonahwilliams and zanderso February 3, 2020 19:18

jonahwilliams reviewed Feb 3, 2020

View reviewed changes

dnfield added 3 commits February 3, 2020 12:40

review

2ade1cd

more review

9b7f792

throw a real exception from mock

8615982

zanderso reviewed Feb 4, 2020

View reviewed changes

zanderso approved these changes Feb 4, 2020

View reviewed changes

jonahwilliams approved these changes Feb 4, 2020

View reviewed changes

dnfield added the waiting for tree to go green label Feb 4, 2020

merge

ca4939a

fluttergithubbot removed the waiting for tree to go green label Feb 4, 2020

re-merge

1a2b0be

dnfield added the waiting for tree to go green label Feb 5, 2020

dnfield merged commit 4b8efad into flutter:master Feb 5, 2020

dnfield deleted the font_subset branch February 5, 2020 04:34

gspencergoog mentioned this pull request Mar 10, 2020

Material Icon font needs to be bundled automatically on some platforms. #52360

Closed

VladyslavBondarenko mentioned this pull request Mar 27, 2020

Custom icon fonts tree shaking #53400

Closed

willlarche mentioned this pull request May 4, 2020

Missing Material Icons #56046

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Conversation

dnfield commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

zanderso 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!

dnfield commented Jan 30, 2020

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!

dnfield commented Jan 29, 2020 •

edited

Loading