From 76cbc462d21cf3e0399bccbac043b003c7efc888 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 7 Oct 2020 13:11:07 -0700 Subject: [PATCH] [flutter_tools] remove all pub caching logic (#66776) There have been some more additional reports of a missing 'package:characters' import after upgrading flutter, as well as problems with detecting the correct language version. 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. --- packages/flutter_tools/lib/src/cache.dart | 2 - .../lib/src/commands/packages.dart | 1 - .../flutter_tools/lib/src/commands/test.dart | 29 ----- .../lib/src/commands/update_packages.dart | 2 - .../lib/src/commands/upgrade.dart | 1 - .../lib/src/commands/version.dart | 1 - .../flutter_tools/lib/src/context_runner.dart | 2 - packages/flutter_tools/lib/src/dart/pub.dart | 123 +++++------------- .../lib/src/runner/flutter_command.dart | 6 +- .../test/general.shard/cache_test.dart | 3 - .../test/general.shard/dart/pub_get_test.dart | 50 ++----- .../downgrade_upgrade_integration_test.dart | 3 +- 12 files changed, 43 insertions(+), 180 deletions(-) diff --git a/packages/flutter_tools/lib/src/cache.dart b/packages/flutter_tools/lib/src/cache.dart index e997b3fdbfa..a54197f4f31 100644 --- a/packages/flutter_tools/lib/src/cache.dart +++ b/packages/flutter_tools/lib/src/cache.dart @@ -700,8 +700,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 index d31b337a15f..ef8c625b702 100644 --- 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 index 6724b450fdf..12a32b516e0 100644 --- 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 names = stringsArg('name'); final List 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 index e30cc56f466..16cf2524d61 100644 --- a/packages/flutter_tools/lib/src/commands/update_packages.dart +++ b/packages/flutter_tools/lib/src/commands/update_packages.dart @@ -330,7 +330,6 @@ class UpdatePackagesCommand extends FlutterCommand { context: PubContext.updatePackages, directory: tempDir.path, upgrade: true, - checkLastModified: false, offline: offline, flutterRootOverride: upgrade ? temporaryFlutterSdk.path @@ -413,7 +412,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 index 39177ebfece..986d776f20a 100644 --- 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/commands/version.dart b/packages/flutter_tools/lib/src/commands/version.dart index 72a33524d3c..717d09e6b75 100644 --- a/packages/flutter_tools/lib/src/commands/version.dart +++ b/packages/flutter_tools/lib/src/commands/version.dart @@ -151,7 +151,6 @@ class VersionCommand extends FlutterCommand { 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 index 50d3dbe5b47..f79a1b0bdfe 100644 --- a/packages/flutter_tools/lib/src/context_runner.dart +++ b/packages/flutter_tools/lib/src/context_runner.dart @@ -236,8 +236,6 @@ Future runInContext( 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 index 9830ff414d5..b18e90887ff 100644 --- 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 get({ @@ -168,88 +162,52 @@ 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)}...', - timeout: const TimeoutConfiguration().slowOperation, + final String command = upgrade ? 'upgrade' : 'get'; + final Status status = _logger.startProgress( + 'Running "flutter pub $command" in ${_fileSystem.path.basename(directory)}...', + timeout: const TimeoutConfiguration().slowOperation, + ); + final bool verbose = _logger.isVerbose; + final List args = [ + if (verbose) + '--verbose' + else + '--verbosity=warning', + ...[ + 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 args = [ - if (verbose) - '--verbose' - else - '--verbosity=warning', - ...[ - 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, @@ -392,23 +350,6 @@ class _DefaultPub implements Pub { return [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 index 7003262c1cc..9b803646728 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart @@ -950,9 +950,9 @@ abstract class FlutterCommand extends Command { // 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.universal}); - await globals.cache.updateAll(await requiredArtifacts); } + Cache.releaseLock(); await validateCommand(); @@ -979,11 +979,7 @@ abstract class FlutterCommand extends Command { context: PubContext.getVerifyContext(name), generateSyntheticPackage: project.manifest.generateSyntheticPackage, ); - // All done updating dependencies. Release the cache lock. - Cache.releaseLock(); await project.ensureReadyForPlatformSpecificTooling(checkProjects: true); - } else { - Cache.releaseLock(); } setupApplicationPackages(); diff --git a/packages/flutter_tools/test/general.shard/cache_test.dart b/packages/flutter_tools/test/general.shard/cache_test.dart index 0e2bca75c56..e12efc10773 100644 --- a/packages/flutter_tools/test/general.shard/cache_test.dart +++ b/packages/flutter_tools/test/general.shard/cache_test.dart @@ -743,9 +743,6 @@ void main() { verify(pub.get( context: PubContext.pubGet, directory: 'packages/flutter_tools', - generateSyntheticPackage: false, - skipPubspecYamlCheck: true, - checkLastModified: false, )).called(1); }); } 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 index 1b76f9d8e8e..6919771558c 100644 --- a/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart +++ b/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart @@ -49,7 +49,7 @@ void main() { FakeAsync().run((FakeAsync time) { expect(processMock.lastPubEnvironment, isNull); expect(logger.statusText, ''); - pub.get(context: PubContext.flutterTests, checkLastModified: false).then((void value) { + pub.get(context: PubContext.flutterTests).then((void value) { error = 'test completed unexpectedly'; }, onError: (dynamic thrownError) { error = 'test failed unexpectedly: $thrownError'; @@ -114,7 +114,7 @@ void main() { processManager: MockProcessManager(66, stderr: 'err1\nerr2\nerr3\n', stdout: 'out1\nout2\nout3\n'), ); try { - await pub.get(context: PubContext.flutterTests, checkLastModified: false); + await pub.get(context: PubContext.flutterTests); throw AssertionError('pubGet did not fail'); } on ToolExit catch (error) { expect(error.message, 'pub get failed (66; err3)'); @@ -149,7 +149,7 @@ void main() { MockDirectory.findCache = true; expect(processMock.lastPubEnvironment, isNull); expect(processMock.lastPubCache, isNull); - pub.get(context: PubContext.flutterTests, checkLastModified: false).then((void value) { + pub.get(context: PubContext.flutterTests).then((void value) { error = 'test completed unexpectedly'; }, onError: (dynamic thrownError) { error = 'test failed unexpectedly: $thrownError'; @@ -182,7 +182,7 @@ void main() { expect(processMock.lastPubCache, isNull); String error; - pub.get(context: PubContext.flutterTests, checkLastModified: false).then((void value) { + pub.get(context: PubContext.flutterTests).then((void value) { error = 'test completed unexpectedly'; }, onError: (dynamic thrownError) { error = 'test failed unexpectedly: $thrownError'; @@ -217,7 +217,6 @@ void main() { await pub.get( context: PubContext.flutterTests, generateSyntheticPackage: true, - checkLastModified: false, ); verify(usage.sendEvent('pub-result', 'flutter-tests', label: 'success')).called(1); @@ -255,7 +254,6 @@ void main() { await pub.get( context: PubContext.flutterTests, generateSyntheticPackage: true, - checkLastModified: false, ); expect( @@ -285,7 +283,7 @@ void main() { ), ); try { - await pub.get(context: PubContext.flutterTests, checkLastModified: false); + await pub.get(context: PubContext.flutterTests); } on ToolExit { // Ignore. } @@ -314,7 +312,7 @@ void main() { fileSystem.file('pubspec.yaml').writeAsStringSync('name: foo'); try { - await pub.get(context: PubContext.flutterTests, checkLastModified: false); + await pub.get(context: PubContext.flutterTests); } on ToolExit { // Ignore. } @@ -386,7 +384,7 @@ void main() { fileSystem.file('pubspec.yaml') ..createSync() ..setLastModifiedSync(DateTime(2001)); - await pub.get(context: PubContext.flutterTests, checkLastModified: true); // pub sets date of .packages to 2002 + await pub.get(context: PubContext.flutterTests); // pub sets date of .packages to 2002 expect(logger.statusText, 'Running "flutter pub get" in /...\n'); expect(logger.errorText, isEmpty); @@ -398,44 +396,12 @@ void main() { .setLastModifiedSync(DateTime(2000)); fileSystem.file('pubspec.yaml') .setLastModifiedSync(DateTime(2001)); - await pub.get(context: PubContext.flutterTests, checkLastModified: true); // pub does nothing + await pub.get(context: PubContext.flutterTests); // pub does nothing expect(logger.statusText, 'Running "flutter pub get" in /...\n'); expect(logger.errorText, isEmpty); expect(fileSystem.file('pubspec.yaml').lastModifiedSync(), DateTime(2001)); // because nothing should touch it logger.clear(); - - // bad scenario 2: pub changes pubspec.yaml instead - fileSystem.file('.dart_tool/package_config.json') - .setLastModifiedSync(DateTime(2000)); - fileSystem.file('pubspec.yaml') - .setLastModifiedSync(DateTime(2001)); - try { - await pub.get(context: PubContext.flutterTests, checkLastModified: true); - expect(true, isFalse, reason: 'pub.get did not throw'); - } on ToolExit catch (error) { - expect(error.message, '/: unexpected concurrent modification of pubspec.yaml while running pub.'); - } - expect(logger.statusText, 'Running "flutter pub get" in /...\n'); - expect(logger.errorText, isEmpty); - expect(fileSystem.file('pubspec.yaml').lastModifiedSync(), DateTime(2002)); // because fake pub above touched it - - // bad scenario 3: pubspec.yaml was created in the future - fileSystem.file('.dart_tool/package_config.json') - .setLastModifiedSync(DateTime(2000)); - fileSystem.file('pubspec.yaml') - .setLastModifiedSync(DateTime(9999)); - assert(DateTime(9999).isAfter(DateTime.now())); - - await pub.get(context: PubContext.flutterTests, checkLastModified: true); // pub does nothing - - expect(logger.statusText, contains('Running "flutter pub get" in /...\n')); - expect(logger.errorText, startsWith( - 'Warning: File "/pubspec.yaml" 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:' - )); - logger.clear(); }); } 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 index 49180fc66aa..44c5b2ca0f7 100644 --- a/packages/flutter_tools/test/integration.shard/downgrade_upgrade_integration_test.dart +++ b/packages/flutter_tools/test/integration.shard/downgrade_upgrade_integration_test.dart @@ -24,7 +24,8 @@ final ProcessUtils processUtils = ProcessUtils(processManager: processManager, l outputPreferences: OutputPreferences.test(wrapText: true), timeoutConfiguration: const TimeoutConfiguration(), )); -final String flutterBin = fileSystem.path.join(getFlutterRoot(), 'bin', 'flutter'); +final String flutterBin = fileSystem.path.join(getFlutterRoot(), 'bin', platform.isWindows ? 'flutter.bat' : 'flutter'); +final String dartBin = fileSystem.path.join(getFlutterRoot(), 'bin', platform.isWindows ? 'dart.bat' : 'dart'); /// A test for flutter upgrade & downgrade that checks out a parallel flutter repo. void main() {