[native_assets_builder] Report dart sources as dependencies of hooks (#…

…1780) Closes: #1770 This PR adds the Dart sources to the dependencies in `HookResult`. It also fixes the dep-file parsing w.r.t. to escapes. ### Implementation details We're passing around `HookOutput`s which is the deserialized `output.json`. We could add the Dart sources as dependencies to it _after_ deserializing, but then the correspondence to the json is lost. So instead I've added an extra return value to the places where we pass `HookOutput` around.
dart-lang · Dec 6, 2024 · 3aba894 · 3aba894
1 parent 0caab92
commit 3aba894
Show file tree

Hide file tree

Showing 9 changed files with 146 additions and 64 deletions.
diff --git a/.github/workflows/native.yaml b/.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/**"

diff --git a/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart b/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 result = await _runHookForPackageCached(
         Hook.build,
         config,
         (config, output) =>
@@ -167,8 +167,9 @@ class NativeAssetsBuildRunner {
         null,
         packageLayout,
       );
-      if (hookOutput == null) return null;
-      hookResult = hookResult.copyAdd(hookOutput);
+      if (result == null) return null;
+      final (hookOutput, hookDeps) = result;
+      hookResult = hookResult.copyAdd(hookOutput, hookDeps);
       globalMetadata[package.name] = (hookOutput as BuildOutput).metadata;
     }
 
@@ -259,7 +260,7 @@ class NativeAssetsBuildRunner {
             'Link configuration for ${package.name} contains errors', errors);
       }
 
-      final hookOutput = await _runHookForPackageCached(
+      final result = await _runHookForPackageCached(
         Hook.link,
         config,
         (config, output) =>
@@ -269,8 +270,9 @@ class NativeAssetsBuildRunner {
         resourceIdentifiers,
         packageLayout,
       );
-      if (hookOutput == null) return null;
-      hookResult = hookResult.copyAdd(hookOutput);
+      if (result == null) return null;
+      final (hookOutput, hookDeps) = result;
+      hookResult = hookResult.copyAdd(hookOutput, hookDeps);
     }
 
     final errors = await applicationAssetValidator(hookResult.encodedAssets);
@@ -403,18 +405,15 @@ class NativeAssetsBuildRunner {
 
       final config = BuildConfig(configBuilder.json);
       final packageConfigUri = packageLayout.packageConfigUri;
-      final (
-        compileSuccess,
-        hookKernelFile,
-        _,
-      ) = await _compileHookForPackageCached(
+      final hookCompileResult = await _compileHookForPackageCached(
         config.packageName,
         config.outputDirectory,
         config.packageRoot.resolve('hook/${hook.scriptName}'),
         packageConfigUri,
         workingDirectory,
       );
-      if (!compileSuccess) return null;
+      if (hookCompileResult == null) return null;
+      final (hookKernelFile, _) = hookCompileResult;
 
       // TODO(https://github.com/dart-lang/native/issues/1321): Should dry runs be cached?
       final buildOutput = await runUnderDirectoriesLock(
@@ -437,12 +436,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,
@@ -461,20 +460,17 @@ class NativeAssetsBuildRunner {
       timeout: singleHookTimeout,
       logger: logger,
       () async {
-        final (
-          compileSuccess,
-          hookKernelFile,
-          hookHashesFile,
-        ) = await _compileHookForPackageCached(
+        final hookCompileResult = await _compileHookForPackageCached(
           config.packageName,
           config.outputDirectory,
           config.packageRoot.resolve('hook/${hook.scriptName}'),
           packageConfigUri,
           workingDirectory,
         );
-        if (!compileSuccess) {
+        if (hookCompileResult == null) {
           return null;
         }
+        final (hookKernelFile, hookHashes) = hookCompileResult;
 
         final buildOutputFile =
             File.fromUri(config.outputDirectory.resolve(hook.outputName));
@@ -510,7 +506,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}'
@@ -533,12 +529,13 @@ ${e.message}
           if (await dependenciesHashFile.exists()) {
             await dependenciesHashFile.delete();
           }
+          return null;
         } else {
           final modifiedDuringBuild = await dependenciesHashes.hashDependencies(
             [
               ...result.dependencies,
               // Also depend on the hook source code.
-              hookHashesFile.uri,
+              hookHashes.file.uri,
             ],
             lastModifiedCutoffTime,
             environment,
@@ -547,7 +544,7 @@ ${e.message}
             logger.severe('File modified during build. Build must be rerun.');
           }
         }
-        return result;
+        return (result, hookHashes.fileSystemEntities);
       },
     );
   }
@@ -685,7 +682,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<(File kernelFile, DependenciesHashFile cacheFile)?>
       _compileHookForPackageCached(
     String packageName,
     Uri outputDirectory,
@@ -721,7 +718,7 @@ ${e.message}
     }
 
     if (!mustCompile) {
-      return (true, kernelFile, dependenciesHashFile);
+      return (kernelFile, dependenciesHashes);
     }
 
     final success = await _compileHookForPackage(
@@ -733,8 +730,7 @@ ${e.message}
       depFile,
     );
     if (!success) {
-      await dependenciesHashFile.delete();
-      return (success, kernelFile, dependenciesHashFile);
+      return null;
     }
 
     final dartSources = await _readDepFile(depFile);
@@ -751,19 +747,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 (kernelFile, dependenciesHashes);
   }
 
   Future<bool> _compileHookForPackage(
@@ -811,6 +795,12 @@ ${compileResult.stdout}
         ''',
       );
       success = false;
+      if (await depFile.exists()) {
+        await depFile.delete();
+      }
+      if (await kernelFile.exists()) {
+        await kernelFile.delete();
+      }
     }
     return success;
   }
@@ -927,3 +917,57 @@ ${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.substring(0, contents.indexOf(': '));
+  contents = contents.substring(output.length + ': '.length).trim();
+  final pathsEscaped = _splitOnNonEscapedSpaces(contents);
+  return pathsEscaped.map(_unescapeDepsPath).toList();
+}
+
+String _unescapeDepsPath(String path) =>
+    path.replaceAll(r'\ ', ' ').replaceAll(r'\\', r'\');
+
+List<String> _splitOnNonEscapedSpaces(String contents) {
+  var index = 0;
+  final result = <String>[];
+  while (index < contents.length) {
+    final start = index;
+    while (index < contents.length) {
+      final u = contents.codeUnitAt(index);
+      if (u == ' '.codeUnitAt(0)) {
+        break;
+      }
+      if (u == r'\'.codeUnitAt(0)) {
+        index++;
+        if (index == contents.length) {
+          throw const FormatException('malformed, ending with backslash');
+        }
+        final v = contents.codeUnitAt(index);
+        assert(v == ' '.codeUnitAt(0) || v == r'\'.codeUnitAt(0));
+      }
+      index++;
+    }
+    result.add(contents.substring(start, index));
+    index++;
+  }
+  return result;
+}
+
+Future<List<Uri>> _readDepFile(File depFile) async {
+  final depFileContents = await depFile.readAsString();
+  final dartSources = parseDepFileInputs(depFileContents);
+  return dartSources.map(Uri.file).toList();
+}
diff --git a/pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart b/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.

diff --git a/pkgs/native_assets_builder/lib/src/model/hook_result.dart b/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> hookDependencies) {
     final mergedMaps = mergeMaps(
         encodedAssetsForLinking,
         hookOutput is BuildOutput
@@ -61,6 +61,7 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult {
       dependencies: [
         ...dependencies,
         ...hookOutput.dependencies,
+        ...hookDependencies,
       ]..sort(_uriCompare),
     );
   }

diff --git a/pkgs/native_assets_builder/test/build_runner/build_dependencies_test.dart b/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()))),
+          );
+        }
       }
     });
   });

diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart b/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'),
-          ],
+          ),
         );
       }
     });