Replace --prefer-shared-library with --build-shared-library (flutter#…

…17394) This replaces the --prefer-shared-library flag, which falls back to regular (non-shared-lib) compile if the NDK is not found, with the --build-shared-library flag, which exits with an error message if the NDK is not found. This simplifies the set of allowed code paths through AOT compile, resulting in better testability and easier-to-follow logic. It also results in more predictable behaviour for continuous integration and other scenarios.
Hades2010 · May 9, 2018 · 8b8d368 · 8b8d368
1 parent d1e49bc
commit 8b8d368
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 24 deletions.
diff --git a/packages/flutter_tools/lib/src/base/build.dart b/packages/flutter_tools/lib/src/base/build.dart
@@ -137,27 +137,35 @@ class AOTSnapshotter {
     @required String packagesPath,
     @required String outputPath,
     @required bool previewDart2,
-    @required bool preferSharedLibrary,
+    @required bool buildSharedLibrary,
     IOSArch iosArch,
     List<String> extraGenSnapshotOptions: const <String>[],
   }) async {
     if (!_isValidAotPlatform(platform, buildMode)) {
       printError('${getNameForTargetPlatform(platform)} does not support AOT compilation.');
-      return -1;
+      return 1;
     }
     // TODO(cbracken): replace IOSArch with TargetPlatform.ios_{armv7,arm64}.
     assert(platform != TargetPlatform.ios || iosArch != null);
 
-    final bool compileToSharedLibrary = preferSharedLibrary && androidSdk.ndkCompiler != null;
-    if (preferSharedLibrary && !compileToSharedLibrary) {
-      printStatus('Could not find NDK compiler. Not building in shared library mode.');
+    // buildSharedLibrary is ignored for iOS builds.
+    if (platform == TargetPlatform.ios)
+      buildSharedLibrary = false;
+
+    if (buildSharedLibrary && androidSdk.ndkCompiler == null) {
+      printError(
+        'Could not find NDK in Android SDK at ${androidSdk.directory}.\n'
+        'Unable to build with --build-shared-library\n'
+        'To install the NDK, see instructions at https://developer.android.com/ndk/guides/'
+      );
+      return 1;
     }
 
     final PackageMap packageMap = new PackageMap(packagesPath);
     final String packageMapError = packageMap.checkValid();
     if (packageMapError != null) {
       printError(packageMapError);
-      return -2;
+      return 1;
     }
 
     final Directory outputDir = fs.directory(outputPath);
@@ -198,7 +206,7 @@ class AOTSnapshotter {
     }
 
     final String assembly = fs.path.join(outputDir.path, 'snapshot_assembly.S');
-    if (compileToSharedLibrary || platform == TargetPlatform.ios) {
+    if (buildSharedLibrary || platform == TargetPlatform.ios) {
       // Assembly AOT snapshot.
       outputPaths.add(assembly);
       genSnapshotArgs.add('--snapshot_kind=app-aot-assembly');
@@ -230,7 +238,7 @@ class AOTSnapshotter {
     final Iterable<String> missingInputs = inputPaths.where((String p) => !fs.isFileSync(p));
     if (missingInputs.isNotEmpty) {
       printError('Missing input files: $missingInputs from $inputPaths');
-      return -3;
+      return 1;
     }
 
     // If inputs and outputs have not changed since last run, skip the build.
@@ -242,7 +250,7 @@ class AOTSnapshotter {
         'targetPlatform': platform.toString(),
         'entryPoint': mainPath,
         'dart2': previewDart2.toString(),
-        'sharedLib': compileToSharedLibrary.toString(),
+        'sharedLib': buildSharedLibrary.toString(),
         'extraGenSnapshotOptions': extraGenSnapshotOptions.join(' '),
       },
       depfilePaths: <String>[depfilePath],
@@ -261,7 +269,7 @@ class AOTSnapshotter {
     );
     if (genSnapshotExitCode != 0) {
       printError('Dart snapshot generator failed with exit code $genSnapshotExitCode');
-      return -4;
+      return genSnapshotExitCode;
     }
 
     // Write path to gen_snapshot, since snapshots have to be re-generated when we roll
@@ -274,10 +282,12 @@ class AOTSnapshotter {
       final RunResult result = await _buildIosFramework(iosArch: iosArch, assemblyPath: assembly, outputPath: outputDir.path);
       if (result.exitCode != 0)
         return result.exitCode;
-    } else if (compileToSharedLibrary) {
+    } else if (buildSharedLibrary) {
       final RunResult result = await _buildAndroidSharedLibrary(assemblyPath: assembly, outputPath: outputDir.path);
-      if (result.exitCode != 0)
+      if (result.exitCode != 0) {
+        printError('Failed to build AOT snapshot. Compiler terminated with exit code ${result.exitCode}');
         return result.exitCode;
+      }
     }
 
     // Compute and record build fingerprint.
@@ -298,8 +308,10 @@ class AOTSnapshotter {
 
     final String assemblyO = fs.path.join(outputPath, 'snapshot_assembly.o');
     final RunResult compileResult = await xcode.cc(commonBuildOptions.toList()..addAll(<String>['-c', assemblyPath, '-o', assemblyO]));
-    if (compileResult.exitCode != 0)
+    if (compileResult.exitCode != 0) {
+      printError('Failed to compile AOT snapshot. Compiler terminated with exit code ${compileResult.exitCode}');
       return compileResult;
+    }
 
     final String frameworkDir = fs.path.join(outputPath, 'App.framework');
     fs.directory(frameworkDir).createSync(recursive: true);
@@ -313,6 +325,9 @@ class AOTSnapshotter {
         assemblyO,
     ]);
     final RunResult linkResult = await xcode.clang(linkArgs);
+    if (linkResult.exitCode != 0) {
+      printError('Failed to link AOT snapshot. Linker terminated with exit code ${compileResult.exitCode}');
+    }
     return linkResult;
   }
 

diff --git a/packages/flutter_tools/lib/src/commands/build_aot.dart b/packages/flutter_tools/lib/src/commands/build_aot.dart
@@ -33,11 +33,16 @@ class BuildAotCommand extends BuildSubCommand {
         hide: !verboseHelp,
         help: 'Preview Dart 2.0 functionality.',
       )
+      ..addFlag('build-shared-library',
+        negatable: false,
+        defaultsTo: false,
+        help: 'Compile to a *.so file (requires NDK when building for Android).'
+      )
       ..addMultiOption('ios-arch',
         splitCommas: true,
         defaultsTo: defaultIOSArchs.map(getNameForIOSArch),
         allowed: IOSArch.values.map(getNameForIOSArch),
-        help: 'iOS architectures to build',
+        help: 'iOS architectures to build.',
       )
       ..addMultiOption(FlutterOptions.kExtraFrontEndOptions,
         splitCommas: true,
@@ -46,10 +51,7 @@ class BuildAotCommand extends BuildSubCommand {
       ..addMultiOption(FlutterOptions.kExtraGenSnapshotOptions,
         splitCommas: true,
         hide: true,
-      )
-      ..addFlag('prefer-shared-library',
-        negatable: false,
-        help: 'Whether to prefer compiling to a *.so file (android only).');
+      );
   }
 
   @override
@@ -115,7 +117,7 @@ class BuildAotCommand extends BuildSubCommand {
             packagesPath: PackageMap.globalPackagesPath,
             outputPath: outputPath,
             previewDart2: previewDart2,
-            preferSharedLibrary: false,
+            buildSharedLibrary: false,
             extraGenSnapshotOptions: argResults[FlutterOptions.kExtraGenSnapshotOptions],
           ).then((int buildExitCode) {
             if (buildExitCode != 0)
@@ -142,16 +144,18 @@ class BuildAotCommand extends BuildSubCommand {
           packagesPath: PackageMap.globalPackagesPath,
           outputPath: outputPath,
           previewDart2: previewDart2,
-          preferSharedLibrary: argResults['prefer-shared-library'],
+          buildSharedLibrary: argResults['build-shared-library'],
           extraGenSnapshotOptions: argResults[FlutterOptions.kExtraGenSnapshotOptions],
         );
         if (snapshotExitCode != 0) {
           printError('Snapshotting exited with non-zero exit code: $snapshotExitCode');
+          return;
         }
       }
     } on String catch (error) {
       // Catch the String exceptions thrown from the `runCheckedSync` methods below.
       printError(error);
+      return;
     }
     status?.stop();
 

diff --git a/packages/flutter_tools/test/base/build_test.dart b/packages/flutter_tools/test/base/build_test.dart
@@ -6,6 +6,7 @@ import 'dart:async';
 import 'dart:convert' show json;
 
 import 'package:file/memory.dart';
+import 'package:flutter_tools/src/android/android_sdk.dart';
 import 'package:flutter_tools/src/artifacts.dart';
 import 'package:flutter_tools/src/build_info.dart';
 import 'package:flutter_tools/src/base/build.dart';
@@ -21,6 +22,7 @@ import 'package:test/test.dart';
 import '../src/context.dart';
 
 class MockFlutterVersion extends Mock implements FlutterVersion {}
+class MockAndroidSdk extends Mock implements AndroidSdk {}
 class MockArtifacts extends Mock implements Artifacts {}
 class MockXcode extends Mock implements Xcode {}
 
@@ -287,6 +289,7 @@ void main() {
     _FakeGenSnapshot genSnapshot;
     MemoryFileSystem fs;
     AOTSnapshotter snapshotter;
+    MockAndroidSdk mockAndroidSdk;
     MockArtifacts mockArtifacts;
     MockXcode mockXcode;
 
@@ -308,6 +311,7 @@ void main() {
 
       genSnapshot = new _FakeGenSnapshot();
       snapshotter = new AOTSnapshotter();
+      mockAndroidSdk = new MockAndroidSdk();
       mockArtifacts = new MockArtifacts();
       mockXcode = new MockXcode();
       for (BuildMode mode in BuildMode.values) {
@@ -320,6 +324,7 @@ void main() {
     });
 
     final Map<Type, Generator> contextOverrides = <Type, Generator>{
+      AndroidSdk: () => mockAndroidSdk,
       Artifacts: () => mockArtifacts,
       FileSystem: () => fs,
       GenSnapshot: () => genSnapshot,
@@ -334,7 +339,7 @@ void main() {
         mainPath: 'main.dill',
         packagesPath: '.packages',
         outputPath: outputPath,
-        preferSharedLibrary: false,
+        buildSharedLibrary: false,
         previewDart2: true,
       ), isNot(equals(0)));
     }, overrides: contextOverrides);
@@ -347,7 +352,7 @@ void main() {
         mainPath: 'main.dill',
         packagesPath: '.packages',
         outputPath: outputPath,
-        preferSharedLibrary: false,
+        buildSharedLibrary: false,
         previewDart2: true,
       ), isNot(0));
     }, overrides: contextOverrides);
@@ -373,7 +378,7 @@ void main() {
         mainPath: 'main.dill',
         packagesPath: '.packages',
         outputPath: outputPath,
-        preferSharedLibrary: false,
+        buildSharedLibrary: false,
         previewDart2: true,
         iosArch: IOSArch.arm64,
       );
@@ -420,7 +425,7 @@ void main() {
         mainPath: 'main.dill',
         packagesPath: '.packages',
         outputPath: outputPath,
-        preferSharedLibrary: false,
+        buildSharedLibrary: false,
         previewDart2: true,
         iosArch: IOSArch.arm64,
       );
@@ -443,5 +448,24 @@ void main() {
         'main.dill',
       ]);
     }, overrides: contextOverrides);
+
+    testUsingContext('returns failure if buildSharedLibrary is true but no NDK is found', () async {
+      final String outputPath = fs.path.join('build', 'foo');
+
+      when(mockAndroidSdk.ndkCompiler).thenReturn(null);
+
+      final int genSnapshotExitCode = await snapshotter.build(
+        platform: TargetPlatform.android_arm,
+        buildMode: BuildMode.release,
+        mainPath: 'main.dill',
+        packagesPath: '.packages',
+        outputPath: outputPath,
+        buildSharedLibrary: true,
+        previewDart2: true,
+      );
+
+      expect(genSnapshotExitCode, isNot(0));
+      expect(genSnapshot.callCount, 0);
+    }, overrides: contextOverrides);
   });
 }