From 3de67cea560a4ca7082f56b1033db1af53845245 Mon Sep 17 00:00:00 2001
From: stuartmorgan <stuartmorgan@google.com>
Date: Tue, 7 Dec 2021 10:02:24 -0500
Subject: [PATCH] [flutter_plugin_tools] Improve package targeting (#4577)
Improve package targeting:
- `--run-on-changed-packages` now includes only changed packages in a federated plugin, not all packages. Include all packages isn't useful since (without changes that would cause them to be included anyway) package dependencies are not path-based between packages.
- `--packages` now allows specifying package names of federated plugins. E.g., `--packages=path_provider_ios` will now work, instead of requiring `--packages=path_provider/path_provider_ios`. The fully qualified form still works as well (and is still needed for app-facing packages to disambiguate from the plugin group).
Fixes https://github.com/flutter/flutter/issues/94618
---
script/tool/CHANGELOG.md | 5 ++
script/tool/README.md | 9 +-
.../tool/lib/src/common/plugin_command.dart | 67 +++++++++++----
.../tool/test/common/plugin_command_test.dart | 85 +++++++++++++++++--
4 files changed, 139 insertions(+), 27 deletions(-)
diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md
index dec218e2ba..72539c24c5 100644
--- a/script/tool/CHANGELOG.md
+++ b/script/tool/CHANGELOG.md
@@ -5,6 +5,11 @@
dependencies to path-based dependencies.
- Adds a (hidden) `--run-on-dirty-packages` flag for use with
`make-deps-path-based` in CI.
+- `--packages` now allows using a federated plugin's package as a target without
+ fully specifying it (if it is not the same as the plugin's name). E.g.,
+ `--packages=path_provide_ios` now works.
+- `--run-on-changed-packages` now includes only the changed packages in a
+ federated plugin, not all packages in that plugin.
- Fix `federation-safety-check` handling of plugin deletion, and of top-level
files in unfederated plugins whose names match federated plugin heuristics
(e.g., `packages/foo/foo_android.iml`).
diff --git a/script/tool/README.md b/script/tool/README.md
index 1a87f09875..613cb5456e 100644
--- a/script/tool/README.md
+++ b/script/tool/README.md
@@ -51,8 +51,13 @@ following shows a number of common commands being run for a specific plugin.
All examples assume running from source; see above for running the
published version instead.
-Note that the `plugins` argument, despite the name, applies to any package.
-(It will likely be renamed `packages` in the future.)
+Most commands take a `--packages` argument to control which package(s) the
+command is targetting. An package name can be any of:
+- The name of a package (e.g., `path_provider_android`).
+- The name of a federated plugin (e.g., `path_provider`), in which case all
+ packages that make up that plugin will be targetted.
+- A combination federated_plugin_name/package_name (e.g.,
+ `path_provider/path_provider` for the app-facing package).
### Format Code
diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart
index 7166c754e1..1846635682 100644
--- a/script/tool/lib/src/common/plugin_command.dart
+++ b/script/tool/lib/src/common/plugin_command.dart
@@ -339,7 +339,7 @@ abstract class PluginCommand extends Command<void> {
final List<String> changedFiles =
await gitVersionFinder.getChangedFiles();
if (!_changesRequireFullTest(changedFiles)) {
- packages = _getChangedPackages(changedFiles);
+ packages = _getChangedPackageNames(changedFiles);
}
} else if (getBoolArg(_runOnDirtyPackagesArg)) {
final GitVersionFinder gitVersionFinder =
@@ -348,7 +348,7 @@ abstract class PluginCommand extends Command<void> {
// _changesRequireFullTest is deliberately not used here, as this flag is
// intended for use in CI to re-test packages changed by
// 'make-deps-path-based'.
- packages = _getChangedPackages(
+ packages = _getChangedPackageNames(
await gitVersionFinder.getChangedFiles(includeUncommitted: true));
// For the same reason, empty is not treated as "all packages" as it is
// for other flags.
@@ -379,21 +379,23 @@ abstract class PluginCommand extends Command<void> {
await for (final FileSystemEntity subdir
in entity.list(followLinks: false)) {
if (_isDartPackage(subdir)) {
- // If --plugin=my_plugin is passed, then match all federated
- // plugins under 'my_plugin'. Also match if the exact plugin is
- // passed.
- final String relativePath =
- path.relative(subdir.path, from: dir.path);
- final String packageName = path.basename(subdir.path);
- final String basenamePath = path.basename(entity.path);
+ // There are three ways for a federated plugin to match:
+ // - package name (path_provider_android)
+ // - fully specified name (path_provider/path_provider_android)
+ // - group name (path_provider), which matches all packages in
+ // the group
+ final Set<String> possibleMatches = <String>{
+ path.basename(subdir.path), // package name
+ path.basename(entity.path), // group name
+ path.relative(subdir.path, from: dir.path), // fully specified
+ };
if (packages.isEmpty ||
- packages.contains(relativePath) ||
- packages.contains(basenamePath)) {
+ packages.intersection(possibleMatches).isNotEmpty) {
yield PackageEnumerationEntry(
RepositoryPackage(subdir as Directory),
- excluded: excludedPluginNames.contains(basenamePath) ||
- excludedPluginNames.contains(packageName) ||
- excludedPluginNames.contains(relativePath));
+ excluded: excludedPluginNames
+ .intersection(possibleMatches)
+ .isNotEmpty);
}
}
}
@@ -454,17 +456,48 @@ abstract class PluginCommand extends Command<void> {
return gitVersionFinder;
}
- // Returns packages that have been changed given a list of changed files.
+ // Returns the names of packages that have been changed given a list of
+ // changed files.
+ //
+ // The names will either be the actual package names, or potentially
+ // group/name specifiers (for example, path_provider/path_provider) for
+ // packages in federated plugins.
//
// The paths must use POSIX separators (e.g., as provided by git output).
- Set<String> _getChangedPackages(List<String> changedFiles) {
+ Set<String> _getChangedPackageNames(List<String> changedFiles) {
final Set<String> packages = <String>{};
+
+ // A helper function that returns true if candidatePackageName looks like an
+ // implementation package of a plugin called pluginName. Used to determine
+ // if .../packages/parentName/candidatePackageName/...
+ // looks like a path in a federated plugin package (candidatePackageName)
+ // rather than a top-level package (parentName).
+ bool isFederatedPackage(String candidatePackageName, String parentName) {
+ return candidatePackageName == parentName ||
+ candidatePackageName.startsWith('${parentName}_');
+ }
+
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) {
- packages.add(pathComponents[packagesIndex + 1]);
+ // Find the name of the directory directly under packages. This is
+ // either the name of the package, or a plugin group directory for
+ // a federated plugin.
+ final String topLevelName = pathComponents[packagesIndex + 1];
+ String packageName = topLevelName;
+ if (packagesIndex + 2 < pathComponents.length &&
+ isFederatedPackage(
+ pathComponents[packagesIndex + 2], topLevelName)) {
+ // This looks like a federated package; use the full specifier if
+ // the name would be ambiguous (i.e., for the app-facing package).
+ packageName = pathComponents[packagesIndex + 2];
+ if (packageName == topLevelName) {
+ packageName = '$topLevelName/$packageName';
+ }
+ }
+ packages.add(packageName);
}
}
if (packages.isEmpty) {
diff --git a/script/tool/test/common/plugin_command_test.dart b/script/tool/test/common/plugin_command_test.dart
index 222df544f3..28a03c61d5 100644
--- a/script/tool/test/common/plugin_command_test.dart
+++ b/script/tool/test/common/plugin_command_test.dart
@@ -180,6 +180,79 @@ void main() {
expect(command.plugins, unorderedEquals(<String>[]));
});
+ test(
+ 'explicitly specifying the plugin (group) name of a federated plugin '
+ 'should include all plugins in the group', () async {
+ processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
+ MockProcess(stdout: '''
+packages/plugin1/plugin1/plugin1.dart
+'''),
+ ];
+ final Directory pluginGroup = packagesDir.childDirectory('plugin1');
+ final Directory appFacingPackage =
+ createFakePlugin('plugin1', pluginGroup);
+ final Directory platformInterfacePackage =
+ createFakePlugin('plugin1_platform_interface', pluginGroup);
+ final Directory implementationPackage =
+ createFakePlugin('plugin1_web', pluginGroup);
+
+ await runCapturingPrint(
+ runner, <String>['sample', '--base-sha=main', '--packages=plugin1']);
+
+ expect(
+ command.plugins,
+ unorderedEquals(<String>[
+ appFacingPackage.path,
+ platformInterfacePackage.path,
+ implementationPackage.path
+ ]));
+ });
+
+ test(
+ 'specifying the app-facing package of a federated plugin using its '
+ 'fully qualified name should include only that package', () async {
+ processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
+ MockProcess(stdout: '''
+packages/plugin1/plugin1/plugin1.dart
+'''),
+ ];
+ final Directory pluginGroup = packagesDir.childDirectory('plugin1');
+ final Directory appFacingPackage =
+ createFakePlugin('plugin1', pluginGroup);
+ createFakePlugin('plugin1_platform_interface', pluginGroup);
+ createFakePlugin('plugin1_web', pluginGroup);
+
+ await runCapturingPrint(runner,
+ <String>['sample', '--base-sha=main', '--packages=plugin1/plugin1']);
+
+ expect(command.plugins, unorderedEquals(<String>[appFacingPackage.path]));
+ });
+
+ test(
+ 'specifying a package of a federated plugin by its name should '
+ 'include only that package', () async {
+ processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
+ MockProcess(stdout: '''
+packages/plugin1/plugin1/plugin1.dart
+'''),
+ ];
+ final Directory pluginGroup = packagesDir.childDirectory('plugin1');
+
+ createFakePlugin('plugin1', pluginGroup);
+ final Directory platformInterfacePackage =
+ createFakePlugin('plugin1_platform_interface', pluginGroup);
+ createFakePlugin('plugin1_web', pluginGroup);
+
+ await runCapturingPrint(runner, <String>[
+ 'sample',
+ '--base-sha=main',
+ '--packages=plugin1_platform_interface'
+ ]);
+
+ expect(command.plugins,
+ unorderedEquals(<String>[platformInterfacePackage.path]));
+ });
+
group('conflicting package selection', () {
test('does not allow --packages with --run-on-changed-packages',
() async {
@@ -442,7 +515,7 @@ packages/plugin1/plugin1_web/plugin1_web.dart
});
test(
- 'changing one plugin in a federated group should include all plugins in the group',
+ 'changing one plugin in a federated group should only include that plugin',
() async {
processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
MockProcess(stdout: '''
@@ -451,17 +524,13 @@ packages/plugin1/plugin1/plugin1.dart
];
final Directory plugin1 =
createFakePlugin('plugin1', packagesDir.childDirectory('plugin1'));
- final Directory plugin2 = createFakePlugin('plugin1_platform_interface',
+ createFakePlugin('plugin1_platform_interface',
packagesDir.childDirectory('plugin1'));
- final Directory plugin3 = createFakePlugin(
- 'plugin1_web', packagesDir.childDirectory('plugin1'));
+ createFakePlugin('plugin1_web', packagesDir.childDirectory('plugin1'));
await runCapturingPrint(runner,
<String>['sample', '--base-sha=main', '--run-on-changed-packages']);
- expect(
- command.plugins,
- unorderedEquals(
- <String>[plugin1.path, plugin2.path, plugin3.path]));
+ expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
});
test('--exclude flag works with --run-on-changed-packages', () async {