[native_assets_*] Error on conflict dylib names (#1512)

Author: dcharkes

.github/workflows/native.yaml

@@ -63,6 +63,9 @@ jobs:
       - run: dart pub get -C test_data/native_add/
         if: ${{ matrix.package == 'native_assets_builder' }}
 
+      - run: dart pub get -C test_data/native_add_duplicate/
+        if: ${{ matrix.package == 'native_assets_builder' }}
+
       - run: dart pub get -C test_data/native_add_v1_0_0/
         if: ${{ matrix.package == 'native_assets_builder' }}
 

pkgs/native_assets_builder/CHANGELOG.md

@@ -1,6 +1,7 @@
 ## 0.8.3-wip
 
-- Added a validation step on the output of the build and link hooks.
+- Added a validation step on the output of the build and link hooks (both as a
+  per package, and as in all the packages together).
 
 ## 0.8.2
 

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

@@ -215,6 +215,16 @@ class NativeAssetsBuildRunner {
       metadata[config.packageName] = hookOutput.metadata;
     }
 
+    // Note the caller will need to check whether there are no duplicates
+    // between the build and link hook.
+    final validateResult = validateNoDuplicateDylibs(hookResult.assets);
+    if (validateResult.isNotEmpty) {
+      for (final error in validateResult) {
+        logger.severe(error);
+      }
+      hookResult = hookResult.copyAdd(HookOutputImpl(), false);
+    }
+
     return hookResult;
   }
 

pkgs/native_assets_builder/test/build_runner/conflicting_dylib_test.dart

@@ -0,0 +1,40 @@
+// Copyright (c) 2023, 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:logging/logging.dart';
+import 'package:test/test.dart';
+
+import '../helpers.dart';
+import 'helpers.dart';
+
+const Timeout longTimeout = Timeout(Duration(minutes: 5));
+
+void main() async {
+  test('conflicting dylib name', timeout: longTimeout, () async {
+    await inTempDir((tempUri) async {
+      await copyTestProjects(targetUri: tempUri);
+      final packageUri = tempUri.resolve('native_add_duplicate/');
+
+      await runPubGet(
+        workingDirectory: packageUri,
+        logger: logger,
+      );
+
+      {
+        final logMessages = <String>[];
+        final result = await build(
+          packageUri,
+          createCapturingLogger(logMessages, level: Level.SEVERE),
+          dartExecutable,
+        );
+        final fullLog = logMessages.join('\n');
+        expect(result.success, false);
+        expect(
+          fullLog,
+          contains('Duplicate dynamic library file name'),
+        );
+      }
+    });
+  });
+}

pkgs/native_assets_builder/test_data/manifest.yaml

@@ -63,6 +63,10 @@
 - native_add/src/native_add.c
 - native_add/src/native_add.h
 - native_add/test/native_add_test.dart
+- native_add_duplicate/hook/build.dart
+- native_add_duplicate/pubspec.yaml
+- native_add_duplicate/src/native_add.c
+- native_add_duplicate/src/native_add.h
 - native_subtract/ffigen.yaml
 - native_subtract/hook/build.dart
 - native_subtract/lib/native_subtract.dart

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

@@ -0,0 +1,30 @@
+// Copyright (c) 2023, 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:logging/logging.dart';
+import 'package:native_assets_cli/native_assets_cli.dart';
+import 'package:native_toolchain_c/native_toolchain_c.dart';
+
+void main(List<String> arguments) async {
+  await build(arguments, (config, output) async {
+    final packageName = config.packageName;
+    const duplicatedPackageName = 'native_add';
+    final cbuilder = CBuilder.library(
+      name: duplicatedPackageName,
+      assetName: 'src/${packageName}_bindings_generated.dart',
+      sources: [
+        'src/$duplicatedPackageName.c',
+      ],
+    );
+    await cbuilder.run(
+      config: config,
+      output: output,
+      logger: Logger('')
+        ..level = Level.ALL
+        ..onRecord.listen((record) {
+          print('${record.level.name}: ${record.time}: ${record.message}');
+        }),
+    );
+  });
+}

pkgs/native_assets_builder/test_data/native_add_duplicate/pubspec.yaml

@@ -0,0 +1,26 @@
+name: native_add_duplicate
+description: Introduces the same dylib as native_add, to introduce a conflict.
+version: 0.1.0
+
+publish_to: none
+
+environment:
+  sdk: '>=3.3.0 <4.0.0'
+
+dependencies:
+  logging: ^1.1.1
+  native_add:
+    path: ../native_add/
+  # native_assets_cli: ^0.7.3
+  native_assets_cli:
+    path: ../../../native_assets_cli/
+  # native_toolchain_c: ^0.5.3
+  native_toolchain_c:
+    path: ../../../native_toolchain_c/
+
+dev_dependencies:
+  ffigen: ^8.0.2
+  lints: ^3.0.0
+  some_dev_dep:
+    path: ../some_dev_dep/
+  test: ^1.23.1

pkgs/native_assets_builder/test_data/native_add_duplicate/src/native_add.c

@@ -0,0 +1,9 @@
+// Copyright (c) 2023, 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.
+
+#include "native_add.h"
+
+int32_t add(int32_t a, int32_t b) {
+   return a + b;
+}

pkgs/native_assets_builder/test_data/native_add_duplicate/src/native_add.h

@@ -0,0 +1,13 @@
+// Copyright (c) 2023, 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.
+
+#include <stdint.h>
+
+#if _WIN32
+#define MYLIB_EXPORT __declspec(dllexport)
+#else
+#define MYLIB_EXPORT
+#endif
+
+MYLIB_EXPORT int32_t add(int32_t a, int32_t b);

pkgs/native_assets_cli/lib/native_assets_cli_internal.dart

@@ -42,4 +42,4 @@ export 'src/model/metadata.dart';
 export 'src/model/resource_identifiers.dart';
 export 'src/model/target.dart';
 export 'src/validator/validator.dart'
-    show ValidateResult, validateBuild, validateLink;
+    show ValidateResult, validateBuild, validateLink, validateNoDuplicateDylibs;

pkgs/native_assets_cli/lib/src/validator/validator.dart

@@ -16,9 +16,6 @@ typedef ValidateResult = ({
   List<String> errors,
 });
 
-// TODO(dacoharkes): More validation in the native_assets_builder.
-// TODO(dacoharkes): NativeCodeAssets and DataAssets should have a file if not
-// dry run.
 Future<ValidateResult> validateBuild(
   BuildConfig config,
   BuildOutput output,
@@ -31,6 +28,7 @@ Future<ValidateResult> validateBuild(
     ...validateNativeCodeAssets(config, output),
     ...validateAssetId(config, output),
     if (!config.dryRun) ...validateNoDuplicateAssetIds(output),
+    ...validateNoDuplicateDylibs(output.assets),
   ];
   return (
     success: errors.isEmpty,
@@ -48,6 +46,7 @@ Future<ValidateResult> validateLink(
     if (!config.dryRun) ...await validateFilesExist(config, output),
     ...validateNativeCodeAssets(config, output),
     if (!config.dryRun) ...validateNoDuplicateAssetIds(output),
+    ...validateNoDuplicateDylibs(output.assets),
   ];
 
   return (
@@ -205,3 +204,33 @@ List<String> validateNoDuplicateAssetIds(
   }
   return errors;
 }
+
+List<String> validateNoDuplicateDylibs(
+  Iterable<Asset> assets,
+) {
+  final errors = <String>[];
+  final fileNameToAssetId = <String, Set<String>>{};
+  for (final asset in assets.whereType<NativeCodeAsset>()) {
+    if (asset.linkMode is! DynamicLoadingBundled) {
+      continue;
+    }
+    final file = asset.file;
+    if (file == null) {
+      continue;
+    }
+    final fileName = file.pathSegments.where((s) => s.isNotEmpty).last;
+    fileNameToAssetId[fileName] ??= {};
+    fileNameToAssetId[fileName]!.add(asset.id);
+  }
+  for (final fileName in fileNameToAssetId.keys) {
+    final assetIds = fileNameToAssetId[fileName]!;
+    if (assetIds.length > 1) {
+      final assetIdsString = assetIds.map((e) => '"$e"').join(', ');
+      final error =
+          'Duplicate dynamic library file name "$fileName" for the following'
+          ' asset ids: $assetIdsString.';
+      errors.add(error);
+    }
+  }
+  return errors;
+}

pkgs/native_assets_cli/test/validator/validator_test.dart

@@ -392,4 +392,47 @@ void main() {
       contains(contains('which is not in supportedAssetTypes')),
     );
   });
+
+  test('duplicate dylib name', () async {
+    final config = BuildConfig.build(
+      outputDirectory: outDirUri,
+      packageName: packageName,
+      packageRoot: tempUri,
+      targetArchitecture: Architecture.arm64,
+      targetOS: OS.iOS,
+      targetIOSSdk: IOSSdk.iPhoneOS,
+      buildMode: BuildMode.release,
+      linkModePreference: LinkModePreference.dynamic,
+      supportedAssetTypes: [NativeCodeAsset.type],
+      linkingEnabled: false,
+    );
+    final output = BuildOutput();
+    final fileName = config.targetOS.dylibFileName('foo');
+    final assetFile = File.fromUri(outDirUri.resolve(fileName));
+    await assetFile.writeAsBytes([1, 2, 3]);
+    output.addAssets([
+      NativeCodeAsset(
+        package: config.packageName,
+        name: 'src/foo.dart',
+        file: assetFile.uri,
+        linkMode: DynamicLoadingBundled(),
+        os: config.targetOS,
+        architecture: config.targetArchitecture,
+      ),
+      NativeCodeAsset(
+        package: config.packageName,
+        name: 'src/bar.dart',
+        file: assetFile.uri,
+        linkMode: DynamicLoadingBundled(),
+        os: config.targetOS,
+        architecture: config.targetArchitecture,
+      ),
+    ]);
+    final result = await validateBuild(config, output);
+    expect(result.success, isFalse);
+    expect(
+      result.errors,
+      contains(contains('Duplicate dynamic library file name')),
+    );
+  });
 }