From 2f2837b30d53c5b72cd73bdd5531a3a2fbe131a6 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 3 Mar 2025 17:08:59 -0800 Subject: [PATCH] Overhaul `update_engine_version.{sh|ps1}` to reflect the new computation flow (#164513) Closes https://github.com/flutter/flutter/issues/164030. Closes https://github.com/flutter/flutter/issues/164315. Towards https://github.com/flutter/flutter/issues/163896. Significantly simplified! We removed: - Conditional checks for monorepo (it's always a monorepo) - Conditional checks for `LUCI_CONTEXT` (LUCI takes care of itself now, see https://github.com/flutter/cocoon/pull/4261) - ... and made the branching logic easier to follow as a result You can see the results first hand in the tests, which are now much simpler. Canonical docs: https://github.com/flutter/flutter/blob/master/docs/tool/Engine-artifacts.md. --- .gitignore | 11 +- bin/internal/update_engine_version.ps1 | 78 ++-- bin/internal/update_engine_version.sh | 90 ++--- .../test/update_engine_version_test.dart | 379 +++++++----------- 4 files changed, 235 insertions(+), 323 deletions(-) diff --git a/.gitignore b/.gitignore index e1616bd967..add0a5894c 100644 --- a/.gitignore +++ b/.gitignore @@ -26,12 +26,21 @@ .vscode/* .ccls-cache +# This file, on the master branch, should never exist or be checked-in. +# +# On a *final* release branch, that is, what will ship to stable or beta, the +# file can be force added (git add --force) and checked-in in order to effectively +# "pin" the engine artifact version so the flutter tool does not need to use git +# to determine the engine artifacts. +# +# See https://github.com/flutter/flutter/blob/main/docs/tool/Engine-artifacts.md. +/bin/internal/engine.version + # Flutter repo-specific /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/update_engine_version.ps1 b/bin/internal/update_engine_version.ps1 index 34ad5b0bfc..8d8af9f9bb 100644 --- a/bin/internal/update_engine_version.ps1 +++ b/bin/internal/update_engine_version.ps1 @@ -2,9 +2,10 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -# Want to test this script? -# $ cd dev/tools -# $ dart test test/update_engine_version_test.dart +# Based on the current repository state, writes the following two files to disk: +# +# bin/cache/engine.stamp <-- SHA of the commit that engine artifacts were built +# bin/cache/engine.realm <-- optional; ; whether the SHA is from presubmit builds or staging (bringup: true). # ---------------------------------- NOTE ---------------------------------- # # @@ -12,6 +13,12 @@ # `update_engine_version.sh` script in the same directory to ensure that Flutter # continues to work across all platforms! # +# https://github.com/flutter/flutter/blob/main/docs/tool/Engine-artifacts.md. +# +# Want to test this script? +# $ cd dev/tools +# $ dart test test/update_engine_version_test.dart +# # -------------------------------------------------------------------------- # $ErrorActionPreference = "Stop" @@ -22,58 +29,47 @@ $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 -} - -# Allow overriding the intended engine version via FLUTTER_PREBUILT_ENGINE_VERSION. +# Check if FLUTTER_PREBUILT_ENGINE_VERSION is set # -# This is for systems, such as Github Actions, where we know ahead of time the -# base-ref we want to use (to download the engine binaries and avoid trying -# to compute one below), or for the Dart HH bot, which wants to try the current -# Flutter framework/engine with a different Dart SDK. +# This is intended for systems where we intentionally want to (ephemerally) use +# a specific engine artifacts version (which includes the Flutter engine and +# the Dart SDK), such as on CI. # -# This environment variable is EXPERIMENTAL. If you are not on the Flutter infra -# or Dart infra teams, this code path might be removed at anytime and cease -# functioning. Please file an issue if you have workflow needs. +# If set, it takes precedence over any other source of engine version. if (![string]::IsNullOrEmpty($env:FLUTTER_PREBUILT_ENGINE_VERSION)) { $engineVersion = $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 ($null -eq $Env:LUCI_CONTEXT) { - $ErrorActionPreference = "Continue" - git -C "$flutterRoot" remote get-url upstream *> $null - $exitCode = $? - $ErrorActionPreference = "Stop" - if ($exitCode) { - $engineVersion = (git -C "$flutterRoot" merge-base HEAD upstream/master) - } else { - $engineVersion = (git -C "$flutterRoot" merge-base HEAD origin/master) - } - } - else { - $engineVersion = (git -C "$flutterRoot" rev-parse HEAD) - } +# Check if bin/internal/engine.version exists and is a tracked file in git. +# +# This is intended for a user-shipped stable or beta release, where the release +# has a specific (pinned) engine artifacts version. +# +# If set, it takes precedence over the git hash. +} elseif (git -C "$flutterRoot" ls-files bin/internal/engine.version) { + $engineVersion = Get-Content -Path "$flutterRoot/bin/internal/engine.version" + +# Fallback to using git to triangulate which upstream/master (or origin/master) +# the current branch is forked from, which would be the last version of the +# engine artifacts built from CI. +} else { + $ErrorActionPreference = "Continue" + git -C "$flutterRoot" remote get-url upstream *> $null + $exitCode = $? + $ErrorActionPreference = "Stop" + if ($exitCode) { + $engineVersion = (git -C "$flutterRoot" merge-base HEAD upstream/master) + } else { + $engineVersion = (git -C "$flutterRoot" merge-base HEAD origin/master) + } } # 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) } diff --git a/bin/internal/update_engine_version.sh b/bin/internal/update_engine_version.sh index e302b9b5d6..8bc0bcaf8a 100755 --- a/bin/internal/update_engine_version.sh +++ b/bin/internal/update_engine_version.sh @@ -3,9 +3,10 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -# Want to test this script? -# $ cd dev/tools -# $ dart test test/update_engine_version_test.dart +# Based on the current repository state, writes the following two files to disk: +# +# bin/cache/engine.stamp <-- SHA of the commit that engine artifacts were built +# bin/cache/engine.realm <-- optional; whether the SHA is from presubmit builds or staging (bringup: true). # ---------------------------------- NOTE ---------------------------------- # # @@ -13,68 +14,63 @@ # `update_engine_version.ps1` script in the same directory to ensure that Flutter # continues to work across all platforms! # +# https://github.com/flutter/flutter/blob/main/docs/tool/Engine-artifacts.md. +# +# Want to test this script? +# $ cd dev/tools +# $ dart test test/update_engine_version_test.dart +# # -------------------------------------------------------------------------- # set -e -# Allow overriding the intended engine version via FLUTTER_PREBUILT_ENGINE_VERSION. -# -# This is for systems, such as Github Actions, where we know ahead of time the -# base-ref we want to use (to download the engine binaries and avoid trying -# to compute one below), or for the Dart HH bot, which wants to try the current -# Flutter framework/engine with a different Dart SDK. -# -# This environment variable is EXPERIMENTAL. If you are not on the Flutter infra -# or Dart infra teams, this code path might be removed at anytime and cease -# functioning. Please file an issue if you have workflow needs. -if [ -n "${FLUTTER_PREBUILT_ENGINE_VERSION}" ]; then - ENGINE_VERSION="${FLUTTER_PREBUILT_ENGINE_VERSION}" -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 +# Check if FLUTTER_PREBUILT_ENGINE_VERSION is set +# +# This is intended for systems where we intentionally want to (ephemerally) use +# a specific engine artifacts version (which includes the Flutter engine and +# the Dart SDK), such as on CI. +# +# If set, it takes precedence over any other source of engine version. +if [ -n "${FLUTTER_PREBUILT_ENGINE_VERSION}" ]; then + ENGINE_VERSION="${FLUTTER_PREBUILT_ENGINE_VERSION}" -# Test for fusion repository and no environment variable override. -if [ -z "$ENGINE_VERSION" ] && [ -f "$FLUTTER_ROOT/DEPS" ] && [ -f "$FLUTTER_ROOT/engine/src/.gn" ]; then - # In a fusion repository; the engine.version comes from the git hashes. - if [ -z "${LUCI_CONTEXT}" ]; then - set +e - # Run the git command and capture the exit code - git -C "$FLUTTER_ROOT" remote get-url upstream > /dev/null 2>&1 - exit_code=$? - set -e +# Check if bin/internal/engine.version exists and is a tracked file in git. +# +# This is intended for a user-shipped stable or beta release, where the release +# has a specific (pinned) engine artifacts version. +# +# If set, it takes precedence over the git hash. +elif [ -n "$(git -C "$FLUTTER_ROOT" ls-files bin/internal/engine.version)" ]; then + ENGINE_VERSION="$(cat "$FLUTTER_ROOT/bin/internal/engine.version")" - if [[ $exit_code -eq 0 ]]; then - ENGINE_VERSION=$(git -C "$FLUTTER_ROOT" merge-base HEAD upstream/master) - else - ENGINE_VERSION=$(git -C "$FLUTTER_ROOT" merge-base HEAD origin/master) - fi +# Fallback to using git to triangulate which upstream/master (or origin/master) +# the current branch is forked from, which would be the last version of the +# engine artifacts built from CI. +else + set +e + # We fallback to origin/master if upstream is not detected. + git -C "$FLUTTER_ROOT" remote get-url upstream >/dev/null 2>&1 + exit_code=$? + set -e + + if [[ $exit_code -eq 0 ]]; then + ENGINE_VERSION=$(git -C "$FLUTTER_ROOT" merge-base HEAD upstream/master) else - ENGINE_VERSION=$(git -C "$FLUTTER_ROOT" rev-parse HEAD) + ENGINE_VERSION=$(git -C "$FLUTTER_ROOT" merge-base HEAD origin/master) fi 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" +echo $ENGINE_VERSION >"$FLUTTER_ROOT/bin/cache/engine.stamp" # 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" + echo $FLUTTER_REALM >"$FLUTTER_ROOT/bin/cache/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" + echo "" >"$FLUTTER_ROOT/bin/cache/engine.realm" fi diff --git a/dev/tools/test/update_engine_version_test.dart b/dev/tools/test/update_engine_version_test.dart index f938a01f8b..bbb03dca5c 100644 --- a/dev/tools/test/update_engine_version_test.dart +++ b/dev/tools/test/update_engine_version_test.dart @@ -9,7 +9,6 @@ import 'dart:io' as io; import 'package:file/file.dart'; import 'package:file/local.dart'; -import 'package:file_testing/file_testing.dart'; import 'package:platform/platform.dart'; import 'package:test/test.dart'; @@ -31,11 +30,13 @@ void main() { // linux: https://learn.microsoft.com/en-us/powershell/scripting/install/installing-powershell-on-linux // // Then, set this variable to true: - const bool usePowershellOnPosix = false; + final bool usePowershellOnPosix = () { + // Intentionally not a const so that linting doesn't go wild across the test. + return false; + }(); const FileSystem localFs = LocalFileSystem(); final _FlutterRootUnderTest flutterRoot = _FlutterRootUnderTest.findWithin( - // ignore: avoid_redundant_argument_values forcePowershell: usePowershellOnPosix, ); @@ -61,24 +62,30 @@ void main() { includeParentEnvironment: false, ); if (result.exitCode != 0) { - print('exitCode: ${result.exitCode}'); + fail('Failed running "$executable $args" (exit code = ${result.exitCode})'); } printIfNotEmpty('stdout', (result.stdout as String).trim()); printIfNotEmpty('stderr', (result.stderr as String).trim()); return result; } + setUpAll(() async { + if (usePowershellOnPosix) { + final io.ProcessResult result = io.Process.runSync('pwsh', ['--version']); + print('Using Powershell (${result.stdout}) on POSIX for local debugging and testing'); + } + }); + setUp(() async { tmpDir = localFs.systemTempDirectory.createTempSync('update_engine_version_test.'); testRoot = _FlutterRootUnderTest.fromPath( tmpDir.childDirectory('flutter').path, - // ignore: avoid_redundant_argument_values forcePowershell: usePowershellOnPosix, ); environment = {}; - if (const LocalPlatform().isWindows) { + if (const LocalPlatform().isWindows || usePowershellOnPosix) { // Copy a minimal set of environment variables needed to run the update_engine_version script in PowerShell. const List powerShellVariables = ['SystemRoot', 'Path', 'PATHEXT']; for (final String key in powerShellVariables) { @@ -121,7 +128,6 @@ void main() { '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', @@ -139,8 +145,7 @@ void main() { ); // Now do cleanup so even if the next step fails, we still deleted tmp. - print(tmpDir); - // tmpDir.deleteSync(recursive: true); + tmpDir.deleteSync(recursive: true); final Set unexpectedFiles = currentFiles.difference(expectedFiles); if (unexpectedFiles.isNotEmpty) { @@ -156,17 +161,22 @@ void main() { } }); + /// Runs `bin/internal/update_engine_version.{sh|ps1}` and returns the process result. + /// + /// If the exit code is 0, it is considered a success, and files should exist as a side-effect. + /// + /// - On Windows, `powershell` is used (to run `update_engine_version.ps1`); + /// - On POSIX, if [usePowershellOnPosix] is set, `pwsh` is used (to run `update_engine_version.ps1`); + /// - Otherwise, `update_engine_version.sh` is used. io.ProcessResult runUpdateEngineVersion() { 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 = []; @@ -174,234 +184,123 @@ void main() { return run(executable, args); } - void setupRepo({required String branch}) { - for (final File f in [testRoot.deps, testRoot.engineSrcGn]) { - f.createSync(recursive: true); - } - + /// Initializes a blank git repo in [testRoot.root]. + void initGitRepoWithBlankInitialCommit() { run('git', ['init', '--initial-branch', 'master']); run('git', ['config', '--local', 'user.email', 'test@example.com']); run('git', ['config', '--local', 'user.name', 'Test User']); run('git', ['add', '.']); run('git', ['commit', '-m', 'Initial commit']); - if (branch != 'master') { - run('git', ['checkout', '-b', branch]); + } + + /// Creates a `bin/internal/engine.version` file in [testRoot]. + /// + /// If [gitTrack] is `false`, the files are left untracked by git. + void pinEngineVersionForReleaseBranch({required String engineHash, bool gitTrack = true}) { + testRoot.binInternalEngineVersion.writeAsStringSync(engineHash); + if (gitTrack) { + run('git', ['add', '-f', 'bin/internal/engine.version']); + run('git', ['commit', '-m', 'tracking engine.version']); } } - const String engineVersionTrackedContents = 'already existing contents'; - void setupTrackedEngineVersion() { - testRoot.binInternalEngineVersion.writeAsStringSync(engineVersionTrackedContents); - run('git', ['add', '-f', 'bin/internal/engine.version']); - run('git', ['commit', '-m', 'tracking engine.version']); - } - + /// Sets up and fetches a [remote] (such as `upstream` or `origin`) for [testRoot.root]. + /// + /// The remote points at itself (`testRoot.root.path`) for ease of testing. void setupRemote({required String remote}) { run('git', ['remote', 'add', remote, testRoot.root.path]); run('git', ['fetch', remote]); } + /// Returns the SHA computed by `merge-base HEAD {{ref}}/master`. + String gitMergeBase({required String ref}) { + final io.ProcessResult mergeBaseHeadOrigin = run('git', [ + 'merge-base', + 'HEAD', + '$ref/master', + ]); + return mergeBaseHeadOrigin.stdout as String; + } + group('if FLUTTER_PREBUILT_ENGINE_VERSION is set', () { setUp(() { environment['FLUTTER_PREBUILT_ENGINE_VERSION'] = '123abc'; - setupRepo(branch: 'master'); + initGitRepoWithBlankInitialCommit(); }); - test('writes it to engine.version with no git interaction', () async { + test('writes it to cache/engine.stamp with no git interaction', () async { runUpdateEngineVersion(); - expect(testRoot.binInternalEngineVersion, exists); - expect( - testRoot.binInternalEngineVersion.readAsStringSync(), - equalsIgnoringWhitespace('123abc'), - ); - expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace('123abc')); - }); - }); - - test('writes nothing, even if files are set, if we are on "stable"', () async { - setupRepo(branch: 'stable'); - setupTrackedEngineVersion(); - setupRemote(remote: 'upstream'); - - runUpdateEngineVersion(); - - expect(testRoot.binInternalEngineVersion, exists); - expect( - 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 { - setupRepo(branch: '3.29.0'); - setupTrackedEngineVersion(); - setupRemote(remote: 'upstream'); - - runUpdateEngineVersion(); - - expect(testRoot.binInternalEngineVersion, exists); - expect( - 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 { - setupRepo(branch: 'beta'); - setupTrackedEngineVersion(); - setupRemote(remote: 'upstream'); - - runUpdateEngineVersion(); - - expect(testRoot.binInternalEngineVersion, exists); - expect( - 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', () { - setUp(() async { - setupRepo(branch: 'master'); + expect(testRoot.binCacheEngineStamp, _hasFileContentsMatching('123abc')); }); - test('merge-base HEAD upstream/master on non-LUCI when upstream is set', () async { - setupRemote(remote: 'upstream'); - - final io.ProcessResult mergeBaseHeadUpstream = run('git', [ - 'merge-base', - 'HEAD', - 'upstream/master', - ]); + test('takes precedence over bin/internal/engine.version, even if set', () async { + pinEngineVersionForReleaseBranch(engineHash: '456def'); runUpdateEngineVersion(); - expect(testRoot.binInternalEngineVersion, exists); - expect( - 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 { - setupRemote(remote: 'origin'); - - final io.ProcessResult mergeBaseHeadOrigin = run('git', [ - 'merge-base', - 'HEAD', - 'origin/master', - ]); - runUpdateEngineVersion(); - - expect(testRoot.binInternalEngineVersion, exists); - expect( - 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 { - environment['LUCI_CONTEXT'] = '_NON_NULL_AND_NON_EMPTY_STRING'; - runUpdateEngineVersion(); - - final io.ProcessResult revParseHead = run('git', ['rev-parse', 'HEAD']); - expect(testRoot.binInternalEngineVersion, exists); - expect( - testRoot.binInternalEngineVersion.readAsStringSync(), - equalsIgnoringWhitespace(revParseHead.stdout as String), - ); - expect( - testRoot.binCacheEngineStamp.readAsStringSync(), - equalsIgnoringWhitespace(revParseHead.stdout as String), - ); + expect(testRoot.binCacheEngineStamp, _hasFileContentsMatching('123abc')); }); }); - group('if DEPS or engine/src/.gn are omitted', () { + group('if bin/internal/engine.version is set', () { setUp(() { - for (final File f in [testRoot.deps, testRoot.engineSrcGn]) { - f.createSync(recursive: true); - } - setupRepo(branch: 'master'); + initGitRepoWithBlankInitialCommit(); + }); + + test('and tracked it is used', () async { + setupRemote(remote: 'upstream'); + pinEngineVersionForReleaseBranch(engineHash: 'abc123'); + runUpdateEngineVersion(); + + expect(testRoot.binCacheEngineStamp, _hasFileContentsMatching('abc123')); + }); + + test('but not tracked, it is ignored', () async { + setupRemote(remote: 'upstream'); + pinEngineVersionForReleaseBranch(engineHash: 'abc123', gitTrack: false); + runUpdateEngineVersion(); + + expect(testRoot.binCacheEngineStamp, _hasFileContentsMatching(gitMergeBase(ref: 'upstream'))); + }); + }); + + group('resolves engine artifacts with git merge-base', () { + setUp(() { + initGitRepoWithBlankInitialCommit(); + }); + + test('default to upstream/master if available', () async { + setupRemote(remote: 'upstream'); + runUpdateEngineVersion(); + + expect(testRoot.binCacheEngineStamp, _hasFileContentsMatching(gitMergeBase(ref: 'upstream'))); + }); + + test('fallsback to origin/master', () async { setupRemote(remote: 'origin'); - }); - - test('[DEPS] engine.version is blank', () async { - testRoot.deps.deleteSync(); - runUpdateEngineVersion(); - expect(testRoot.binInternalEngineVersion, exists); - expect(testRoot.binInternalEngineVersion.readAsStringSync(), equalsIgnoringWhitespace('')); - expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace('')); - }); - - test('[engine/src/.gn] engine.version is blank', () async { - testRoot.engineSrcGn.deleteSync(); - - runUpdateEngineVersion(); - - expect(testRoot.binInternalEngineVersion, exists); - expect(testRoot.binInternalEngineVersion.readAsStringSync(), equalsIgnoringWhitespace('')); - expect(testRoot.binCacheEngineStamp.readAsStringSync(), equalsIgnoringWhitespace('')); + expect(testRoot.binCacheEngineStamp, _hasFileContentsMatching(gitMergeBase(ref: 'origin'))); }); }); group('engine.realm', () { setUp(() { - for (final File f in [testRoot.deps, testRoot.engineSrcGn]) { - f.createSync(recursive: true); - } - setupRepo(branch: 'master'); - setupRemote(remote: 'origin'); + initGitRepoWithBlankInitialCommit(); + environment['FLUTTER_PREBUILT_ENGINE_VERSION'] = '123abc'; }); - test('is empty if the FLUTTER_REALM environment variable is not set', () { - expect(environment, isNot(contains('FLUTTER_REALM'))); - + test('is empty by default', () async { runUpdateEngineVersion(); - expect(testRoot.binCacheEngineRealm, exists); - expect(testRoot.binCacheEngineRealm.readAsStringSync(), equalsIgnoringWhitespace('')); - expect(testRoot.binInternalEngineRealm, exists); - expect(testRoot.binInternalEngineRealm.readAsStringSync(), equalsIgnoringWhitespace('')); + expect(testRoot.binCacheEngineRealm, _hasFileContentsMatching('')); }); - test('contains the FLUTTER_REALM environment variable', () async { + test('is the value in FLUTTER_REALM if set', () 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'), - ); + expect(testRoot.binCacheEngineRealm, _hasFileContentsMatching('flutter_archives_v2')); }); }); } @@ -414,13 +313,7 @@ void main() { /// ```txt /// ├── bin /// │ ├── internal -/// │ │ ├── engine.version -/// │ │ ├── engine.realm /// │ │ └── update_engine_version.{sh|ps1} -/// │ └── engine -/// │ └── src -/// │ └── .gn -/// └── DEPS /// ``` final class _FlutterRootUnderTest { /// Creates a root-under test using [path] as the root directory. @@ -435,15 +328,10 @@ final class _FlutterRootUnderTest { final Directory root = fileSystem.directory(path); return _FlutterRootUnderTest._( root, - deps: root.childFile('DEPS'), - engineSrcGn: root.childFile(fileSystem.path.join('engine', 'src', '.gn')), 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( @@ -473,55 +361,30 @@ final class _FlutterRootUnderTest { 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, }); final Directory root; - /// `DEPS`. - /// - /// The presenence of this file is an indicator we are in a fused (mono) repo. - final File deps; - - /// `engine/src/.gn`. - /// - /// The presenence of this file is an indicator we are in a fused (mono) repo. - final File engineSrcGn; - /// `bin/internal/engine.version`. /// - /// This file contains a SHA of which engine binaries to download. + /// This file contains a pinned 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. + /// If omitted, the file is ignored. + final File binInternalEngineVersion; /// `bin/cache/engine.stamp`. /// /// This file contains a _computed_ SHA of which engine binaries to download. final File binCacheEngineStamp; - /// `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). - 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] + /// which instructs the tool where the SHA stored in [binCacheEngineStamp] /// should be fetched from (it differs for presubmits run for flutter/flutter /// and builds downloaded by end-users or by postsubmits). final File binCacheEngineRealm; @@ -540,3 +403,51 @@ extension on File { copySync(newPath); } } + +/// Returns a matcher, that, given [contents]: +/// +/// 1. Asserts the 'actual' entity is a [File]; +/// 2. Asserts that the file exists; +/// 3. Asserts that the file's contents, after applying [collapseWhitespace], is the same as +/// [contents], after applying [collapseWhitespace]. +/// +/// This replaces multiple other matchers, and still provides a high-quality error message +/// when it fails. +Matcher _hasFileContentsMatching(String contents) { + return _ExistsWithStringContentsIgnoringWhitespace(contents); +} + +final class _ExistsWithStringContentsIgnoringWhitespace extends Matcher { + _ExistsWithStringContentsIgnoringWhitespace(String contents) + : _expected = collapseWhitespace(contents); + + final String _expected; + + @override + bool matches(Object? item, _) { + if (item is! File || !item.existsSync()) { + return false; + } + final String actual = item.readAsStringSync(); + return collapseWhitespace(actual) == collapseWhitespace(_expected); + } + + @override + Description describe(Description description) { + return description.add('a file exists that matches (ignoring whitespace): $_expected'); + } + + @override + Description describeMismatch(Object? item, Description mismatch, _, _) { + if (item is! File) { + return mismatch.add('is not a file (${item.runtimeType})'); + } + if (!item.existsSync()) { + return mismatch.add('does not exist'); + } + return mismatch + .add('is ') + .addDescriptionOf(collapseWhitespace(item.readAsStringSync())) + .add(' with whitespace compressed'); + } +}