Changes as per review

pkgs/native_assets_builder/CHANGELOG.md

@@ -1,7 +1,7 @@
 ## 0.7.0-wip
 
 - Fix test.
-- Add support for `link.dart`.
+- Add support for `hook/link.dart`.
 
 ## 0.6.0
 

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

@@ -83,7 +83,9 @@ class NativeAssetsBuildPlanner {
   }
 }
 
-/// A graph of package dependencies, encoded as package name -> list of package
+/// A graph of package dependencies.
+///
+/// This is encoded as a mapping from package name to list of package
 /// dependencies.
 class PackageGraph {
   final Map<String, List<String>> map;

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

@@ -11,7 +11,7 @@ import 'package:package_config/package_config.dart';
 
 import '../model/build_result.dart';
 import '../model/dry_run_result.dart';
-import '../model/forge_result.dart';
+import '../model/hook_result.dart';
 import '../model/link_result.dart';
 import '../package_layout/package_layout.dart';
 import '../utils/run_process.dart';
@@ -125,9 +125,12 @@ class NativeAssetsBuildRunner {
     assert(hook == Hook.link || buildResult == null);
 
     packageLayout ??= await PackageLayout.fromRootPackageRoot(workingDirectory);
-    final packagesWithBuild = await packageLayout.packagesWithAssets(hook);
+    final packagesWithAssets = await packageLayout.packagesWithAssets(hook);
     final (buildPlan, packageGraph, planSuccess) = await _plannedPackages(
-        packagesWithBuild, packageLayout, runPackageName);
+      packagesWithAssets,
+      packageLayout,
+      runPackageName,
+    );
     final hookResult = HookResult.failure();
     if (!planSuccess) {
       return hookResult;
@@ -140,33 +143,23 @@ class NativeAssetsBuildRunner {
         packageName: package.name,
         targetMetadata: metadata,
       );
-      final buildConfig = await _cliConfig(
-        packageName: package.name,
-        packageRoot: packageLayout.packageRoot(package.name),
-        target: target,
-        buildMode: buildMode,
-        linkMode: linkModePreference,
-        buildParentDir: packageLayout.dartToolNativeAssetsBuilder,
-        dependencyMetadata: dependencyMetadata,
-        cCompilerConfig: cCompilerConfig,
-        targetIOSSdk: targetIOSSdk,
-        targetAndroidNdkApi: targetAndroidNdkApi,
-        supportedAssetTypes: supportedAssetTypes,
-        hook: hook,
+      final config = await _cliConfig(
+        package,
+        packageLayout,
+        target,
+        buildMode,
+        linkModePreference,
+        dependencyMetadata,
+        cCompilerConfig,
+        targetIOSSdk,
+        targetAndroidNdkApi,
+        supportedAssetTypes,
+        hook,
+        resourceIdentifiers,
+        buildResult,
       );
-      final HookConfigImpl config;
-      if (hook == Hook.link) {
-        config = LinkConfigImpl.fromValues(
-          resourceIdentifierUri: resourceIdentifiers,
-          buildConfig: buildConfig,
-          assetsForLinking:
-              buildResult!.assetsForLinking[buildConfig.packageName] ?? [],
-        );
-      } else {
-        config = buildConfig;
-      }
 
-      final (buildOutput, packageSuccess) = await _buildPackageCached(
+      final (buildOutput, packageSuccess) = await _runHookForPackageCached(
         hook,
         config,
         packageLayout.packageConfigUri,
@@ -183,6 +176,47 @@ class NativeAssetsBuildRunner {
     return hookResult.withSuccess(success);
   }
 
+  Future<HookConfigImpl> _cliConfig(
+    Package package,
+    PackageLayout packageLayout,
+    Target target,
+    BuildModeImpl buildMode,
+    LinkModePreferenceImpl linkModePreference,
+    DependencyMetadata? dependencyMetadata,
+    CCompilerConfigImpl? cCompilerConfig,
+    IOSSdkImpl? targetIOSSdk,
+    int? targetAndroidNdkApi,
+    Iterable<String>? supportedAssetTypes,
+    Hook hook,
+    Uri? resourceIdentifiers,
+    BuildResult? buildResult,
+  ) async {
+    final buildConfig = await _buildConfig(
+      packageName: package.name,
+      packageRoot: packageLayout.packageRoot(package.name),
+      target: target,
+      buildMode: buildMode,
+      linkMode: linkModePreference,
+      buildParentDir: packageLayout.dartToolNativeAssetsBuilder,
+      dependencyMetadata: dependencyMetadata,
+      cCompilerConfig: cCompilerConfig,
+      targetIOSSdk: targetIOSSdk,
+      targetAndroidNdkApi: targetAndroidNdkApi,
+      supportedAssetTypes: supportedAssetTypes,
+      hook: hook,
+    );
+    if (hook == Hook.link) {
+      return LinkConfigImpl.fromValues(
+        resourceIdentifierUri: resourceIdentifiers,
+        buildConfig: buildConfig,
+        assetsForLinking:
+            buildResult!.assetsForLinking[buildConfig.packageName] ?? [],
+      );
+    } else {
+      return buildConfig;
+    }
+  }
+
   /// [workingDirectory] is expected to contain `.dart_tool`.
   ///
   /// This method is invoked by launchers such as dartdev (for `dart run`) and
@@ -207,9 +241,9 @@ class NativeAssetsBuildRunner {
       packageLayout,
       runPackageName,
     );
-    final buildResult = HookResult.failure();
+    final hookResult = HookResult.failure();
     if (!planSuccess) {
-      return buildResult;
+      return hookResult;
     }
     var success = true;
     for (final package in buildPlan) {
@@ -221,7 +255,7 @@ class NativeAssetsBuildRunner {
         buildParentDir: packageLayout.dartToolNativeAssetsBuilder,
         supportedAssetTypes: supportedAssetTypes,
       );
-      final (buildOutput, packageSuccess) = await _buildPackage(
+      final (buildOutput, packageSuccess) = await _runHookForPackage(
         Hook.build,
         config,
         packageLayout.packageConfigUri,
@@ -234,27 +268,27 @@ class NativeAssetsBuildRunner {
           case NativeCodeAssetImpl _:
             if (asset.architecture != null) {
               // Backwards compatibility, if an architecture is provided use it.
-              buildResult.assets.add(asset);
+              hookResult.assets.add(asset);
             } else {
               // Dry run does not report architecture. Dart VM branches on OS
               // and Target when looking up assets, so populate assets for all
               // architectures.
               for (final architecture in asset.os.architectures) {
-                buildResult.assets
+                hookResult.assets
                     .add(asset.copyWith(architecture: architecture));
               }
             }
           case DataAssetImpl _:
-            buildResult.assets.add(asset);
+            hookResult.assets.add(asset);
         }
       }
       success &= packageSuccess;
     }
-    return buildResult.withSuccess(success);
+    return hookResult.withSuccess(success);
   }
 
-  Future<_PackageBuildRecord> _buildPackageCached(
-    Hook step,
+  Future<_PackageBuildRecord> _runHookForPackageCached(
+    Hook hook,
     HookConfigImpl config,
     Uri packageConfigUri,
     Uri workingDirectory,
@@ -266,22 +300,23 @@ class NativeAssetsBuildRunner {
       await Directory.fromUri(outDir).create(recursive: true);
     }
 
-    final buildOutput = HookOutputImpl.readFromFile(file: config.outputFile);
-    if (buildOutput != null) {
-      final lastBuilt = buildOutput.timestamp.roundDownToSeconds();
-      final lastChange = await buildOutput.dependenciesModel.lastModified();
+    final hookOutput = HookOutputImpl.readFromFile(file: config.outputFile);
+    if (hookOutput != null) {
+      final lastBuilt = hookOutput.timestamp.roundDownToSeconds();
+      final lastChange = await hookOutput.dependenciesModel.lastModified();
 
       if (lastBuilt.isAfter(lastChange)) {
-        logger.info('Skipping build for ${config.packageName} in $outDir. '
-            'Last build on $lastBuilt, last input change on $lastChange.');
+        logger
+            .info('Skipping ${hook.name} for ${config.packageName} in $outDir. '
+                'Last build on $lastBuilt, last input change on $lastChange.');
         // All build flags go into [outDir]. Therefore we do not have to check
         // here whether the config is equal.
-        return (buildOutput, true);
+        return (hookOutput, true);
       }
     }
 
-    return await _buildPackage(
-      step,
+    return await _runHookForPackage(
+      hook,
       config,
       packageConfigUri,
       workingDirectory,
@@ -290,7 +325,7 @@ class NativeAssetsBuildRunner {
     );
   }
 
-  Future<_PackageBuildRecord> _buildPackage(
+  Future<_PackageBuildRecord> _runHookForPackage(
     Hook hook,
     HookConfigImpl config,
     Uri packageConfigUri,
@@ -389,7 +424,7 @@ Contents: ${File.fromUri(config.outputFile).readAsStringSync()}.
     }
   }
 
-  static Future<BuildConfigImpl> _cliConfig({
+  static Future<BuildConfigImpl> _buildConfig({
     required String packageName,
     required Uri packageRoot,
     required Target target,

pkgs/native_assets_builder/lib/src/model/forge_result.dart → pkgs/native_assets_builder/lib/src/model/hook_result.dart

@@ -43,10 +43,11 @@ final class HookResult implements BuildResult, DryRunResult, LinkResult {
       assetsForLinking,
       buildOutput.assetsForLinking,
       value: (assets1, assets2) {
-        if (assets1.any((asset) => assets2.contains(asset)) ||
-            assets2.any((asset) => assets1.contains(asset))) {
+        final twoInOne = assets1.where((asset) => assets2.contains(asset));
+        final oneInTwo = assets2.where((asset) => assets1.contains(asset));
+        if (twoInOne.isNotEmpty || oneInTwo.isNotEmpty) {
           throw ArgumentError(
-              'Found assets with same ID in $assets1 and $assets2');
+              'Found assets with same IDs, ${[...oneInTwo, ...twoInOne]}');
         }
         return [
           ...assets1,

pkgs/native_assets_builder/lib/src/package_layout/package_layout.dart

@@ -95,15 +95,14 @@ class PackageLayout {
   // the protocol version pre 1.2.0 on some future version.)
   Future<List<Package>> packagesWithAssets(Hook hook) async => switch (hook) {
         Hook.build => _packagesWithBuildAssets ??=
-            await _packagesWithAssets(hook),
-        Hook.link => _packagesWithLinkAssets ??=
-            await _packagesWithAssets(hook),
+            await _packagesWithHook(hook),
+        Hook.link => _packagesWithLinkAssets ??= await _packagesWithHook(hook),
       };
 
   List<Package>? _packagesWithBuildAssets;
   List<Package>? _packagesWithLinkAssets;
 
-  Future<List<Package>> _packagesWithAssets(Hook hook) async {
+  Future<List<Package>> _packagesWithHook(Hook hook) async {
     final result = <Package>[];
     for (final package in packageConfig.packages) {
       final packageRoot = package.root;

pkgs/native_assets_builder/test/build_runner/link_test.dart

@@ -1,7 +1,9 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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 'package:native_assets_cli/native_assets_cli.dart' as cli;
+import 'package:native_assets_cli/src/api/asset.dart';
 import 'package:test/test.dart';
 
 import '../helpers.dart';
@@ -47,22 +49,34 @@ void main() async {
     timeout: longTimeout,
     () async {
       await inTempDir((tempUri) async {
+        // From package:complex_link_helper
+        const builtHelperAssets = ['data_helper_0', 'data_helper_1'];
+        const helperAssetsForLinking = ['data_helper_2', 'data_helper_3'];
+        // From package:complex_link
+        const mainAssetsForLinking = ['data_0', 'data_1'];
+        final assetsForLinking = [
+          ...helperAssetsForLinking,
+          ...mainAssetsForLinking,
+        ];
+        final linkedAssets = assetsForLinking.skip(1);
+
         await copyTestProjects(targetUri: tempUri);
         final packageUri = tempUri.resolve('complex_link/');
 
         // First, run `pub get`, we need pub to resolve our dependencies.
-        await runPubGet(
-          workingDirectory: packageUri,
-          logger: logger,
-        );
+        await runPubGet(workingDirectory: packageUri, logger: logger);
 
         final buildResult = await build(
           packageUri,
           logger,
           dartExecutable,
         );
         expect(buildResult.success, true);
-        expect(buildResult.assets.length, 2);
+        expect(_getNames(buildResult.assets), orderedEquals(builtHelperAssets));
+        expect(
+          _getNames(buildResult.assetsForLinking['complex_link']!),
+          orderedEquals(assetsForLinking),
+        );
 
         final linkResult = await link(
           packageUri,
@@ -71,9 +85,12 @@ void main() async {
           buildResult: buildResult,
         );
         expect(linkResult.success, true);
-        const assetsLeft = 2 + 2 - 1;
-        expect(linkResult.assets.length, assetsLeft);
+
+        expect(_getNames(linkResult.assets), orderedEquals(linkedAssets));
       });
     },
   );
 }
+
+Iterable<String> _getNames(List<AssetImpl> assets) =>
+    assets.whereType<cli.DataAsset>().map((asset) => asset.name);

pkgs/native_assets_builder/test_data/complex_link/hook/build.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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.
 

pkgs/native_assets_builder/test_data/complex_link/hook/link.dart

@@ -7,6 +7,9 @@ import 'package:native_assets_cli/native_assets_cli.dart';
 void main(List<String> args) async {
   await link(
     args,
-    (config, output) async => output.addAssets(config.assets.skip(1)),
+    (config, output) async => output.addAssets(treeshake(config.assets)),
   );
 }
+
+Iterable<Asset> treeshake(List<Asset> assets) =>
+    assets.where((asset) => !asset.id.endsWith('data_helper_2'));

pkgs/native_assets_builder/test_data/complex_link_helper/hook/build.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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.
 

pkgs/native_assets_builder/test_data/complex_link_helper/lib/complex_link_helper.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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.
 

pkgs/native_assets_builder/test_data/simple_link/bin/simple_link.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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.
 

pkgs/native_assets_builder/test_data/simple_link/hook/build.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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.
 

pkgs/native_assets_builder/test_data/simple_link/hook/link.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
 // 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.