[native_assets_builder] Report dart sources as dependencies of hooks

Author: dcharkes

.github/workflows/native.yaml

@@ -7,7 +7,7 @@ permissions: read-all
 
 on:
   pull_request:
-    branches: [main]
+    # No `branches:` to enable stacked PRs on GitHub.
     paths:
       - ".github/workflows/native.yaml"
       - "pkgs/native_assets_builder/**"

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

@@ -157,7 +157,7 @@ class NativeAssetsBuildRunner {
             'Build configuration for ${package.name} contains errors', errors);
       }
 
-      final hookOutput = await _runHookForPackageCached(
+      final (hookOutput, hookDeps) = await _runHookForPackageCached(
         Hook.build,
         config,
         (config, output) =>
@@ -168,7 +168,7 @@ class NativeAssetsBuildRunner {
         packageLayout,
       );
       if (hookOutput == null) return null;
-      hookResult = hookResult.copyAdd(hookOutput);
+      hookResult = hookResult.copyAdd(hookOutput, hookDeps!);
       globalMetadata[package.name] = (hookOutput as BuildOutput).metadata;
     }
 
@@ -259,7 +259,7 @@ class NativeAssetsBuildRunner {
             'Link configuration for ${package.name} contains errors', errors);
       }
 
-      final hookOutput = await _runHookForPackageCached(
+      final (hookOutput, hookDeps) = await _runHookForPackageCached(
         Hook.link,
         config,
         (config, output) =>
@@ -270,7 +270,7 @@ class NativeAssetsBuildRunner {
         packageLayout,
       );
       if (hookOutput == null) return null;
-      hookResult = hookResult.copyAdd(hookOutput);
+      hookResult = hookResult.copyAdd(hookOutput, hookDeps!);
     }
 
     final errors = await applicationAssetValidator(hookResult.encodedAssets);
@@ -437,12 +437,12 @@ class NativeAssetsBuildRunner {
         ),
       );
       if (buildOutput == null) return null;
-      hookResult = hookResult.copyAdd(buildOutput);
+      hookResult = hookResult.copyAdd(buildOutput, [/*dry run is not cached*/]);
     }
     return hookResult;
   }
 
-  Future<HookOutput?> _runHookForPackageCached(
+  Future<(HookOutput?, List<Uri>?)> _runHookForPackageCached(
     Hook hook,
     HookConfig config,
     _HookValidator validator,
@@ -464,7 +464,7 @@ class NativeAssetsBuildRunner {
         final (
           compileSuccess,
           hookKernelFile,
-          hookHashesFile,
+          hookHashes,
         ) = await _compileHookForPackageCached(
           config.packageName,
           config.outputDirectory,
@@ -473,7 +473,7 @@ class NativeAssetsBuildRunner {
           workingDirectory,
         );
         if (!compileSuccess) {
-          return null;
+          return (null, null);
         }
 
         final buildOutputFile =
@@ -497,7 +497,7 @@ ${hook.outputName} contained a format error.
 Contents: ${buildOutputFile.readAsStringSync()}.
 ${e.message}
         ''');
-            return null;
+            return (null, null);
           }
 
           final outdatedDependency =
@@ -510,7 +510,7 @@ ${e.message}
             );
             // All build flags go into [outDir]. Therefore we do not have to
             // check here whether the config is equal.
-            return output;
+            return (output, hookHashes.fileSystemEntities);
           }
           logger.info(
             'Rerunning ${hook.name} for ${config.packageName}'
@@ -538,7 +538,7 @@ ${e.message}
             [
               ...result.dependencies,
               // Also depend on the hook source code.
-              hookHashesFile.uri,
+              hookHashes.file.uri,
             ],
             lastModifiedCutoffTime,
             environment,
@@ -547,7 +547,7 @@ ${e.message}
             logger.severe('File modified during build. Build must be rerun.');
           }
         }
-        return result;
+        return (result, hookHashes.fileSystemEntities);
       },
     );
   }
@@ -685,7 +685,7 @@ ${e.message}
   ///
   /// TODO(https://github.com/dart-lang/native/issues/1578): Compile only once
   /// instead of per config. This requires more locking.
-  Future<(bool success, File kernelFile, File cacheFile)>
+  Future<(bool success, File kernelFile, DependenciesHashFile cacheFile)>
       _compileHookForPackageCached(
     String packageName,
     Uri outputDirectory,
@@ -721,7 +721,7 @@ ${e.message}
     }
 
     if (!mustCompile) {
-      return (true, kernelFile, dependenciesHashFile);
+      return (true, kernelFile, dependenciesHashes);
     }
 
     final success = await _compileHookForPackage(
@@ -734,7 +734,7 @@ ${e.message}
     );
     if (!success) {
       await dependenciesHashFile.delete();
-      return (success, kernelFile, dependenciesHashFile);
+      return (success, kernelFile, dependenciesHashes);
     }
 
     final dartSources = await _readDepFile(depFile);
@@ -751,19 +751,7 @@ ${e.message}
       logger.severe('File modified during build. Build must be rerun.');
     }
 
-    return (success, kernelFile, dependenciesHashFile);
-  }
-
-  Future<List<Uri>> _readDepFile(File depFile) async {
-    // Format: `path/to/my.dill: path/to/my.dart, path/to/more.dart`
-    final depFileContents = await depFile.readAsString();
-    final dartSources = depFileContents
-        .trim()
-        .split(' ')
-        .skip(1) // '<kernel file>:'
-        .map(Uri.file)
-        .toList();
-    return dartSources;
+    return (success, kernelFile, dependenciesHashes);
   }
 
   Future<bool> _compileHookForPackage(
@@ -927,3 +915,42 @@ ${compileResult.stdout}
 extension on Uri {
   Uri get parent => File(toFilePath()).parent.uri;
 }
+
+/// Parses depfile contents.
+///
+/// Format: `path/to/my.dill: path/to/my.dart, path/to/more.dart`
+///
+/// However, the spaces in paths are escaped with backslashes, and the
+/// backslashes are escaped with backslashes:
+///
+/// ```dart
+/// String _escapePath(String path) {
+///   return path.replaceAll('\\', '\\\\').replaceAll(' ', '\\ ');
+/// }
+/// ```
+List<String> parseDepFileInputs(String contents) {
+  final output = contents.split(': ').first;
+  contents = contents.substring(output.length + ': '.length).trim();
+  final inputs = <String>[];
+  var currentWord = '';
+  var escape = false;
+  for (var i = 0; i < contents.length; i++) {
+    if (contents[i] == r'\' && !escape) {
+      escape = true;
+    } else if (contents[i] == ' ' && !escape) {
+      inputs.add(currentWord);
+      currentWord = '';
+    } else {
+      currentWord += contents[i];
+      escape = false;
+    }
+  }
+  inputs.add(currentWord);
+  return inputs;
+}
+
+Future<List<Uri>> _readDepFile(File depFile) async {
+  final depFileContents = await depFile.readAsString();
+  final dartSources = parseDepFileInputs(depFileContents);
+  return dartSources.map(Uri.file).toList();
+}

pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart

@@ -13,19 +13,21 @@ import '../utils/uri.dart';
 
 class DependenciesHashFile {
   DependenciesHashFile({
-    required File file,
-  }) : _file = file;
+    required this.file,
+  });
 
-  final File _file;
+  final File file;
   FileSystemHashes _hashes = FileSystemHashes();
 
+  List<Uri> get fileSystemEntities => _hashes.files.map((e) => e.path).toList();
+
   Future<void> _readFile() async {
-    if (!await _file.exists()) {
+    if (!await file.exists()) {
       _hashes = FileSystemHashes();
       return;
     }
     final jsonObject =
-        (json.decode(utf8.decode(await _file.readAsBytes())) as Map)
+        (json.decode(utf8.decode(await file.readAsBytes())) as Map)
             .cast<String, Object>();
     _hashes = FileSystemHashes.fromJson(jsonObject);
   }
@@ -70,7 +72,7 @@ class DependenciesHashFile {
     return modifiedAfterTimeStamp;
   }
 
-  Future<void> _persist() => _file.writeAsString(json.encode(_hashes.toJson()));
+  Future<void> _persist() => file.writeAsString(json.encode(_hashes.toJson()));
 
   /// Reads the file with hashes and reports if there is an outdated file,
   /// directory or environment variable.

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

@@ -39,7 +39,7 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult {
         dependencies: dependencies ?? [],
       );
 
-  HookResult copyAdd(HookOutput hookOutput) {
+  HookResult copyAdd(HookOutput hookOutput, List<Uri> dependencies) {
     final mergedMaps = mergeMaps(
         encodedAssetsForLinking,
         hookOutput is BuildOutput
@@ -59,8 +59,9 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult {
       ],
       encodedAssetsForLinking: mergedMaps,
       dependencies: [
-        ...dependencies,
+        ...this.dependencies,
         ...hookOutput.dependencies,
+        ...dependencies,
       ]..sort(_uriCompare),
     );
   }

pkgs/native_assets_builder/test/build_runner/build_dependencies_test.dart

@@ -50,13 +50,28 @@ void main() async {
         expect(result.encodedAssets.length, 2);
         expect(
           result.dependencies,
-          [
-            tempUri.resolve('native_add/').resolve('src/native_add.c'),
-            tempUri
-                .resolve('native_subtract/')
-                .resolve('src/native_subtract.c'),
-          ],
+          containsAll([
+            tempUri.resolve('native_add/src/native_add.c'),
+            tempUri.resolve('native_subtract/src/native_subtract.c'),
+            if (!Platform.isWindows) ...[
+              tempUri.resolve('native_add/hook/build.dart'),
+              tempUri.resolve('native_subtract/hook/build.dart'),
+            ],
+          ]),
         );
+        if (Platform.isWindows) {
+          expect(
+            // https://github.com/dart-lang/sdk/issues/59657
+            // Deps file on windows sometimes have lowercase drive letters.
+            // File.exists will work, but Uri equality doesn't.
+            result.dependencies
+                .map((e) => Uri.file(e.toFilePath().toLowerCase())),
+            containsAll([
+              tempUri.resolve('native_add/hook/build.dart'),
+              tempUri.resolve('native_subtract/hook/build.dart'),
+            ].map((e) => Uri.file(e.toFilePath().toLowerCase()))),
+          );
+        }
       }
     });
   });

pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart

@@ -44,9 +44,9 @@ void main() async {
         );
         expect(
           result.dependencies,
-          [
+          contains(
             packageUri.resolve('src/native_add.c'),
-          ],
+          ),
         );
       }
 
@@ -80,9 +80,9 @@ void main() async {
         );
         expect(
           result.dependencies,
-          [
+          contains(
             packageUri.resolve('src/native_add.c'),
-          ],
+          ),
         );
       }
     });

pkgs/native_assets_builder/test/build_runner/build_runner_failure_test.dart

@@ -39,9 +39,9 @@ void main() async {
             symbols: ['add']);
         expect(
           result.dependencies,
-          [
+          contains(
             packageUri.resolve('src/native_add.c'),
-          ],
+          ),
         );
       }
 
@@ -95,9 +95,9 @@ void main() async {
             symbols: ['add']);
         expect(
           result.dependencies,
-          [
+          contains(
             packageUri.resolve('src/native_add.c'),
-          ],
+          ),
         );
       }
     });

pkgs/native_assets_builder/test/build_runner/build_runner_non_root_package_test.dart

@@ -55,9 +55,9 @@ void main() async {
         expect(result.encodedAssets, isNotEmpty);
         expect(
           result.dependencies,
-          [
+          contains(
             packageUri.resolve('src/native_add.c'),
-          ],
+          ),
         );
         expect(
           logMessages.join('\n'),

pkgs/native_assets_builder/test/build_runner/parse_dep_file_test.dart

@@ -0,0 +1,17 @@
+// 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:native_assets_builder/src/build_runner/build_runner.dart';
+import 'package:test/test.dart';
+
+void main() {
+  test('parseDepFileInputs', () {
+    expect(
+      parseDepFileInputs(
+        r'C:\\Program\ Files\\foo.dill: C:\\Program\ Files\\foo.dart',
+      ),
+      [r'C:\Program Files\foo.dart'],
+    );
+  });
+}