[native_assets_builder] Stop throwing from BuildRunner (#108)

Author: dcharkes

pkgs/native_assets_builder/CHANGELOG.md

@@ -1,7 +1,11 @@
 ## 0.2.0-wip
 
-- **Breaking change** `NativeAssetsBuildRunner`s method now return an object
+- **Breaking change** `NativeAssetsBuildRunner`s methods now return an object
   ([#105](https://github.com/dart-lang/native/issues/105)).
+- **Breaking change** `NativeAssetsBuildRunner`s methods now return value now
+  contain a success bool instead of throwing
+  ([#106](https://github.com/dart-lang/native/issues/106)). Error messages are
+  streamed to the logger.
 - Use an `out/` sub directory for building native assets
   ([#98](https://github.com/dart-lang/native/issues/98)).
 - Check asset ids on having having a package uri with the owning package

pkgs/native_assets_builder/lib/src/build_runner/build_planner.dart

@@ -6,23 +6,27 @@ import 'dart:convert';
 import 'dart:io';
 
 import 'package:graphs/graphs.dart' as graphs;
+import 'package:logging/logging.dart';
 import 'package:package_config/package_config.dart';
 
 class NativeAssetsBuildPlanner {
   final PackageGraph packageGraph;
   final List<Package> packagesWithNativeAssets;
   final Uri dartExecutable;
+  final Logger logger;
 
   NativeAssetsBuildPlanner({
     required this.packageGraph,
     required this.packagesWithNativeAssets,
     required this.dartExecutable,
+    required this.logger,
   });
 
   static Future<NativeAssetsBuildPlanner> fromRootPackageRoot({
     required Uri rootPackageRoot,
     required List<Package> packagesWithNativeAssets,
     required Uri dartExecutable,
+    required Logger logger,
   }) async {
     final result = await Process.run(
       dartExecutable.toFilePath(),
@@ -39,32 +43,35 @@ class NativeAssetsBuildPlanner {
       packageGraph: packageGraph,
       packagesWithNativeAssets: packagesWithNativeAssets,
       dartExecutable: dartExecutable,
+      logger: logger,
     );
   }
 
-  List<Package> plan() {
+  (List<Package> packages, bool success) plan() {
     final packageMap = {
       for (final package in packagesWithNativeAssets) package.name: package
     };
     final packagesToBuild = packageMap.keys.toSet();
     final stronglyConnectedComponents = packageGraph.computeStrongComponents();
     final result = <Package>[];
+    var success = true;
     for (final stronglyConnectedComponent in stronglyConnectedComponents) {
       final stronglyConnectedComponentWithNativeAssets = [
         for (final packageName in stronglyConnectedComponent)
           if (packagesToBuild.contains(packageName)) packageName
       ];
       if (stronglyConnectedComponentWithNativeAssets.length > 1) {
-        throw Exception(
+        logger.severe(
           'Cyclic dependency for native asset builds in the following '
-          'packages: $stronglyConnectedComponent.',
+          'packages: $stronglyConnectedComponentWithNativeAssets.',
         );
+        success = false;
       } else if (stronglyConnectedComponentWithNativeAssets.length == 1) {
         result.add(
             packageMap[stronglyConnectedComponentWithNativeAssets.single]!);
       }
     }
-    return result;
+    return (result, success);
   }
 }
 

pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart

@@ -51,11 +51,20 @@ class NativeAssetsBuildRunner {
       rootPackageRoot: packageLayout.rootPackageRoot,
       packagesWithNativeAssets: packagesWithNativeAssets,
       dartExecutable: Uri.file(Platform.resolvedExecutable),
+      logger: logger,
     );
-    final plan = planner.plan();
+    final (plan, planSuccess) = planner.plan();
+    if (!planSuccess) {
+      return _BuildResultImpl(
+        assets: [],
+        dependencies: [],
+        success: false,
+      );
+    }
     final assets = <Asset>[];
     final dependencies = <Uri>[];
     final metadata = <String, Metadata>{};
+    var success = true;
     for (final package in plan) {
       final dependencyMetadata = _metadataForPackage(
         packageGraph: planner.packageGraph,
@@ -73,22 +82,28 @@ class NativeAssetsBuildRunner {
         targetIOSSdk: targetIOSSdk,
         targetAndroidNdkApi: targetAndroidNdkApi,
       );
-      final (packageAssets, packageDependencies, packageMetadata) =
-          await _buildPackageCached(
+      final (
+        packageAssets,
+        packageDependencies,
+        packageMetadata,
+        packageSuccess,
+      ) = await _buildPackageCached(
         config,
         packageLayout.packageConfigUri,
         workingDirectory,
         includeParentEnvironment,
       );
       assets.addAll(packageAssets);
       dependencies.addAll(packageDependencies);
+      success &= packageSuccess;
       if (packageMetadata != null) {
         metadata[config.packageName] = packageMetadata;
       }
     }
     return _BuildResultImpl(
       assets: assets,
       dependencies: dependencies..sort(_uriCompare),
+      success: success,
     );
   }
 
@@ -112,9 +127,17 @@ class NativeAssetsBuildRunner {
       rootPackageRoot: packageLayout.rootPackageRoot,
       packagesWithNativeAssets: packagesWithNativeAssets,
       dartExecutable: Uri.file(Platform.resolvedExecutable),
+      logger: logger,
     );
-    final plan = planner.plan();
+    final (plan, planSuccess) = planner.plan();
+    if (!planSuccess) {
+      return _DryRunResultImpl(
+        assets: [],
+        success: false,
+      );
+    }
     final assets = <Asset>[];
+    var success = true;
     for (final package in plan) {
       final config = await _cliConfigDryRun(
         packageName: package.name,
@@ -123,21 +146,23 @@ class NativeAssetsBuildRunner {
         linkMode: linkModePreference,
         buildParentDir: packageLayout.dartToolNativeAssetsBuilder,
       );
-      final (packageAssets, _, _) = await _buildPackage(
+      final (packageAssets, _, _, packageSuccess) = await _buildPackage(
         config,
         packageLayout.packageConfigUri,
         workingDirectory,
         includeParentEnvironment,
         dryRun: true,
       );
       assets.addAll(packageAssets);
+      success &= packageSuccess;
     }
     return _DryRunResultImpl(
       assets: assets,
+      success: success,
     );
   }
 
-  Future<(List<Asset>, List<Uri>, Metadata?)> _buildPackageCached(
+  Future<_PackageBuildRecord> _buildPackageCached(
     BuildConfig config,
     Uri packageConfigUri,
     Uri workingDirectory,
@@ -163,7 +188,7 @@ class NativeAssetsBuildRunner {
       final assets = buildOutput!.assets;
       final dependencies = buildOutput.dependencies.dependencies;
       final metadata = buildOutput.metadata;
-      return (assets, dependencies, metadata);
+      return (assets, dependencies, metadata, true);
     }
 
     return await _buildPackage(
@@ -175,7 +200,7 @@ class NativeAssetsBuildRunner {
     );
   }
 
-  Future<(List<Asset>, List<Uri>, Metadata?)> _buildPackage(
+  Future<_PackageBuildRecord> _buildPackage(
     BuildConfig config,
     Uri packageConfigUri,
     Uri workingDirectory,
@@ -193,25 +218,77 @@ class NativeAssetsBuildRunner {
       // Ensure we'll never read outdated build results.
       await buildOutputFile.delete();
     }
-    await runProcess(
+    final arguments = [
+      '--packages=${packageConfigUri.toFilePath()}',
+      buildDotDart.toFilePath(),
+      '--config=${configFile.toFilePath()}',
+    ];
+    final result = await runProcess(
       workingDirectory: workingDirectory,
       executable: dartExecutable,
-      arguments: [
-        '--packages=${packageConfigUri.toFilePath()}',
-        buildDotDart.toFilePath(),
-        '--config=${configFile.toFilePath()}',
-      ],
+      arguments: arguments,
       logger: logger,
       includeParentEnvironment: includeParentEnvironment,
-      expectedExitCode: 0,
-      throwOnUnexpectedExitCode: true,
     );
-    final buildOutput = await BuildOutput.readFromFile(outDir: outDir);
-    final assets = buildOutput?.assets ?? [];
-    validateAssetsPackage(assets, config.packageName);
-    final dependencies = buildOutput?.dependencies.dependencies ?? [];
-    final metadata = dryRun ? null : buildOutput?.metadata;
-    return (assets, dependencies, metadata);
+    var success = true;
+    if (result.exitCode != 0) {
+      final printWorkingDir = workingDirectory != Directory.current.uri;
+      final commandString = [
+        if (printWorkingDir) '(cd ${workingDirectory.toFilePath()};',
+        dartExecutable.toFilePath(),
+        ...arguments.map((a) => a.contains(' ') ? "'$a'" : a),
+        if (printWorkingDir) ')',
+      ].join(' ');
+      logger.severe(
+        '''
+Building native assets for package:${config.packageName} failed.
+build.dart returned with exit code: ${result.exitCode}.
+To reproduce run:
+$commandString
+stderr:
+${result.stderr}
+stdout:
+${result.stdout}
+        ''',
+      );
+      success = false;
+    }
+
+    try {
+      final buildOutput = await BuildOutput.readFromFile(outDir: outDir);
+      final assets = buildOutput?.assets ?? [];
+      success &= validateAssetsPackage(assets, config.packageName);
+      final dependencies = buildOutput?.dependencies.dependencies ?? [];
+      final metadata = dryRun ? null : buildOutput?.metadata;
+      return (assets, dependencies, metadata, success);
+    } on FormatException catch (e) {
+      logger.severe('''
+Building native assets for package:${config.packageName} failed.
+build_output.yaml contained a format error.
+${e.message}
+        ''');
+      success = false;
+      return (<Asset>[], <Uri>[], Metadata({}), false);
+      // TODO(https://github.com/dart-lang/native/issues/109): Stop throwing
+      // type errors in native_assets_cli, release a new version of that package
+      // and then remove this.
+      // ignore: avoid_catching_errors
+    } on TypeError {
+      logger.severe('''
+Building native assets for package:${config.packageName} failed.
+build_output.yaml contained a format error.
+        ''');
+      success = false;
+      return (<Asset>[], <Uri>[], Metadata({}), false);
+    } finally {
+      if (!success) {
+        final buildOutputFile =
+            File.fromUri(outDir.resolve(BuildOutput.fileName));
+        if (await buildOutputFile.exists()) {
+          await buildOutputFile.delete();
+        }
+      }
+    }
   }
 
   static Future<BuildConfig> _cliConfig({
@@ -292,33 +369,53 @@ class NativeAssetsBuildRunner {
     };
   }
 
-  void validateAssetsPackage(List<Asset> assets, String packageName) {
+  bool validateAssetsPackage(List<Asset> assets, String packageName) {
     final invalidAssetIds = assets
         .map((a) => a.name)
         .where((n) => !n.startsWith('package:$packageName/'))
         .toSet()
         .toList()
       ..sort();
-    if (invalidAssetIds.isNotEmpty) {
-      throw FormatException(
+    final success = invalidAssetIds.isEmpty;
+    if (!success) {
+      logger.severe(
         '`package:$packageName` declares the following assets which do not '
         'start with `package:$packageName/`: ${invalidAssetIds.join(', ')}.',
       );
     }
+    return success;
   }
 }
 
+typedef _PackageBuildRecord = (
+  List<Asset>,
+  List<Uri> dependencies,
+  Metadata?,
+  bool success,
+);
+
 /// The result from a [NativeAssetsBuildRunner.dryRun].
 abstract interface class DryRunResult {
   /// The native assets for all [Target]s for the build or dry run.
   List<Asset> get assets;
+
+  /// Whether all builds completed without errors.
+  ///
+  /// All error messages are streamed to [NativeAssetsBuildRunner.logger].
+  bool get success;
 }
 
 final class _DryRunResultImpl implements DryRunResult {
   @override
   final List<Asset> assets;
 
-  _DryRunResultImpl({required this.assets});
+  @override
+  final bool success;
+
+  _DryRunResultImpl({
+    required this.assets,
+    required this.success,
+  });
 }
 
 /// The result from a [NativeAssetsBuildRunner.build].
@@ -339,9 +436,13 @@ final class _BuildResultImpl implements BuildResult {
   @override
   final List<Uri> dependencies;
 
+  @override
+  final bool success;
+
   _BuildResultImpl({
     required this.assets,
     required this.dependencies,
+    required this.success,
   });
 }
 

pkgs/native_assets_builder/test/build_runner/build_planner_test.dart

@@ -43,8 +43,9 @@ void main() async {
         packageGraph: graph,
         packagesWithNativeAssets: packagesWithNativeAssets,
         dartExecutable: Uri.file(Platform.resolvedExecutable),
+        logger: logger,
       );
-      final buildPlan = planner.plan();
+      final (buildPlan, _) = planner.plan();
       expect(buildPlan.length, 1);
       expect(buildPlan.single.name, 'native_add');
     });
@@ -61,12 +62,14 @@ void main() async {
           await PackageLayout.fromRootPackageRoot(nativeAddUri);
       final packagesWithNativeAssets =
           await packageLayout.packagesWithNativeAssets;
-      final buildPlan = (await NativeAssetsBuildPlanner.fromRootPackageRoot(
+      final nativeAssetsBuildPlanner =
+          await NativeAssetsBuildPlanner.fromRootPackageRoot(
         rootPackageRoot: nativeAddUri,
         packagesWithNativeAssets: packagesWithNativeAssets,
         dartExecutable: Uri.file(Platform.resolvedExecutable),
-      ))
-          .plan();
+        logger: logger,
+      );
+      final (buildPlan, _) = nativeAssetsBuildPlanner.plan();
       expect(buildPlan.length, 1);
       expect(buildPlan.single.name, 'native_add');
     });