From a15a81be218f8b2fa2f63d7cd0f8cb0b3a6c2e08 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 17 Dec 2019 14:10:36 -0800 Subject: [PATCH] Fix androidSdk NPE (#47187) --- .../lib/src/android/android_builder.dart | 6 +- .../lib/src/android/android_sdk.dart | 2 +- .../flutter_tools/lib/src/android/gradle.dart | 17 +-- .../lib/src/application_package.dart | 2 +- .../lib/src/commands/build_aar.dart | 5 + .../lib/src/commands/build_apk.dart | 5 + .../lib/src/commands/build_appbundle.dart | 5 + packages/flutter_tools/lib/src/project.dart | 5 +- .../commands/build_aar_test.dart | 92 ++++++---------- .../commands/build_apk_test.dart | 101 +++++------------- .../commands/build_appbundle_test.dart | 98 +++++------------ .../test/src/android_common.dart | 16 +++ 12 files changed, 135 insertions(+), 219 deletions(-) diff --git a/packages/flutter_tools/lib/src/android/android_builder.dart b/packages/flutter_tools/lib/src/android/android_builder.dart index 50e2f1b977..ec21efab06 100644 --- a/packages/flutter_tools/lib/src/android/android_builder.dart +++ b/packages/flutter_tools/lib/src/android/android_builder.dart @@ -86,7 +86,7 @@ class _AndroidBuilderImpl extends AndroidBuilder { buildNumber: buildNumber, ); } finally { - androidSdk.reinitialize(); + androidSdk?.reinitialize(); } } @@ -106,7 +106,7 @@ class _AndroidBuilderImpl extends AndroidBuilder { localGradleErrors: gradleErrors, ); } finally { - androidSdk.reinitialize(); + androidSdk?.reinitialize(); } } @@ -126,7 +126,7 @@ class _AndroidBuilderImpl extends AndroidBuilder { localGradleErrors: gradleErrors, ); } finally { - androidSdk.reinitialize(); + androidSdk?.reinitialize(); } } } diff --git a/packages/flutter_tools/lib/src/android/android_sdk.dart b/packages/flutter_tools/lib/src/android/android_sdk.dart index 7e744f7d5b..7adc6c784e 100644 --- a/packages/flutter_tools/lib/src/android/android_sdk.dart +++ b/packages/flutter_tools/lib/src/android/android_sdk.dart @@ -55,7 +55,7 @@ String getAdbPath([ AndroidSdk existingSdk ]) { if (sdk?.latestVersion == null) { return os.which('adb')?.path; } else { - return sdk.adbPath; + return sdk?.adbPath; } } diff --git a/packages/flutter_tools/lib/src/android/gradle.dart b/packages/flutter_tools/lib/src/android/gradle.dart index 28ee9947b5..6bfd93cef7 100644 --- a/packages/flutter_tools/lib/src/android/gradle.dart +++ b/packages/flutter_tools/lib/src/android/gradle.dart @@ -149,7 +149,7 @@ Future checkGradleDependencies() async { workingDirectory: flutterProject.android.hostAppGradleRoot.path, environment: gradleEnvironment, ); - androidSdk.reinitialize(); + androidSdk?.reinitialize(); progress.stop(); } @@ -224,9 +224,13 @@ Future buildGradleApp({ bool shouldBuildPluginAsAar = false, int retries = 1, }) async { - if (androidSdk == null) { - exitWithNoSdkMessage(); - } + assert(project != null); + assert(androidBuildInfo != null); + assert(target != null); + assert(isBuildingBundle != null); + assert(localGradleErrors != null); + assert(androidSdk != null); + if (!project.android.isUsingGradle) { _exitWithProjectNotUsingGradleMessage(); } @@ -489,10 +493,7 @@ Future buildGradleAar({ assert(target != null); assert(androidBuildInfo != null); assert(outputDirectory != null); - - if (androidSdk == null) { - exitWithNoSdkMessage(); - } + assert(androidSdk != null); final FlutterManifest manifest = project.manifest; if (!manifest.isModule && !manifest.isPlugin) { diff --git a/packages/flutter_tools/lib/src/application_package.dart b/packages/flutter_tools/lib/src/application_package.dart index cb761d0396..b0e1f4728c 100644 --- a/packages/flutter_tools/lib/src/application_package.dart +++ b/packages/flutter_tools/lib/src/application_package.dart @@ -41,7 +41,7 @@ class ApplicationPackageFactory { case TargetPlatform.android_arm64: case TargetPlatform.android_x64: case TargetPlatform.android_x86: - if (androidSdk?.licensesAvailable == true && androidSdk.latestVersion == null) { + if (androidSdk?.licensesAvailable == true && androidSdk?.latestVersion == null) { await checkGradleDependencies(); } return applicationBinary == null diff --git a/packages/flutter_tools/lib/src/commands/build_aar.dart b/packages/flutter_tools/lib/src/commands/build_aar.dart index 96c7c1cbad..84a83d8e90 100644 --- a/packages/flutter_tools/lib/src/commands/build_aar.dart +++ b/packages/flutter_tools/lib/src/commands/build_aar.dart @@ -5,6 +5,8 @@ import 'dart:async'; import '../android/android_builder.dart'; +import '../android/android_sdk.dart'; +import '../android/gradle_utils.dart'; import '../base/common.dart'; import '../base/os.dart'; import '../build_info.dart'; @@ -85,6 +87,9 @@ class BuildAarCommand extends BuildSubCommand { @override Future runCommand() async { + if (androidSdk == null) { + exitWithNoSdkMessage(); + } final Set androidBuildInfo = {}; final Iterable targetArchitectures = diff --git a/packages/flutter_tools/lib/src/commands/build_apk.dart b/packages/flutter_tools/lib/src/commands/build_apk.dart index 366ae57a6b..05948bc8bf 100644 --- a/packages/flutter_tools/lib/src/commands/build_apk.dart +++ b/packages/flutter_tools/lib/src/commands/build_apk.dart @@ -5,6 +5,8 @@ import 'dart:async'; import '../android/android_builder.dart'; +import '../android/android_sdk.dart'; +import '../android/gradle_utils.dart'; import '../base/terminal.dart'; import '../build_info.dart'; import '../cache.dart'; @@ -77,6 +79,9 @@ class BuildApkCommand extends BuildSubCommand { @override Future runCommand() async { + if (androidSdk == null) { + exitWithNoSdkMessage(); + } final BuildInfo buildInfo = getBuildInfo(); final AndroidBuildInfo androidBuildInfo = AndroidBuildInfo( buildInfo, diff --git a/packages/flutter_tools/lib/src/commands/build_appbundle.dart b/packages/flutter_tools/lib/src/commands/build_appbundle.dart index 74c0f5cb26..7748e8daa9 100644 --- a/packages/flutter_tools/lib/src/commands/build_appbundle.dart +++ b/packages/flutter_tools/lib/src/commands/build_appbundle.dart @@ -5,6 +5,8 @@ import 'dart:async'; import '../android/android_builder.dart'; +import '../android/android_sdk.dart'; +import '../android/gradle_utils.dart'; import '../build_info.dart'; import '../cache.dart'; import '../project.dart'; @@ -69,6 +71,9 @@ class BuildAppBundleCommand extends BuildSubCommand { @override Future runCommand() async { + if (androidSdk == null) { + exitWithNoSdkMessage(); + } final AndroidBuildInfo androidBuildInfo = AndroidBuildInfo(getBuildInfo(), targetArchs: stringsArg('target-platform').map(getAndroidArchForName), shrink: boolArg('shrink'), diff --git a/packages/flutter_tools/lib/src/project.dart b/packages/flutter_tools/lib/src/project.dart index 2671a03df4..798caa7464 100644 --- a/packages/flutter_tools/lib/src/project.dart +++ b/packages/flutter_tools/lib/src/project.dart @@ -29,14 +29,15 @@ FlutterProjectFactory get projectFactory => context.get() class FlutterProjectFactory { FlutterProjectFactory(); - final Map _projects = + @visibleForTesting + final Map projects = {}; /// Returns a [FlutterProject] view of the given directory or a ToolExit error, /// if `pubspec.yaml` or `example/pubspec.yaml` is invalid. FlutterProject fromDirectory(Directory directory) { assert(directory != null); - return _projects.putIfAbsent(directory.path, /* ifAbsent */ () { + return projects.putIfAbsent(directory.path, /* ifAbsent */ () { final FlutterManifest manifest = FlutterProject._readManifest( directory.childFile(bundle.defaultManifestPath).path, ); diff --git a/packages/flutter_tools/test/general.shard/commands/build_aar_test.dart b/packages/flutter_tools/test/general.shard/commands/build_aar_test.dart index f3ba526d35..303dcd0b7c 100644 --- a/packages/flutter_tools/test/general.shard/commands/build_aar_test.dart +++ b/packages/flutter_tools/test/general.shard/commands/build_aar_test.dart @@ -5,13 +5,12 @@ import 'dart:io' show Process, ProcessResult; import 'package:args/command_runner.dart'; -import 'package:file/memory.dart'; import 'package:flutter_tools/src/android/android_builder.dart'; import 'package:flutter_tools/src/android/android_sdk.dart'; import 'package:flutter_tools/src/base/file_system.dart'; -import 'package:flutter_tools/src/base/platform.dart'; import 'package:flutter_tools/src/cache.dart'; import 'package:flutter_tools/src/commands/build_aar.dart'; +import 'package:flutter_tools/src/project.dart'; import 'package:flutter_tools/src/reporting/reporting.dart'; import 'package:mockito/mockito.dart'; import 'package:process/process.dart'; @@ -90,18 +89,14 @@ void main() { Directory tempDir; AndroidSdk mockAndroidSdk; Usage mockUsage; - FileSystem memoryFileSystem; setUp(() { mockUsage = MockUsage(); when(mockUsage.isFirstRun).thenReturn(true); - memoryFileSystem = MemoryFileSystem(); - tempDir = memoryFileSystem.systemTempDirectory.createTempSync('flutter_tools_packages_test.'); - memoryFileSystem.currentDirectory = tempDir; + tempDir = fs.systemTempDirectory.createTempSync('flutter_tools_packages_test.'); mockProcessManager = MockProcessManager(); - when(mockProcessManager.run(any, workingDirectory: anyNamed('workingDirectory'), environment: anyNamed('environment'))) @@ -118,65 +113,49 @@ void main() { when(mockAndroidSdk.directory).thenReturn('irrelevant'); }); + tearDown(() { + tryToDelete(tempDir); + }); + group('AndroidSdk', () { testUsingContext('validateSdkWellFormed() not called, sdk reinitialized', () async { - final Directory gradleCacheDir = memoryFileSystem - .directory('/flutter_root/bin/cache/artifacts/gradle_wrapper') - ..createSync(recursive: true); - gradleCacheDir.childFile(platform.isWindows ? 'gradlew.bat' : 'gradlew').createSync(); + final String projectPath = await createProject(tempDir, + arguments: ['--no-pub', '--template=module']); - tempDir.childFile('pubspec.yaml') - ..createSync(recursive: true) - ..writeAsStringSync('''name: test -environment: - sdk: ">=2.1.0 <3.0.0" -dependencies: - flutter: - sdk: flutter -dev_dependencies: - flutter_test: - sdk: flutter -flutter: - plugin: - androidPackage: com.example.blah - pluginClass: BlahPlugin -'''); - tempDir.childFile('.packages').createSync(recursive: true); - final Directory androidDir = tempDir.childDirectory('android'); - androidDir - .childFile('build.gradle') - .createSync(recursive: true); - androidDir - .childDirectory('app') - .childFile('build.gradle') - ..createSync(recursive: true) - ..writeAsStringSync('apply from: irrelevant/flutter.gradle'); - androidDir - .childFile('gradle.properties') - .createSync(recursive: true); - androidDir - .childDirectory('gradle') - .childDirectory('wrapper') - .childFile('gradle-wrapper.properties') - .createSync(recursive: true); - tempDir - .childDirectory('build') - .childDirectory('outputs') - .childDirectory('repo') - .createSync(recursive: true); - tempDir - .childDirectory('lib') - .childFile('main.dart') - .createSync(recursive: true); - await runBuildAarCommand(tempDir.path); + await expectLater( + runBuildAarCommand( + projectPath, + arguments: ['--no-pub'], + ), + throwsToolExit(), + ); verifyNever(mockAndroidSdk.validateSdkWellFormed()); verify(mockAndroidSdk.reinitialize()).called(1); }, overrides: { AndroidSdk: () => mockAndroidSdk, + FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir), + ProcessManager: () => mockProcessManager, + }); + + testUsingContext('throws throwsToolExit if AndroidSdk is null', () async { + final String projectPath = await createProject(tempDir, + arguments: ['--no-pub', '--template=module']); + + await expectLater(() async { + await runBuildAarCommand( + projectPath, + arguments: ['--no-pub'], + ); + }, throwsToolExit( + message: '[!] No Android SDK found. Try setting the ANDROID_HOME environment variable', + )); + }, + overrides: { + AndroidSdk: () => null, + FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir), ProcessManager: () => mockProcessManager, - FileSystem: () => memoryFileSystem, }); }); }); @@ -191,7 +170,6 @@ Future runBuildAarCommand( await runner.run([ 'aar', '--no-pub', - '--flutter-root=/flutter_root', ...?arguments, fs.path.join(target, 'lib', 'main.dart'), ]); diff --git a/packages/flutter_tools/test/general.shard/commands/build_apk_test.dart b/packages/flutter_tools/test/general.shard/commands/build_apk_test.dart index 29c49f6bb7..88900ab4a7 100644 --- a/packages/flutter_tools/test/general.shard/commands/build_apk_test.dart +++ b/packages/flutter_tools/test/general.shard/commands/build_apk_test.dart @@ -5,7 +5,6 @@ import 'dart:io'; import 'package:args/command_runner.dart'; -import 'package:file/memory.dart'; import 'package:flutter_tools/src/android/android_builder.dart'; import 'package:flutter_tools/src/android/android_sdk.dart'; import 'package:flutter_tools/src/base/context.dart'; @@ -140,70 +139,15 @@ void main() { }); group('AndroidSdk', () { - FileSystem memoryFileSystem; - setUp(() { - memoryFileSystem = MemoryFileSystem(); - - tempDir = memoryFileSystem.systemTempDirectory.createTempSync('flutter_tools_packages_test.'); - memoryFileSystem.currentDirectory = tempDir; - - gradlew = memoryFileSystem.path.join(tempDir.path, 'flutter_project', 'android', - platform.isWindows ? 'gradlew.bat' : 'gradlew'); - }); testUsingContext('validateSdkWellFormed() not called, sdk reinitialized', () async { - final Directory gradleCacheDir = memoryFileSystem - .directory('/flutter_root/bin/cache/artifacts/gradle_wrapper') - ..createSync(recursive: true); - - gradleCacheDir.childFile(platform.isWindows ? 'gradlew.bat' : 'gradlew').createSync(); - - tempDir.childFile('pubspec.yaml') - ..createSync(recursive: true) - ..writeAsStringSync('''name: test -environment: - sdk: ">=2.1.0 <3.0.0" -dependencies: - flutter: - sdk: flutter -dev_dependencies: - flutter_test: - sdk: flutter -flutter: -'''); - tempDir.childFile('.packages').createSync(recursive: true); - final Directory androidDir = tempDir.childDirectory('android'); - androidDir - .childFile('build.gradle') - .createSync(recursive: true); - androidDir - .childDirectory('app') - .childFile('build.gradle') - ..createSync(recursive: true) - ..writeAsStringSync('apply from: irrelevant/flutter.gradle'); - androidDir - .childFile('gradle.properties') - .createSync(recursive: true); - androidDir - .childDirectory('gradle') - .childDirectory('wrapper') - .childFile('gradle-wrapper.properties') - .createSync(recursive: true); - tempDir - .childDirectory('build') - .childDirectory('outputs') - .childDirectory('repo') - .createSync(recursive: true); - tempDir - .childDirectory('lib') - .childFile('main.dart') - .createSync(recursive: true); - when(mockProcessManager.run(any, - workingDirectory: anyNamed('workingDirectory'), - environment: anyNamed('environment'))) - .thenAnswer((_) => Future.value(ProcessResult(0, 0, 'any', ''))); + final String projectPath = await createProject(tempDir, + arguments: ['--no-pub', '--template=app']); await expectLater( - runBuildApkCommand(tempDir.path, arguments: ['--no-pub', '--flutter-root=/flutter_root']), + runBuildApkCommand( + projectPath, + arguments: ['--no-pub'], + ), throwsToolExit(message: 'Gradle task assembleRelease failed with exit code 1'), ); @@ -212,7 +156,26 @@ flutter: }, overrides: { AndroidSdk: () => mockAndroidSdk, - FileSystem: () => memoryFileSystem, + FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir), + ProcessManager: () => mockProcessManager, + }); + + testUsingContext('throws throwsToolExit if AndroidSdk is null', () async { + final String projectPath = await createProject(tempDir, + arguments: ['--no-pub', '--template=app']); + + await expectLater(() async { + await runBuildApkCommand( + projectPath, + arguments: ['--no-pub'], + ); + }, throwsToolExit( + message: '[!] No Android SDK found. Try setting the ANDROID_HOME environment variable', + )); + }, + overrides: { + AndroidSdk: () => null, + FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir), ProcessManager: () => mockProcessManager, }); }); @@ -451,18 +414,6 @@ Future runBuildApkCommand( return command; } -class FakeFlutterProjectFactory extends FlutterProjectFactory { - FakeFlutterProjectFactory(this.directoryOverride) : - assert(directoryOverride != null); - - final Directory directoryOverride; - - @override - FlutterProject fromDirectory(Directory _) { - return super.fromDirectory(directoryOverride.childDirectory('flutter_project')); - } -} - class MockAndroidSdk extends Mock implements AndroidSdk {} class MockProcessManager extends Mock implements ProcessManager {} class MockProcess extends Mock implements Process {} diff --git a/packages/flutter_tools/test/general.shard/commands/build_appbundle_test.dart b/packages/flutter_tools/test/general.shard/commands/build_appbundle_test.dart index 1acf9e8942..bccbd0d59b 100644 --- a/packages/flutter_tools/test/general.shard/commands/build_appbundle_test.dart +++ b/packages/flutter_tools/test/general.shard/commands/build_appbundle_test.dart @@ -5,7 +5,6 @@ import 'dart:io'; import 'package:args/command_runner.dart'; -import 'package:file/memory.dart'; import 'package:flutter_tools/src/android/android_builder.dart'; import 'package:flutter_tools/src/android/android_sdk.dart'; import 'package:flutter_tools/src/base/context.dart'; @@ -125,67 +124,15 @@ void main() { }); group('AndroidSdk', () { - FileSystem memoryFileSystem; - - setUp(() { - memoryFileSystem = MemoryFileSystem(); - - tempDir = memoryFileSystem.systemTempDirectory.createTempSync('flutter_tools_packages_test.'); - memoryFileSystem.currentDirectory = tempDir; - - gradlew = memoryFileSystem.path.join(tempDir.path, 'flutter_project', 'android', - platform.isWindows ? 'gradlew.bat' : 'gradlew'); - }); - testUsingContext('validateSdkWellFormed() not called, sdk reinitialized', () async { - final Directory gradleCacheDir = memoryFileSystem - .directory('/flutter_root/bin/cache/artifacts/gradle_wrapper') - ..createSync(recursive: true); - gradleCacheDir.childFile(platform.isWindows ? 'gradlew.bat' : 'gradlew').createSync(); - - tempDir.childFile('pubspec.yaml') - ..createSync(recursive: true) - ..writeAsStringSync('''name: test -environment: - sdk: ">=2.1.0 <3.0.0" -dependencies: - flutter: - sdk: flutter -dev_dependencies: - flutter_test: - sdk: flutter -flutter: -'''); - tempDir.childFile('.packages').createSync(recursive: true); - final Directory androidDir = tempDir.childDirectory('android'); - androidDir.childFile('build.gradle').createSync(recursive: true); - androidDir - .childDirectory('app') - .childFile('build.gradle') - ..createSync(recursive: true) - ..writeAsStringSync('apply from: irrelevant/flutter.gradle'); - androidDir - .childFile('gradle.properties') - .createSync(recursive: true); - androidDir - .childDirectory('gradle') - .childDirectory('wrapper') - .childFile('gradle-wrapper.properties') - .createSync(recursive: true); - tempDir.childDirectory('build') - .childDirectory('outputs') - .childDirectory('repo') - .createSync(recursive: true); - tempDir.childDirectory('lib') - .childFile('main.dart') - .createSync(recursive: true); - when(mockProcessManager.run(any, - workingDirectory: anyNamed('workingDirectory'), - environment: anyNamed('environment'))) - .thenAnswer((_) => Future.value(ProcessResult(0, 0, 'any', ''))); + final String projectPath = await createProject(tempDir, + arguments: ['--no-pub', '--template=app']); await expectLater( - runBuildAppBundleCommand(tempDir.path, arguments: ['--no-pub', '--flutter-root=/flutter_root']), + runBuildAppBundleCommand( + projectPath, + arguments: ['--no-pub'], + ), throwsToolExit(message: 'Gradle task bundleRelease failed with exit code 1'), ); @@ -194,8 +141,27 @@ flutter: }, overrides: { AndroidSdk: () => mockAndroidSdk, + FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir), + ProcessManager: () => mockProcessManager, + }); + + testUsingContext('throws throwsToolExit if AndroidSdk is null', () async { + final String projectPath = await createProject(tempDir, + arguments: ['--no-pub', '--template=app']); + + await expectLater(() async { + await runBuildAppBundleCommand( + projectPath, + arguments: ['--no-pub'], + ); + }, throwsToolExit( + message: '[!] No Android SDK found. Try setting the ANDROID_HOME environment variable', + )); + }, + overrides: { + AndroidSdk: () => null, + FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir), ProcessManager: () => mockProcessManager, - FileSystem: () => memoryFileSystem, }); }); @@ -437,18 +403,6 @@ Future runBuildAppBundleCommand( return command; } -class FakeFlutterProjectFactory extends FlutterProjectFactory { - FakeFlutterProjectFactory(this._directoryOverride) : - assert(_directoryOverride != null); - - final Directory _directoryOverride; - - @override - FlutterProject fromDirectory(Directory _) { - return super.fromDirectory(_directoryOverride.childDirectory('flutter_project')); - } -} - class MockAndroidSdk extends Mock implements AndroidSdk {} class MockProcessManager extends Mock implements ProcessManager {} class MockProcess extends Mock implements Process {} diff --git a/packages/flutter_tools/test/src/android_common.dart b/packages/flutter_tools/test/src/android_common.dart index c156d5a344..01eec1743e 100644 --- a/packages/flutter_tools/test/src/android_common.dart +++ b/packages/flutter_tools/test/src/android_common.dart @@ -5,6 +5,7 @@ import 'package:meta/meta.dart'; import 'package:flutter_tools/src/android/android_builder.dart'; +import 'package:flutter_tools/src/base/file_system.dart'; import 'package:flutter_tools/src/build_info.dart'; import 'package:flutter_tools/src/project.dart'; @@ -33,3 +34,18 @@ class FakeAndroidBuilder implements AndroidBuilder { @required String target, }) async {} } + +/// Creates a [FlutterProject] in a directory named [flutter_project] +/// within [directoryOverride]. +class FakeFlutterProjectFactory extends FlutterProjectFactory { + FakeFlutterProjectFactory(this.directoryOverride) : + assert(directoryOverride != null); + + final Directory directoryOverride; + + @override + FlutterProject fromDirectory(Directory _) { + projects.clear(); + return super.fromDirectory(directoryOverride.childDirectory('flutter_project')); + } +}