Address moritz comments

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

@@ -47,7 +47,7 @@ class NativeAssetsBuildPlanner {
     );
   }
 
-  (List<Package> packages, bool success) plan({
+  List<Package>? plan({
     String? runPackageName,
   }) {
     final PackageGraph packageGraph;
@@ -62,7 +62,6 @@ class NativeAssetsBuildPlanner {
     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)
@@ -73,13 +72,13 @@ class NativeAssetsBuildPlanner {
           'Cyclic dependency for native asset builds in the following '
           'packages: $stronglyConnectedComponentWithNativeAssets.',
         );
-        success = false;
+        return null;
       } else if (stronglyConnectedComponentWithNativeAssets.length == 1) {
         result.add(
             packageMap[stronglyConnectedComponentWithNativeAssets.single]!);
       }
     }
-    return (result, success);
+    return result;
   }
 }
 

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

@@ -83,6 +83,9 @@ class NativeAssetsBuildRunner {
   /// If provided, only assets of all transitive dependencies of
   /// [runPackageName] are built.
   ///
+  /// The given [applicationAssetValidator] is only used if the build is
+  /// performed without linking (i.e. [linkingEnabled] is `false`).
+  ///
   /// The native assets build runner does not support reentrancy for identical
   /// [BuildConfig] and [LinkConfig]! For more info see:
   /// https://github.com/dart-lang/native/issues/1319
@@ -102,13 +105,13 @@ class NativeAssetsBuildRunner {
   }) async {
     packageLayout ??= await PackageLayout.fromRootPackageRoot(workingDirectory);
 
-    final (buildPlan, packageGraph, planSuccess) = await _makePlan(
+    final (buildPlan, packageGraph) = await _makePlan(
       hook: Hook.build,
       packageLayout: packageLayout,
       buildResult: null,
       runPackageName: runPackageName,
     );
-    if (!planSuccess) return null;
+    if (buildPlan == null) return null;
 
     var hookResult = HookResult();
     final globalMetadata = <String, Metadata>{};
@@ -134,8 +137,12 @@ class NativeAssetsBuildRunner {
           metadata: metadata,
         );
 
-      final (buildDirUri, outDirUri, outDirSharedUri) = await _setupDiretories(
-          Hook.build, packageLayout, configBuilder, package);
+      final (buildDirUri, outDirUri, outDirSharedUri) = await _setupDirectories(
+        Hook.build,
+        packageLayout,
+        configBuilder,
+        package,
+      );
 
       configBuilder.setupBuildRunConfig(
         outputDirectory: outDirUri,
@@ -176,10 +183,7 @@ class NativeAssetsBuildRunner {
     final errors = await applicationAssetValidator(hookResult.encodedAssets);
     if (errors.isEmpty) return hookResult;
 
-    logger.severe('Application asset verification failed:');
-    for (final error in errors) {
-      logger.severe('- $error');
-    }
+    _printErrors('Application asset verification failed', errors);
     return null;
   }
 
@@ -211,13 +215,13 @@ class NativeAssetsBuildRunner {
   }) async {
     packageLayout ??= await PackageLayout.fromRootPackageRoot(workingDirectory);
 
-    final (buildPlan, packageGraph, planSuccess) = await _makePlan(
+    final (buildPlan, packageGraph) = await _makePlan(
       hook: Hook.link,
       packageLayout: packageLayout,
       buildResult: buildResult,
       runPackageName: runPackageName,
     );
-    if (!planSuccess) return null;
+    if (buildPlan == null) return null;
 
     var hookResult = HookResult(encodedAssets: buildResult.encodedAssets);
     for (final package in buildPlan) {
@@ -232,8 +236,8 @@ class NativeAssetsBuildRunner {
         ..setupLinkConfig(
             assets: buildResult.encodedAssetsForLinking[package.name] ?? []);
 
-      final (buildDirUri, outDirUri, outDirSharedUri) = await _setupDiretories(
-          Hook.build, packageLayout, configBuilder, package);
+      final (buildDirUri, outDirUri, outDirSharedUri) = await _setupDirectories(
+          Hook.link, packageLayout, configBuilder, package);
 
       File? resourcesFile;
       if (resourceIdentifiers != null) {
@@ -288,7 +292,7 @@ class NativeAssetsBuildRunner {
     return null;
   }
 
-  Future<(Uri, Uri, Uri)> _setupDiretories(
+  Future<(Uri, Uri, Uri)> _setupDirectories(
       Hook hook,
       PackageLayout packageLayout,
       HookConfigBuilder configBuilder,
@@ -357,12 +361,12 @@ class NativeAssetsBuildRunner {
     const hook = Hook.build;
 
     packageLayout ??= await PackageLayout.fromRootPackageRoot(workingDirectory);
-    final (buildPlan, _, planSuccess) = await _makePlan(
+    final (buildPlan, _) = await _makePlan(
       hook: hook,
       packageLayout: packageLayout,
       runPackageName: runPackageName,
     );
-    if (!planSuccess) {
+    if (buildPlan == null) {
       return null;
     }
 
@@ -403,10 +407,7 @@ class NativeAssetsBuildRunner {
         outputDirectoryShared: outputDirectoryShared,
       );
 
-      final config = hook == Hook.build
-          ? BuildConfig(configBuilder.json)
-          : LinkConfig(configBuilder.json);
-
+      final config = BuildConfig(configBuilder.json);
       final packageConfigUri = packageLayout.packageConfigUri;
       final (
         compileSuccess,
@@ -488,13 +489,7 @@ class NativeAssetsBuildRunner {
         if (buildOutputFile.existsSync()) {
           late final HookOutput output;
           try {
-            final decode =
-                const Utf8Decoder().fuse(const JsonDecoder()).convert;
-            final hookOutputJson = decode(buildOutputFile.readAsBytesSync())
-                as Map<String, Object?>;
-            output = hook == Hook.build
-                ? BuildOutput(hookOutputJson)
-                : LinkOutput(hookOutputJson);
+            output = _readHookOutputFromUri(hook, buildOutputFile);
           } on FormatException catch (e) {
             logger.severe('''
 Building assets for package:${config.packageName} failed.
@@ -556,11 +551,11 @@ ${e.message}
         const JsonEncoder.withIndent(' ').convert(config.json);
     logger.info('config.json contents: $configFileContents');
     await File.fromUri(configFile).writeAsString(configFileContents);
-    final hookOutputFile = config.outputDirectory.resolve(hook.outputName);
-    final buildOutputFile = File.fromUri(hookOutputFile);
-    if (await buildOutputFile.exists()) {
+    final hookOutputUri = config.outputDirectory.resolve(hook.outputName);
+    final hookOutputFile = File.fromUri(hookOutputUri);
+    if (await hookOutputFile.exists()) {
       // Ensure we'll never read outdated build results.
-      await buildOutputFile.delete();
+      await hookOutputFile.delete();
     }
 
     final arguments = [
@@ -603,14 +598,7 @@ ${result.stdout}
     }
 
     try {
-      final decode = const Utf8Decoder().fuse(const JsonDecoder()).convert;
-
-      final hookOutputJson =
-          decode(buildOutputFile.readAsBytesSync()) as Map<String, Object?>;
-      final output = hook == Hook.build
-          ? BuildOutput(hookOutputJson)
-          : LinkOutput(hookOutputJson);
-
+      final output = _readHookOutputFromUri(hook, hookOutputFile);
       final errors = await _validate(config, output, packageLayout, validator);
       if (errors.isNotEmpty) {
         _printErrors(
@@ -625,14 +613,14 @@ ${result.stdout}
 Building assets for package:${config.packageName} failed.
 ${hook.outputName} contained a format error.
 
-Contents: ${buildOutputFile.readAsStringSync()}.
+Contents: ${hookOutputFile.readAsStringSync()}.
 ${e.message}
         ''');
       return null;
     } finally {
       if (deleteOutputIfExists) {
-        if (await buildOutputFile.exists()) {
-          await buildOutputFile.delete();
+        if (await hookOutputFile.exists()) {
+          await hookOutputFile.delete();
         }
       }
     }
@@ -787,12 +775,11 @@ ${compileResult.stdout}
     errors.addAll(await validator(config, output));
 
     if (config is BuildConfig) {
-      // XXX
-      output as BuildOutput;
       final packagesWithLink =
           (await packageLayout.packagesWithAssets(Hook.link))
               .map((p) => p.name);
-      for (final targetPackage in output.encodedAssetsForLinking.keys) {
+      for (final targetPackage
+          in (output as BuildOutput).encodedAssetsForLinking.keys) {
         if (!packagesWithLink.contains(targetPackage)) {
           for (final asset in output.encodedAssetsForLinking[targetPackage]!) {
             errors.add(
@@ -806,8 +793,7 @@ ${compileResult.stdout}
     return errors;
   }
 
-  Future<(List<Package> plan, PackageGraph? dependencyGraph, bool success)>
-      _makePlan({
+  Future<(List<Package>? plan, PackageGraph? dependencyGraph)> _makePlan({
     required PackageLayout packageLayout,
     String? runPackageName,
     required Hook hook,
@@ -824,18 +810,18 @@ ${compileResult.stdout}
           final dependencyGraph = PackageGraph({
             for (final p in packagesWithHook) p.name: [],
           });
-          return (packagesWithHook, dependencyGraph, true);
+          return (packagesWithHook, dependencyGraph);
         } else {
           final planner = await NativeAssetsBuildPlanner.fromRootPackageRoot(
             rootPackageRoot: packageLayout.rootPackageRoot,
             packagesWithNativeAssets: packagesWithHook,
             dartExecutable: Uri.file(Platform.resolvedExecutable),
             logger: logger,
           );
-          final (plan, planSuccess) = planner.plan(
+          final plan = planner.plan(
             runPackageName: runPackageName,
           );
-          return (plan, planner.packageGraph, planSuccess);
+          return (plan, planner.packageGraph);
         }
       case Hook.link:
         // Link hooks are not run in any particular order.
@@ -858,7 +844,16 @@ ${compileResult.stdout}
         }
         packageGraph = null;
     }
-    return (buildPlan, packageGraph, true);
+    return (buildPlan, packageGraph);
+  }
+
+  HookOutput _readHookOutputFromUri(Hook hook, File hookOutputFile) {
+    final decode = const Utf8Decoder().fuse(const JsonDecoder()).convert;
+    final hookOutputJson =
+        decode(hookOutputFile.readAsBytesSync()) as Map<String, Object?>;
+    return hook == Hook.build
+        ? BuildOutput(hookOutputJson)
+        : LinkOutput(hookOutputJson);
   }
 }
 

pkgs/native_assets_builder/test/build_runner/build_planner_test.dart

@@ -45,8 +45,8 @@ void main() async {
         dartExecutable: Uri.file(Platform.resolvedExecutable),
         logger: logger,
       );
-      final (buildPlan, _) = planner.plan();
-      expect(buildPlan.length, 1);
+      final buildPlan = planner.plan();
+      expect(buildPlan!.length, 1);
       expect(buildPlan.single.name, 'native_add');
     });
   });
@@ -70,8 +70,8 @@ void main() async {
         dartExecutable: Uri.file(Platform.resolvedExecutable),
         logger: logger,
       );
-      final (buildPlan, _) = nativeAssetsBuildPlanner.plan();
-      expect(buildPlan.length, 1);
+      final buildPlan = nativeAssetsBuildPlanner.plan();
+      expect(buildPlan!.length, 1);
       expect(buildPlan.single.name, 'native_add');
     });
   });
@@ -97,10 +97,10 @@ void main() async {
           dartExecutable: Uri.file(Platform.resolvedExecutable),
           logger: logger,
         );
-        final (buildPlan, _) = nativeAssetsBuildPlanner.plan(
+        final buildPlan = nativeAssetsBuildPlanner.plan(
           runPackageName: runPackageName,
         );
-        expect(buildPlan.length, 0);
+        expect(buildPlan!.length, 0);
       });
     });
   }

pkgs/native_assets_cli/lib/src/model/hook.dart

@@ -16,8 +16,5 @@ enum Hook {
 
   String get scriptName => '$_scriptName.dart';
 
-  String get outputName => switch (this) {
-        Hook.build => 'build_output.json',
-        Hook.link => 'link_output.json'
-      };
+  String get outputName => '${_scriptName}_output.json';
 }

pkgs/native_assets_cli/test/checksum_test.dart

@@ -2,13 +2,16 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:convert';
+
 import 'package:native_assets_cli/code_assets_builder.dart';
 import 'package:native_assets_cli/data_assets_builder.dart';
 import 'package:test/test.dart';
 
 void main() {
   test('checksum', () async {
     // metadata, cc, link vs build, metadata, haslink
+    final configs = <String>[];
     final checksums = <String>[];
     for (final os in [OS.linux, OS.macOS]) {
       for (final buildMode in [BuildMode.release, BuildMode.debug]) {
@@ -25,6 +28,8 @@ void main() {
                     buildMode: buildMode,
                   )
                   ..setupBuildConfig(dryRun: dryRun, linkingEnabled: linking);
+                configs.add(
+                    const JsonEncoder.withIndent(' ').convert(builder.json));
                 checksums.add(builder.computeChecksum());
               }
             }
@@ -38,7 +43,7 @@ void main() {
 
     // If format or algorithm for checksumming changes we'd like to know (by
     // needing to update this list).
-    expect(checksums, <String>[
+    final expectedChecksums = <String>[
       '05cdbdf4976a68c33e75e6b57781c5f5',
       'c36ad7dc2f0846ed134029edaeb59195',
       '7f90e825f08edafe99ac7314d02f46e0',
@@ -103,6 +108,13 @@ void main() {
       'd1fd0e95194d8d4bc513666e2067548c',
       '0541de11331a9ca647f7cbde69c1abf4',
       'c8c85515946c890e3056f379ca757cfe',
-    ]);
+    ];
+    for (var i = 0; i < checksums.length; ++i) {
+      if (checksums[i] != expectedChecksums[i]) {
+        print('Expected ${expectedChecksums[i]} but was ${checksums[i]}');
+        print('Config:\n${configs[i]}');
+      }
+      expect(checksums[i], expectedChecksums[i]);
+    }
   });
 }