Roll-forward #164317: Use bin/cache/engine.stamp (#164401)

... and this time, create `bin/cache` before trying to write to it!

(Tests updated to catch this regression)

.gitignore

@@ -1,9 +1,6 @@
 # Do not remove or rename entries in this file, only add new ones
 # See https://github.com/flutter/flutter/issues/128635 for more context.
 
-# This is dynamic in a monorepo
-bin/internal/engine.version
-
 # Miscellaneous
 *.class
 *.lock
@@ -33,6 +30,8 @@ bin/internal/engine.version
 /bin/cache/
 /bin/internal/bootstrap.bat
 /bin/internal/bootstrap.sh
+/bin/internal/engine.realm
+/bin/internal/engine.version
 /bin/mingit/
 /dev/benchmarks/mega_gallery/
 /dev/bots/.recipe_deps

bin/internal/update_engine_version.ps1

@@ -19,9 +19,13 @@ $ErrorActionPreference = "Stop"
 $progName = Split-Path -parent $MyInvocation.MyCommand.Definition
 $flutterRoot = (Get-Item $progName).parent.parent.FullName
 
+# Generate a bin/cache directory, which won't initially exist for a fresh checkout.
+New-Item -Path "$flutterRoot/bin/cache" -ItemType Directory -Force | Out-Null
+
 # On stable, beta, and release tags, the engine.version is tracked by git - do not override it.
 $trackedEngine = (git -C "$flutterRoot" ls-files bin/internal/engine.version) | Out-String
 if ($trackedEngine.length -ne 0) {
+  Copy-Item -Path "$flutterRoot/bin/internal/engine.version" -Destination "$flutterRoot/bin/cache/engine.stamp" -Force
   return
 }
 
@@ -40,7 +44,7 @@ if (![string]::IsNullOrEmpty($env:FLUTTER_PREBUILT_ENGINE_VERSION)) {
 }
 
 # Test for fusion repository
-if ([string]::IsNullOrEmpty($engineVersion) -and (Test-Path "$flutterRoot\DEPS" -PathType Leaf) -and (Test-Path "$flutterRoot\engine\src\.gn" -PathType Leaf)) {
+if ([string]::IsNullOrEmpty($engineVersion) -and (Test-Path "$flutterRoot/DEPS" -PathType Leaf) -and (Test-Path "$flutterRoot/engine/src/.gn" -PathType Leaf)) {
     if ($null -eq $Env:LUCI_CONTEXT) {
         $ErrorActionPreference = "Continue"
         git -C "$flutterRoot" remote get-url upstream *> $null
@@ -59,9 +63,17 @@ if ([string]::IsNullOrEmpty($engineVersion) -and (Test-Path "$flutterRoot\DEPS"
 
 # Write the engine version out so downstream tools know what to look for.
 $utf8NoBom = New-Object System.Text.UTF8Encoding($false)
-[System.IO.File]::WriteAllText("$flutterRoot\bin\internal\engine.version", $engineVersion, $utf8NoBom)
+[System.IO.File]::WriteAllText("$flutterRoot/bin/cache/engine.stamp", $engineVersion, $utf8NoBom)
+# TODO(matanlurey): Stop writing to internal/engine.version. https://github.com/flutter/flutter/issues/164315.
+[System.IO.File]::WriteAllText("$flutterRoot/bin/internal/engine.version", $engineVersion, $utf8NoBom)
 
 # The realm on CI is passed in.
 if ($Env:FLUTTER_REALM) {
-    [System.IO.File]::WriteAllText("$flutterRoot\bin\internal\engine.realm", $Env:FLUTTER_REALM, $utf8NoBom)
+    [System.IO.File]::WriteAllText("$flutterRoot/bin/cache/engine.realm", $Env:FLUTTER_REALM, $utf8NoBom)
+    # TODO(matanlurey): Stop writing to internal/engine.realm. https://github.com/flutter/flutter/issues/164315.
+    [System.IO.File]::WriteAllText("$flutterRoot/bin/internal/engine.realm", $Env:FLUTTER_REALM, $utf8NoBom)
+} else {
+    [System.IO.File]::WriteAllText("$flutterRoot/bin/cache/engine.realm", "", $utf8NoBom)
+    # TODO(matanlurey): Stop writing to internal/engine.realm. https://github.com/flutter/flutter/issues/164315.
+    [System.IO.File]::WriteAllText("$flutterRoot/bin/internal/engine.realm", "", $utf8NoBom)
 }

bin/internal/update_engine_version.sh

@@ -33,9 +33,13 @@ fi
 
 FLUTTER_ROOT="$(dirname "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")"
 
+# Generate a bin/cache directory, which won't initially exist for a fresh checkout.
+mkdir -p "$FLUTTER_ROOT/bin/cache"
+
 # On stable, beta, and release tags, the engine.version is tracked by git - do not override it.
 TRACKED_ENGINE="$(git -C "$FLUTTER_ROOT" ls-files bin/internal/engine.version)"
 if [[ -n "$TRACKED_ENGINE" ]]; then
+  cp $FLUTTER_ROOT/bin/internal/engine.version $FLUTTER_ROOT/bin/cache/engine.stamp
   exit
 fi
 
@@ -60,9 +64,17 @@ if [ -z "$ENGINE_VERSION" ] && [ -f "$FLUTTER_ROOT/DEPS" ] && [ -f "$FLUTTER_ROO
 fi
 
 # Write the engine version out so downstream tools know what to look for.
+echo $ENGINE_VERSION > "$FLUTTER_ROOT/bin/cache/engine.stamp"
+# TODO(matanlurey): Stop writing to internal/engine.version. https://github.com/flutter/flutter/issues/164315.
 echo $ENGINE_VERSION > "$FLUTTER_ROOT/bin/internal/engine.version"
 
 # The realm on CI is passed in.
 if [ -n "${FLUTTER_REALM}" ]; then
+  echo $FLUTTER_REALM > "$FLUTTER_ROOT/bin/cache/engine.realm"
+  # TODO(matanlurey): Stop writing to internal/engine.realm. https://github.com/flutter/flutter/issues/164315.
   echo $FLUTTER_REALM > "$FLUTTER_ROOT/bin/internal/engine.realm"
+else
+  echo "" > "$FLUTTER_ROOT/bin/cache/engine.realm"
+  # TODO(matanlurey): Stop writing to internal/engine.realm. https://github.com/flutter/flutter/issues/164315.
+  echo "" > "$FLUTTER_ROOT/bin/internal/engine.realm"
 fi

dev/tools/test/update_engine_version_test.dart

@@ -23,8 +23,21 @@ import 'package:test/test.dart';
 //////////////////////////////////////////////////////////////////////
 
 void main() {
+  // Want to test the powershell (update_engine_version.ps1) file, but running
+  // a macOS or Linux machine? You can install powershell and then opt-in to
+  // running `pwsh bin/internal/update_engine_version.ps1`.
+  //
+  // macOS: https://learn.microsoft.com/en-us/powershell/scripting/install/installing-powershell-on-macos
+  // linux: https://learn.microsoft.com/en-us/powershell/scripting/install/installing-powershell-on-linux
+  //
+  // Then, set this variable to true:
+  const bool usePowershellOnPosix = false;
+
   const FileSystem localFs = LocalFileSystem();
-  final _FlutterRootUnderTest flutterRoot = _FlutterRootUnderTest.findWithin();
+  final _FlutterRootUnderTest flutterRoot = _FlutterRootUnderTest.findWithin(
+    // ignore: avoid_redundant_argument_values
+    forcePowershell: usePowershellOnPosix,
+  );
 
   late Directory tmpDir;
   late _FlutterRootUnderTest testRoot;
@@ -57,7 +70,11 @@ void main() {
 
   setUp(() async {
     tmpDir = localFs.systemTempDirectory.createTempSync('update_engine_version_test.');
-    testRoot = _FlutterRootUnderTest.fromPath(tmpDir.childDirectory('flutter').path);
+    testRoot = _FlutterRootUnderTest.fromPath(
+      tmpDir.childDirectory('flutter').path,
+      // ignore: avoid_redundant_argument_values
+      forcePowershell: usePowershellOnPosix,
+    );
 
     environment = <String, String>{};
     environment.addAll(io.Platform.environment);
@@ -67,17 +84,84 @@ void main() {
     flutterRoot.binInternalUpdateEngineVersion.copySyncRecursive(
       testRoot.binInternalUpdateEngineVersion.path,
     );
+
+    // Regression test for https://github.com/flutter/flutter/pull/164396;
+    // on a fresh checkout bin/cache does not exist, so avoid trying to create
+    // this folder.
+    if (testRoot.root.childDirectory('cache').existsSync()) {
+      fail('Do not initially create a bin/cache directory, it should be created by the script.');
+    }
   });
 
   tearDown(() {
-    tmpDir.deleteSync(recursive: true);
+    // Git adds a lot of files, we don't want to test for them.
+    final Directory gitDir = testRoot.root.childDirectory('.git');
+    if (gitDir.existsSync()) {
+      gitDir.deleteSync(recursive: true);
+    }
+
+    // Take a snapshot of files we expect to be created or otherwise exist.
+    //
+    // This gives a "dirty" check that we did not change the output characteristics
+    // of the tool without adding new tests for the new files.
+    final Set<String> expectedFiles = <String>{
+      localFs.path.join('bin', 'cache', 'engine.realm'),
+      localFs.path.join('bin', 'cache', 'engine.stamp'),
+      localFs.path.join(
+        'bin',
+        'internal',
+        localFs.path.basename(testRoot.binInternalUpdateEngineVersion.path),
+      ),
+      localFs.path.join('bin', 'internal', 'engine.realm'),
+      localFs.path.join('bin', 'internal', 'engine.version'),
+      localFs.path.join('engine', 'src', '.gn'),
+      'DEPS',
+    };
+    final Set<String> currentFiles =
+        tmpDir
+            .listSync(recursive: true)
+            .whereType<File>()
+            .map((File e) => localFs.path.relative(e.path, from: testRoot.root.path))
+            .toSet();
+
+    // If this test failed, print out the current directory structure.
+    printOnFailure(
+      'Files in virtual "flutter" directory when test failed:\n\n${(currentFiles.toList()..sort()).join('\n')}',
+    );
+
+    // Now do cleanup so even if the next step fails, we still deleted tmp.
+    print(tmpDir);
+    // tmpDir.deleteSync(recursive: true);
+
+    final Set<String> unexpectedFiles = currentFiles.difference(expectedFiles);
+    if (unexpectedFiles.isNotEmpty) {
+      final StringBuffer message = StringBuffer(
+        '\nOne or more files were generated by ${localFs.path.basename(testRoot.binInternalUpdateEngineVersion.path)} that were not expected:\n\n',
+      );
+      message.writeAll(unexpectedFiles, '\n');
+      message.writeln('\n');
+      message.writeln(
+        'If this was intentional update "expectedFiles" in dev/tools/test/update_engine_version_test.dart and add *new* tests for the new outputs.',
+      );
+      fail('$message');
+    }
   });
 
   io.ProcessResult runUpdateEngineVersion() {
-    final (String executable, List<String> args) =
+    final String executable;
-        const LocalPlatform().isWindows
+    final List<String> args;
-            ? ('powershell', <String>[testRoot.binInternalUpdateEngineVersion.path])
+    if (const LocalPlatform().isWindows) {
-            : (testRoot.binInternalUpdateEngineVersion.path, <String>[]);
+      executable = 'powershell';
+      args = <String>[testRoot.binInternalUpdateEngineVersion.path];
+      // ignore: dead_code
+    } else if (usePowershellOnPosix) {
+      executable = 'pwsh';
+      args = <String>[testRoot.binInternalUpdateEngineVersion.path];
+      // ignore: dead_code
+    } else {
+      executable = testRoot.binInternalUpdateEngineVersion.path;
+      args = <String>[];
+    }
     return run(executable, args);
   }
 
@@ -120,6 +204,7 @@ void main() {
         testRoot.binInternalEngineVersion.readAsStringSync(),
         equalsIgnoringWhitespace('123abc'),
       );
+      expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace('123abc'));
     });
   });
 
@@ -135,6 +220,10 @@ void main() {
       testRoot.binInternalEngineVersion.readAsStringSync(),
       equalsIgnoringWhitespace(engineVersionTrackedContents),
     );
+    expect(
+      testRoot.binCacheEngineStamp.readAsStringSync(),
+      equalsIgnoringWhitespace(engineVersionTrackedContents),
+    );
   });
 
   test('writes nothing, even if files are set, if we are on "3.29.0"', () async {
@@ -149,6 +238,10 @@ void main() {
       testRoot.binInternalEngineVersion.readAsStringSync(),
       equalsIgnoringWhitespace(engineVersionTrackedContents),
     );
+    expect(
+      testRoot.binCacheEngineStamp.readAsStringSync(),
+      equalsIgnoringWhitespace(engineVersionTrackedContents),
+    );
   });
 
   test('writes nothing, even if files are set, if we are on "beta"', () async {
@@ -163,6 +256,10 @@ void main() {
       testRoot.binInternalEngineVersion.readAsStringSync(),
       equalsIgnoringWhitespace(engineVersionTrackedContents),
     );
+    expect(
+      testRoot.binCacheEngineStamp.readAsStringSync(),
+      equalsIgnoringWhitespace(engineVersionTrackedContents),
+    );
   });
 
   group('if DEPS and engine/src/.gn are present, engine.version is derived from', () {
@@ -185,6 +282,10 @@ void main() {
         testRoot.binInternalEngineVersion.readAsStringSync(),
         equalsIgnoringWhitespace(mergeBaseHeadUpstream.stdout as String),
       );
+      expect(
+        testRoot.binCacheEngineStamp.readAsStringSync(),
+        equalsIgnoringWhitespace(mergeBaseHeadUpstream.stdout as String),
+      );
     });
 
     test('merge-base HEAD origin/master on non-LUCI when upstream is not set', () async {
@@ -202,6 +303,10 @@ void main() {
         testRoot.binInternalEngineVersion.readAsStringSync(),
         equalsIgnoringWhitespace(mergeBaseHeadOrigin.stdout as String),
       );
+      expect(
+        testRoot.binCacheEngineStamp.readAsStringSync(),
+        equalsIgnoringWhitespace(mergeBaseHeadOrigin.stdout as String),
+      );
     });
 
     test('rev-parse HEAD when running on LUCI', () async {
@@ -214,6 +319,10 @@ void main() {
         testRoot.binInternalEngineVersion.readAsStringSync(),
         equalsIgnoringWhitespace(revParseHead.stdout as String),
       );
+      expect(
+        testRoot.binCacheEngineStamp.readAsStringSync(),
+        equalsIgnoringWhitespace(revParseHead.stdout as String),
+      );
     });
   });
 
@@ -233,6 +342,7 @@ void main() {
 
       expect(testRoot.binInternalEngineVersion, exists);
       expect(testRoot.binInternalEngineVersion.readAsStringSync(), equalsIgnoringWhitespace(''));
+      expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace(''));
     });
 
     test('[engine/src/.gn] engine.version is blank', () async {
@@ -242,6 +352,45 @@ void main() {
 
       expect(testRoot.binInternalEngineVersion, exists);
       expect(testRoot.binInternalEngineVersion.readAsStringSync(), equalsIgnoringWhitespace(''));
+      expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace(''));
+    });
+  });
+
+  group('engine.realm', () {
+    setUp(() {
+      for (final File f in <File>[testRoot.deps, testRoot.engineSrcGn]) {
+        f.createSync(recursive: true);
+      }
+      setupRepo(branch: 'master');
+      setupRemote(remote: 'origin');
+    });
+
+    test('is empty if the FLUTTER_REALM environment variable is not set', () {
+      expect(environment, isNot(contains('FLUTTER_REALM')));
+
+      runUpdateEngineVersion();
+
+      expect(testRoot.binCacheEngineRealm, exists);
+      expect(testRoot.binCacheEngineRealm.readAsStringSync(), equalsIgnoringWhitespace(''));
+      expect(testRoot.binInternalEngineRealm, exists);
+      expect(testRoot.binInternalEngineRealm.readAsStringSync(), equalsIgnoringWhitespace(''));
+    });
+
+    test('contains the FLUTTER_REALM environment variable', () async {
+      environment['FLUTTER_REALM'] = 'flutter_archives_v2';
+
+      runUpdateEngineVersion();
+
+      expect(testRoot.binCacheEngineRealm, exists);
+      expect(
+        testRoot.binCacheEngineRealm.readAsStringSync(),
+        equalsIgnoringWhitespace('flutter_archives_v2'),
+      );
+      expect(testRoot.binInternalEngineRealm, exists);
+      expect(
+        testRoot.binInternalEngineRealm.readAsStringSync(),
+        equalsIgnoringWhitespace('flutter_archives_v2'),
+      );
     });
   });
 }
@@ -270,6 +419,7 @@ final class _FlutterRootUnderTest {
     String path, {
     FileSystem fileSystem = const LocalFileSystem(),
     Platform platform = const LocalPlatform(),
+    bool forcePowershell = false,
   }) {
     final Directory root = fileSystem.directory(path);
     return _FlutterRootUnderTest._(
@@ -279,23 +429,26 @@ final class _FlutterRootUnderTest {
       binInternalEngineVersion: root.childFile(
         fileSystem.path.join('bin', 'internal', 'engine.version'),
       ),
+      binCacheEngineRealm: root.childFile(fileSystem.path.join('bin', 'cache', 'engine.realm')),
       binInternalEngineRealm: root.childFile(
         fileSystem.path.join('bin', 'internal', 'engine.realm'),
       ),
+      binCacheEngineStamp: root.childFile(fileSystem.path.join('bin', 'cache', 'engine.stamp')),
       binInternalUpdateEngineVersion: root.childFile(
         fileSystem.path.join(
           'bin',
           'internal',
-          'update_engine_version.${platform.isWindows ? 'ps1' : 'sh'}',
+          'update_engine_version.${platform.isWindows || forcePowershell ? 'ps1' : 'sh'}',
         ),
       ),
     );
   }
 
-  factory _FlutterRootUnderTest.findWithin([
+  factory _FlutterRootUnderTest.findWithin({
     String? path,
     FileSystem fileSystem = const LocalFileSystem(),
-  ]) {
+    bool forcePowershell = false,
+  }) {
     path ??= fileSystem.currentDirectory.path;
     Directory current = fileSystem.directory(path);
     while (!current.childFile('DEPS').existsSync()) {
@@ -304,14 +457,16 @@ final class _FlutterRootUnderTest {
       }
       current = current.parent;
     }
-    return _FlutterRootUnderTest.fromPath(current.path);
+    return _FlutterRootUnderTest.fromPath(current.path, forcePowershell: forcePowershell);
   }
 
   const _FlutterRootUnderTest._(
     this.root, {
     required this.deps,
     required this.engineSrcGn,
+    required this.binCacheEngineStamp,
     required this.binInternalEngineVersion,
+    required this.binCacheEngineRealm,
     required this.binInternalEngineRealm,
     required this.binInternalUpdateEngineVersion,
   });
@@ -331,13 +486,35 @@ final class _FlutterRootUnderTest {
   /// `bin/internal/engine.version`.
   ///
   /// This file contains a SHA of which engine binaries to download.
-  final File binInternalEngineVersion;
+  ///
+  /// Currently, the SHA is either _computed_ or _pre-determined_, based on if
+  /// the file is checked-in and tracked. That behavior is changing, and in the
+  /// future this will be a checked-in file and not computed.
+  ///
+  /// See also: https://github.com/flutter/flutter/issues/164315.
+  final File binInternalEngineVersion; // TODO(matanlurey): Update these docs.
+
+  /// `bin/cache/engine.stamp`.
+  ///
+  /// This file contains a _computed_ SHA of which engine binaries to download.
+  final File binCacheEngineStamp;
 
   /// `bin/internal/engine.realm`.
   ///
-  /// It is a mystery what this file contains, but it's set by `FLUTTER_REALM`.
+  /// If non-empty, the value comes from the environment variable `FLUTTER_REALM`,
+  /// which instructs the tool where the SHA stored in [binInternalEngineVersion]
+  /// should be fetched from (it differs for presubmits run for flutter/flutter
+  /// and builds downloaded by end-users or by postsubmits).
   final File binInternalEngineRealm;
 
+  /// `bin/cache/engine.realm`.
+  ///
+  /// If non-empty, the value comes from the environment variable `FLUTTER_REALM`,
+  /// which instructs the tool where the SHA stored in [binInternalEngineVersion]
+  /// should be fetched from (it differs for presubmits run for flutter/flutter
+  /// and builds downloaded by end-users or by postsubmits).
+  final File binCacheEngineRealm;
+
   /// `bin/internal/update_engine_version.{sh|ps1}`.
   ///
   /// This file contains a shell script that conditionally writes, on execution: