From 3843b38e2ee8d3f0b1362b46dbb5b5ac7991eefc Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Mon, 30 Jan 2023 13:32:05 -0800 Subject: [PATCH] [tool] Improve main-branch detection (#7038) * [tool] Improve main-branch detection Currently main-branch detection for `--packages-for-branch` looks at branch names, but this no longer works on LUCI which now seems to be checking out specific hashes rather than branches. This updates the behavior so that it will treat any hash that is an ancestor of `main` as being part of `main`, which should allow post-submit detection to work under LUCI. Fixes https://github.com/flutter/flutter/issues/119330 * Fix throw * Fix typos * Update comment --- script/tool/CHANGELOG.md | 5 ++ .../tool/lib/src/common/package_command.dart | 34 +++++++++-- script/tool/pubspec.yaml | 2 +- .../test/common/package_command_test.dart | 56 ++++++++++++++++--- 4 files changed, 83 insertions(+), 14 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 34def6ecf676..3c4905ad7071 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.13.4+1 + +* Makes `--packages-for-branch` detect any commit on `main` as being `main`, + so that it works with pinned checkouts (e.g., on LUCI). + ## 0.13.4 * Adds the ability to validate minimum supported Dart/Flutter versions in diff --git a/script/tool/lib/src/common/package_command.dart b/script/tool/lib/src/common/package_command.dart index 0e83d03e9846..0e084c4c2d5c 100644 --- a/script/tool/lib/src/common/package_command.dart +++ b/script/tool/lib/src/common/package_command.dart @@ -316,17 +316,28 @@ abstract class PackageCommand extends Command { } else if (getBoolArg(_packagesForBranchArg)) { final String? branch = await _getBranch(); if (branch == null) { - printError('Unabled to determine branch; --$_packagesForBranchArg can ' + printError('Unable to determine branch; --$_packagesForBranchArg can ' 'only be used in a git repository.'); throw ToolExit(exitInvalidArguments); } else { // Configure the change finder the correct mode for the branch. - final bool lastCommitOnly = branch == 'main' || branch == 'master'; + // Log the mode to make it easier to audit logs to see that the + // intended diff was used (or why). + final bool lastCommitOnly; + if (branch == 'main' || branch == 'master') { + print('--$_packagesForBranchArg: running on default branch.'); + lastCommitOnly = true; + } else if (await _isCheckoutFromBranch('main')) { + print( + '--$_packagesForBranchArg: running on a commit from default branch.'); + lastCommitOnly = true; + } else { + print('--$_packagesForBranchArg: running on branch "$branch".'); + lastCommitOnly = false; + } if (lastCommitOnly) { - // Log the mode to make it easier to audit logs to see that the - // intended diff was used. - print('--$_packagesForBranchArg: running on default branch; ' - 'using parent commit as the diff base.'); + print( + '--$_packagesForBranchArg: using parent commit as the diff base.'); changedFileFinder = GitVersionFinder(await gitDir, 'HEAD~'); } else { changedFileFinder = await retrieveVersionFinder(); @@ -522,6 +533,17 @@ abstract class PackageCommand extends Command { return packages; } + // Returns true if the current checkout is on an ancestor of [branch]. + // + // This is used because CI may check out a specific hash rather than a branch, + // in which case branch-name detection won't work. + Future _isCheckoutFromBranch(String branchName) async { + final io.ProcessResult result = await (await gitDir).runCommand( + ['merge-base', '--is-ancestor', 'HEAD', branchName], + throwOnError: false); + return result.exitCode == 0; + } + Future _getBranch() async { final io.ProcessResult branchResult = await (await gitDir).runCommand( ['rev-parse', '--abbrev-ref', 'HEAD'], diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml index a8df2a9cd23a..73429638a402 100644 --- a/script/tool/pubspec.yaml +++ b/script/tool/pubspec.yaml @@ -1,7 +1,7 @@ name: flutter_plugin_tools description: Productivity utils for flutter/plugins and flutter/packages repository: https://github.com/flutter/plugins/tree/main/script/tool -version: 0.13.4 +version: 0.13.4+1 dependencies: args: ^2.1.0 diff --git a/script/tool/test/common/package_command_test.dart b/script/tool/test/common/package_command_test.dart index aa0a20253955..ed76408bb300 100644 --- a/script/tool/test/common/package_command_test.dart +++ b/script/tool/test/common/package_command_test.dart @@ -778,7 +778,8 @@ packages/b_package/lib/src/foo.dart MockProcess(stdout: 'a-branch'), ]; processRunner.mockProcessesForExecutable['git-merge-base'] = [ - MockProcess(stdout: 'abc123'), + MockProcess(exitCode: 1), // --is-ancestor check + MockProcess(stdout: 'abc123'), // finding merge base ]; final RepositoryPackage plugin1 = createFakePlugin('plugin1', packagesDir); @@ -791,6 +792,7 @@ packages/b_package/lib/src/foo.dart expect( output, containsAllInOrder([ + contains('--packages-for-branch: running on branch "a-branch"'), contains( 'Running for all packages that have diffs relative to "abc123"'), ])); @@ -822,8 +824,9 @@ packages/b_package/lib/src/foo.dart expect( output, containsAllInOrder([ - contains('--packages-for-branch: running on default branch; ' - 'using parent commit as the diff base'), + contains('--packages-for-branch: running on default branch.'), + contains( + '--packages-for-branch: using parent commit as the diff base'), contains( 'Running for all packages that have diffs relative to "HEAD~"'), ])); @@ -836,7 +839,45 @@ packages/b_package/lib/src/foo.dart )); }); - test('tests all packages on master', () async { + test( + 'only tests changed packages relative to the previous commit if ' + 'running on a specific hash from main', () async { + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: 'packages/plugin1/plugin1.dart'), + ]; + processRunner.mockProcessesForExecutable['git-rev-parse'] = [ + MockProcess(stdout: 'HEAD'), + ]; + final RepositoryPackage plugin1 = + createFakePlugin('plugin1', packagesDir); + createFakePlugin('plugin2', packagesDir); + + final List output = await runCapturingPrint( + runner, ['sample', '--packages-for-branch']); + + expect(command.plugins, unorderedEquals([plugin1.path])); + expect( + output, + containsAllInOrder([ + contains( + '--packages-for-branch: running on a commit from default branch.'), + contains( + '--packages-for-branch: using parent commit as the diff base'), + contains( + 'Running for all packages that have diffs relative to "HEAD~"'), + ])); + // Ensure that it's diffing against the prior commit. + expect( + processRunner.recordedCalls, + contains( + const ProcessCall( + 'git-diff', ['--name-only', 'HEAD~', 'HEAD'], null), + )); + }); + + test( + 'only tests changed packages relative to the previous commit on master', + () async { processRunner.mockProcessesForExecutable['git-diff'] = [ MockProcess(stdout: 'packages/plugin1/plugin1.dart'), ]; @@ -854,8 +895,9 @@ packages/b_package/lib/src/foo.dart expect( output, containsAllInOrder([ - contains('--packages-for-branch: running on default branch; ' - 'using parent commit as the diff base'), + contains('--packages-for-branch: running on default branch.'), + contains( + '--packages-for-branch: using parent commit as the diff base'), contains( 'Running for all packages that have diffs relative to "HEAD~"'), ])); @@ -887,7 +929,7 @@ packages/b_package/lib/src/foo.dart expect( output, containsAllInOrder([ - contains('Unabled to determine branch'), + contains('Unable to determine branch'), ])); }); });