-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[flutter_plugin_tool] Move branch-switching logic from tool_runner.sh to tool #4268
Changes from 6 commits
da8e15b
7c74f7d
31ba457
243ba40
38fe65b
5b177bf
bcd1561
984f1d8
56d86fd
fc4fd9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:io' as io; | ||
| import 'dart:math'; | ||
|
|
||
| import 'package:args/command_runner.dart'; | ||
|
|
@@ -72,11 +73,18 @@ abstract class PluginCommand extends Command<void> { | |
| ); | ||
| argParser.addFlag(_runOnChangedPackagesArg, | ||
| help: 'Run the command on changed packages/plugins.\n' | ||
| 'If the $_packagesArg is specified, this flag is ignored.\n' | ||
| 'If no packages have changed, or if there have been changes that may\n' | ||
| 'affect all packages, the command runs on all packages.\n' | ||
| 'The packages excluded with $_excludeArg is also excluded even if changed.\n' | ||
| 'See $_kBaseSha if a custom base is needed to determine the diff.'); | ||
| 'See $_kBaseSha if a custom base is needed to determine the diff.\n\n' | ||
| 'Cannot be combined with $_packagesArg.\n'); | ||
| argParser.addFlag(_packagesForBranchArg, | ||
| help: | ||
| 'This runs on all packages (equivalent to no package selection flag)\n' | ||
| 'on master, and behaves like --run-on-changed-packages on any other branch.\n\n' | ||
| 'Cannot be combined with $_packagesArg.\n\n' | ||
| 'This is intended for use in CI.\n', | ||
| hide: true); | ||
| argParser.addOption(_kBaseSha, | ||
| help: 'The base sha used to determine git diff. \n' | ||
| 'This is useful when $_runOnChangedPackagesArg is specified.\n' | ||
|
|
@@ -89,6 +97,7 @@ abstract class PluginCommand extends Command<void> { | |
| static const String _shardCountArg = 'shardCount'; | ||
| static const String _excludeArg = 'exclude'; | ||
| static const String _runOnChangedPackagesArg = 'run-on-changed-packages'; | ||
| static const String _packagesForBranchArg = 'packages-for-branch'; | ||
| static const String _kBaseSha = 'base-sha'; | ||
|
|
||
| /// The directory containing the plugin packages. | ||
|
|
@@ -266,15 +275,50 @@ abstract class PluginCommand extends Command<void> { | |
| /// is a sibling of the packages directory. This is used for a small number | ||
| /// of packages in the flutter/packages repository. | ||
| Stream<PackageEnumerationEntry> _getAllPackages() async* { | ||
| final Set<String> packageSelectionFlags = <String>{ | ||
| _packagesArg, | ||
| _runOnChangedPackagesArg, | ||
| _packagesForBranchArg, | ||
| }; | ||
| if (packageSelectionFlags | ||
| .where((String flag) => argResults!.wasParsed(flag)) | ||
| .length > | ||
| 1) { | ||
| printError('Only one of --$_packagesArg, --$_runOnChangedPackagesArg, or ' | ||
| '--$_packagesForBranchArg can be provided.'); | ||
| throw ToolExit(exitInvalidArguments); | ||
| } | ||
|
|
||
| Set<String> plugins = Set<String>.from(getStringListArg(_packagesArg)); | ||
|
|
||
| final bool runOnChangedPackages; | ||
| if (getBoolArg(_runOnChangedPackagesArg)) { | ||
| runOnChangedPackages = true; | ||
| } else if (getBoolArg(_packagesForBranchArg)) { | ||
| final String? branch = await _getBranch(); | ||
| if (branch == null) { | ||
| printError('Unabled to determine branch; --$_packagesForBranchArg can ' | ||
| 'only be used in a git repository.'); | ||
| throw ToolExit(exitInvalidArguments); | ||
| } else { | ||
| runOnChangedPackages = branch != 'master'; | ||
| // Log the mode for auditing what was intended to run. | ||
| print('--$_packagesForBranchArg: running on ' | ||
| '${runOnChangedPackages ? 'changed' : 'all'} packages'); | ||
| } | ||
| } else { | ||
| runOnChangedPackages = false; | ||
| } | ||
|
|
||
| final Set<String> excludedPluginNames = getExcludedPackageNames(); | ||
|
|
||
| final bool runOnChangedPackages = getBoolArg(_runOnChangedPackagesArg); | ||
| if (plugins.isEmpty && | ||
| runOnChangedPackages && | ||
| !(await _changesRequireFullTest())) { | ||
| plugins = await _getChangedPackages(); | ||
| if (runOnChangedPackages) { | ||
| final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); | ||
| final List<String> changedFiles = | ||
| await gitVersionFinder.getChangedFiles(); | ||
| if (!_changesRequireFullTest(changedFiles)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit fyi: This would be more efficient as a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I can change it in a follow-up. Neither the API nor the way it's being used are new in this PR, I'm just rearranging the flow a bit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, having looked at the details I'm not clear how it would be more efficient. At this call site I need this list in two places: So the difference would be that instead of taking a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, if you are going to cache the result it doesn't make a difference, it's more of an academic change. Returning a Stream is better because it allows people to process the output while it is being parsed. This is especially helpful when you are working on input from files/pipes. This also means the operation isn't memory bound and if a consumer decides to stop consuming mid-stream resources won't be wasted processing the output that happens after it stops. It doesn't mean a lick of difference if you cache the results though so you can process it twice.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The supply-side issue here is that the |
||
| plugins = _getChangedPackages(changedFiles); | ||
| } | ||
| } | ||
|
|
||
| final Directory thirdPartyPackagesDirectory = packagesDir.parent | ||
|
|
@@ -374,14 +418,10 @@ abstract class PluginCommand extends Command<void> { | |
| return gitVersionFinder; | ||
| } | ||
|
|
||
| // Returns packages that have been changed relative to the git base. | ||
| Future<Set<String>> _getChangedPackages() async { | ||
| final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); | ||
|
|
||
| final List<String> allChangedFiles = | ||
| await gitVersionFinder.getChangedFiles(); | ||
| // Returns packages that have been changed given a list of changed files. | ||
| Set<String> _getChangedPackages(List<String> changedFiles) { | ||
| final Set<String> packages = <String>{}; | ||
| for (final String path in allChangedFiles) { | ||
| for (final String path in changedFiles) { | ||
| final List<String> pathComponents = path.split('/'); | ||
|
||
| final int packagesIndex = | ||
| pathComponents.indexWhere((String element) => element == 'packages'); | ||
|
|
@@ -398,11 +438,19 @@ abstract class PluginCommand extends Command<void> { | |
| return packages; | ||
| } | ||
|
|
||
| Future<String?> _getBranch() async { | ||
| final io.ProcessResult branchResult = await (await gitDir).runCommand( | ||
| <String>['rev-parse', '--abbrev-ref', 'HEAD'], | ||
| throwOnError: false); | ||
| if (branchResult.exitCode != 0) { | ||
| return null; | ||
| } | ||
| return (branchResult.stdout as String).trim(); | ||
| } | ||
|
|
||
| // Returns true if one or more files changed that have the potential to affect | ||
| // any plugin (e.g., CI script changes). | ||
| Future<bool> _changesRequireFullTest() async { | ||
| final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); | ||
|
|
||
| bool _changesRequireFullTest(List<String> changedFiles) { | ||
| const List<String> specialFiles = <String>[ | ||
| '.ci.yaml', // LUCI config. | ||
| '.cirrus.yml', // Cirrus config. | ||
|
|
@@ -417,9 +465,7 @@ abstract class PluginCommand extends Command<void> { | |
| // check below is done via string prefixing. | ||
| assert(specialDirectories.every((String dir) => dir.endsWith('/'))); | ||
|
|
||
| final List<String> allChangedFiles = | ||
| await gitVersionFinder.getChangedFiles(); | ||
| return allChangedFiles.any((String path) => | ||
| return changedFiles.any((String path) => | ||
| specialFiles.contains(path) || | ||
| specialDirectories.any((String dir) => path.startsWith(dir))); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is really an improvement because now we've introduced the risk of someone forgetting COMMON_TOOL_FLAGS. I looked at the cirrus documentation and it doesn't look like you could make a function like $RUN_PLUGIN_TOOL(podspecs).
If you made the command being run not have to be positional with the presence of a new flag you could extract
RUN_PLUGIN_TOOL=dart $PLUGIN_TOOL $COMMON_TOOL_FLAGS --commandThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I went back and forth on this for the same reason, and didn't see a good solution. The main benefit is that it eliminates the strange setup where
tool_runner.shis reading a magic env variable, and.cirrus.ymlsets the magic variable, but it's totally non-obvious in each one in isolation what this variable is.But I'll just revert this part for now and revisit it later.
I looked into this a bit, and AFAICT allowing flags to be before the command with
CommandRunnerrequires declaring them at different level that has totally different (and much more awkward) affordances for reading the flags. I can probably move just this one flag as a hack, but it seems ugly. Maybe I'll find a better solution when revisiting.