fold environment hashing in to the dependencies hash file

dcharkes · dcharkes · commit 3113f5e9548d · 2024-12-03T15:04:38.000+01:00
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
@@ -6,7 +6,6 @@ import 'dart:async';
 import 'dart:convert';
 import 'dart:io';
 
-import 'package:collection/collection.dart';
 import 'package:logging/logging.dart';
 import 'package:native_assets_cli/native_assets_cli_internal.dart';
 import 'package:package_config/package_config.dart';
@@ -442,8 +441,6 @@ class NativeAssetsBuildRunner {
     return hookResult;
   }
 
-  // TODO(https://github.com/dart-lang/native/issues/32): Rerun hook if
-  // environment variables change.
   Future<HookOutput?> _runHookForPackageCached(
     Hook hook,
     HookConfig config,
@@ -453,6 +450,7 @@ class NativeAssetsBuildRunner {
     Uri? resources,
     PackageLayout packageLayout,
   ) async {
+    final environment = Platform.environment;
     final outDir = config.outputDirectory;
     return await runUnderDirectoriesLock(
       [
@@ -486,12 +484,7 @@ class NativeAssetsBuildRunner {
         final dependenciesHashes =
             DependenciesHashFile(file: dependenciesHashFile);
         final lastModifiedCutoffTime = DateTime.now();
-        final environmentFile = File.fromUri(
-          config.outputDirectory.resolve('../environment.json'),
-        );
-        if (buildOutputFile.existsSync() &&
-            dependenciesHashFile.existsSync() &&
-            environmentFile.existsSync()) {
+        if (buildOutputFile.existsSync() && dependenciesHashFile.existsSync()) {
           late final HookOutput output;
           try {
             output = _readHookOutputFromUri(hook, buildOutputFile);
@@ -506,14 +499,9 @@ ${e.message}
             return null;
           }
 
-          final outdatedFile =
-              await dependenciesHashes.findOutdatedFileSystemEntity();
-          final environmentChanged = (!await environmentFile.exists()) ||
-              !const MapEquality<String, String>().equals(
-                  (json.decode(await environmentFile.readAsString()) as Map)
-                      .cast<String, String>(),
-                  Platform.environment);
-          if (outdatedFile == null && !environmentChanged) {
+          final outdatedDependency =
+              await dependenciesHashes.findOutdatedDependency(environment);
+          if (outdatedDependency == null) {
             logger.info(
               'Skipping ${hook.name} for ${config.packageName}'
               ' in ${outDir.toFilePath()}.'
@@ -523,19 +511,10 @@ ${e.message}
             // check here whether the config is equal.
             return output;
           }
-          if (outdatedFile != null) {
-            logger.info(
-              'Rerunning ${hook.name} for ${config.packageName}'
-              ' in ${outDir.toFilePath()}.'
-              ' ${outdatedFile.toFilePath()} changed.',
-            );
-          } else {
-            logger.info(
-              'Rerunning ${hook.name} for ${config.packageName}'
-              ' in ${outDir.toFilePath()}.'
-              ' The environment variables changed.',
-            );
-          }
+          logger.info(
+            'Rerunning ${hook.name} for ${config.packageName}'
+            ' in ${outDir.toFilePath()}. $outdatedDependency',
+          );
         }
 
         final result = await _runHookForPackage(
@@ -553,17 +532,14 @@ ${e.message}
             await dependenciesHashFile.delete();
           }
         } else {
-          await environmentFile.writeAsString(
-            json.encode(Platform.environment),
-          );
-          final modifiedDuringBuild =
-              await dependenciesHashes.hashFilesAndDirectories(
+          final modifiedDuringBuild = await dependenciesHashes.hashDependencies(
             [
               ...result.dependencies,
               // Also depend on the hook source code.
               hookHashesFile.uri,
             ],
-            validBeforeLastModified: lastModifiedCutoffTime,
+            lastModifiedCutoffTime,
+            environment,
           );
           if (modifiedDuringBuild != null) {
             logger.severe('File modified during build. Build must be rerun.');
@@ -690,6 +666,7 @@ ${e.message}
     Uri packageConfigUri,
     Uri workingDirectory,
   ) async {
+    final environment = Platform.environment;
     final kernelFile = File.fromUri(
       outputDirectory.resolve('../hook.dill'),
     );
@@ -705,13 +682,12 @@ ${e.message}
     if (!await dependenciesHashFile.exists()) {
       mustCompile = true;
     } else {
-      final outdatedFile =
-          await dependenciesHashes.findOutdatedFileSystemEntity();
-      if (outdatedFile != null) {
+      final outdatedDependency =
+          await dependenciesHashes.findOutdatedDependency(environment);
+      if (outdatedDependency != null) {
         mustCompile = true;
         logger.info(
-          'Recompiling ${scriptUri.toFilePath()}, '
-          '${outdatedFile.toFilePath()} changed.',
+          'Recompiling ${scriptUri.toFilePath()}. $outdatedDependency',
         );
       }
     }
@@ -734,14 +710,14 @@ ${e.message}
     }
 
     final dartSources = await _readDepFile(depFile);
-    final modifiedDuringBuild =
-        await dependenciesHashes.hashFilesAndDirectories(
+    final modifiedDuringBuild = await dependenciesHashes.hashDependencies(
       [
         ...dartSources,
         // If the Dart version changed, recompile.
         dartExecutable.resolve('../version'),
       ],
-      validBeforeLastModified: lastModifiedCutoffTime,
+      lastModifiedCutoffTime,
+      environment,
     );
     if (modifiedDuringBuild != null) {
       logger.severe('File modified during build. Build must be rerun.');
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
@@ -35,21 +35,22 @@ class DependenciesHashFile {
   /// Populate the hashes and persist file with entries from
   /// [fileSystemEntities].
   ///
-  /// 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?> hashFilesAndDirectories(
-    List<Uri> fileSystemEntities, {
-    DateTime? validBeforeLastModified,
-  }) async {
+  /// If [fileSystemValidBeforeLastModified] is provided, any entities that were
+  /// modified after [fileSystemValidBeforeLastModified] 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?> hashDependencies(
+    List<Uri> fileSystemEntities,
+    DateTime fileSystemValidBeforeLastModified,
+    Map<String, String> environment,
+  ) async {
     _reset();
 
     Uri? modifiedAfterTimeStamp;
     for (final uri in fileSystemEntities) {
       int hash;
-      if (validBeforeLastModified != null &&
-          (await uri.fileSystemEntity.lastModified())
-              .isAfter(validBeforeLastModified)) {
+      if ((await uri.fileSystemEntity.lastModified())
+          .isAfter(fileSystemValidBeforeLastModified)) {
         hash = _hashLastModifiedAfterCutoff;
         modifiedAfterTimeStamp = uri;
       } else {
@@ -61,30 +62,55 @@ class DependenciesHashFile {
       }
       _hashes.files.add(FilesystemEntityHash(uri, hash));
     }
+    for (final entry in environment.entries) {
+      _hashes.environment.add(EnvironmentVariableHash(
+          entry.key, _hashEnvironmentValue(entry.value)));
+    }
     await _persist();
     return modifiedAfterTimeStamp;
   }
 
   Future<void> _persist() => _file.writeAsString(json.encode(_hashes.toJson()));
 
-  /// Reads the file with hashes and finds an outdated file or directory if it
-  /// exists.
-  Future<Uri?> findOutdatedFileSystemEntity() async {
+  /// Reads the file with hashes and reports if there is an outdated file,
+  /// directory or environment variable.
+  Future<String?> findOutdatedDependency(
+    Map<String, String> environment,
+  ) async {
     await _readFile();
 
     for (final savedHash in _hashes.files) {
       final uri = savedHash.path;
       final savedHashValue = savedHash.hash;
-      final int hashValue;
       if (_isDirectoryPath(uri.path)) {
-        hashValue = await _hashDirectory(uri);
+        final hashValue = await _hashDirectory(uri);
+        if (savedHashValue != hashValue) {
+          return 'Directory contents changed: ${uri.toFilePath()}.';
+        }
       } else {
-        hashValue = await _hashFile(uri);
+        final hashValue = await _hashFile(uri);
+        if (savedHashValue != hashValue) {
+          return 'File contents changed: ${uri.toFilePath()}.';
+        }
+      }
+    }
+
+    // Check if env vars changed or were removed.
+    for (final savedHash in _hashes.environment) {
+      final hashValue = _hashEnvironmentValue(environment[savedHash.key]);
+      if (savedHash.hash != hashValue) {
+        return 'Environment variable changed: ${savedHash.key}.';
       }
-      if (savedHashValue != hashValue) {
-        return uri;
+    }
+
+    // Check if env vars were added.
+    final savedEnvKeys = _hashes.environment.map((e) => e.key).toSet();
+    for (final envKey in environment.keys) {
+      if (!savedEnvKeys.contains(envKey)) {
+        return 'Environment variable changed: $envKey.';
       }
     }
+
     return null;
   }
 
@@ -113,6 +139,11 @@ class DependenciesHashFile {
     return _md5int64(utf8.encode(childrenNames));
   }
 
+  int _hashEnvironmentValue(String? value) {
+    if (value == null) return _hashNotExists;
+    return _md5int64(utf8.encode(value));
+  }
+
   /// Predefined hash for files and directories that do not exist.
   ///
   /// There are two predefined hash values. The chance that a predefined hash
@@ -135,27 +166,43 @@ class DependenciesHashFile {
 class FileSystemHashes {
   FileSystemHashes({
     List<FilesystemEntityHash>? files,
-  }) : files = files ?? [];
+    List<EnvironmentVariableHash>? environment,
+  })  : files = files ?? [],
+        environment = environment ?? [];
 
   factory FileSystemHashes.fromJson(Map<String, Object> json) {
-    final rawEntries = (json[_entitiesKey] as List).cast<Object>();
+    final rawFilesystemEntries =
+        (json[_filesystemKey] as List?)?.cast<Object>() ?? [];
     final files = <FilesystemEntityHash>[
-      for (final rawEntry in rawEntries)
+      for (final rawEntry in rawFilesystemEntries)
         FilesystemEntityHash._fromJson((rawEntry as Map).cast()),
     ];
+    final rawEnvironmentEntries =
+        (json[_environmentKey] as List?)?.cast<Object>() ?? [];
+    final environment = <EnvironmentVariableHash>[
+      for (final rawEntry in rawEnvironmentEntries)
+        EnvironmentVariableHash._fromJson((rawEntry as Map).cast()),
+    ];
     return FileSystemHashes(
       files: files,
+      environment: environment,
     );
   }
 
   final List<FilesystemEntityHash> files;
+  final List<EnvironmentVariableHash> environment;
 
-  static const _entitiesKey = 'entities';
+  static const _filesystemKey = 'file_system';
+
+  static const _environmentKey = 'environment';
 
   Map<String, Object> toJson() => <String, Object>{
-        _entitiesKey: <Object>[
+        _filesystemKey: <Object>[
           for (final FilesystemEntityHash file in files) file.toJson(),
         ],
+        _environmentKey: <Object>[
+          for (final EnvironmentVariableHash env in environment) env.toJson(),
+        ],
       };
 }
 
@@ -190,6 +237,32 @@ class FilesystemEntityHash {
       };
 }
 
+class EnvironmentVariableHash {
+  EnvironmentVariableHash(
+    this.key,
+    this.hash,
+  );
+
+  factory EnvironmentVariableHash._fromJson(Map<String, Object> json) =>
+      EnvironmentVariableHash(
+        json[_keyKey] as String,
+        json[_hashKey] as int,
+      );
+
+  static const _keyKey = 'key';
+  static const _hashKey = 'hash';
+
+  final String key;
+
+  /// A 64 bit hash.
+  final int hash;
+
+  Object toJson() => <String, Object>{
+        _keyKey: key,
+        _hashKey: hash,
+      };
+}
+
 bool _isDirectoryPath(String path) =>
     path.endsWith(Platform.pathSeparator) || path.endsWith('/');
 
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
@@ -140,7 +140,7 @@ void main() async {
           stringContainsInOrder(
             [
               'Rerunning build for native_add in',
-              '${cUri.toFilePath()} changed.'
+              'File contents changed: ${cUri.toFilePath()}.'
             ],
           ),
         );
@@ -248,18 +248,24 @@ void main() async {
 
         // Simulate that the environment variables changed by augmenting the
         // persisted environment from the last invocation.
-        final environmentFile = File.fromUri(
+        final dependenciesHashFile = File.fromUri(
           CodeAsset.fromEncoded(result.encodedAssets.single)
               .file!
               .parent
               .parent
-              .resolve('environment.json'),
+              .resolve('dependencies.dependencies_hash_file.json'),
         );
-        expect(await environmentFile.exists(), true);
-        await environmentFile.writeAsString(jsonEncode({
-          ...Platform.environment,
-          'SOME_KEY_THAT_IS_NOT_ALREADY_IN_THE_ENVIRONMENT': 'some_value',
-        }));
+        expect(await dependenciesHashFile.exists(), true);
+        final dependenciesContent =
+            jsonDecode(await dependenciesHashFile.readAsString())
+                as Map<Object, Object?>;
+        const modifiedEnvKey = 'PATH';
+        (dependenciesContent['environment'] as List<dynamic>).add({
+          'key': modifiedEnvKey,
+          'hash': 123456789,
+        });
+        await dependenciesHashFile
+            .writeAsString(jsonEncode(dependenciesContent));
 
         (await build(
           packageUri,
@@ -278,6 +284,10 @@ void main() async {
           logMessages.join('\n'),
           isNot(contains('Skipping build for native_add')),
         );
+        expect(
+          logMessages.join('\n'),
+          contains('Environment variable changed: $modifiedEnvKey.'),
+        );
         logMessages.clear();
       });
     },
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
