From 8950d3ccc467ca59ebe7a2b828ed3683fac3880a Mon Sep 17 00:00:00 2001 From: Hixie Date: Wed, 9 Mar 2016 16:33:12 -0800 Subject: [PATCH] Make flutter analyze useful for package conflicts Also, resolve our package conflict, since reflectable has been fixed. --- packages/flutter/pubspec.yaml | 8 - .../lib/src/commands/analyze.dart | 167 +++++++++++++----- packages/flutter_tools/lib/src/dart/pub.dart | 2 +- 3 files changed, 119 insertions(+), 58 deletions(-) diff --git a/packages/flutter/pubspec.yaml b/packages/flutter/pubspec.yaml index f75dc72161..fdd9eafaed 100644 --- a/packages/flutter/pubspec.yaml +++ b/packages/flutter/pubspec.yaml @@ -10,14 +10,6 @@ dependencies: vector_math: '>=1.4.5 <2.0.0' quiver: '>=0.21.4 <0.22.0' - # We have to pin analyzer to 0.27.1 because the flx package depends - # on pointycastle which depends on reflectable which depends on - # analyzer 0.27.1 and if we don't pin it here, then different - # packages end up bringing in different analyzer versions which - # results in 'flutter analyze' (which uses an entirely different - # analyzer, by the way!) complaining about the inconsistency. - analyzer: 0.27.1 - sky_engine: path: ../../bin/cache/pkg/sky_engine sky_services: diff --git a/packages/flutter_tools/lib/src/commands/analyze.dart b/packages/flutter_tools/lib/src/commands/analyze.dart index 7b5da71ff5..220c6399fa 100644 --- a/packages/flutter_tools/lib/src/commands/analyze.dart +++ b/packages/flutter_tools/lib/src/commands/analyze.dart @@ -27,7 +27,7 @@ bool isDartFile(FileSystemEntity entry) => entry is File && entry.path.endsWith( bool isDartTestFile(FileSystemEntity entry) => entry is File && entry.path.endsWith('_test.dart'); bool isDartBenchmarkFile(FileSystemEntity entry) => entry is File && entry.path.endsWith('_bench.dart'); -bool _addPackage(String directoryPath, List dartFiles, Set pubSpecDirectories) { +void _addPackage(String directoryPath, List dartFiles, Set pubSpecDirectories) { final int originalDartFilesCount = dartFiles.length; // .../directoryPath/*/bin/*.dart @@ -90,11 +90,8 @@ bool _addPackage(String directoryPath, List dartFiles, Set pubSp } } - if (originalDartFilesCount != dartFiles.length) { + if (originalDartFilesCount != dartFiles.length) pubSpecDirectories.add(directoryPath); - return true; - } - return false; } /// Adds all packages in [subPath], assuming a flat directory structure, i.e. @@ -142,9 +139,6 @@ class AnalyzeCommand extends FlutterCommand { Set pubSpecDirectories = new HashSet(); List dartFiles = argResults.rest.toList(); - bool foundAnyInCurrentDirectory = false; - bool foundAnyInFlutterRepo = false; - for (String file in dartFiles) { file = path.normalize(path.absolute(file)); String root = path.rootPrefix(file); @@ -152,11 +146,6 @@ class AnalyzeCommand extends FlutterCommand { file = path.dirname(file); if (FileSystemEntity.isFileSync(path.join(file, 'pubspec.yaml'))) { pubSpecDirectories.add(file); - if (file == path.normalize(path.absolute(ArtifactStore.flutterRoot))) { - foundAnyInFlutterRepo = true; - } else if (file == path.normalize(path.absolute(path.current))) { - foundAnyInCurrentDirectory = true; - } break; } } @@ -172,16 +161,12 @@ class AnalyzeCommand extends FlutterCommand { foundOne = true; } } - if (foundOne) { + if (foundOne) pubSpecDirectories.add('.'); - foundAnyInCurrentDirectory = true; - } } - if (argResults['current-package']) { - if (_addPackage('.', dartFiles, pubSpecDirectories)) - foundAnyInCurrentDirectory = true; - } + if (argResults['current-package']) + _addPackage('.', dartFiles, pubSpecDirectories); if (argResults['flutter-repo']) { //examples/*/ as package @@ -237,25 +222,24 @@ class AnalyzeCommand extends FlutterCommand { mainBody.writeln('import \'${dartFiles[index]}\' as file$index;'); mainBody.writeln('void main() { }'); - // prepare a union of all the .packages files - Map packages = {}; - bool hadInconsistentRequirements = false; + // determine what all the various .packages files depend on + PackageDependencyTracker dependencies = new PackageDependencyTracker(); for (Directory directory in pubSpecDirectories.map((path) => new Directory(path))) { String pubSpecYamlPath = path.join(directory.path, 'pubspec.yaml'); File pubSpecYamlFile = new File(pubSpecYamlPath); if (pubSpecYamlFile.existsSync()) { + // we are analyzing the actual canonical source for this package; + // make sure we remember that, in case all the packages are actually + // pointing elsewhere somehow. Pubspec pubSpecYaml = await Pubspec.load(pubSpecYamlPath); String packageName = pubSpecYaml.name; String packagePath = path.normalize(path.absolute(path.join(directory.path, 'lib'))); - if (packages.containsKey(packageName) && packages[packageName] != packagePath) { - printError('Inconsistent requirements for $packageName; using $packagePath (and not ${packages[packageName]}).'); - hadInconsistentRequirements = true; - } - packages[packageName] = packagePath; + dependencies.addCanonicalCase(packageName, packagePath, pubSpecYamlPath); } - File dotPackages = new File(path.join(directory.path, '.packages')); + String dotPackagesPath = path.join(directory.path, '.packages'); + File dotPackages = new File(dotPackagesPath); if (dotPackages.existsSync()) { - Map dependencies = {}; + // this directory has opinions about what we should be using dotPackages .readAsStringSync() .split('\n') @@ -263,27 +247,23 @@ class AnalyzeCommand extends FlutterCommand { .forEach((line) { int colon = line.indexOf(':'); if (colon > 0) - dependencies[line.substring(0, colon)] = path.normalize(path.absolute(directory.path, path.fromUri(line.substring(colon+1)))); + dependencies.add(line.substring(0, colon), path.normalize(path.absolute(directory.path, path.fromUri(line.substring(colon+1)))), dotPackagesPath); }); - for (String package in dependencies.keys) { - if (packages.containsKey(package)) { - if (packages[package] != dependencies[package]) { - printError('Inconsistent requirements for $package; using ${packages[package]} (and not ${dependencies[package]}).'); - hadInconsistentRequirements = true; - } - } else { - packages[package] = dependencies[package]; - } - } } } - if (hadInconsistentRequirements) { - if (foundAnyInFlutterRepo) - printError('You may need to run "flutter update-packages --upgrade".'); - if (foundAnyInCurrentDirectory) - printError('You may need to run "pub upgrade".'); - } + // prepare a union of all the .packages files + if (dependencies.hasConflicts) { + printError(dependencies.generateConflictReport()); + printError('Make sure you have run "pub upgrade" in all the directories mentioned above.'); + if (dependencies.hasConflictsAffectingFlutterRepo) + printError('For packages in the flutter repository, try using "flutter update-packages --upgrade" to do all of them at once.'); + printError('If this does not help, to track down the conflict you can use "pub deps --style=list" and "pub upgrade --verbosity=solver" in the affected directories.'); + return 1; + } + Map packages = dependencies.asPackageMap(); + + // override the sky_engine and sky_services packages if the user is using a local build String buildDir = buildConfigurations.firstWhere((BuildConfiguration config) => config.testable, orElse: () => null)?.buildDir; if (buildDir != null) { packages['sky_engine'] = path.join(buildDir, 'gen/dart-pkg/sky_engine/lib'); @@ -468,11 +448,11 @@ linter: host.deleteSync(recursive: true); - if (exitCode < 0 || exitCode > 3) // 0 = nothing, 1 = hints, 2 = warnings, 3 = errors + if (exitCode < 0 || exitCode > 3) // analyzer exit codes: 0 = nothing, 1 = hints, 2 = warnings, 3 = errors return exitCode; if (errorCount > 0) - return 1; // Doesn't this mean 'hints' per the above comment? + return 1; // we consider any level of error to be an error exit (we don't report different levels) if (argResults['congratulate']) printStatus('No analyzer warnings! (ran in ${elapsed}s)'); return 0; @@ -589,6 +569,95 @@ linter: } } +class PackageDependency { + // This is a map from dependency targets (lib directories) to a list + // of places that ask for that target (.packages or pubspec.yaml files) + Map> values = >{}; + String canonicalSource; + void addCanonicalCase(String packagePath, String pubSpecYamlPath) { + assert(canonicalSource == null); + add(packagePath, pubSpecYamlPath); + canonicalSource = pubSpecYamlPath; + } + void add(String packagePath, String sourcePath) { + values.putIfAbsent(packagePath, () => []).add(sourcePath); + } + bool get hasConflict => values.length > 1; + bool get hasConflictAffectingFlutterRepo { + assert(path.isAbsolute(ArtifactStore.flutterRoot)); + for (List targetSources in values.values) { + for (String source in targetSources) { + assert(path.isAbsolute(source)); + if (path.isWithin(ArtifactStore.flutterRoot, source)) + return true; + } + } + return false; + } + void describeConflict(StringBuffer result) { + assert(hasConflict); + List targets = values.keys.toList(); + targets.sort((String a, String b) => values[b].length.compareTo(values[a].length)); + for (String target in targets) { + int count = values[target].length; + result.writeln(' $count ${count == 1 ? 'source wants' : 'sources want'} "$target":'); + bool canonical = false; + for (String source in values[target]) { + result.writeln(' $source'); + if (source == canonicalSource) + canonical = true; + } + if (canonical) { + result.writeln(' (This is the actual package definition, so it is considered the canonical "right answer".)'); + } + } + } + String get target => values.keys.single; +} + +class PackageDependencyTracker { + // This is a map from package names to objects that track the paths + // involved (sources and targets). + Map packages = {}; + + PackageDependency getPackageDependency(String packageName) { + return packages.putIfAbsent(packageName, () => new PackageDependency()); + } + + void addCanonicalCase(String packageName, String packagePath, String pubSpecYamlPath) { + getPackageDependency(packageName).addCanonicalCase(packagePath, pubSpecYamlPath); + } + + void add(String packageName, String packagePath, String dotPackagesPath) { + getPackageDependency(packageName).add(packagePath, dotPackagesPath); + } + + bool get hasConflicts { + return packages.values.any((PackageDependency dependency) => dependency.hasConflict); + } + + bool get hasConflictsAffectingFlutterRepo { + return packages.values.any((PackageDependency dependency) => dependency.hasConflictAffectingFlutterRepo); + } + + String generateConflictReport() { + assert(hasConflicts); + StringBuffer result = new StringBuffer(); + for (String package in packages.keys.where((String package) => packages[package].hasConflict)) { + result.writeln('Package "$package" has conflicts:'); + packages[package].describeConflict(result); + } + return result.toString(); + } + + Map asPackageMap() { + Map result = {}; + for (String package in packages.keys) + result[package] = packages[package].target; + return result; + } +} + class AnalysisServer { AnalysisServer(this.sdk, this.directories); diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart index 4e49cc49d7..e3b1e54921 100644 --- a/packages/flutter_tools/lib/src/dart/pub.dart +++ b/packages/flutter_tools/lib/src/dart/pub.dart @@ -31,8 +31,8 @@ Future pubGet({ } if (!checkLastModified || !pubSpecLock.existsSync() || pubSpecYaml.lastModifiedSync().isAfter(pubSpecLock.lastModifiedSync())) { - printStatus("Running 'pub get' in $directory${Platform.pathSeparator}..."); String command = upgrade ? 'upgrade' : 'get'; + printStatus("Running 'pub $command' in $directory${Platform.pathSeparator}..."); int code = await runCommandAndStreamOutput( [sdkBinaryName('pub'), '--verbosity=warning', command], workingDirectory: directory