diff --git a/dev/conductor/core/lib/src/packages_autoroller.dart b/dev/conductor/core/lib/src/packages_autoroller.dart index 2f64ef070c..c7535372f8 100644 --- a/dev/conductor/core/lib/src/packages_autoroller.dart +++ b/dev/conductor/core/lib/src/packages_autoroller.dart @@ -6,13 +6,13 @@ import 'dart:convert'; import 'dart:io' as io; import 'package:file/file.dart'; -import 'package:meta/meta.dart'; import 'package:process/process.dart'; import 'git.dart'; import 'globals.dart'; import 'repository.dart'; import 'stdio.dart'; +import 'validate_checkout_post_gradle_regeneration.dart'; /// A service for rolling the SDK's pub packages to latest and open a PR upstream. class PackageAutoroller { @@ -52,6 +52,8 @@ class PackageAutoroller { static const String hostname = 'github.com'; + String get gitAuthor => '$githubUsername <$githubUsername@google.com>'; + String get prBody { return ''' This PR was generated by the automated @@ -95,7 +97,7 @@ This PR was generated by the automated log('Packages are already at latest.'); return; } - await generateGradleLockfiles(await framework.checkoutDirectory); + await _regenerateGradleLockfiles(await framework.checkoutDirectory); await pushBranch(); await createPr(repository: await framework.checkoutDirectory); await authLogout(); @@ -114,8 +116,6 @@ This PR was generated by the automated Future updatePackages({ bool verbose = true, }) async { - final String author = '$githubUsername <$githubUsername@google.com>'; - await framework.newBranch(await featureBranchName); final io.Process flutterProcess = await framework.streamFlutter([ if (verbose) '--verbose', @@ -126,25 +126,44 @@ This PR was generated by the automated if (exitCode != 0) { throw ConductorException('Failed to update packages with exit code $exitCode'); } - // If the git checkout is clean, then pub packages are already at latest that cleanly resolve. + // If the git checkout is clean, then pub packages are already at latest + // that cleanly resolve. if (await framework.gitCheckoutClean()) { return false; } await framework.commit( 'roll packages', addFirst: true, - author: author, + author: gitAuthor, ); return true; } - @visibleForTesting - Future generateGradleLockfiles(Directory repoRoot) async { + Future _regenerateGradleLockfiles(Directory repoRoot) async { await framework.runDart([ '${repoRoot.path}/dev/tools/bin/generate_gradle_lockfiles.dart', '--no-gradle-generation', '--no-exclusion', ]); + switch (CheckoutStatePostGradleRegeneration(await framework.gitStatus(), framework.fileSystem.path)) { + // If the git checkout is clean, we did not update any lockfiles and we do + // not need an additional commit. + case NoDiff(): + return; + case OnlyLockfileChanges(): + await framework.commit( + 'Re-generate Gradle lockfiles', + addFirst: true, + author: gitAuthor, + ); + case NonLockfileChanges(changes: final List changes): + throw StateError( + 'Expected all diffs after re-generating gradle lockfiles to end in ' + '`.lockfile`, but encountered: $changes', + ); + case MalformedLine(line: final String line): + throw StateError('Unexpected line of STDOUT from git status: "$line"'); + } } Future pushBranch() async { diff --git a/dev/conductor/core/lib/src/repository.dart b/dev/conductor/core/lib/src/repository.dart index 8ca5f78f83..a9dcb1cb67 100644 --- a/dev/conductor/core/lib/src/repository.dart +++ b/dev/conductor/core/lib/src/repository.dart @@ -258,14 +258,21 @@ abstract class Repository { ); } - /// Verify the repository's git checkout is clean. - Future gitCheckoutClean() async { - final String output = await git.getOutput( + /// Get the working tree status. + /// + /// Calls `git status --porcelain` which should output in a stable format + /// across git versions. + Future gitStatus() async { + return git.getOutput( ['status', '--porcelain'], 'check that the git checkout is clean', workingDirectory: (await checkoutDirectory).path, ); - return output == ''; + } + + /// Verify the repository's git checkout is clean. + Future gitCheckoutClean() async { + return (await gitStatus()).isEmpty; } /// Return the revision for the branch point between two refs. diff --git a/dev/conductor/core/lib/src/validate_checkout_post_gradle_regeneration.dart b/dev/conductor/core/lib/src/validate_checkout_post_gradle_regeneration.dart new file mode 100644 index 0000000000..7c59d2b556 --- /dev/null +++ b/dev/conductor/core/lib/src/validate_checkout_post_gradle_regeneration.dart @@ -0,0 +1,77 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:path/path.dart' show Context; + +/// Possible states of the Flutter repo checkout after Gradle lockfile +/// regeneration. +sealed class CheckoutStatePostGradleRegeneration { + factory CheckoutStatePostGradleRegeneration(String gitStatusOutput, Context context) { + gitStatusOutput = gitStatusOutput.trim(); + if (gitStatusOutput.isEmpty) { + return const NoDiff(); + } + + final List changes = gitStatusOutput.trim().split('\n'); + final List changedPaths = []; + for (final String line in changes) { + final RegExpMatch? match = pattern.firstMatch(line); + if (match == null) { + return MalformedLine(line); + } + changedPaths.add(match.group(1)!); + } + + final List nonLockfileDiffs = changedPaths.where((String path) { + final String extension = context.extension(path); + return extension != '.lockfile'; + }).toList(); + + if (nonLockfileDiffs.isNotEmpty) { + return NonLockfileChanges(nonLockfileDiffs); + } + + return const OnlyLockfileChanges(); + } + + /// Output format for `git status --porcelain` and `git status --short`. + /// + /// The first capture group is the path to the file or directory changed, + /// relative to the root of the repository. + /// + /// See `man git-status` for more reference. + static final RegExp pattern = RegExp(r'[ACDMRTU ]{1,2} (\S+)'); +} + +/// No files were changed, no commit needed. +final class NoDiff implements CheckoutStatePostGradleRegeneration { + const NoDiff(); +} + +/// Only files ending in *.lockfile were changed; changes can be committed. +final class OnlyLockfileChanges implements CheckoutStatePostGradleRegeneration { + const OnlyLockfileChanges(); +} + +/// There are changed files that do not end in *.lockfile; fail the script. +/// +/// Because the script to regenerate Gradle lockfiles triggers a Gradle build, +/// and because the packages_autoroller can have its PRs merged without a +/// human review, we are conservative about what changes we commit. +final class NonLockfileChanges implements CheckoutStatePostGradleRegeneration { + const NonLockfileChanges(this.changes); + + final List changes; +} + +/// A line in the output of `git status` does not match the expected pattern; +/// fail the script. +/// +/// This likely means there is a bug in the regular expression, and it needs +/// to be updated. +final class MalformedLine implements CheckoutStatePostGradleRegeneration { + const MalformedLine(this.line); + + final String line; +} diff --git a/dev/conductor/core/test/packages_autoroller_test.dart b/dev/conductor/core/test/packages_autoroller_test.dart index 4984319fc6..0bd2d54f41 100644 --- a/dev/conductor/core/test/packages_autoroller_test.dart +++ b/dev/conductor/core/test/packages_autoroller_test.dart @@ -8,7 +8,9 @@ import 'dart:io' as io; import 'package:conductor_core/conductor_core.dart'; import 'package:conductor_core/packages_autoroller.dart'; +import 'package:conductor_core/src/validate_checkout_post_gradle_regeneration.dart'; import 'package:file/memory.dart'; +import 'package:path/path.dart' show Context, Style; import 'package:platform/platform.dart'; import '../bin/packages_autoroller.dart' show run; @@ -438,6 +440,39 @@ void main() { '--no-gradle-generation', '--no-exclusion', ]), + const FakeCommand(command: [ + 'git', + 'status', + '--porcelain', + ], stdout: ''' + M dev/integration_tests/ui/android/project-app.lockfile + M examples/image_list/android/project-app.lockfile +'''), + const FakeCommand(command: [ + 'git', + 'status', + '--porcelain', + ], stdout: ''' + M dev/integration_tests/ui/android/project-app.lockfile + M examples/image_list/android/project-app.lockfile +'''), + const FakeCommand(command: [ + 'git', + 'add', + '--all', + ]), + const FakeCommand(command: [ + 'git', + 'commit', + '--message', + 'Re-generate Gradle lockfiles', + '--author="flutter-pub-roller-bot "', + ]), + const FakeCommand(command: [ + 'git', + 'rev-parse', + 'HEAD', + ], stdout: '234deadbeef'), const FakeCommand(command: [ 'git', 'push', @@ -540,6 +575,55 @@ void main() { stdio.printTrace('Using $token'); expect(stdio.logs.last, '[trace] Using $replacement'); }); + + group('CheckoutStatePostGradleRegeneration', () { + final Context ctx = Context(style: Style.posix); + + test('empty input returns NoDiff', () { + expect( + CheckoutStatePostGradleRegeneration('', ctx), + const NoDiff(), + ); + }); + + test('only *.lockfile changes returns OnlyLockfileChanges', () { + expect( + CheckoutStatePostGradleRegeneration(''' + A dev/benchmarks/test_apps/stocks/android/buildscript-gradle.lockfile + M dev/integration_tests/ui/android/project-app.lockfile + M examples/image_list/android/project-app.lockfile +''', ctx), + const OnlyLockfileChanges(), + ); + }); + + test('if a *.zip file is added returns NonLockfileChanges', () { + const String pathToZip = 'dev/benchmarks/test_apps/stocks/android/very-large-archive.zip'; + CheckoutStatePostGradleRegeneration result = CheckoutStatePostGradleRegeneration(''' + A dev/benchmarks/test_apps/stocks/android/buildscript-gradle.lockfile + A $pathToZip + M dev/integration_tests/ui/android/project-app.lockfile + M examples/image_list/android/project-app.lockfile +''', ctx); + expect(result, isA()); + result = result as NonLockfileChanges; + expect(result.changes, hasLength(1)); + expect(result.changes.single, pathToZip); + }); + + test('if it contains a line not matching the regex returns MalformedLine', () { + const String malformedLine = 'New Git Output.'; + CheckoutStatePostGradleRegeneration result = CheckoutStatePostGradleRegeneration(''' +$malformedLine + A dev/benchmarks/test_apps/stocks/android/buildscript-gradle.lockfile + M dev/integration_tests/ui/android/project-app.lockfile + M examples/image_list/android/project-app.lockfile +''', ctx); + expect(result, isA()); + result = result as MalformedLine; + expect(result.line, malformedLine); + }); + }); } class _NoOpStdin extends Fake implements io.Stdin {}