From 34a11c405d073ae2b5f408cbc14e33db289a1a54 Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Fri, 28 Feb 2025 20:16:36 +0000 Subject: [PATCH] Reverts "Write an identical value to `bin/cache/engine.stamp` to prepare for migration (#164317)" (#164396) Reverts: flutter/flutter#164317 Initiated by: matanlurey Reason for reverting: `bin/cache` does not exist on a fresh checkout, and `echo bin/cache/...` will fail as a result. This blocked the google3 roll, but would also break new checkouts of Flutter, for regular users/contributors. Original PR Author: matanlurey Reviewed By: {jtmcdole} This change reverts the following previous change: Towards https://github.com/flutter/flutter/issues/164315. This PR just writes `bin/cache/engine.stamp` identically to how `bin/internal/engine.version` would otherwise be written, with a caveat that _if_ `engine.version` is tracked, it is now _copied_ to `bin/cache/engine.stamp`. After this lands, I'll send PRs to update tooling that looks for `engine.version` and give a heads up to the larger team (i.e. Dart HH bot or whomever we will break by doing this). Co-authored-by: auto-submit[bot] --- .gitignore | 5 +- bin/internal/engine.realm | 0 bin/internal/update_engine_version.ps1 | 11 +- bin/internal/update_engine_version.sh | 9 -- .../test/update_engine_version_test.dart | 145 +----------------- 5 files changed, 6 insertions(+), 164 deletions(-) create mode 100644 bin/internal/engine.realm diff --git a/.gitignore b/.gitignore index e1616bd967..1198fbaa72 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,9 @@ # 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 @@ -30,8 +33,6 @@ /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 new file mode 100644 index 0000000000..e69de29bb2 diff --git a/bin/internal/update_engine_version.ps1 b/bin/internal/update_engine_version.ps1 index 2b73dce02c..af087ceaed 100644 --- a/bin/internal/update_engine_version.ps1 +++ b/bin/internal/update_engine_version.ps1 @@ -22,7 +22,6 @@ $flutterRoot = (Get-Item $progName).parent.parent.FullName # 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 } @@ -60,17 +59,9 @@ 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\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\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) -} \ No newline at end of file +} diff --git a/bin/internal/update_engine_version.sh b/bin/internal/update_engine_version.sh index f7e405012f..f4f675f878 100755 --- a/bin/internal/update_engine_version.sh +++ b/bin/internal/update_engine_version.sh @@ -36,7 +36,6 @@ FLUTTER_ROOT="$(dirname "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" # 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 @@ -61,17 +60,9 @@ 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 db6307205b..0ffd16c898 100644 --- a/dev/tools/test/update_engine_version_test.dart +++ b/dev/tools/test/update_engine_version_test.dart @@ -58,7 +58,6 @@ void main() { setUp(() async { tmpDir = localFs.systemTempDirectory.createTempSync('update_engine_version_test.'); testRoot = _FlutterRootUnderTest.fromPath(tmpDir.childDirectory('flutter').path); - testRoot.root.childDirectory('bin').childDirectory('cache').createSync(recursive: true); environment = {}; environment.addAll(io.Platform.environment); @@ -71,56 +70,7 @@ void main() { }); tearDown(() { - // 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: ${(currentFiles.toList()..sort()).join('\n')}', - ); - - // Now do cleanup so even if the next step fails, we still deleted tmp. tmpDir.deleteSync(recursive: true); - - final Set unexpectedFiles = currentFiles.difference(expectedFiles); - if (unexpectedFiles.isNotEmpty) { - final StringBuffer message = StringBuffer( - 'One 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() { @@ -170,7 +120,6 @@ void main() { testRoot.binInternalEngineVersion.readAsStringSync(), equalsIgnoringWhitespace('123abc'), ); - expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace('123abc')); }); }); @@ -186,10 +135,6 @@ 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 { @@ -204,10 +149,6 @@ 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 { @@ -222,10 +163,6 @@ 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', () { @@ -248,10 +185,6 @@ 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 { @@ -269,10 +202,6 @@ 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 { @@ -285,10 +214,6 @@ void main() { testRoot.binInternalEngineVersion.readAsStringSync(), equalsIgnoringWhitespace(revParseHead.stdout as String), ); - expect( - testRoot.binCacheEngineStamp.readAsStringSync(), - equalsIgnoringWhitespace(revParseHead.stdout as String), - ); }); }); @@ -308,7 +233,6 @@ 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 { @@ -318,45 +242,6 @@ 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'), - ); }); }); } @@ -394,11 +279,9 @@ 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', @@ -428,9 +311,7 @@ final class _FlutterRootUnderTest { this.root, { required this.deps, required this.engineSrcGn, - required this.binCacheEngineStamp, required this.binInternalEngineVersion, - required this.binCacheEngineRealm, required this.binInternalEngineRealm, required this.binInternalUpdateEngineVersion, }); @@ -450,35 +331,13 @@ final class _FlutterRootUnderTest { /// `bin/internal/engine.version`. /// /// This file contains a SHA of which engine binaries to download. - /// - /// 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; + final File binInternalEngineVersion; /// `bin/internal/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). + /// It is a mystery what this file contains, but it's set by `FLUTTER_REALM`. 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: