Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[flutter_plugin_tool] Move branch-switching logic from tool_runner.sh to tool #4268

Merged
merged 10 commits into from
Aug 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion script/tool/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## NEXT
## 0.6.0

- Added Android native integration test support to `native-test`.
- Added a new `android-lint` command to lint Android plugin native code.
Expand All @@ -9,6 +9,8 @@
`--no-push-flags`. Releases now always tag and push.
- **Breaking change**: `publish`'s `--package` flag has been replaced with the
`--packages` flag used by most other packages.
- **Breaking change** Passing both `--run-on-changed-packages` and `--packages`
is now an error; previously it the former would be ignored.

## 0.5.0

Expand Down
90 changes: 69 additions & 21 deletions script/tool/lib/src/common/plugin_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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'
Expand All @@ -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.
Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit fyi: This would be more efficient as a Stream<String> instead of Future<List<String>>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. getChangedFiles is taking the full stdout from git (the process API for the tools aren't stream-based) and splitting it by lines, giving a list. So returning a Stream instead of a List doesn't change anything there.

At this call site I need this list in two places: _changesRequireFullTest and _getChangedPackages. I can't consume the stream twice, so I would need to .toList() it here.

So the difference would be that instead of taking a List and returning it to a caller that needs a List, I would be taking a List, converting it to a Stream, and giving the stream to the caller to immediately turn back to a List. Am I missing a different way of structuring this?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

The supply-side issue here is that the git package doesn't steam git output, it uses run and returns the full output as a String.

plugins = _getChangedPackages(changedFiles);
}
}

final Directory thirdPartyPackagesDirectory = packagesDir.parent
Expand Down Expand Up @@ -374,15 +418,13 @@ 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.
//
// The paths must use POSIX separators (e.g., as provided by git output).
Set<String> _getChangedPackages(List<String> changedFiles) {
final Set<String> packages = <String>{};
for (final String path in allChangedFiles) {
final List<String> pathComponents = path.split('/');
for (final String path in changedFiles) {
final List<String> pathComponents = p.posix.split(path);
final int packagesIndex =
pathComponents.indexWhere((String element) => element == 'packages');
if (packagesIndex != -1) {
Expand All @@ -398,11 +440,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.
Expand All @@ -417,9 +467,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)));
}
Expand Down
2 changes: 1 addition & 1 deletion script/tool/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: flutter_plugin_tools
description: Productivity utils for flutter/plugins and flutter/packages
repository: https://github.com/flutter/plugins/tree/master/script/tool
version: 0.5.0
version: 0.6.0

dependencies:
args: ^2.1.0
Expand Down
Loading