[native_assets_builder] Recompile hook kernel if Dart changes (#1763)

Addresses the remaining comments on #1750. I'm unsure how to test the recompiling of the kernel file if Dart changes without writing a test that downloads a different Dart SDK.
dart-lang · Nov 29, 2024 · 9c33168 · 9c33168
1 parent e69c74d
commit 9c33168
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 67 deletions.
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
@@ -510,20 +510,23 @@ ${e.message}
         ''');
             return null;
           }
-
-          final outdated =
-              (await dependenciesHashes.findOutdatedFileSystemEntity()) != null;
-          if (!outdated) {
+          final outdatedFile =
+              await dependenciesHashes.findOutdatedFileSystemEntity();
+          if (outdatedFile == null) {
             logger.info(
-              [
-                'Skipping ${hook.name} for ${config.packageName} in $outDir.',
-                'Last build on ${output.timestamp}.',
-              ].join(' '),
+              'Skipping ${hook.name} for ${config.packageName}'
+              ' in ${outDir.toFilePath()}.'
+              ' Last build on ${output.timestamp}.',
             );
             // All build flags go into [outDir]. Therefore we do not have to
             // check here whether the config is equal.
             return output;
           }
+          logger.info(
+            'Rerunning ${hook.name} for ${config.packageName}'
+            ' in ${outDir.toFilePath()}.'
+            ' ${outdatedFile.toFilePath()} changed.',
+          );
         }
 
         final result = await _runHookForPackage(
@@ -542,7 +545,8 @@ ${e.message}
             await dependenciesHashFile.delete();
           }
         } else {
-          final modifiedDuringBuild = await dependenciesHashes.hashFiles(
+          final modifiedDuringBuild =
+              await dependenciesHashes.hashFilesAndDirectories(
             [
               ...result.dependencies,
               // Also depend on the hook source code.
@@ -689,50 +693,68 @@ ${e.message}
     );
     final dependenciesHashes = DependenciesHashFile(file: dependenciesHashFile);
     final lastModifiedCutoffTime = DateTime.now();
-    final bool mustCompile;
+    var mustCompile = false;
     if (!await dependenciesHashFile.exists()) {
       mustCompile = true;
     } else {
-      mustCompile =
-          (await dependenciesHashes.findOutdatedFileSystemEntity()) != null;
+      final outdatedFile =
+          await dependenciesHashes.findOutdatedFileSystemEntity();
+      if (outdatedFile != null) {
+        mustCompile = true;
+        logger.info(
+          'Recompiling ${scriptUri.toFilePath()}, '
+          '${outdatedFile.toFilePath()} changed.',
+        );
+      }
     }
-    final bool success;
+
     if (!mustCompile) {
-      success = true;
-    } else {
-      success = await _compileHookForPackage(
-        packageName,
-        scriptUri,
-        packageConfigUri,
-        workingDirectory,
-        includeParentEnvironment,
-        kernelFile,
-        depFile,
-      );
+      return (true, kernelFile, dependenciesHashFile);
+    }
 
-      if (success) {
-        // 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();
-        final modifiedDuringBuild = await dependenciesHashes.hashFiles(
-          dartSources,
-          validBeforeLastModified: lastModifiedCutoffTime,
-        );
-        if (modifiedDuringBuild != null) {
-          logger.severe('File modified during build. Build must be rerun.');
-        }
-      } else {
-        await dependenciesHashFile.delete();
-      }
+    final success = await _compileHookForPackage(
+      packageName,
+      scriptUri,
+      packageConfigUri,
+      workingDirectory,
+      includeParentEnvironment,
+      kernelFile,
+      depFile,
+    );
+    if (!success) {
+      await dependenciesHashFile.delete();
+      return (success, kernelFile, dependenciesHashFile);
+    }
+
+    final dartSources = await _readDepFile(depFile);
+    final modifiedDuringBuild =
+        await dependenciesHashes.hashFilesAndDirectories(
+      [
+        ...dartSources,
+        // If the Dart version changed, recompile.
+        dartExecutable.resolve('../version'),
+      ],
+      validBeforeLastModified: lastModifiedCutoffTime,
+    );
+    if (modifiedDuringBuild != null) {
+      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;
+  }
+
   Future<bool> _compileHookForPackage(
     String packageName,
     Uri scriptUri,

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
@@ -26,7 +26,7 @@ class DependenciesHashFile {
     }
     final jsonObject =
         (json.decode(utf8.decode(await _file.readAsBytes())) as Map)
-            .cast<String, dynamic>();
+            .cast<String, Object>();
     _hashes = FileSystemHashes.fromJson(jsonObject);
   }
 
@@ -38,7 +38,7 @@ class DependenciesHashFile {
   /// If [validBeforeLastModified] is provided, any entities that were modified
   /// after [validBeforeLastModified] will get a dummy hash so that they will
   /// show up as outdated. If any such entity exists, its uri will be returned.
-  Future<Uri?> hashFiles(
+  Future<Uri?> hashFilesAndDirectories(
     List<Uri> fileSystemEntities, {
     DateTime? validBeforeLastModified,
   }) async {
@@ -134,32 +134,25 @@ class DependenciesHashFile {
 /// [Directory] hashes are a hash of the names of the direct children.
 class FileSystemHashes {
   FileSystemHashes({
-    this.version = 1,
     List<FilesystemEntityHash>? files,
   }) : files = files ?? [];
 
-  factory FileSystemHashes.fromJson(Map<String, dynamic> json) {
-    final version = json[_versionKey] as int;
-    final rawEntries =
-        (json[_entitiesKey] as List<dynamic>).cast<Map<String, dynamic>>();
+  factory FileSystemHashes.fromJson(Map<String, Object> json) {
+    final rawEntries = (json[_entitiesKey] as List).cast<Object>();
     final files = <FilesystemEntityHash>[
-      for (final Map<String, dynamic> rawEntry in rawEntries)
-        FilesystemEntityHash._fromJson(rawEntry),
+      for (final rawEntry in rawEntries)
+        FilesystemEntityHash._fromJson((rawEntry as Map).cast()),
     ];
     return FileSystemHashes(
-      version: version,
       files: files,
     );
   }
 
-  final int version;
   final List<FilesystemEntityHash> files;
 
-  static const _versionKey = 'version';
   static const _entitiesKey = 'entities';
 
   Map<String, Object> toJson() => <String, Object>{
-        _versionKey: version,
         _entitiesKey: <Object>[
           for (final FilesystemEntityHash file in files) file.toJson(),
         ],
@@ -177,7 +170,7 @@ class FilesystemEntityHash {
     this.hash,
   );
 
-  factory FilesystemEntityHash._fromJson(Map<String, dynamic> json) =>
+  factory FilesystemEntityHash._fromJson(Map<String, Object> json) =>
       FilesystemEntityHash(
         _fileSystemPathToUri(json[_pathKey] as String),
         json[_hashKey] as int,

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
@@ -61,6 +61,12 @@ void main() async {
           buildValidator: validateCodeAssetBuildOutput,
           applicationAssetValidator: validateCodeAssetInApplication,
         ))!;
+        final hookUri = packageUri.resolve('hook/build.dart');
+        print(logMessages.join('\n'));
+        expect(
+          logMessages.join('\n'),
+          isNot(contains('Recompiling ${hookUri.toFilePath()}')),
+        );
         expect(
           logMessages.join('\n'),
           contains('Skipping build for native_add'),
@@ -87,10 +93,14 @@ void main() async {
       await copyTestProjects(targetUri: tempUri);
       final packageUri = tempUri.resolve('native_add/');
 
+      final logMessages = <String>[];
+      final logger = createCapturingLogger(logMessages);
+
       await runPubGet(
         workingDirectory: packageUri,
         logger: logger,
       );
+      logMessages.clear();
 
       {
         final result = (await build(
@@ -105,6 +115,7 @@ void main() async {
         await expectSymbols(
             asset: CodeAsset.fromEncoded(result.encodedAssets.single),
             symbols: ['add']);
+        logMessages.clear();
       }
 
       await copyTestProjects(
@@ -122,6 +133,18 @@ void main() async {
           buildValidator: validateCodeAssetBuildOutput,
           applicationAssetValidator: validateCodeAssetInApplication,
         ))!;
+
+        final cUri = packageUri.resolve('src/').resolve('native_add.c');
+        expect(
+          logMessages.join('\n'),
+          stringContainsInOrder(
+            [
+              'Rerunning build for native_add in',
+              '${cUri.toFilePath()} changed.'
+            ],
+          ),
+        );
+
         await expectSymbols(
           asset: CodeAsset.fromEncoded(result.encodedAssets.single),
           symbols: ['add', 'subtract'],
@@ -181,14 +204,13 @@ void main() async {
             buildValidator: validateCodeAssetBuildOutput,
             applicationAssetValidator: validateCodeAssetInApplication,
           ))!;
-          {
-            final compiledHook = logMessages
-                .where((m) =>
-                    m.contains('dart compile kernel') ||
-                    m.contains('dart.exe compile kernel'))
-                .isNotEmpty;
-            expect(compiledHook, isTrue);
-          }
+
+          final hookUri = packageUri.resolve('hook/build.dart');
+          expect(
+            logMessages.join('\n'),
+            contains('Recompiling ${hookUri.toFilePath()}'),
+          );
+
           logMessages.clear();
           await expectSymbols(
             asset: CodeAsset.fromEncoded(result.encodedAssets.single),

diff --git a/pkgs/native_assets_builder/test/dependencies_hash_file/dependencies_hash_file_test.dart b/pkgs/native_assets_builder/test/dependencies_hash_file/dependencies_hash_file_test.dart
@@ -43,7 +43,7 @@ void main() async {
         await tempFile.writeAsString('hello');
         await subFile.writeAsString('world');
 
-        await hashes.hashFiles([
+        await hashes.hashFilesAndDirectories([
           tempFile.uri,
           tempSubDir.uri,
         ]);
@@ -95,7 +95,7 @@ void main() async {
 
       // If a file is modified after the valid timestamp, it should be marked
       // as changed.
-      await hashes.hashFiles(
+      await hashes.hashFilesAndDirectories(
         [
           tempFile.uri,
         ],