[flutter_tools] update build rules to depend on subset of package_con…

…fig contents (flutter#67165) Split from flutter#66776 Even if pub does not change the packge_config contents, it will still update a timestamp in one of the fields. This causes unnecessary rebuilds. To fix this, generate an additional file when running pub get that only contains the relevant fields and then update the KernelSnapshot rule to depend on it only.
sambhurst · Oct 7, 2020 · 13bf341 · 13bf341
1 parent ab9373b
commit 13bf341
Show file tree

Hide file tree

Showing 7 changed files with 189 additions and 109 deletions.
diff --git a/packages/flutter_tools/lib/src/build_system/targets/android.dart b/packages/flutter_tools/lib/src/build_system/targets/android.dart
@@ -192,7 +192,6 @@ class AndroidAot extends AotElfBase {
   List<Source> get inputs => <Source>[
     const Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/android.dart'),
     const Source.pattern('{BUILD_DIR}/app.dill'),
-    const Source.pattern('{PROJECT_DIR}/.packages'),
     const Source.artifact(Artifact.engineDartBinary),
     const Source.artifact(Artifact.skyEnginePath),
     Source.artifact(Artifact.genSnapshot,

diff --git a/packages/flutter_tools/lib/src/build_system/targets/common.dart b/packages/flutter_tools/lib/src/build_system/targets/common.dart
@@ -161,8 +161,12 @@ class ReleaseCopyFlutterBundle extends CopyFlutterBundle {
   List<Target> get dependencies => const <Target>[];
 }
 
-
 /// Generate a snapshot of the dart code used in the program.
+///
+/// Note that this target depends on the `.dart_tool/package_config.json` file
+/// even though it is not listed as an input. Pub inserts a timestamp into
+/// the file which causes unecessary rebuilds, so instead a subset of the contents
+/// are used an input instead.
 class KernelSnapshot extends Target {
   const KernelSnapshot();
 
@@ -171,7 +175,7 @@ class KernelSnapshot extends Target {
 
   @override
   List<Source> get inputs => const <Source>[
-    Source.pattern('{PROJECT_DIR}/.packages'),
+    Source.pattern('{PROJECT_DIR}/.dart_tool/package_config_subset'),
     Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/common.dart'),
     Source.artifact(Artifact.platformKernelDill),
     Source.artifact(Artifact.engineDartBinary),
@@ -209,7 +213,9 @@ class KernelSnapshot extends Target {
     }
     final BuildMode buildMode = getBuildModeForName(environment.defines[kBuildMode]);
     final String targetFile = environment.defines[kTargetFile] ?? environment.fileSystem.path.join('lib', 'main.dart');
-    final File packagesFile = environment.projectDir.childFile('.packages');
+    final File packagesFile = environment.projectDir
+      .childDirectory('.dart_tool')
+      .childFile('package_config.json');
     final String targetFileAbsolute = environment.fileSystem.file(targetFile).absolute.path;
     // everything besides 'false' is considered to be enabled.
     final bool trackWidgetCreation = environment.defines[kTrackWidgetCreation] != 'false';
@@ -240,7 +246,7 @@ class KernelSnapshot extends Target {
     }
 
     final PackageConfig packageConfig = await loadPackageConfigWithLogging(
-      environment.projectDir.childFile('.packages'),
+      packagesFile,
       logger: environment.logger,
     );
 
@@ -341,7 +347,6 @@ class AotElfProfile extends AotElfBase {
   List<Source> get inputs => <Source>[
     const Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/common.dart'),
     const Source.pattern('{BUILD_DIR}/app.dill'),
-    const Source.pattern('{PROJECT_DIR}/.packages'),
     const Source.artifact(Artifact.engineDartBinary),
     const Source.artifact(Artifact.skyEnginePath),
     Source.artifact(Artifact.genSnapshot,
@@ -374,7 +379,6 @@ class AotElfRelease extends AotElfBase {
   List<Source> get inputs => <Source>[
     const Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/common.dart'),
     const Source.pattern('{BUILD_DIR}/app.dill'),
-    const Source.pattern('{PROJECT_DIR}/.packages'),
     const Source.artifact(Artifact.engineDartBinary),
     const Source.artifact(Artifact.skyEnginePath),
     Source.artifact(Artifact.genSnapshot,

diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart
@@ -250,10 +250,11 @@ class _DefaultPub implements Pub {
         'The time now is: $now'
       );
     }
-    // Insert references to synthetic flutter package.
-    if (generateSyntheticPackage) {
-      await _updatePackageConfig(packageConfigFile, generatedDirectory);
-    }
+    await _updatePackageConfig(
+      packageConfigFile,
+      generatedDirectory,
+      generateSyntheticPackage,
+    );
   }
 
   @override
@@ -458,13 +459,35 @@ class _DefaultPub implements Pub {
     return environment;
   }
 
-  /// Insert the flutter_gen synthetic package into the package configuration file if
-  /// there is an l10n.yaml.
-  Future<void> _updatePackageConfig(File packageConfigFile, Directory generatedDirectory) async {
-    if (!packageConfigFile.existsSync()) {
+  /// Update the package configuration file.
+  ///
+  /// Creates a corresponding `package_config_subset` file that is used by the build
+  /// system to avoid rebuilds caused by an updated pub timestamp.
+  ///
+  /// if [generateSyntheticPackage] is true then insert flutter_gen synthetic
+  /// package into the package configuration. This is used by the l10n localization
+  /// tooling to insert a new reference into the package_config file, allowing the import
+  /// of a package URI that is not specified in the pubspec.yaml
+  ///
+  /// For more information, see:
+  ///   * [generateLocalizations], `in lib/src/localizations/gen_l10n.dart`
+  Future<void> _updatePackageConfig(
+    File packageConfigFile,
+    Directory generatedDirectory,
+    bool generateSyntheticPackage,
+  ) async {
+    final PackageConfig packageConfig = await loadPackageConfigWithLogging(packageConfigFile, logger: _logger);
+
+    packageConfigFile.parent
+      .childFile('package_config_subset')
+      .writeAsStringSync(_computePackageConfigSubset(
+        packageConfig,
+        _fileSystem,
+      ));
+
+    if (!generateSyntheticPackage) {
       return;
     }
-    final PackageConfig packageConfig = await loadPackageConfigWithLogging(packageConfigFile, logger: _logger);
     final Package flutterGen = Package('flutter_gen', generatedDirectory.uri, languageVersion: LanguageVersion(2, 8));
     if (packageConfig.packages.any((Package package) => package.name == 'flutter_gen')) {
       return;
@@ -480,4 +503,18 @@ class _DefaultPub implements Pub {
       await savePackageConfig(newPackageConfig, packageConfigFile.parent.parent);
     }
   }
+
+  // Subset the package config file to only the parts that are relevant for
+  // rerunning the dart compiler.
+  String _computePackageConfigSubset(PackageConfig packageConfig, FileSystem fileSystem) {
+    final StringBuffer buffer = StringBuffer();
+    for (final Package package in packageConfig.packages) {
+      buffer.writeln(package.name);
+      buffer.writeln(package.languageVersion);
+      buffer.writeln(package.root);
+      buffer.writeln(package.packageUriRoot);
+    }
+    buffer.writeln(packageConfig.version);
+    return buffer.toString();
+  }
 }
diff --git a/packages/flutter_tools/test/commands.shard/hermetic/analyze_continuously_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/analyze_continuously_test.dart
@@ -117,7 +117,6 @@ void main() {
       platform: const LocalPlatform(),
       usage: globals.flutterUsage,
       botDetector: globals.botDetector,
-      toolStampFile: () => globals.fs.file('test'),
     );
     await pub.get(
       context: PubContext.flutterTests,

diff --git a/...shard/build_system/targets/dart_test.dart → ...ard/build_system/targets/common_test.dart b/...shard/build_system/targets/dart_test.dart → ...ard/build_system/targets/common_test.dart
@@ -72,7 +72,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot handles null result from kernel compilation', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final String build = androidEnvironment.buildDir.path;
     processManager.addCommands(<FakeCommand>[
       FakeCommand(command: <String>[
@@ -91,7 +93,7 @@ void main() {
         '--aot',
         '--tfa',
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',
@@ -106,7 +108,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot does not use track widget creation on profile builds', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final String build = androidEnvironment.buildDir.path;
     processManager.addCommands(<FakeCommand>[
       FakeCommand(command: <String>[
@@ -125,7 +129,7 @@ void main() {
         '--aot',
         '--tfa',
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',
@@ -140,7 +144,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot correctly handles an empty string in ExtraFrontEndOptions', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final String build = androidEnvironment.buildDir.path;
     processManager.addCommands(<FakeCommand>[
       FakeCommand(command: <String>[
@@ -159,7 +165,7 @@ void main() {
         '--aot',
         '--tfa',
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',
@@ -175,7 +181,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot correctly forwards ExtraFrontEndOptions', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final String build = androidEnvironment.buildDir.path;
     processManager.addCommands(<FakeCommand>[
       FakeCommand(command: <String>[
@@ -194,7 +202,7 @@ void main() {
         '--aot',
         '--tfa',
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',
@@ -212,7 +220,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot can disable track-widget-creation on debug builds', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final String build = androidEnvironment.buildDir.path;
     processManager.addCommands(<FakeCommand>[
       FakeCommand(command: <String>[
@@ -230,7 +240,7 @@ void main() {
         ...buildModeOptions(BuildMode.debug),
         '--no-link-platform',
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',
@@ -247,7 +257,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot forces platform linking on debug for darwin target platforms', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final String build = androidEnvironment.buildDir.path;
     processManager.addCommands(<FakeCommand>[
       FakeCommand(command: <String>[
@@ -264,7 +276,7 @@ void main() {
         '-Ddart.developer.causal_async_stacks=true',
         ...buildModeOptions(BuildMode.debug),
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',
@@ -283,7 +295,9 @@ void main() {
   });
 
   testWithoutContext('KernelSnapshot does use track widget creation on debug builds', () async {
-    fileSystem.file('.packages').writeAsStringSync('\n');
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('{"configVersion": 2, "packages":[]}');
     final Environment testEnvironment = Environment.test(
       fileSystem.currentDirectory,
       defines: <String, String>{
@@ -313,7 +327,7 @@ void main() {
         '--track-widget-creation',
         '--no-link-platform',
         '--packages',
-        '/.packages',
+        '/.dart_tool/package_config.json',
         '--output-dill',
         '$build/app.dill',
         '--depfile',

diff --git a/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart b/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart
@@ -223,6 +223,52 @@ void main() {
     verify(usage.sendEvent('pub-result', 'flutter-tests', label: 'success')).called(1);
   });
 
+  testWithoutContext('package_config_subset file is generated from packages and not timestamp', () async {
+    final FileSystem fileSystem = MemoryFileSystem.test();
+    final MockUsage usage = MockUsage();
+    final Pub pub = Pub(
+      fileSystem: fileSystem,
+      logger: BufferLogger.test(),
+      processManager: MockProcessManager(0),
+      botDetector: const BotDetectorAlwaysNo(),
+      usage: usage,
+      platform: FakePlatform(
+        environment: const <String, String>{
+          'PUB_CACHE': 'custom/pub-cache/path',
+        }
+      ),
+    );
+    fileSystem.file('pubspec.yaml').createSync();
+    fileSystem.file('.dart_tool/package_config.json')
+      ..createSync(recursive: true)
+      ..writeAsStringSync('''
+      {"configVersion": 2,"packages": [
+        {
+          "name": "flutter_tools",
+          "rootUri": "../",
+          "packageUri": "lib/",
+          "languageVersion": "2.7"
+        }
+      ],"generated":"some-time"}
+''');
+
+    await pub.get(
+      context: PubContext.flutterTests,
+      generateSyntheticPackage: true,
+      checkLastModified: false,
+    );
+
+    expect(
+      fileSystem.file('.dart_tool/package_config_subset').readAsStringSync(),
+      'flutter_tools\n'
+      '2.7\n'
+      'file:///\n'
+      'file:///lib/\n'
+      '2\n',
+    );
+  });
+
+
   testWithoutContext('analytics sent on failure', () async {
     MockDirectory.findCache = true;
     final MockUsage usage = MockUsage();