From 65835af4e68271619cbef0156aaabdb4d4460183 Mon Sep 17 00:00:00 2001 From: Michael Goderbauer Date: Tue, 28 Feb 2017 17:21:17 -0800 Subject: [PATCH] Roll forward #8467 (#8477) * Revert "Revert "Simplify path handling logic in dependency checker and devFS (#8414)" (#8467)" This reverts commit 96ba7f76d26ed4e3eb81f30032303fbab9b28aef. * Intentionally use a self-package URI in flutter_gallery * tests to catch problems with self-package imports --- examples/flutter_gallery/lib/demo/all.dart | 4 +- .../lib/src/dart/dependencies.dart | 13 +--- .../lib/src/dependency_checker.dart | 15 +--- packages/flutter_tools/lib/src/devfs.dart | 75 +++++++------------ packages/flutter_tools/lib/src/run_hot.dart | 13 +--- .../test/dart_dependencies_test.dart | 4 +- packages/flutter_tools/test/devfs_test.dart | 39 ++++++---- packages/flutter_tools/test/src/mocks.dart | 4 + 8 files changed, 64 insertions(+), 103 deletions(-) diff --git a/examples/flutter_gallery/lib/demo/all.dart b/examples/flutter_gallery/lib/demo/all.dart index ed8618f7a96..552753a8717 100644 --- a/examples/flutter_gallery/lib/demo/all.dart +++ b/examples/flutter_gallery/lib/demo/all.dart @@ -33,6 +33,8 @@ export 'snack_bar_demo.dart'; export 'tabs_demo.dart'; export 'tabs_fab_demo.dart'; export 'text_field_demo.dart'; -export 'tooltip_demo.dart'; export 'two_level_list_demo.dart'; export 'typography_demo.dart'; + +// Intentionally use a self-package URL to make sure we don't break these. +export 'package:flutter_gallery/demo/tooltip_demo.dart'; diff --git a/packages/flutter_tools/lib/src/dart/dependencies.dart b/packages/flutter_tools/lib/src/dart/dependencies.dart index 6d55526598a..3c0f10d091e 100644 --- a/packages/flutter_tools/lib/src/dart/dependencies.dart +++ b/packages/flutter_tools/lib/src/dart/dependencies.dart @@ -5,7 +5,6 @@ import 'dart:convert'; import '../artifacts.dart'; -import '../base/file_system.dart'; import '../base/process.dart'; class DartDependencySetBuilder { @@ -30,16 +29,6 @@ class DartDependencySetBuilder { String output = runSyncAndThrowStdErrOnError(args); - final List lines = LineSplitter.split(output).toList(); - final Set minimalDependencies = new Set(); - for (String line in lines) { - if (!line.startsWith('package:')) { - // We convert the uris so that they are relative to the project - // root. - line = fs.path.relative(line, from: projectRootPath); - } - minimalDependencies.add(line); - } - return minimalDependencies; + return new Set.from(LineSplitter.split(output)); } } diff --git a/packages/flutter_tools/lib/src/dependency_checker.dart b/packages/flutter_tools/lib/src/dependency_checker.dart index fa9d73c1a71..d9efa5e3908 100644 --- a/packages/flutter_tools/lib/src/dependency_checker.dart +++ b/packages/flutter_tools/lib/src/dependency_checker.dart @@ -5,7 +5,6 @@ import 'asset.dart'; import 'base/file_system.dart'; import 'dart/dependencies.dart'; -import 'dart/package_map.dart'; import 'globals.dart'; class DependencyChecker { @@ -18,10 +17,7 @@ class DependencyChecker { /// if it cannot be determined. bool check(DateTime threshold) { _dependencies.clear(); - PackageMap packageMap; - // Parse the package map. try { - packageMap = new PackageMap(builder.packagesFilePath)..load(); _dependencies.add(builder.packagesFilePath); } catch (e, st) { printTrace('DependencyChecker: could not parse .packages file:\n$e\n$st'); @@ -29,16 +25,7 @@ class DependencyChecker { } // Build the set of Dart dependencies. try { - Set dependencies = builder.build(); - for (String path in dependencies) { - // Ensure all paths are absolute. - if (path.startsWith('package:')) { - path = packageMap.pathForPackage(Uri.parse(path)); - } else { - path = fs.path.join(builder.projectRootPath, path); - } - _dependencies.add(path); - } + _dependencies.addAll(builder.build()); } catch (e, st) { printTrace('DependencyChecker: error determining .dart dependencies:\n$e\n$st'); return true; diff --git a/packages/flutter_tools/lib/src/devfs.dart b/packages/flutter_tools/lib/src/devfs.dart index 6f18a7d2696..c82bd74660d 100644 --- a/packages/flutter_tools/lib/src/devfs.dart +++ b/packages/flutter_tools/lib/src/devfs.dart @@ -461,10 +461,10 @@ class DevFS { } bool _shouldIgnore(String devicePath) { - List ignoredPrefixes = ['android/', + List ignoredPrefixes = ['android' + fs.path.separator, getBuildDirectory(), - 'ios/', - '.pub/']; + 'ios' + fs.path.separator, + '.pub' + fs.path.separator]; for (String ignoredPrefix in ignoredPrefixes) { if (devicePath.startsWith(ignoredPrefix)) return true; @@ -473,16 +473,14 @@ class DevFS { } Future _scanDirectory(Directory directory, - {String directoryName, + {String directoryNameOnDevice, bool recursive: false, bool ignoreDotFiles: true, - String packagesDirectoryName, Set fileFilter}) async { - String prefix = directoryName; - if (prefix == null) { - prefix = fs.path.relative(directory.path, from: rootDirectory.path); - if (prefix == '.') - prefix = ''; + if (directoryNameOnDevice == null) { + directoryNameOnDevice = fs.path.relative(directory.path, from: rootDirectory.path); + if (directoryNameOnDevice == '.') + directoryNameOnDevice = ''; } try { Stream files = @@ -508,24 +506,8 @@ class DevFS { } final String relativePath = fs.path.relative(file.path, from: directory.path); - final String devicePath = fs.path.join(prefix, relativePath); - bool filtered = false; - if ((fileFilter != null) && - !fileFilter.contains(devicePath)) { - if (packagesDirectoryName != null) { - // Double check the filter for packages/packagename/ - final String packagesDevicePath = - fs.path.join(packagesDirectoryName, relativePath); - if (!fileFilter.contains(packagesDevicePath)) { - // File was not in the filter set. - filtered = true; - } - } else { - // File was not in the filter set. - filtered = true; - } - } - if (filtered) { + final String devicePath = fs.path.join(directoryNameOnDevice, relativePath); + if ((fileFilter != null) && !fileFilter.contains(file.absolute.path)) { // Skip files that are not included in the filter. continue; } @@ -548,27 +530,26 @@ class DevFS { PackageMap packageMap = new PackageMap(_packagesFilePath); for (String packageName in packageMap.map.keys) { - Uri uri = packageMap.map[packageName]; - // This project's own package. - final bool isProjectPackage = uri.toString() == 'lib/'; - final String directoryName = - isProjectPackage ? 'lib' : fs.path.join('packages', packageName); - // If this is the project's package, we need to pass both - // package: and lib/ as paths to be checked against - // the filter because we must support both package: imports and relative - // path imports within the project's own code. - final String packagesDirectoryName = - isProjectPackage ? fs.path.join('packages', packageName) : null; - Directory directory = fs.directory(uri); - bool packageExists = - await _scanDirectory(directory, - directoryName: directoryName, - recursive: true, - packagesDirectoryName: packagesDirectoryName, - fileFilter: fileFilter); + Uri packageUri = packageMap.map[packageName]; + String packagePath = fs.path.fromUri(packageUri); + Directory packageDirectory = fs.directory(packageUri); + String directoryNameOnDevice = fs.path.join('packages', packageName); + bool packageExists; + + if (fs.path.isWithin(rootDirectory.path, packagePath)) { + // We already scanned everything under the root directory. + packageExists = packageDirectory.existsSync(); + directoryNameOnDevice = fs.path.relative(packagePath, from: rootDirectory.path); + } else { + packageExists = + await _scanDirectory(packageDirectory, + directoryNameOnDevice: directoryNameOnDevice, + recursive: true, + fileFilter: fileFilter); + } if (packageExists) { sb ??= new StringBuffer(); - sb.writeln('$packageName:$directoryName'); + sb.writeln('$packageName:$directoryNameOnDevice'); } } if (sb != null) { diff --git a/packages/flutter_tools/lib/src/run_hot.dart b/packages/flutter_tools/lib/src/run_hot.dart index 93c6cb7d986..ed2e2630385 100644 --- a/packages/flutter_tools/lib/src/run_hot.dart +++ b/packages/flutter_tools/lib/src/run_hot.dart @@ -98,18 +98,7 @@ class HotRunner extends ResidentRunner { new DartDependencySetBuilder( mainPath, projectRootPath, packagesFilePath); try { - Set dependencies = dartDependencySetBuilder.build(); - _dartDependencies = new Set(); - for (String path in dependencies) { - // We need to tweak package: uris so that they reflect their devFS - // location. - if (path.startsWith('package:')) { - // Swap out package: for packages/ because we place all package - // sources under packages/. - path = path.replaceFirst('package:', 'packages/'); - } - _dartDependencies.add(path); - } + _dartDependencies = new Set.from(dartDependencySetBuilder.build()); } catch (error) { printStatus('Error detected in application source code:', emphasis: true); printError('$error'); diff --git a/packages/flutter_tools/test/dart_dependencies_test.dart b/packages/flutter_tools/test/dart_dependencies_test.dart index bdb47ba33ad..0bb6d1f333a 100644 --- a/packages/flutter_tools/test/dart_dependencies_test.dart +++ b/packages/flutter_tools/test/dart_dependencies_test.dart @@ -21,8 +21,8 @@ void main() { DartDependencySetBuilder builder = new DartDependencySetBuilder(mainPath, testPath, packagesPath); Set dependencies = builder.build(); - expect(dependencies.contains('main.dart'), isTrue); - expect(dependencies.contains('foo.dart'), isTrue); + expect(dependencies.contains(mainPath), isTrue); + expect(dependencies.contains(fs.path.join(testPath, 'foo.dart')), isTrue); }); testUsingContext('syntax_error', () { final String testPath = fs.path.join(dataPath, 'syntax_error'); diff --git a/packages/flutter_tools/test/devfs_test.dart b/packages/flutter_tools/test/devfs_test.dart index 73d5bd78646..33ceea04bbe 100644 --- a/packages/flutter_tools/test/devfs_test.dart +++ b/packages/flutter_tools/test/devfs_test.dart @@ -4,6 +4,7 @@ import 'dart:async'; import 'dart:convert'; +import 'dart:io' as io; import 'package:flutter_tools/src/asset.dart'; import 'package:flutter_tools/src/base/io.dart'; @@ -18,7 +19,7 @@ import 'src/context.dart'; import 'src/mocks.dart'; void main() { - final String filePath = fs.path.join('bar', 'foo.txt'); + final String filePath = fs.path.join('lib', 'foo.txt'); final String filePath2 = fs.path.join('foo', 'bar.txt'); Directory tempDir; String basePath; @@ -69,6 +70,7 @@ void main() { File file = fs.file(fs.path.join(basePath, filePath)); await file.parent.create(recursive: true); file.writeAsBytesSync([1, 2, 3]); + _packages['my_project'] = 'lib'; // simulate package await _createPackage('somepkg', 'somefile.txt'); @@ -80,12 +82,20 @@ void main() { int bytes = await devFS.update(); devFSOperations.expectMessages([ - 'writeFile test .packages', - 'writeFile test ${fs.path.join('bar', 'foo.txt')}', + 'writeFile test ${fs.path.join('lib', 'foo.txt')}', 'writeFile test ${fs.path.join('packages', 'somepkg', 'somefile.txt')}', + 'writeFile test .packages', ]); expect(devFS.assetPathsToEvict, isEmpty); - expect(bytes, 31); + + List packageSpecOnDevice = LineSplitter.split(UTF8.decode( + await devFSOperations.devicePathToContent['.packages'].contentsAsBytes() + )).toList(); + expect(packageSpecOnDevice, + unorderedEquals(['my_project:lib', 'somepkg:packages/somepkg']) + ); + + expect(bytes, 46); }); testUsingContext('add new file to local file system', () async { File file = fs.file(fs.path.join(basePath, filePath2)); @@ -110,7 +120,7 @@ void main() { await file.writeAsBytes([1, 2, 3, 4, 5, 6]); bytes = await devFS.update(); devFSOperations.expectMessages([ - 'writeFile test ${fs.path.join('bar', 'foo.txt')}', + 'writeFile test ${fs.path.join('lib', 'foo.txt')}', ]); expect(devFS.assetPathsToEvict, isEmpty); expect(bytes, 6); @@ -120,7 +130,7 @@ void main() { await file.delete(); int bytes = await devFS.update(); devFSOperations.expectMessages([ - 'deleteFile test ${fs.path.join('bar', 'foo.txt')}', + 'deleteFile test ${fs.path.join('lib', 'foo.txt')}', ]); expect(devFS.assetPathsToEvict, isEmpty); expect(bytes, 0); @@ -133,7 +143,7 @@ void main() { 'writeFile test ${fs.path.join('packages', 'newpkg', 'anotherfile.txt')}', ]); expect(devFS.assetPathsToEvict, isEmpty); - expect(bytes, 51); + expect(bytes, 66); }); testUsingContext('add an asset bundle', () async { assetBundle.entries['a.txt'] = new DevFSStringContent('abc'); @@ -197,7 +207,7 @@ void main() { devFSOperations.expectMessages(['destroy test']); expect(devFS.assetPathsToEvict, isEmpty); }); - }); + }, skip: io.Platform.isWindows); group('devfs remote', () { MockVMService vmService; @@ -230,11 +240,11 @@ void main() { int bytes = await devFS.update(); vmService.expectMessages([ 'writeFile test .packages', - 'writeFile test ${fs.path.join('bar', 'foo.txt')}', + 'writeFile test ${fs.path.join('lib', 'foo.txt')}', 'writeFile test ${fs.path.join('packages', 'somepkg', 'somefile.txt')}', ]); expect(devFS.assetPathsToEvict, isEmpty); - expect(bytes, 31); + expect(bytes, 46); }, timeout: const Timeout(const Duration(seconds: 5))); testUsingContext('delete dev file system', () async { @@ -242,7 +252,7 @@ void main() { vmService.expectMessages(['_deleteDevFS {fsName: test}']); expect(devFS.assetPathsToEvict, isEmpty); }); - }); + }, skip: io.Platform.isWindows); } class MockVMService extends BasicMock implements VMService { @@ -311,7 +321,7 @@ class MockVM implements VM { final List _tempDirs = []; -final Map _packages = {}; +final Map _packages = {}; Directory _newTempDir() { Directory tempDir = fs.systemTempDirectory.createTempSync('devfs${_tempDirs.length}'); @@ -330,10 +340,9 @@ Future _createPackage(String pkgName, String pkgFileName) async { File pkgFile = fs.file(fs.path.join(pkgTempDir.path, pkgName, 'lib', pkgFileName)); await pkgFile.parent.create(recursive: true); pkgFile.writeAsBytesSync([11, 12, 13]); - _packages[pkgName] = pkgTempDir; + _packages[pkgName] = pkgFile.parent.path; StringBuffer sb = new StringBuffer(); - _packages.forEach((String pkgName, Directory pkgTempDir) { - Uri pkgPath = fs.path.toUri(fs.path.join(pkgTempDir.path, pkgName, 'lib')); + _packages.forEach((String pkgName, String pkgPath) { sb.writeln('$pkgName:$pkgPath'); }); fs.file(fs.path.join(_tempDirs[0].path, '.packages')).writeAsStringSync(sb.toString()); diff --git a/packages/flutter_tools/test/src/mocks.dart b/packages/flutter_tools/test/src/mocks.dart index 13c817d3dae..e8c61c05547 100644 --- a/packages/flutter_tools/test/src/mocks.dart +++ b/packages/flutter_tools/test/src/mocks.dart @@ -95,6 +95,8 @@ class BasicMock { } class MockDevFSOperations extends BasicMock implements DevFSOperations { + Map devicePathToContent = {}; + @override Future create(String fsName) async { messages.add('create $fsName'); @@ -109,10 +111,12 @@ class MockDevFSOperations extends BasicMock implements DevFSOperations { @override Future writeFile(String fsName, String devicePath, DevFSContent content) async { messages.add('writeFile $fsName $devicePath'); + devicePathToContent[devicePath] = content; } @override Future deleteFile(String fsName, String devicePath) async { messages.add('deleteFile $fsName $devicePath'); + devicePathToContent.remove(devicePath); } }