From d859e2f43e16f64035e1d6cd8a43edc39a98af8c Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 28 Feb 2025 16:10:44 -0800 Subject: [PATCH] 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 | 5 +- bin/internal/engine.realm | 0 bin/internal/update_engine_version.ps1 | 18 +- bin/internal/update_engine_version.sh | 12 ++ .../test/update_engine_version_test.dart | 203 ++++++++++++++++-- 5 files changed, 219 insertions(+), 19 deletions(-) delete mode 100644 bin/internal/engine.realm diff --git a/.gitignore b/.gitignore index 1198fbaa72..e1616bd967 100644 --- a/.gitignore +++ b/.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 diff --git a/bin/internal/engine.realm b/bin/internal/engine.realm deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/bin/internal/update_engine_version.ps1 b/bin/internal/update_engine_version.ps1 index af087ceaed..34ad5b0bfc 100644 --- a/bin/internal/update_engine_version.ps1 +++ b/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) } diff --git a/bin/internal/update_engine_version.sh b/bin/internal/update_engine_version.sh index f4f675f878..e302b9b5d6 100755 --- a/bin/internal/update_engine_version.sh +++ b/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 diff --git a/dev/tools/test/update_engine_version_test.dart b/dev/tools/test/update_engine_version_test.dart index 0ffd16c898..ca5c6114d6 100644 --- a/dev/tools/test/update_engine_version_test.dart +++ b/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 = {}; 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 expectedFiles = { + 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 currentFiles = + tmpDir + .listSync(recursive: true) + .whereType() + .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 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 args) = - const LocalPlatform().isWindows - ? ('powershell', [testRoot.binInternalUpdateEngineVersion.path]) - : (testRoot.binInternalUpdateEngineVersion.path, []); + final String executable; + final List args; + if (const LocalPlatform().isWindows) { + executable = 'powershell'; + args = [testRoot.binInternalUpdateEngineVersion.path]; + // ignore: dead_code + } else if (usePowershellOnPosix) { + executable = 'pwsh'; + args = [testRoot.binInternalUpdateEngineVersion.path]; + // ignore: dead_code + } else { + executable = testRoot.binInternalUpdateEngineVersion.path; + args = []; + } 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 [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: