From 62f0754e12880ae9f18b8e7832abd661ad270bd9 Mon Sep 17 00:00:00 2001
From: Daco Harkes <dacoharkes@google.com>
Date: Fri, 29 Nov 2024 09:38:46 +0100
Subject: [PATCH 1/3] [native_assets_builder] Don't cache if env vars change

---
 .../lib/src/build_runner/build_runner.dart    | 55 +++++++-------
 .../lib/src/utils/run_process.dart            | 13 ----
 .../build_runner_caching_test.dart            | 64 +++++++++++++++-
 .../build_runner_reusability_test.dart        |  4 -
 .../build_runner_run_in_isolation_test.dart   | 75 -------------------
 .../concurrency_shared_test_helper.dart       |  1 -
 .../build_runner/concurrency_test_helper.dart |  1 -
 .../test/build_runner/helpers.dart            |  9 ---
 pkgs/native_assets_builder/test/helpers.dart  |  2 -
 .../lib/src/utils/run_process.dart            | 17 +----
 10 files changed, 94 insertions(+), 147 deletions(-)
 delete mode 100644 pkgs/native_assets_builder/test/build_runner/build_runner_run_in_isolation_test.dart

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
index 1559ec803..0a584eb49 100644
--- 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,6 +6,7 @@ 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';
@@ -96,7 +97,6 @@ class NativeAssetsBuildRunner {
     required OS targetOS,
     required BuildMode buildMode,
     required Uri workingDirectory,
-    required bool includeParentEnvironment,
     PackageLayout? packageLayout,
     String? runPackageName,
     required List<String> supportedAssetTypes,
@@ -165,7 +165,6 @@ class NativeAssetsBuildRunner {
             buildValidator(config as BuildConfig, output as BuildOutput),
         packageLayout.packageConfigUri,
         workingDirectory,
-        includeParentEnvironment,
         null,
         packageLayout,
       );
@@ -205,7 +204,6 @@ class NativeAssetsBuildRunner {
     required BuildMode buildMode,
     required Uri workingDirectory,
     required ApplicationAssetValidator applicationAssetValidator,
-    required bool includeParentEnvironment,
     PackageLayout? packageLayout,
     Uri? resourceIdentifiers,
     String? runPackageName,
@@ -269,7 +267,6 @@ class NativeAssetsBuildRunner {
             linkValidator(config as LinkConfig, output as LinkOutput),
         packageLayout.packageConfigUri,
         workingDirectory,
-        includeParentEnvironment,
         resourceIdentifiers,
         packageLayout,
       );
@@ -330,7 +327,6 @@ class NativeAssetsBuildRunner {
     required OS targetOS,
     required Uri workingDirectory,
     required bool linkingEnabled,
-    required bool includeParentEnvironment,
     PackageLayout? packageLayout,
     String? runPackageName,
     required List<String> supportedAssetTypes,
@@ -341,7 +337,6 @@ class NativeAssetsBuildRunner {
         validator: (HookConfig config, HookOutput output) =>
             buildValidator(config as BuildConfig, output as BuildOutput),
         workingDirectory: workingDirectory,
-        includeParentEnvironment: includeParentEnvironment,
         packageLayout: packageLayout,
         runPackageName: runPackageName,
         supportedAssetTypes: supportedAssetTypes,
@@ -353,7 +348,6 @@ class NativeAssetsBuildRunner {
     required _HookValidator validator,
     required OS targetOS,
     required Uri workingDirectory,
-    required bool includeParentEnvironment,
     PackageLayout? packageLayout,
     String? runPackageName,
     required bool linkingEnabled,
@@ -420,7 +414,6 @@ class NativeAssetsBuildRunner {
         config.packageRoot.resolve('hook/${hook.scriptName}'),
         packageConfigUri,
         workingDirectory,
-        includeParentEnvironment,
       );
       if (!compileSuccess) return null;
 
@@ -438,7 +431,6 @@ class NativeAssetsBuildRunner {
           validator,
           packageConfigUri,
           workingDirectory,
-          includeParentEnvironment,
           null,
           hookKernelFile,
           packageLayout!,
@@ -458,7 +450,6 @@ class NativeAssetsBuildRunner {
     _HookValidator validator,
     Uri packageConfigUri,
     Uri workingDirectory,
-    bool includeParentEnvironment,
     Uri? resources,
     PackageLayout packageLayout,
   ) async {
@@ -481,7 +472,6 @@ class NativeAssetsBuildRunner {
           config.packageRoot.resolve('hook/${hook.scriptName}'),
           packageConfigUri,
           workingDirectory,
-          includeParentEnvironment,
         );
         if (!compileSuccess) {
           return null;
@@ -496,7 +486,12 @@ class NativeAssetsBuildRunner {
         final dependenciesHashes =
             DependenciesHashFile(file: dependenciesHashFile);
         final lastModifiedCutoffTime = DateTime.now();
-        if (buildOutputFile.existsSync() && dependenciesHashFile.existsSync()) {
+        final environmentFile = File.fromUri(
+          config.outputDirectory.resolve('../environment.json'),
+        );
+        if (buildOutputFile.existsSync() &&
+            dependenciesHashFile.existsSync() &&
+            environmentFile.existsSync()) {
           late final HookOutput output;
           try {
             output = _readHookOutputFromUri(hook, buildOutputFile);
@@ -510,9 +505,15 @@ ${e.message}
         ''');
             return null;
           }
+
           final outdatedFile =
               await dependenciesHashes.findOutdatedFileSystemEntity();
-          if (outdatedFile == null) {
+          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) {
             logger.info(
               'Skipping ${hook.name} for ${config.packageName}'
               ' in ${outDir.toFilePath()}.'
@@ -522,11 +523,19 @@ ${e.message}
             // check here whether the config is equal.
             return output;
           }
-          logger.info(
-            'Rerunning ${hook.name} for ${config.packageName}'
-            ' in ${outDir.toFilePath()}.'
-            ' ${outdatedFile.toFilePath()} changed.',
-          );
+          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.',
+            );
+          }
         }
 
         final result = await _runHookForPackage(
@@ -535,7 +544,6 @@ ${e.message}
           validator,
           packageConfigUri,
           workingDirectory,
-          includeParentEnvironment,
           resources,
           hookKernelFile,
           packageLayout,
@@ -545,6 +553,9 @@ ${e.message}
             await dependenciesHashFile.delete();
           }
         } else {
+          await environmentFile.writeAsString(
+            json.encode(Platform.environment),
+          );
           final modifiedDuringBuild =
               await dependenciesHashes.hashFilesAndDirectories(
             [
@@ -569,7 +580,6 @@ ${e.message}
     _HookValidator validator,
     Uri packageConfigUri,
     Uri workingDirectory,
-    bool includeParentEnvironment,
     Uri? resources,
     File hookKernelFile,
     PackageLayout packageLayout,
@@ -597,7 +607,6 @@ ${e.message}
       executable: dartExecutable,
       arguments: arguments,
       logger: logger,
-      includeParentEnvironment: includeParentEnvironment,
     );
 
     var deleteOutputIfExists = false;
@@ -680,7 +689,6 @@ ${e.message}
     Uri scriptUri,
     Uri packageConfigUri,
     Uri workingDirectory,
-    bool includeParentEnvironment,
   ) async {
     final kernelFile = File.fromUri(
       outputDirectory.resolve('../hook.dill'),
@@ -717,7 +725,6 @@ ${e.message}
       scriptUri,
       packageConfigUri,
       workingDirectory,
-      includeParentEnvironment,
       kernelFile,
       depFile,
     );
@@ -760,7 +767,6 @@ ${e.message}
     Uri scriptUri,
     Uri packageConfigUri,
     Uri workingDirectory,
-    bool includeParentEnvironment,
     File kernelFile,
     File depFile,
   ) async {
@@ -777,7 +783,6 @@ ${e.message}
       executable: dartExecutable,
       arguments: compileArguments,
       logger: logger,
-      includeParentEnvironment: includeParentEnvironment,
     );
     var success = true;
     if (compileResult.exitCode != 0) {
diff --git a/pkgs/native_assets_builder/lib/src/utils/run_process.dart b/pkgs/native_assets_builder/lib/src/utils/run_process.dart
index cb989c7bc..efb712012 100644
--- a/pkgs/native_assets_builder/lib/src/utils/run_process.dart
+++ b/pkgs/native_assets_builder/lib/src/utils/run_process.dart
@@ -25,19 +25,6 @@ Future<RunProcessResult> runProcess({
   int expectedExitCode = 0,
   bool throwOnUnexpectedExitCode = false,
 }) async {
-  if (Platform.isWindows && !includeParentEnvironment) {
-    const winEnvKeys = [
-      'SYSTEMROOT',
-      'TEMP',
-      'TMP',
-    ];
-    environment = {
-      for (final winEnvKey in winEnvKeys)
-        winEnvKey: Platform.environment[winEnvKey]!,
-      ...?environment,
-    };
-  }
-
   final printWorkingDir =
       workingDirectory != null && workingDirectory != Directory.current.uri;
   final commandString = [
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
index 42c5e4939..0517083e7 100644
--- 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
@@ -2,6 +2,7 @@
 // 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:convert';
 import 'dart:io';
 
 import 'package:test/test.dart';
@@ -62,7 +63,6 @@ void main() async {
           applicationAssetValidator: validateCodeAssetInApplication,
         ))!;
         final hookUri = packageUri.resolve('hook/build.dart');
-        print(logMessages.join('\n'));
         expect(
           logMessages.join('\n'),
           isNot(contains('Recompiling ${hookUri.toFilePath()}')),
@@ -220,4 +220,66 @@ void main() async {
       });
     },
   );
+
+  test(
+    'change environment',
+    timeout: longTimeout,
+    () async {
+      await inTempDir((tempUri) 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(
+          packageUri,
+          logger,
+          dartExecutable,
+          supportedAssetTypes: [CodeAsset.type],
+          configValidator: validateCodeAssetBuildConfig,
+          buildValidator: validateCodeAssetBuildOutput,
+          applicationAssetValidator: validateCodeAssetInApplication,
+        ))!;
+        logMessages.clear();
+
+        // Simulate that the environment variables changed by augmenting the
+        // persisted environment from the last invocation.
+        final environmentFile = File.fromUri(
+          CodeAsset.fromEncoded(result.encodedAssets.single)
+              .file!
+              .parent
+              .parent
+              .resolve('environment.json'),
+        );
+        expect(await environmentFile.exists(), true);
+        await environmentFile.writeAsString(jsonEncode({
+          ...Platform.environment,
+          'SOME_KEY_THAT_IS_NOT_ALREADY_IN_THE_ENVIRONMENT': 'some_value',
+        }));
+
+        (await build(
+          packageUri,
+          logger,
+          dartExecutable,
+          supportedAssetTypes: [CodeAsset.type],
+          configValidator: validateCodeAssetBuildConfig,
+          buildValidator: validateCodeAssetBuildOutput,
+          applicationAssetValidator: validateCodeAssetInApplication,
+        ))!;
+        expect(
+          logMessages.join('\n'),
+          contains('hook.dill'),
+        );
+        expect(
+          logMessages.join('\n'),
+          isNot(contains('Skipping build for native_add')),
+        );
+        logMessages.clear();
+      });
+    },
+  );
 }
diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart
index 19e58520d..2f8d34b45 100644
--- a/pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart
+++ b/pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart
@@ -37,7 +37,6 @@ void main() async {
         configCreator: configCreator,
         targetOS: Target.current.os,
         workingDirectory: packageUri,
-        includeParentEnvironment: true,
         linkingEnabled: false,
         supportedAssetTypes: [],
         buildValidator: (config, output) async => [],
@@ -46,7 +45,6 @@ void main() async {
         configCreator: configCreator,
         targetOS: Target.current.os,
         workingDirectory: packageUri,
-        includeParentEnvironment: true,
         linkingEnabled: false,
         supportedAssetTypes: [],
         buildValidator: (config, output) async => [],
@@ -56,7 +54,6 @@ void main() async {
         targetOS: OS.current,
         buildMode: BuildMode.release,
         workingDirectory: packageUri,
-        includeParentEnvironment: true,
         linkingEnabled: false,
         supportedAssetTypes: [],
         configValidator: (config) async => [],
@@ -68,7 +65,6 @@ void main() async {
         buildMode: BuildMode.release,
         targetOS: OS.current,
         workingDirectory: packageUri,
-        includeParentEnvironment: true,
         linkingEnabled: false,
         supportedAssetTypes: [],
         configValidator: (config) async => [],
diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_run_in_isolation_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_run_in_isolation_test.dart
deleted file mode 100644
index 45f6bbc17..000000000
--- a/pkgs/native_assets_builder/test/build_runner/build_runner_run_in_isolation_test.dart
+++ /dev/null
@@ -1,75 +0,0 @@
-// 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 'dart:io';
-
-import 'package:test/test.dart';
-
-import '../helpers.dart';
-import 'helpers.dart';
-
-const Timeout longTimeout = Timeout(Duration(minutes: 5));
-
-void main() async {
-  final env = Platform.environment;
-  final cc = env['DART_HOOK_TESTING_C_COMPILER__CC'];
-  final ar = env['DART_HOOK_TESTING_C_COMPILER__AR'];
-  final ld = env['DART_HOOK_TESTING_C_COMPILER__LD'];
-  final envScript = env['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT'];
-  final envScriptArgs =
-      env['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT_ARGUMENTS']
-          ?.split(' ')
-          .map((arg) => arg.trim())
-          .where((arg) => arg.isNotEmpty)
-          .toList();
-
-  if (cc == null) {
-    // We don't set any compiler paths on the GitHub CI.
-    // We neither set compiler paths on MacOS on the Dart SDK CI
-    // in pkg/test_runner/lib/src/configuration.dart
-    // nativeCompilerEnvironmentVariables.
-    //
-    // We could potentially run this test if we default to some compilers
-    // we find on the path before running the test. However, the logic for
-    // discovering compilers is currently hidden inside
-    // package:native_toolchain_c.
-    return;
-  }
-
-  test('run in isolation', timeout: longTimeout, () async {
-    await inTempDir((tempUri) async {
-      await copyTestProjects(targetUri: tempUri);
-      final packageUri = tempUri.resolve('native_add/');
-
-      await runPubGet(workingDirectory: packageUri, logger: logger);
-
-      printOnFailure('cc: $cc');
-
-      final result = (await build(
-        packageUri,
-        logger,
-        dartExecutable,
-        // Manually pass in a compiler.
-        cCompilerConfig: CCompilerConfig(
-          archiver: ar?.fileUri,
-          compiler: cc.fileUri,
-          envScript: envScript?.fileUri,
-          envScriptArgs: envScriptArgs,
-          linker: ld?.fileUri,
-        ),
-        // Prevent any other environment variables.
-        includeParentEnvironment: false,
-        supportedAssetTypes: [CodeAsset.type],
-        configValidator: validateCodeAssetBuildConfig,
-        buildValidator: validateCodeAssetBuildOutput,
-        applicationAssetValidator: validateCodeAssetInApplication,
-      ))!;
-      expect(result.encodedAssets.length, 1);
-    });
-  });
-}
-
-extension on String {
-  Uri get fileUri => Uri.file(this);
-}
diff --git a/pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart b/pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart
index cb69d2833..7b9e5d5cf 100644
--- a/pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart
+++ b/pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart
@@ -24,7 +24,6 @@ void main(List<String> args) async {
     buildMode: BuildMode.release,
     targetOS: target.os,
     workingDirectory: packageUri,
-    includeParentEnvironment: true,
     linkingEnabled: false,
     supportedAssetTypes: [DataAsset.type],
     configValidator: validateDataAssetBuildConfig,
diff --git a/pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart b/pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart
index bebf171a7..6d3bf6a85 100644
--- a/pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart
+++ b/pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart
@@ -35,7 +35,6 @@ void main(List<String> args) async {
     buildMode: BuildMode.release,
     targetOS: OS.current,
     workingDirectory: packageUri,
-    includeParentEnvironment: true,
     linkingEnabled: false,
     supportedAssetTypes: [CodeAsset.type, DataAsset.type],
     configValidator: (config) async => [
diff --git a/pkgs/native_assets_builder/test/build_runner/helpers.dart b/pkgs/native_assets_builder/test/build_runner/helpers.dart
index c80f3f798..61f6fa343 100644
--- a/pkgs/native_assets_builder/test/build_runner/helpers.dart
+++ b/pkgs/native_assets_builder/test/build_runner/helpers.dart
@@ -37,7 +37,6 @@ Future<BuildResult?> build(
   required ApplicationAssetValidator applicationAssetValidator,
   LinkModePreference linkModePreference = LinkModePreference.dynamic,
   CCompilerConfig? cCompilerConfig,
-  bool includeParentEnvironment = true,
   List<String>? capturedLogs,
   PackageLayout? packageLayout,
   String? runPackageName,
@@ -75,7 +74,6 @@ Future<BuildResult?> build(
       buildMode: BuildMode.release,
       targetOS: targetOS,
       workingDirectory: packageUri,
-      includeParentEnvironment: includeParentEnvironment,
       packageLayout: packageLayout,
       runPackageName: runPackageName,
       linkingEnabled: linkingEnabled,
@@ -105,7 +103,6 @@ Future<LinkResult?> link(
   required ApplicationAssetValidator applicationAssetValidator,
   LinkModePreference linkModePreference = LinkModePreference.dynamic,
   CCompilerConfig? cCompilerConfig,
-  bool includeParentEnvironment = true,
   List<String>? capturedLogs,
   PackageLayout? packageLayout,
   required BuildResult buildResult,
@@ -143,7 +140,6 @@ Future<LinkResult?> link(
       buildMode: BuildMode.release,
       targetOS: target?.os ?? OS.current,
       workingDirectory: packageUri,
-      includeParentEnvironment: includeParentEnvironment,
       packageLayout: packageLayout,
       buildResult: buildResult,
       resourceIdentifiers: resourceIdentifiers,
@@ -171,7 +167,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
   required BuildValidator buildValidator,
   required LinkValidator linkValidator,
   required ApplicationAssetValidator applicationAssetValidator,
-  bool includeParentEnvironment = true,
   List<String>? capturedLogs,
   PackageLayout? packageLayout,
   String? runPackageName,
@@ -203,7 +198,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
         buildMode: BuildMode.release,
         targetOS: target?.os ?? OS.current,
         workingDirectory: packageUri,
-        includeParentEnvironment: includeParentEnvironment,
         packageLayout: packageLayout,
         runPackageName: runPackageName,
         linkingEnabled: true,
@@ -237,7 +231,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
         buildMode: BuildMode.release,
         targetOS: target?.os ?? OS.current,
         workingDirectory: packageUri,
-        includeParentEnvironment: includeParentEnvironment,
         packageLayout: packageLayout,
         buildResult: buildResult,
         resourceIdentifiers: resourceIdentifiers,
@@ -279,7 +272,6 @@ Future<BuildDryRunResult?> buildDryRun(
   required BuildValidator buildValidator,
   LinkModePreference linkModePreference = LinkModePreference.dynamic,
   CCompilerConfig? cCompilerConfig,
-  bool includeParentEnvironment = true,
   List<String>? capturedLogs,
   PackageLayout? packageLayout,
   required bool linkingEnabled,
@@ -297,7 +289,6 @@ Future<BuildDryRunResult?> buildDryRun(
           ),
         targetOS: Target.current.os,
         workingDirectory: packageUri,
-        includeParentEnvironment: includeParentEnvironment,
         packageLayout: packageLayout,
         linkingEnabled: linkingEnabled,
         supportedAssetTypes: supportedAssetTypes,
diff --git a/pkgs/native_assets_builder/test/helpers.dart b/pkgs/native_assets_builder/test/helpers.dart
index 3bb4c710a..ba864da71 100644
--- a/pkgs/native_assets_builder/test/helpers.dart
+++ b/pkgs/native_assets_builder/test/helpers.dart
@@ -69,7 +69,6 @@ Future<run_process.RunProcessResult> runProcess({
   List<String> arguments = const [],
   Uri? workingDirectory,
   Map<String, String>? environment,
-  bool includeParentEnvironment = true,
   required Logger? logger,
   bool captureOutput = true,
   int expectedExitCode = 0,
@@ -80,7 +79,6 @@ Future<run_process.RunProcessResult> runProcess({
       arguments: arguments,
       workingDirectory: workingDirectory,
       environment: environment,
-      includeParentEnvironment: includeParentEnvironment,
       logger: logger,
       captureOutput: captureOutput,
       expectedExitCode: expectedExitCode,
diff --git a/pkgs/native_toolchain_c/lib/src/utils/run_process.dart b/pkgs/native_toolchain_c/lib/src/utils/run_process.dart
index f9204e5b9..989251409 100644
--- a/pkgs/native_toolchain_c/lib/src/utils/run_process.dart
+++ b/pkgs/native_toolchain_c/lib/src/utils/run_process.dart
@@ -18,25 +18,11 @@ Future<RunProcessResult> runProcess({
   List<String> arguments = const [],
   Uri? workingDirectory,
   Map<String, String>? environment,
-  bool includeParentEnvironment = true,
   required Logger? logger,
   bool captureOutput = true,
   int expectedExitCode = 0,
   bool throwOnUnexpectedExitCode = false,
 }) async {
-  if (Platform.isWindows && !includeParentEnvironment) {
-    const winEnvKeys = [
-      'SYSTEMROOT',
-      'TEMP',
-      'TMP',
-    ];
-    environment = {
-      for (final winEnvKey in winEnvKeys)
-        winEnvKey: Platform.environment[winEnvKey]!,
-      ...?environment,
-    };
-  }
-
   final printWorkingDir =
       workingDirectory != null && workingDirectory != Directory.current.uri;
   final commandString = [
@@ -55,8 +41,7 @@ Future<RunProcessResult> runProcess({
     arguments,
     workingDirectory: workingDirectory?.toFilePath(),
     environment: environment,
-    includeParentEnvironment: includeParentEnvironment,
-    runInShell: Platform.isWindows && !includeParentEnvironment,
+    runInShell: Platform.isWindows && workingDirectory != null,
   );
 
   final stdoutSub = process.stdout

From 3113f5e9548d4b24985df7da37c7c6537d0fbd50 Mon Sep 17 00:00:00 2001
From: Daco Harkes <dacoharkes@google.com>
Date: Tue, 3 Dec 2024 14:49:09 +0100
Subject: [PATCH 2/3] fold environment hashing in to the dependencies hash file

---
 .../lib/src/build_runner/build_runner.dart    |  64 +++-------
 .../dependencies_hash_file.dart               | 119 ++++++++++++++----
 .../build_runner_caching_test.dart            |  26 ++--
 .../dependencies_hash_file_test.dart          |  59 ++++++---
 4 files changed, 177 insertions(+), 91 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
index 0a584eb49..f5a900280 100644
--- 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
index 259be5793..ba818bd32 100644
--- 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
index 0517083e7..851458b14 100644
--- 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
index ae4fd2522..c222b31a4 100644
--- 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
@@ -11,6 +11,8 @@ import 'package:test/test.dart';
 import '../helpers.dart';
 
 void main() async {
+  final environment = Platform.environment;
+
   test('json format', () async {
     await inTempDir((tempUri) async {
       final hashes = FileSystemHashes(
@@ -43,66 +45,91 @@ void main() async {
         await tempFile.writeAsString('hello');
         await subFile.writeAsString('world');
 
-        await hashes.hashFilesAndDirectories([
-          tempFile.uri,
-          tempSubDir.uri,
-        ]);
+        await hashes.hashDependencies(
+          [
+            tempFile.uri,
+            tempSubDir.uri,
+          ],
+          (await tempFile.lastModified()).add(const Duration(minutes: 1)),
+          environment,
+        );
       }
 
       await reset();
 
       // No changes
-      expect(await hashes.findOutdatedFileSystemEntity(), isNull);
+      expect(await hashes.findOutdatedDependency(environment), isNull);
 
       // Change file contents.
       await tempFile.writeAsString('asdf');
-      expect(await hashes.findOutdatedFileSystemEntity(), tempFile.uri);
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempFile.uri.toFilePath()),
+      );
       await reset();
 
       // Delete file.
       await tempFile.delete();
-      expect(await hashes.findOutdatedFileSystemEntity(), tempFile.uri);
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempFile.uri.toFilePath()),
+      );
       await reset();
 
       // Add file to tracked directory.
       final subFile2 = File.fromUri(tempSubDir.uri.resolve('baz.txt'));
       await subFile2.create(recursive: true);
       await subFile2.writeAsString('hello');
-      expect(await hashes.findOutdatedFileSystemEntity(), tempSubDir.uri);
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempSubDir.uri.toFilePath()),
+      );
       await reset();
 
       // Delete file from tracked directory.
       await subFile.delete();
-      expect(await hashes.findOutdatedFileSystemEntity(), tempSubDir.uri);
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempSubDir.uri.toFilePath()),
+      );
       await reset();
 
       // Delete tracked directory.
       await tempSubDir.delete(recursive: true);
-      expect(await hashes.findOutdatedFileSystemEntity(), tempSubDir.uri);
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempSubDir.uri.toFilePath()),
+      );
       await reset();
 
       // Add directory to tracked directory.
       final subDir2 = Directory.fromUri(tempSubDir.uri.resolve('baz/'));
       await subDir2.create(recursive: true);
-      expect(await hashes.findOutdatedFileSystemEntity(), tempSubDir.uri);
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempSubDir.uri.toFilePath()),
+      );
       await reset();
 
       // Overwriting a file with identical contents.
       await tempFile.writeAsString('something something');
       await tempFile.writeAsString('hello');
-      expect(await hashes.findOutdatedFileSystemEntity(), isNull);
+      expect(await hashes.findOutdatedDependency(environment), isNull);
       await reset();
 
       // If a file is modified after the valid timestamp, it should be marked
       // as changed.
-      await hashes.hashFilesAndDirectories(
+      await hashes.hashDependencies(
         [
           tempFile.uri,
         ],
-        validBeforeLastModified: (await tempFile.lastModified())
-            .subtract(const Duration(seconds: 1)),
+        (await tempFile.lastModified()).subtract(const Duration(seconds: 1)),
+        environment,
+      );
+      expect(
+        await hashes.findOutdatedDependency(environment),
+        contains(tempFile.uri.toFilePath()),
       );
-      expect(await hashes.findOutdatedFileSystemEntity(), tempFile.uri);
     });
   });
 }

From 4814ef92db556cfb6c52cfa24f59e4bbee38d1a0 Mon Sep 17 00:00:00 2001
From: Daco Harkes <dacoharkes@google.com>
Date: Wed, 4 Dec 2024 10:51:14 +0100
Subject: [PATCH 3/3] address comments

---
 .../dependencies_hash_file.dart                   | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

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
index ba818bd32..2b1c49259 100644
--- 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
@@ -33,12 +33,12 @@ class DependenciesHashFile {
   void _reset() => _hashes = FileSystemHashes();
 
   /// Populate the hashes and persist file with entries from
-  /// [fileSystemEntities].
+  /// [fileSystemEntities] and [environment].
   ///
-  /// 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.
+  /// Any file system 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,
@@ -135,8 +135,9 @@ class DependenciesHashFile {
       return _hashNotExists;
     }
     final children = directory.listSync(followLinks: true, recursive: false);
-    final childrenNames = children.map((e) => _pathBaseName(e.path)).join(';');
-    return _md5int64(utf8.encode(childrenNames));
+    final childrenNames = children.map((e) => _pathBaseName(e.path)).toList()
+      ..sort();
+    return _md5int64(utf8.encode(childrenNames.join(';')));
   }
 
   int _hashEnvironmentValue(String? value) {