[flutter_tools] Reland: simplify pub cache logic (flutter#67589)

Jonah Williams · web-flow · commit cb67513f29db · 2020-10-23T10:00:56.000-07:00
There have been some more additional reports of a missing 'package:characters' import after upgrading flutter. This has me concerned that our pub caching logic is incorrect. Instead of the tool attempting to guess when pub should be run, always delegate to pub. Also takes an opportunity to fix the kernel snapshot depending on the .packages or package_config. Due to the generated: date field this causes extra rebuilds. Instead when pub get is run, write out an additional file with just the package contents and version. Fixes flutter#66777 Fixes flutter#65723
diff --git a/packages/flutter_tools/lib/src/base/build.dart b/packages/flutter_tools/lib/src/base/build.dart
@@ -109,7 +109,6 @@ class AOTSnapshotter {
     @required TargetPlatform platform,
     @required BuildMode buildMode,
     @required String mainPath,
-    @required String packagesPath,
     @required String outputPath,
     DarwinArch darwinArch,
     List<String> extraGenSnapshotOptions = const <String>[],
diff --git a/packages/flutter_tools/lib/src/build_system/targets/android.dart b/packages/flutter_tools/lib/src/build_system/targets/android.dart
@@ -233,7 +233,6 @@ class AndroidAot extends AotElfBase {
       platform: targetPlatform,
       buildMode: buildMode,
       mainPath: environment.buildDir.childFile('app.dill').path,
-      packagesPath: environment.projectDir.childFile('.packages').path,
       outputPath: output.path,
       bitcode: false,
       extraGenSnapshotOptions: extraGenSnapshotOptions,
diff --git a/packages/flutter_tools/lib/src/build_system/targets/common.dart b/packages/flutter_tools/lib/src/build_system/targets/common.dart
@@ -323,7 +323,6 @@ abstract class AotElfBase extends Target {
       platform: targetPlatform,
       buildMode: buildMode,
       mainPath: environment.buildDir.childFile('app.dill').path,
-      packagesPath: environment.projectDir.childFile('.packages').path,
       outputPath: outputPath,
       bitcode: false,
       extraGenSnapshotOptions: extraGenSnapshotOptions,
diff --git a/packages/flutter_tools/lib/src/build_system/targets/ios.dart b/packages/flutter_tools/lib/src/build_system/targets/ios.dart
@@ -87,7 +87,6 @@ abstract class AotAssemblyBase extends Target {
         platform: targetPlatform,
         buildMode: buildMode,
         mainPath: environment.buildDir.childFile('app.dill').path,
-        packagesPath: environment.projectDir.childFile('.packages').path,
         outputPath: environment.fileSystem.path.join(buildOutputPath, getNameForDarwinArch(darwinArch)),
         darwinArch: darwinArch,
         bitcode: bitcode,
@@ -128,7 +127,6 @@ class AotAssemblyRelease extends AotAssemblyBase {
   List<Source> get inputs => const <Source>[
     Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/ios.dart'),
     Source.pattern('{BUILD_DIR}/app.dill'),
-    Source.pattern('{PROJECT_DIR}/.packages'),
     Source.artifact(Artifact.engineDartBinary),
     Source.artifact(Artifact.skyEnginePath),
     // TODO(jonahwilliams): cannot reference gen_snapshot with artifacts since
@@ -163,7 +161,6 @@ class AotAssemblyProfile extends AotAssemblyBase {
   List<Source> get inputs => const <Source>[
     Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/ios.dart'),
     Source.pattern('{BUILD_DIR}/app.dill'),
-    Source.pattern('{PROJECT_DIR}/.packages'),
     Source.artifact(Artifact.engineDartBinary),
     Source.artifact(Artifact.skyEnginePath),
     // TODO(jonahwilliams): cannot reference gen_snapshot with artifacts since
diff --git a/packages/flutter_tools/lib/src/build_system/targets/macos.dart b/packages/flutter_tools/lib/src/build_system/targets/macos.dart
@@ -227,7 +227,6 @@ class CompileMacOSFramework extends Target {
       outputPath: environment.buildDir.path,
       platform: TargetPlatform.darwin_x64,
       darwinArch: DarwinArch.x86_64,
-      packagesPath: environment.projectDir.childFile('.packages').path,
       splitDebugInfo: splitDebugInfo,
       dartObfuscation: dartObfuscation,
       extraGenSnapshotOptions: extraGenSnapshotOptions,
diff --git a/packages/flutter_tools/lib/src/build_system/targets/web.dart b/packages/flutter_tools/lib/src/build_system/targets/web.dart
@@ -183,7 +183,7 @@ class Dart2JSTarget extends Target {
     Source.artifact(Artifact.dart2jsSnapshot),
     Source.artifact(Artifact.engineDartBinary),
     Source.pattern('{BUILD_DIR}/main.dart'),
-    Source.pattern('{PROJECT_DIR}/.packages'),
+    Source.pattern('{PROJECT_DIR}/.dart_tool/package_config_subset'),
   ];
 
   @override
diff --git a/packages/flutter_tools/lib/src/cache.dart b/packages/flutter_tools/lib/src/cache.dart
@@ -703,8 +703,6 @@ class PubDependencies extends ArtifactSet {
       context: PubContext.pubGet,
       directory: _fileSystem.path.join(_flutterRoot(), 'packages', 'flutter_tools'),
       generateSyntheticPackage: false,
-      skipPubspecYamlCheck: true,
-      checkLastModified: false,
     );
   }
 }
diff --git a/packages/flutter_tools/lib/src/commands/packages.dart b/packages/flutter_tools/lib/src/commands/packages.dart
@@ -117,7 +117,6 @@ class PackagesGetCommand extends FlutterCommand {
         directory: directory,
         upgrade: upgrade ,
         offline: boolArg('offline'),
-        checkLastModified: false,
         generateSyntheticPackage: flutterProject.manifest.generateSyntheticPackage,
       );
       pubGetTimer.stop();
diff --git a/packages/flutter_tools/lib/src/commands/test.dart b/packages/flutter_tools/lib/src/commands/test.dart
@@ -8,11 +8,8 @@ import '../asset.dart';
 import '../base/common.dart';
 import '../base/file_system.dart';
 import '../build_info.dart';
-import '../build_system/build_system.dart';
 import '../bundle.dart';
 import '../cache.dart';
-import '../dart/generate_synthetic_packages.dart';
-import '../dart/pub.dart';
 import '../devfs.dart';
 import '../globals.dart' as globals;
 import '../project.dart';
@@ -169,32 +166,6 @@ class TestCommand extends FlutterCommand {
         'directory (or one of its subdirectories).');
     }
     final FlutterProject flutterProject = FlutterProject.current();
-    if (shouldRunPub) {
-      if (flutterProject.manifest.generateSyntheticPackage) {
-        final Environment environment = Environment(
-          artifacts: globals.artifacts,
-          logger: globals.logger,
-          cacheDir: globals.cache.getRoot(),
-          engineVersion: globals.flutterVersion.engineRevision,
-          fileSystem: globals.fs,
-          flutterRootDir: globals.fs.directory(Cache.flutterRoot),
-          outputDir: globals.fs.directory(getBuildDirectory()),
-          processManager: globals.processManager,
-          projectDir: flutterProject.directory,
-        );
-
-        await generateLocalizationsSyntheticPackage(
-          environment: environment,
-          buildSystem: globals.buildSystem,
-        );
-      }
-
-      await pub.get(
-        context: PubContext.getVerifyContext(name),
-        skipPubspecYamlCheck: true,
-        generateSyntheticPackage: flutterProject.manifest.generateSyntheticPackage,
-      );
-    }
     final bool buildTestAssets = boolArg('test-assets');
     final List<String> names = stringsArg('name');
     final List<String> plainNames = stringsArg('plain-name');
diff --git a/packages/flutter_tools/lib/src/commands/update_packages.dart b/packages/flutter_tools/lib/src/commands/update_packages.dart
@@ -329,7 +329,6 @@ class UpdatePackagesCommand extends FlutterCommand {
           context: PubContext.updatePackages,
           directory: tempDir.path,
           upgrade: true,
-          checkLastModified: false,
           offline: offline,
           flutterRootOverride: upgrade
             ? temporaryFlutterSdk.path
@@ -412,7 +411,6 @@ class UpdatePackagesCommand extends FlutterCommand {
       await pub.get(
         context: PubContext.updatePackages,
         directory: dir.path,
-        checkLastModified: false,
         offline: offline,
         generateSyntheticPackage: false,
       );
diff --git a/packages/flutter_tools/lib/src/commands/upgrade.dart b/packages/flutter_tools/lib/src/commands/upgrade.dart
@@ -296,7 +296,6 @@ class UpgradeCommandRunner {
         context: PubContext.pubUpgrade,
         directory: projectRoot,
         upgrade: true,
-        checkLastModified: false,
         generateSyntheticPackage: false,
       );
     }
diff --git a/packages/flutter_tools/lib/src/context_runner.dart b/packages/flutter_tools/lib/src/context_runner.dart
@@ -234,8 +234,6 @@ Future<T> runInContext<T>(
         botDetector: globals.botDetector,
         platform: globals.platform,
         usage: globals.flutterUsage,
-        // Avoid a circular dependency by making this access lazy.
-        toolStampFile: () => globals.cache.getStampFileFor('flutter_tools'),
       ),
       ShutdownHooks: () => ShutdownHooks(logger: globals.logger),
       Stdio: () => Stdio(),
diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart
@@ -80,7 +80,6 @@ abstract class Pub {
     @required Platform platform,
     @required BotDetector botDetector,
     @required Usage usage,
-    File Function() toolStampFile,
   }) = _DefaultPub;
 
   /// Runs `pub get`.
@@ -93,8 +92,6 @@ abstract class Pub {
     bool skipIfAbsent = false,
     bool upgrade = false,
     bool offline = false,
-    bool checkLastModified = true,
-    bool skipPubspecYamlCheck = false,
     bool generateSyntheticPackage = false,
     String flutterRootOverride,
   });
@@ -141,9 +138,7 @@ class _DefaultPub implements Pub {
     @required Platform platform,
     @required BotDetector botDetector,
     @required Usage usage,
-   File Function() toolStampFile,
-  }) : _toolStampFile = toolStampFile,
-       _fileSystem = fileSystem,
+  }) : _fileSystem = fileSystem,
        _logger = logger,
        _platform = platform,
        _botDetector = botDetector,
@@ -159,7 +154,6 @@ class _DefaultPub implements Pub {
   final Platform _platform;
   final BotDetector _botDetector;
   final Usage _usage;
-  final File Function() _toolStampFile;
 
   @override
   Future<void> get({
@@ -168,87 +162,51 @@ class _DefaultPub implements Pub {
     bool skipIfAbsent = false,
     bool upgrade = false,
     bool offline = false,
-    bool checkLastModified = true,
-    bool skipPubspecYamlCheck = false,
     bool generateSyntheticPackage = false,
     String flutterRootOverride,
   }) async {
     directory ??= _fileSystem.currentDirectory.path;
-
-    final File pubSpecYaml = _fileSystem.file(
-      _fileSystem.path.join(directory, 'pubspec.yaml'));
     final File packageConfigFile = _fileSystem.file(
       _fileSystem.path.join(directory, '.dart_tool', 'package_config.json'));
     final Directory generatedDirectory = _fileSystem.directory(
       _fileSystem.path.join(directory, '.dart_tool', 'flutter_gen'));
 
-    if (!skipPubspecYamlCheck && !pubSpecYaml.existsSync()) {
-      if (!skipIfAbsent) {
-        throwToolExit('$directory: no pubspec.yaml found');
-      }
-      return;
-    }
-
-    final DateTime originalPubspecYamlModificationTime = pubSpecYaml.lastModifiedSync();
-
-    if (!checkLastModified || _shouldRunPubGet(
-      pubSpecYaml: pubSpecYaml,
-      packageConfigFile: packageConfigFile,
-    )) {
-      final String command = upgrade ? 'upgrade' : 'get';
-      final Status status = _logger.startProgress(
-        'Running "flutter pub $command" in ${_fileSystem.path.basename(directory)}...',
+    final String command = upgrade ? 'upgrade' : 'get';
+    final Status status = _logger.startProgress(
+      'Running "flutter pub $command" in ${_fileSystem.path.basename(directory)}...',
+    );
+    final bool verbose = _logger.isVerbose;
+    final List<String> args = <String>[
+      if (verbose)
+        '--verbose'
+      else
+        '--verbosity=warning',
+      ...<String>[
+        command,
+        '--no-precompile',
+      ],
+      if (offline)
+        '--offline',
+    ];
+    try {
+      await batch(
+        args,
+        context: context,
+        directory: directory,
+        failureMessage: 'pub $command failed',
+        retry: true,
+        flutterRootOverride: flutterRootOverride,
       );
-      final bool verbose = _logger.isVerbose;
-      final List<String> args = <String>[
-        if (verbose)
-          '--verbose'
-        else
-          '--verbosity=warning',
-        ...<String>[
-          command,
-          '--no-precompile',
-        ],
-        if (offline)
-          '--offline',
-      ];
-      try {
-        await batch(
-          args,
-          context: context,
-          directory: directory,
-          failureMessage: 'pub $command failed',
-          retry: true,
-          flutterRootOverride: flutterRootOverride,
-        );
-        status.stop();
-      // The exception is rethrown, so don't catch only Exceptions.
-      } catch (exception) { // ignore: avoid_catches_without_on_clauses
-        status.cancel();
-        rethrow;
-      }
+      status.stop();
+    // The exception is rethrown, so don't catch only Exceptions.
+    } catch (exception) { // ignore: avoid_catches_without_on_clauses
+      status.cancel();
+      rethrow;
     }
 
     if (!packageConfigFile.existsSync()) {
       throwToolExit('$directory: pub did not create .dart_tools/package_config.json file.');
     }
-    if (pubSpecYaml.lastModifiedSync() != originalPubspecYamlModificationTime) {
-      throwToolExit(
-        '$directory: unexpected concurrent modification of '
-        'pubspec.yaml while running pub.');
-    }
-    // We don't check if dotPackages was actually modified, because as far as we can tell sometimes
-    // pub will decide it does not need to actually modify it.
-    final DateTime now = DateTime.now();
-    if (now.isBefore(originalPubspecYamlModificationTime)) {
-      _logger.printError(
-        'Warning: File "${_fileSystem.path.absolute(pubSpecYaml.path)}" was created in the future. '
-        'Optimizations that rely on comparing time stamps will be unreliable. Check your '
-        'system clock for accuracy.\n'
-        'The timestamp was: $originalPubspecYamlModificationTime\n'
-        'The time now is: $now'
-      );
-    }
     await _updatePackageConfig(
       packageConfigFile,
       generatedDirectory,
@@ -391,23 +349,6 @@ class _DefaultPub implements Pub {
     return <String>[sdkPath, ...arguments];
   }
 
-  bool _shouldRunPubGet({ @required File pubSpecYaml, @required File packageConfigFile }) {
-    if (!packageConfigFile.existsSync()) {
-      return true;
-    }
-    final DateTime dotPackagesLastModified = packageConfigFile.lastModifiedSync();
-    if (pubSpecYaml.lastModifiedSync().isAfter(dotPackagesLastModified)) {
-      return true;
-    }
-    final File toolStampFile = _toolStampFile != null ? _toolStampFile() : null;
-    if (toolStampFile != null &&
-        toolStampFile.existsSync() &&
-        toolStampFile.lastModifiedSync().isAfter(dotPackagesLastModified)) {
-      return true;
-    }
-    return false;
-  }
-
   // Returns the environment value that should be used when running pub.
   //
   // Includes any existing environment variable, if one exists.
diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart
@@ -982,9 +982,9 @@ abstract class FlutterCommand extends Command<void> {
       // First always update universal artifacts, as some of these (e.g.
       // ios-deploy on macOS) are required to determine `requiredArtifacts`.
       await globals.cache.updateAll(<DevelopmentArtifact>{DevelopmentArtifact.universal});
-
       await globals.cache.updateAll(await requiredArtifacts);
     }
+    Cache.releaseLock();
 
     await validateCommand();
 
@@ -1011,11 +1011,7 @@ abstract class FlutterCommand extends Command<void> {
         context: PubContext.getVerifyContext(name),
         generateSyntheticPackage: project.manifest.generateSyntheticPackage,
       );
-      // All done updating dependencies. Release the cache lock.
-      Cache.releaseLock();
       await project.regeneratePlatformSpecificTooling();
-    } else {
-      Cache.releaseLock();
     }
 
     setupApplicationPackages();
diff --git a/packages/flutter_tools/test/general.shard/base/build_test.dart b/packages/flutter_tools/test/general.shard/base/build_test.dart
diff --git a/packages/flutter_tools/test/general.shard/cache_test.dart b/packages/flutter_tools/test/general.shard/cache_test.dart
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
diff --git a/packages/flutter_tools/test/integration.shard/downgrade_upgrade_integration_test.dart b/packages/flutter_tools/test/integration.shard/downgrade_upgrade_integration_test.dart

Original file line number	Diff line number	Diff line change
`@@ -703,8 +703,6 @@ class PubDependencies extends ArtifactSet {`
`703`	`703`	`context: PubContext.pubGet,`
`704`	`704`	`directory: _fileSystem.path.join(_flutterRoot(), 'packages', 'flutter_tools'),`
`705`	`705`	`generateSyntheticPackage: false,`
`706`		`- skipPubspecYamlCheck: true,`
`707`		`- checkLastModified: false,`
`708`	`706`	`);`
`709`	`707`	`}`
`710`	`708`	`}`