[native_assets_cli] Add outputDirectoryShared to config (#1557)

Author: dcharkes

.github/workflows/native.yaml

@@ -96,6 +96,9 @@ jobs:
       - run: dart pub get -C test_data/add_asset_link/
         if: ${{ matrix.package == 'native_assets_builder' }}
 
+      - run: dart pub get -C test_data/transformer/
+        if: ${{ matrix.package == 'native_assets_builder' }}
+
       - run: dart pub get -C test_data/treeshaking_native_libs/
         if: ${{ matrix.package == 'native_assets_builder' }}
 

pkgs/native_assets_builder/CHANGELOG.md

@@ -1,6 +1,7 @@
 ## 0.8.4-wip
 
-- Nothing yet.
+- Also lock `BuildConfig` and `LinkConfig` `outputDirectoryShared` when invoking
+  hooks to prevent concurrency issues with shared output caching.
 
 ## 0.8.3
 

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

@@ -6,11 +6,11 @@ import 'dart:async';
 import 'dart:io';
 
 import 'package:logging/logging.dart';
-import 'package:native_assets_cli/locking.dart';
 import 'package:native_assets_cli/native_assets_cli.dart' as api;
 import 'package:native_assets_cli/native_assets_cli_internal.dart';
 import 'package:package_config/package_config.dart';
 
+import '../locking/locking.dart';
 import '../model/build_dry_run_result.dart';
 import '../model/build_result.dart';
 import '../model/hook_result.dart';
@@ -263,13 +263,21 @@ class NativeAssetsBuildRunner {
     );
     final buildDirUri =
         packageLayout.dartToolNativeAssetsBuilder.resolve('$buildDirName/');
-    final outDirUri = buildDirUri.resolve('out/');
-    final outDir = Directory.fromUri(outDirUri);
+    final outputDirectory = buildDirUri.resolve('out/');
+    final outDir = Directory.fromUri(outputDirectory);
     if (!await outDir.exists()) {
       // TODO(https://dartbug.com/50565): Purge old or unused folders.
       await outDir.create(recursive: true);
     }
 
+    final outputDirectoryShared = packageLayout.dartToolNativeAssetsBuilder
+        .resolve('shared/${package.name}/$hook/');
+    final outDirShared = Directory.fromUri(outputDirectoryShared);
+    if (!await outDirShared.exists()) {
+      // TODO(https://dartbug.com/50565): Purge old or unused folders.
+      await outDirShared.create(recursive: true);
+    }
+
     if (hook == Hook.link) {
       File? resourcesFile;
       if (resourceIdentifiers != null) {
@@ -279,7 +287,8 @@ class NativeAssetsBuildRunner {
       }
 
       return LinkConfigImpl(
-        outputDirectory: outDirUri,
+        outputDirectory: outputDirectory,
+        outputDirectoryShared: outputDirectoryShared,
         packageName: package.name,
         packageRoot: package.root,
         targetOS: target.os,
@@ -297,7 +306,8 @@ class NativeAssetsBuildRunner {
       );
     } else {
       return BuildConfigImpl(
-        outputDirectory: outDirUri,
+        outputDirectory: outputDirectory,
+        outputDirectoryShared: outputDirectoryShared,
         packageName: package.name,
         packageRoot: package.root,
         targetOS: target.os,
@@ -428,8 +438,11 @@ class NativeAssetsBuildRunner {
         continue;
       }
       // TODO(https://github.com/dart-lang/native/issues/1321): Should dry runs be cached?
-      var (buildOutput, packageSuccess) = await runUnderDirectoryLock(
-        Directory.fromUri(config.outputDirectory.parent),
+      var (buildOutput, packageSuccess) = await runUnderDirectoriesLock(
+        [
+          Directory.fromUri(config.outputDirectoryShared.parent),
+          Directory.fromUri(config.outputDirectory.parent),
+        ],
         timeout: singleHookTimeout,
         logger: logger,
         () => _runHookForPackage(
@@ -482,8 +495,11 @@ class NativeAssetsBuildRunner {
     PackageLayout packageLayout,
   ) async {
     final outDir = config.outputDirectory;
-    return await runUnderDirectoryLock(
-      Directory.fromUri(config.outputDirectory.parent),
+    return await runUnderDirectoriesLock(
+      [
+        Directory.fromUri(config.outputDirectoryShared.parent),
+        Directory.fromUri(config.outputDirectory.parent),
+      ],
       timeout: singleHookTimeout,
       logger: logger,
       () async {
@@ -784,15 +800,27 @@ ${compileResult.stdout}
       hook: hook,
       linkingEnabled: linkingEnabled,
     );
+
     final outDirUri = buildParentDir.resolve('$buildDirName/out/');
     final outDir = Directory.fromUri(outDirUri);
     if (!await outDir.exists()) {
       await outDir.create(recursive: true);
     }
+
+    // Shared between dry run and wet run.
+    final outputDirectoryShared =
+        buildParentDir.resolve('shared/${package.name}/$hook/out/');
+    final outDirShared = Directory.fromUri(outputDirectoryShared);
+    if (!await outDirShared.exists()) {
+      // TODO(https://dartbug.com/50565): Purge old or unused folders.
+      await outDirShared.create(recursive: true);
+    }
+
     switch (hook) {
       case Hook.build:
         return BuildConfigImpl.dryRun(
           outputDirectory: outDirUri,
+          outputDirectoryShared: outputDirectoryShared,
           packageName: packageName,
           packageRoot: packageRoot,
           targetOS: targetOS,
@@ -803,6 +831,7 @@ ${compileResult.stdout}
       case Hook.link:
         return LinkConfigImpl.dryRun(
           outputDirectory: outDirUri,
+          outputDirectoryShared: outputDirectoryShared,
           packageName: packageName,
           packageRoot: packageRoot,
           targetOS: targetOS,

pkgs/native_assets_cli/lib/src/locking/locking.dart renamed to pkgs/native_assets_builder/lib/src/locking/locking.dart

@@ -7,6 +7,28 @@ import 'dart:io';
 
 import 'package:logging/logging.dart';
 
+Future<T> runUnderDirectoriesLock<T>(
+  List<Directory> directories,
+  Future<T> Function() callback, {
+  Duration? timeout,
+  Logger? logger,
+}) async {
+  if (directories.isEmpty) {
+    return await callback();
+  }
+  return await runUnderDirectoryLock(
+    directories.first,
+    () => runUnderDirectoriesLock<T>(
+      directories.skip(1).toList(),
+      callback,
+      timeout: timeout,
+      logger: logger,
+    ),
+    timeout: timeout,
+    logger: logger,
+  );
+}
+
 /// Run [callback] with this Dart process having exclusive access to
 /// [directory].
 ///

pkgs/native_assets_builder/test/build_runner/concurrency_shared_test.dart

@@ -0,0 +1,76 @@
+// 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 'dart:async';
+
+import 'package:native_assets_builder/src/utils/run_process.dart'
+    show RunProcessResult;
+import 'package:native_assets_cli/native_assets_cli_internal.dart';
+import 'package:test/test.dart';
+
+import '../helpers.dart';
+import 'helpers.dart';
+
+const Timeout longTimeout = Timeout(Duration(minutes: 5));
+
+void main() async {
+  const packageName = 'transformer';
+
+  test('Concurrent invocations', timeout: longTimeout, () async {
+    await inTempDir((tempUri) async {
+      final packageUri = tempUri.resolve('$packageName/');
+      await copyTestProjects(
+        sourceUri: testDataUri.resolve('$packageName/'),
+        targetUri: packageUri,
+      );
+
+      await runPubGet(
+        workingDirectory: packageUri,
+        logger: logger,
+      );
+
+      Future<RunProcessResult> runBuildInProcess(
+        Target target,
+      ) async {
+        final result = await runProcess(
+          executable: dartExecutable,
+          arguments: [
+            pkgNativeAssetsBuilderUri
+                .resolve(
+                  'test/build_runner/concurrency_shared_test_helper.dart',
+                )
+                .toFilePath(),
+            packageUri.toFilePath(),
+            target.toString(),
+          ],
+          workingDirectory: packageUri,
+          logger: logger,
+        );
+        expect(result.exitCode, 0);
+        return result;
+      }
+
+      // Simulate running `dart run` concurrently in 3 different terminals.
+      // Twice for the same target and also for a different target.
+      final results = await Future.wait([
+        runBuildInProcess(Target.androidArm64),
+        runBuildInProcess(Target.current),
+        runBuildInProcess(Target.current),
+      ]);
+      final stdouts = results.map((e) => e.stdout).join('\n');
+      // One run for the two targets wins in running first.
+      expect(stdouts, stringContainsInOrder(['Reused 0 cached files.']));
+      // The other run reuses the cached results.
+      expect(stdouts, stringContainsInOrder(['Reused 10 cached files.']));
+      // One of the two runs for the same target will not be invoked at all.
+      expect(
+        stdouts,
+        stringContainsInOrder([
+          'Waiting to be able to obtain lock of directory',
+          'Skipping build for',
+        ]),
+      );
+    });
+  });
+}

pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart

@@ -0,0 +1,38 @@
+// 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:logging/logging.dart';
+import 'package:native_assets_builder/native_assets_builder.dart';
+import 'package:native_assets_cli/native_assets_cli.dart';
+import 'package:native_assets_cli/native_assets_cli_internal.dart';
+
+import '../helpers.dart';
+
+// Is invoked concurrently multiple times in separate processes.
+void main(List<String> args) async {
+  final packageUri = Uri.directory(args[0]);
+  final target = Target.fromString(args[1]);
+
+  final logger = Logger('')
+    ..level = Level.ALL
+    ..onRecord.listen((event) => print(event.message));
+
+  final result = await NativeAssetsBuildRunner(
+    logger: logger,
+    dartExecutable: dartExecutable,
+  ).build(
+    buildMode: BuildModeImpl.release,
+    linkModePreference: LinkModePreferenceImpl.dynamic,
+    target: target,
+    workingDirectory: packageUri,
+    includeParentEnvironment: true,
+    linkingEnabled: false,
+    supportedAssetTypes: [DataAsset.type],
+    targetAndroidNdkApi: target.os == OS.android ? 30 : null,
+  );
+  if (!result.success) {
+    throw Error();
+  }
+  print('done');
+}

pkgs/native_assets_cli/test/locking/locking_test.dart renamed to pkgs/native_assets_builder/test/locking/locking_test.dart

@@ -14,6 +14,8 @@ import '../helpers.dart';
 const Timeout longTimeout = Timeout(Duration(minutes: 5));
 
 void main() async {
+  final packageUri = findPackageRoot('native_assets_builder');
+
   test('Concurrent invocations', timeout: longTimeout, () async {
     await inTempDir((tempUri) async {
       Future<ProcessResult> runInProcess() async {

pkgs/native_assets_cli/test/locking/locking_test_helper.dart renamed to pkgs/native_assets_builder/test/locking/locking_test_helper.dart

@@ -4,7 +4,7 @@
 
 import 'dart:io';
 
-import 'package:native_assets_cli/locking.dart';
+import 'package:native_assets_builder/src/locking/locking.dart';
 
 void main(List<String> args) async {
   final directory = Directory.fromUri(Uri.directory(args[0]));

pkgs/native_assets_builder/test/test_data/native_dynamic_linking_test.dart

@@ -24,13 +24,18 @@ void main() async {
     test(
       'native_dynamic_linking build$testSuffix',
       () => inTempDir((tempUri) async {
+        final outputDirectory = tempUri.resolve('out/');
+        await Directory.fromUri(outputDirectory).create();
+        final outputDirectoryShared = tempUri.resolve('out_shared/');
+        await Directory.fromUri(outputDirectoryShared).create();
         final testTempUri = tempUri.resolve('test1/');
         await Directory.fromUri(testTempUri).create();
         final testPackageUri = testDataUri.resolve('$name/');
         final dartUri = Uri.file(Platform.resolvedExecutable);
 
         final config = BuildConfigImpl(
-          outputDirectory: tempUri,
+          outputDirectory: outputDirectory,
+          outputDirectoryShared: outputDirectoryShared,
           packageName: name,
           packageRoot: testPackageUri,
           targetOS: OSImpl.current,
@@ -62,7 +67,7 @@ void main() async {
         }
         expect(processResult.exitCode, 0);
 
-        final buildOutputUri = tempUri.resolve('build_output.json');
+        final buildOutputUri = outputDirectory.resolve('build_output.json');
         final buildOutput = HookOutputImpl.fromJsonString(
             await File.fromUri(buildOutputUri).readAsString());
         final assets = buildOutput.assets;
@@ -100,9 +105,12 @@ void main() async {
             environment: {
               // Add the directory containing the linked dynamic libraries to
               // the PATH so that the dynamic linker can find them.
+              // TODO(https://github.com/dart-lang/sdk/issues/56551): We could
+              // skip this if Dart would implicitly add dylib containing
+              // directories to the PATH.
               if (Platform.isWindows)
-                'PATH':
-                    '${tempUri.toFilePath()};${Platform.environment['PATH']}',
+                'PATH': '${outputDirectory.toFilePath()};'
+                    '${Platform.environment['PATH']}',
             },
             throwOnUnexpectedExitCode: true,
             logger: logger,