From 085f0d27396532fbf2d53ae5d079a405ba6bc8aa Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 27 Jan 2023 10:49:39 -0500 Subject: [PATCH 1/4] [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 --- 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..e74977c2eb48 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 commin 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..5d9a336cec79 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; } + // Retruns true if the environment indicates that the current treeish is from + // [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 ProcessResult = await (await gitDir) + .runCommand(['merge-base', '--is-ancestor', 'HEAD', 'main']); + return ProcessResult.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'), ])); }); }); From e36a4acbdfe167ca4ad1cb9aba3e1e173cd580ee Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 27 Jan 2023 12:44:31 -0500 Subject: [PATCH 2/4] Fix throw --- script/tool/lib/src/common/package_command.dart | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/script/tool/lib/src/common/package_command.dart b/script/tool/lib/src/common/package_command.dart index 5d9a336cec79..fdeea51f4a59 100644 --- a/script/tool/lib/src/common/package_command.dart +++ b/script/tool/lib/src/common/package_command.dart @@ -539,9 +539,10 @@ abstract class PackageCommand extends Command { // 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 ProcessResult = await (await gitDir) - .runCommand(['merge-base', '--is-ancestor', 'HEAD', 'main']); - return ProcessResult.exitCode == 0; + final io.ProcessResult result = await (await gitDir).runCommand( + ['merge-base', '--is-ancestor', 'HEAD', branchName], + throwOnError: false); + return result.exitCode == 0; } Future _getBranch() async { From b714de4cd0fdd60c0742397403269aa979388902 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 30 Jan 2023 14:24:18 -0500 Subject: [PATCH 3/4] Fix typos --- script/tool/CHANGELOG.md | 2 +- script/tool/lib/src/common/package_command.dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index e74977c2eb48..3c4905ad7071 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,6 +1,6 @@ ## 0.13.4+1 -* Makes `--packages-for-branch` detect any commin on `main` as being `main`, +* 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 diff --git a/script/tool/lib/src/common/package_command.dart b/script/tool/lib/src/common/package_command.dart index fdeea51f4a59..92fa2bac5b22 100644 --- a/script/tool/lib/src/common/package_command.dart +++ b/script/tool/lib/src/common/package_command.dart @@ -533,7 +533,7 @@ abstract class PackageCommand extends Command { return packages; } - // Retruns true if the environment indicates that the current treeish is from + // Returns true if the environment indicates that the current treeish is from // [branch]. // // This is used because CI may check out a specific hash rather than a branch, From bba5c6a3bff8fcf1184bed7363e44c2edf4d599f Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 30 Jan 2023 14:27:28 -0500 Subject: [PATCH 4/4] Update comment --- script/tool/lib/src/common/package_command.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/script/tool/lib/src/common/package_command.dart b/script/tool/lib/src/common/package_command.dart index 92fa2bac5b22..0e084c4c2d5c 100644 --- a/script/tool/lib/src/common/package_command.dart +++ b/script/tool/lib/src/common/package_command.dart @@ -533,8 +533,7 @@ abstract class PackageCommand extends Command { return packages; } - // Returns true if the environment indicates that the current treeish is from - // [branch]. + // 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.