From 660ec207e6f49130242ff721908c44b1b3c15622 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Thu, 20 May 2021 16:58:11 -0700 Subject: [PATCH] [flutter_conductor] Re-land auto-apply dart revision (#82985) --- dev/tools/lib/globals.dart | 11 +- dev/tools/lib/proto/conductor_state.pb.dart | 18 +++ .../lib/proto/conductor_state.pbjson.dart | 3 +- dev/tools/lib/proto/conductor_state.proto | 3 + dev/tools/lib/repository.dart | 45 ++++++ dev/tools/lib/start.dart | 35 ++++- dev/tools/lib/state.dart | 3 + dev/tools/test/repository_test.dart | 137 +++++++++++++++++- dev/tools/test/start_test.dart | 50 ++++++- 9 files changed, 282 insertions(+), 23 deletions(-) diff --git a/dev/tools/lib/globals.dart b/dev/tools/lib/globals.dart index 2ab44ca6eb..770ea29aa6 100644 --- a/dev/tools/lib/globals.dart +++ b/dev/tools/lib/globals.dart @@ -95,20 +95,25 @@ bool assertsEnabled() { /// /// The environment is favored over CLI args since the latter can have a default /// value, which the environment should be able to override. -String getValueFromEnvOrArgs( +String? getValueFromEnvOrArgs( String name, ArgResults argResults, - Map env, + Map env, { + bool allowNull = false, + } ) { final String envName = fromArgToEnvName(name); if (env[envName] != null ) { - return env[envName]!; + return env[envName]; } final String? argValue = argResults[name] as String?; if (argValue != null) { return argValue; } + if (allowNull) { + return null; + } throw ConductorException( 'Expected either the CLI arg --$name or the environment variable $envName ' 'to be provided!'); diff --git a/dev/tools/lib/proto/conductor_state.pb.dart b/dev/tools/lib/proto/conductor_state.pb.dart index 58aee2cb59..6bba337e04 100644 --- a/dev/tools/lib/proto/conductor_state.pb.dart +++ b/dev/tools/lib/proto/conductor_state.pb.dart @@ -202,6 +202,8 @@ class Repository extends $pb.GeneratedMessage { ..pc( 7, const $core.bool.fromEnvironment('protobuf.omit_field_names') ? '' : 'cherrypicks', $pb.PbFieldType.PM, subBuilder: Cherrypick.create) + ..aOS(8, const $core.bool.fromEnvironment('protobuf.omit_field_names') ? '' : 'dartRevision', + protoName: 'dartRevision') ..hasRequiredFields = false; Repository._() : super(); @@ -213,6 +215,7 @@ class Repository extends $pb.GeneratedMessage { Remote upstream, Remote mirror, $core.Iterable cherrypicks, + $core.String dartRevision, }) { final _result = create(); if (candidateBranch != null) { @@ -236,6 +239,9 @@ class Repository extends $pb.GeneratedMessage { if (cherrypicks != null) { _result.cherrypicks.addAll(cherrypicks); } + if (dartRevision != null) { + _result.dartRevision = dartRevision; + } return _result; } factory Repository.fromBuffer($core.List<$core.int> i, [$pb.ExtensionRegistry r = $pb.ExtensionRegistry.EMPTY]) => @@ -338,6 +344,18 @@ class Repository extends $pb.GeneratedMessage { @$pb.TagNumber(7) $core.List get cherrypicks => $_getList(6); + + @$pb.TagNumber(8) + $core.String get dartRevision => $_getSZ(7); + @$pb.TagNumber(8) + set dartRevision($core.String v) { + $_setString(7, v); + } + + @$pb.TagNumber(8) + $core.bool hasDartRevision() => $_has(7); + @$pb.TagNumber(8) + void clearDartRevision() => clearField(8); } class ConductorState extends $pb.GeneratedMessage { diff --git a/dev/tools/lib/proto/conductor_state.pbjson.dart b/dev/tools/lib/proto/conductor_state.pbjson.dart index 003b03ab90..33d2295658 100644 --- a/dev/tools/lib/proto/conductor_state.pbjson.dart +++ b/dev/tools/lib/proto/conductor_state.pbjson.dart @@ -80,12 +80,13 @@ const Repository$json = const { const {'1': 'upstream', '3': 5, '4': 1, '5': 11, '6': '.conductor_state.Remote', '10': 'upstream'}, const {'1': 'mirror', '3': 6, '4': 1, '5': 11, '6': '.conductor_state.Remote', '10': 'mirror'}, const {'1': 'cherrypicks', '3': 7, '4': 3, '5': 11, '6': '.conductor_state.Cherrypick', '10': 'cherrypicks'}, + const {'1': 'dartRevision', '3': 8, '4': 1, '5': 9, '10': 'dartRevision'}, ], }; /// Descriptor for `Repository`. Decode as a `google.protobuf.DescriptorProto`. final $typed_data.Uint8List repositoryDescriptor = $convert.base64Decode( - 'CgpSZXBvc2l0b3J5EigKD2NhbmRpZGF0ZUJyYW5jaBgBIAEoCVIPY2FuZGlkYXRlQnJhbmNoEigKD3N0YXJ0aW5nR2l0SGVhZBgCIAEoCVIPc3RhcnRpbmdHaXRIZWFkEiYKDmN1cnJlbnRHaXRIZWFkGAMgASgJUg5jdXJyZW50R2l0SGVhZBIiCgxjaGVja291dFBhdGgYBCABKAlSDGNoZWNrb3V0UGF0aBIzCgh1cHN0cmVhbRgFIAEoCzIXLmNvbmR1Y3Rvcl9zdGF0ZS5SZW1vdGVSCHVwc3RyZWFtEi8KBm1pcnJvchgGIAEoCzIXLmNvbmR1Y3Rvcl9zdGF0ZS5SZW1vdGVSBm1pcnJvchI9CgtjaGVycnlwaWNrcxgHIAMoCzIbLmNvbmR1Y3Rvcl9zdGF0ZS5DaGVycnlwaWNrUgtjaGVycnlwaWNrcw=='); + 'CgpSZXBvc2l0b3J5EigKD2NhbmRpZGF0ZUJyYW5jaBgBIAEoCVIPY2FuZGlkYXRlQnJhbmNoEigKD3N0YXJ0aW5nR2l0SGVhZBgCIAEoCVIPc3RhcnRpbmdHaXRIZWFkEiYKDmN1cnJlbnRHaXRIZWFkGAMgASgJUg5jdXJyZW50R2l0SGVhZBIiCgxjaGVja291dFBhdGgYBCABKAlSDGNoZWNrb3V0UGF0aBIzCgh1cHN0cmVhbRgFIAEoCzIXLmNvbmR1Y3Rvcl9zdGF0ZS5SZW1vdGVSCHVwc3RyZWFtEi8KBm1pcnJvchgGIAEoCzIXLmNvbmR1Y3Rvcl9zdGF0ZS5SZW1vdGVSBm1pcnJvchI9CgtjaGVycnlwaWNrcxgHIAMoCzIbLmNvbmR1Y3Rvcl9zdGF0ZS5DaGVycnlwaWNrUgtjaGVycnlwaWNrcxIiCgxkYXJ0UmV2aXNpb24YCCABKAlSDGRhcnRSZXZpc2lvbg=='); @$core.Deprecated('Use conductorStateDescriptor instead') const ConductorState$json = const { '1': 'ConductorState', diff --git a/dev/tools/lib/proto/conductor_state.proto b/dev/tools/lib/proto/conductor_state.proto index 1b9129d90d..50b75942bc 100644 --- a/dev/tools/lib/proto/conductor_state.proto +++ b/dev/tools/lib/proto/conductor_state.proto @@ -79,6 +79,9 @@ message Repository { // Desired cherrypicks. repeated Cherrypick cherrypicks = 7; + + // Only for engine repositories. + string dartRevision = 8; } message ConductorState { diff --git a/dev/tools/lib/repository.dart b/dev/tools/lib/repository.dart index 14536889c4..6ccf10e214 100644 --- a/dev/tools/lib/repository.dart +++ b/dev/tools/lib/repository.dart @@ -365,6 +365,26 @@ abstract class Repository { ); } + String commit( + String message, { + bool addFirst = false, + }) { + assert(!message.contains("'")); + if (addFirst) { + git.run( + ['add', '--all'], + 'add all changes to the index', + workingDirectory: checkoutDirectory.path, + ); + } + git.run( + ['commit', "--message='$message'"], + 'commit changes', + workingDirectory: checkoutDirectory.path, + ); + return reverseParse('HEAD'); + } + /// Create an empty commit and return the revision. @visibleForTesting String authorEmptyCommit([String message = 'An empty commit']) { @@ -617,6 +637,31 @@ class EngineRepository extends Repository { static const String defaultUpstream = 'https://github.com/flutter/engine.git'; static const String defaultBranch = 'master'; + /// Update the `dart_revision` entry in the DEPS file. + void updateDartRevision( + String newRevision, { + @visibleForTesting File? depsFile, + }) { + assert(newRevision.length == 40); + depsFile ??= checkoutDirectory.childFile('DEPS'); + final String fileContent = depsFile.readAsStringSync(); + final RegExp dartPattern = RegExp("[ ]+'dart_revision': '([a-z0-9]{40})',"); + final Iterable allMatches = dartPattern.allMatches(fileContent); + if (allMatches.length != 1) { + throw ConductorException( + 'Unexpected content in the DEPS file at ${depsFile.path}\n' + 'Expected to find pattern ${dartPattern.pattern} 1 times, but got ' + '${allMatches.length}.' + ); + } + final String updatedFileContent = fileContent.replaceFirst( + dartPattern, + " 'dart_revision': '$newRevision',", + ); + + depsFile.writeAsStringSync(updatedFileContent); + } + @override Repository cloneRepository(String? cloneName) { assert(localUpstream); diff --git a/dev/tools/lib/start.dart b/dev/tools/lib/start.dart index b9237d1996..dfa7f83832 100644 --- a/dev/tools/lib/start.dart +++ b/dev/tools/lib/start.dart @@ -22,14 +22,15 @@ import './state.dart'; import './stdio.dart'; const String kCandidateOption = 'candidate-branch'; -const String kReleaseOption = 'release-channel'; -const String kStateOption = 'state-file'; -const String kFrameworkMirrorOption = 'framework-mirror'; -const String kEngineMirrorOption = 'engine-mirror'; -const String kFrameworkUpstreamOption = 'framework-upstream'; +const String kDartRevisionOption = 'dart-revision'; +const String kEngineCherrypicksOption = 'engine-cherrypicks'; const String kEngineUpstreamOption = 'engine-upstream'; const String kFrameworkCherrypicksOption = 'framework-cherrypicks'; -const String kEngineCherrypicksOption = 'engine-cherrypicks'; +const String kFrameworkMirrorOption = 'framework-mirror'; +const String kFrameworkUpstreamOption = 'framework-upstream'; +const String kEngineMirrorOption = 'engine-mirror'; +const String kReleaseOption = 'release-channel'; +const String kStateOption = 'state-file'; /// Command to print the status of the current Flutter release. class StartCommand extends Command { @@ -86,6 +87,10 @@ class StartCommand extends Command { help: 'Framework cherrypick hashes to be applied.', defaultsTo: [], ); + argParser.addOption( + kDartRevisionOption, + help: 'New Dart revision to cherrypick.', + ); final Git git = Git(processManager); conductorVersion = git.getOutput( ['rev-parse', 'HEAD'], @@ -172,10 +177,17 @@ class StartCommand extends Command { argResults, platform.environment, ); + final String dartRevision = getValueFromEnvOrArgs( + kDartRevisionOption, + argResults, + platform.environment, + allowNull: true, + ); if (!releaseCandidateBranchRegex.hasMatch(candidateBranch)) { throw ConductorException( - 'Invalid release candidate branch "$candidateBranch". ' - 'Text should match the regex pattern /${releaseCandidateBranchRegex.pattern}/.'); + 'Invalid release candidate branch "$candidateBranch". Text should ' + 'match the regex pattern /${releaseCandidateBranchRegex.pattern}/.', + ); } final Int64 unixDate = Int64(DateTime.now().millisecondsSinceEpoch); @@ -197,9 +209,15 @@ class StartCommand extends Command { url: engineMirror, ), ); + // Create a new branch so that we don't accidentally push to upstream // candidateBranch. engine.newBranch('cherrypicks-$candidateBranch'); + + if (dartRevision != null && dartRevision.isNotEmpty) { + engine.updateDartRevision(dartRevision); + engine.commit('Update Dart SDK to $dartRevision', addFirst: true); + } final List engineCherrypicks = _sortCherrypicks( repository: engine, cherrypicks: engineCherrypickRevisions, @@ -230,6 +248,7 @@ class StartCommand extends Command { currentGitHead: engineHead, checkoutPath: engine.checkoutDirectory.path, cherrypicks: engineCherrypicks, + dartRevision: dartRevision, ); final FrameworkRepository framework = FrameworkRepository( checkouts, diff --git a/dev/tools/lib/state.dart b/dev/tools/lib/state.dart index 37d8706861..2bc915ecce 100644 --- a/dev/tools/lib/state.dart +++ b/dev/tools/lib/state.dart @@ -57,6 +57,9 @@ String presentState(pb.ConductorState state) { } else { buffer.writeln('0 Engine cherrypicks.'); } + if (state.engine.dartRevision != null && state.engine.dartRevision.isNotEmpty) { + buffer.writeln('New Dart SDK revision: ${state.engine.dartRevision}'); + } buffer.writeln('Framework Repo'); buffer.writeln('\tCandidate branch: ${state.framework.candidateBranch}'); buffer.writeln('\tStarting git HEAD: ${state.framework.startingGitHead}'); diff --git a/dev/tools/test/repository_test.dart b/dev/tools/test/repository_test.dart index 87363f2af2..aec659ad5c 100644 --- a/dev/tools/test/repository_test.dart +++ b/dev/tools/test/repository_test.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'package:dev_tools/repository.dart'; +import 'package:file/file.dart'; import 'package:file/memory.dart'; import 'package:platform/platform.dart'; @@ -12,9 +13,20 @@ import './common.dart'; void main() { group('repository', () { + late FakePlatform platform; + const String rootDir = '/'; + + setUp(() { + final String pathSeparator = const LocalPlatform().pathSeparator; + platform = FakePlatform( + environment: { + 'HOME': ['path', 'to', 'home'].join(pathSeparator), + }, + pathSeparator: pathSeparator, + ); + }); + test('canCherryPick returns true if git cherry-pick returns 0', () { - const LocalPlatform platform = LocalPlatform(); - const String rootDir = '/'; const String commit = 'abc123'; final TestStdio stdio = TestStdio(); @@ -66,8 +78,6 @@ void main() { }); test('canCherryPick returns false if git cherry-pick returns non-zero', () { - const LocalPlatform platform = LocalPlatform(); - const String rootDir = '/'; const String commit = 'abc123'; final TestStdio stdio = TestStdio(); @@ -123,8 +133,6 @@ void main() { }); test('cherryPick() applies the commit', () { - const LocalPlatform platform = LocalPlatform(); - const String rootDir = '/'; const String commit = 'abc123'; final TestStdio stdio = TestStdio(); @@ -168,5 +176,122 @@ void main() { repository.cherryPick(commit); expect(processManager.hasRemainingExpectations, false); }); + + test('updateDartRevision() updates the DEPS file', () { + const String previousDartRevision = '171876a4e6cf56ee6da1f97d203926bd7afda7ef'; + const String nextDartRevision = 'f6c91128be6b77aef8351e1e3a9d07c85bc2e46e'; + final TestStdio stdio = TestStdio(); + final MemoryFileSystem fileSystem = MemoryFileSystem.test(); + final FakeProcessManager processManager = FakeProcessManager.empty(); + + final Checkouts checkouts = Checkouts( + fileSystem: fileSystem, + parentDirectory: fileSystem.directory(rootDir), + platform: platform, + processManager: processManager, + stdio: stdio, + ); + + final EngineRepository repo = EngineRepository(checkouts); + final File depsFile = fileSystem.file('/DEPS'); + depsFile.writeAsStringSync(generateMockDeps(previousDartRevision)); + repo.updateDartRevision(nextDartRevision, depsFile: depsFile); + final String updatedDepsFileContent = depsFile.readAsStringSync(); + expect(updatedDepsFileContent, generateMockDeps(nextDartRevision)); + }); + + test('updateDartRevision() throws exception on malformed DEPS file', () { + const String nextDartRevision = 'f6c91128be6b77aef8351e1e3a9d07c85bc2e46e'; + final TestStdio stdio = TestStdio(); + final MemoryFileSystem fileSystem = MemoryFileSystem.test(); + final FakeProcessManager processManager = FakeProcessManager.empty(); + + final Checkouts checkouts = Checkouts( + fileSystem: fileSystem, + parentDirectory: fileSystem.directory(rootDir), + platform: platform, + processManager: processManager, + stdio: stdio, + ); + + final EngineRepository repo = EngineRepository(checkouts); + final File depsFile = fileSystem.file('/DEPS'); + depsFile.writeAsStringSync(''' +vars = { +}'''); + expect( + () => repo.updateDartRevision(nextDartRevision, depsFile: depsFile), + throwsExceptionWith('Unexpected content in the DEPS file at'), + ); + }); + + test('commit() passes correct commit message', () { + const String commit1 = 'abc123'; + const String commit2 = 'def456'; + const String message = 'This is a commit message.'; + final TestStdio stdio = TestStdio(); + final MemoryFileSystem fileSystem = MemoryFileSystem.test(); + final FakeProcessManager processManager = FakeProcessManager.list([ + FakeCommand(command: [ + 'git', + 'clone', + '--origin', + 'upstream', + '--', + EngineRepository.defaultUpstream, + fileSystem.path + .join(rootDir, 'flutter_conductor_checkouts', 'engine'), + ]), + const FakeCommand(command: [ + 'git', + 'checkout', + 'upstream/master', + ]), + const FakeCommand(command: [ + 'git', + 'rev-parse', + 'HEAD', + ], stdout: commit1), + const FakeCommand(command: [ + 'git', + 'commit', + "--message='$message'", + ]), + const FakeCommand(command: [ + 'git', + 'rev-parse', + 'HEAD', + ], stdout: commit2), + ]); + + final Checkouts checkouts = Checkouts( + fileSystem: fileSystem, + parentDirectory: fileSystem.directory(rootDir), + platform: platform, + processManager: processManager, + stdio: stdio, + ); + + final EngineRepository repo = EngineRepository(checkouts); + repo.commit(message); + }); }); } + +String generateMockDeps(String dartRevision) { + return ''' +vars = { + 'chromium_git': 'https://chromium.googlesource.com', + 'swiftshader_git': 'https://swiftshader.googlesource.com', + 'dart_git': 'https://dart.googlesource.com', + 'flutter_git': 'https://flutter.googlesource.com', + 'fuchsia_git': 'https://fuchsia.googlesource.com', + 'github_git': 'https://github.com', + 'skia_git': 'https://skia.googlesource.com', + 'ocmock_git': 'https://github.com/erikdoe/ocmock.git', + 'skia_revision': '4e9d5e2bdf04c58bc0bff57be7171e469e5d7175', + + 'dart_revision': '$dartRevision', + 'dart_boringssl_gen_rev': '7322fc15cc065d8d2957fccce6b62a509dc4d641', +}'''; +} diff --git a/dev/tools/test/start_test.dart b/dev/tools/test/start_test.dart index 6924502cc2..999bf6499b 100644 --- a/dev/tools/test/start_test.dart +++ b/dev/tools/test/start_test.dart @@ -120,6 +120,14 @@ void main() { test('creates state file if provided correct inputs', () async { const String revision2 = 'def789'; const String revision3 = '123abc'; + const String previousDartRevision = '171876a4e6cf56ee6da1f97d203926bd7afda7ef'; + const String nextDartRevision = 'f6c91128be6b77aef8351e1e3a9d07c85bc2e46e'; + + final Directory engine = fileSystem.directory(checkoutsParentDirectory) + .childDirectory('flutter_conductor_checkouts') + .childDirectory('engine'); + + final File depsFile = engine.childFile('DEPS'); final List engineCommands = [ FakeCommand( @@ -130,12 +138,13 @@ void main() { 'upstream', '--', EngineRepository.defaultUpstream, - fileSystem.path.join( - checkoutsParentDirectory, - 'flutter_conductor_checkouts', - 'engine', - ), + engine.path, ], + onRun: () { + // Create the DEPS file which the tool will update + engine.createSync(recursive: true); + depsFile.writeAsStringSync(generateMockDeps(previousDartRevision)); + } ), const FakeCommand( command: ['git', 'remote', 'add', 'mirror', engineMirror], @@ -158,6 +167,16 @@ void main() { 'cherrypicks-$candidateBranch', ], ), + const FakeCommand( + command: ['git', 'add', '--all'], + ), + const FakeCommand( + command: ['git', 'commit', "--message='Update Dart SDK to $nextDartRevision'"], + ), + const FakeCommand( + command: ['git', 'rev-parse', 'HEAD'], + stdout: revision2, + ), const FakeCommand( command: ['git', 'rev-parse', 'HEAD'], stdout: revision2, @@ -233,6 +252,8 @@ void main() { releaseChannel, '--$kStateOption', stateFilePath, + '--$kDartRevisionOption', + nextDartRevision, ]); final File stateFile = fileSystem.file(stateFilePath); @@ -246,6 +267,7 @@ void main() { expect(state.releaseChannel, releaseChannel); expect(state.engine.candidateBranch, candidateBranch); expect(state.engine.startingGitHead, revision2); + expect(state.engine.dartRevision, nextDartRevision); expect(state.framework.candidateBranch, candidateBranch); expect(state.framework.startingGitHead, revision3); expect(state.lastPhase, ReleasePhase.INITIALIZE); @@ -255,3 +277,21 @@ void main() { 'windows': const Skip('Flutter Conductor only supported on macos/linux'), }); } + +String generateMockDeps(String dartRevision) { + return ''' +vars = { + 'chromium_git': 'https://chromium.googlesource.com', + 'swiftshader_git': 'https://swiftshader.googlesource.com', + 'dart_git': 'https://dart.googlesource.com', + 'flutter_git': 'https://flutter.googlesource.com', + 'fuchsia_git': 'https://fuchsia.googlesource.com', + 'github_git': 'https://github.com', + 'skia_git': 'https://skia.googlesource.com', + 'ocmock_git': 'https://github.com/erikdoe/ocmock.git', + 'skia_revision': '4e9d5e2bdf04c58bc0bff57be7171e469e5d7175', + + 'dart_revision': '$dartRevision', + 'dart_boringssl_gen_rev': '7322fc15cc065d8d2957fccce6b62a509dc4d641', +}'''; +}