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

Commit 3ce611e

Browse files
stuartmorgan-gstuartmorgan-g
authored
[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 flutter/flutter#94618
1 parent 2681f50 commit 3ce611e

4 files changed

Lines changed: 139 additions & 27 deletions

File tree

‎script/tool/CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@
55
dependencies to path-based dependencies.
66
- Adds a (hidden) `--run-on-dirty-packages` flag for use with
77
`make-deps-path-based` in CI.
8+
- `--packages` now allows using a federated plugin's package as a target without
9+
fully specifying it (if it is not the same as the plugin's name). E.g.,
10+
`--packages=path_provide_ios` now works.
11+
- `--run-on-changed-packages` now includes only the changed packages in a
12+
federated plugin, not all packages in that plugin.
813
- Fix `federation-safety-check` handling of plugin deletion, and of top-level
914
files in unfederated plugins whose names match federated plugin heuristics
1015
(e.g., `packages/foo/foo_android.iml`).

‎script/tool/README.md‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,13 @@ following shows a number of common commands being run for a specific plugin.
5151
All examples assume running from source; see above for running the
5252
published version instead.
5353

54-
Note that the `plugins` argument, despite the name, applies to any package.
55-
(It will likely be renamed `packages` in the future.)
54+
Most commands take a `--packages` argument to control which package(s) the
55+
command is targetting. An package name can be any of:
56+
- The name of a package (e.g., `path_provider_android`).
57+
- The name of a federated plugin (e.g., `path_provider`), in which case all
58+
packages that make up that plugin will be targetted.
59+
- A combination federated_plugin_name/package_name (e.g.,
60+
`path_provider/path_provider` for the app-facing package).
5661

5762
### Format Code
5863

‎script/tool/lib/src/common/plugin_command.dart‎

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ abstract class PluginCommand extends Command<void> {
339339
final List<String> changedFiles =
340340
await gitVersionFinder.getChangedFiles();
341341
if (!_changesRequireFullTest(changedFiles)) {
342-
packages = _getChangedPackages(changedFiles);
342+
packages = _getChangedPackageNames(changedFiles);
343343
}
344344
} else if (getBoolArg(_runOnDirtyPackagesArg)) {
345345
final GitVersionFinder gitVersionFinder =
@@ -348,7 +348,7 @@ abstract class PluginCommand extends Command<void> {
348348
// _changesRequireFullTest is deliberately not used here, as this flag is
349349
// intended for use in CI to re-test packages changed by
350350
// 'make-deps-path-based'.
351-
packages = _getChangedPackages(
351+
packages = _getChangedPackageNames(
352352
await gitVersionFinder.getChangedFiles(includeUncommitted: true));
353353
// For the same reason, empty is not treated as "all packages" as it is
354354
// for other flags.
@@ -379,21 +379,23 @@ abstract class PluginCommand extends Command<void> {
379379
await for (final FileSystemEntity subdir
380380
in entity.list(followLinks: false)) {
381381
if (_isDartPackage(subdir)) {
382-
// If --plugin=my_plugin is passed, then match all federated
383-
// plugins under 'my_plugin'. Also match if the exact plugin is
384-
// passed.
385-
final String relativePath =
386-
path.relative(subdir.path, from: dir.path);
387-
final String packageName = path.basename(subdir.path);
388-
final String basenamePath = path.basename(entity.path);
382+
// There are three ways for a federated plugin to match:
383+
// - package name (path_provider_android)
384+
// - fully specified name (path_provider/path_provider_android)
385+
// - group name (path_provider), which matches all packages in
386+
// the group
387+
final Set<String> possibleMatches = <String>{
388+
path.basename(subdir.path), // package name
389+
path.basename(entity.path), // group name
390+
path.relative(subdir.path, from: dir.path), // fully specified
391+
};
389392
if (packages.isEmpty ||
390-
packages.contains(relativePath) ||
391-
packages.contains(basenamePath)) {
393+
packages.intersection(possibleMatches).isNotEmpty) {
392394
yield PackageEnumerationEntry(
393395
RepositoryPackage(subdir as Directory),
394-
excluded: excludedPluginNames.contains(basenamePath) ||
395-
excludedPluginNames.contains(packageName) ||
396-
excludedPluginNames.contains(relativePath));
396+
excluded: excludedPluginNames
397+
.intersection(possibleMatches)
398+
.isNotEmpty);
397399
}
398400
}
399401
}
@@ -454,17 +456,48 @@ abstract class PluginCommand extends Command<void> {
454456
return gitVersionFinder;
455457
}
456458

457-
// Returns packages that have been changed given a list of changed files.
459+
// Returns the names of packages that have been changed given a list of
460+
// changed files.
461+
//
462+
// The names will either be the actual package names, or potentially
463+
// group/name specifiers (for example, path_provider/path_provider) for
464+
// packages in federated plugins.
458465
//
459466
// The paths must use POSIX separators (e.g., as provided by git output).
460-
Set<String> _getChangedPackages(List<String> changedFiles) {
467+
Set<String> _getChangedPackageNames(List<String> changedFiles) {
461468
final Set<String> packages = <String>{};
469+
470+
// A helper function that returns true if candidatePackageName looks like an
471+
// implementation package of a plugin called pluginName. Used to determine
472+
// if .../packages/parentName/candidatePackageName/...
473+
// looks like a path in a federated plugin package (candidatePackageName)
474+
// rather than a top-level package (parentName).
475+
bool isFederatedPackage(String candidatePackageName, String parentName) {
476+
return candidatePackageName == parentName ||
477+
candidatePackageName.startsWith('${parentName}_');
478+
}
479+
462480
for (final String path in changedFiles) {
463481
final List<String> pathComponents = p.posix.split(path);
464482
final int packagesIndex =
465483
pathComponents.indexWhere((String element) => element == 'packages');
466484
if (packagesIndex != -1) {
467-
packages.add(pathComponents[packagesIndex + 1]);
485+
// Find the name of the directory directly under packages. This is
486+
// either the name of the package, or a plugin group directory for
487+
// a federated plugin.
488+
final String topLevelName = pathComponents[packagesIndex + 1];
489+
String packageName = topLevelName;
490+
if (packagesIndex + 2 < pathComponents.length &&
491+
isFederatedPackage(
492+
pathComponents[packagesIndex + 2], topLevelName)) {
493+
// This looks like a federated package; use the full specifier if
494+
// the name would be ambiguous (i.e., for the app-facing package).
495+
packageName = pathComponents[packagesIndex + 2];
496+
if (packageName == topLevelName) {
497+
packageName = '$topLevelName/$packageName';
498+
}
499+
}
500+
packages.add(packageName);
468501
}
469502
}
470503
if (packages.isEmpty) {

‎script/tool/test/common/plugin_command_test.dart‎

Lines changed: 77 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,79 @@ void main() {
180180
expect(command.plugins, unorderedEquals(<String>[]));
181181
});
182182

183+
test(
184+
'explicitly specifying the plugin (group) name of a federated plugin '
185+
'should include all plugins in the group', () async {
186+
processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
187+
MockProcess(stdout: '''
188+
packages/plugin1/plugin1/plugin1.dart
189+
'''),
190+
];
191+
final Directory pluginGroup = packagesDir.childDirectory('plugin1');
192+
final Directory appFacingPackage =
193+
createFakePlugin('plugin1', pluginGroup);
194+
final Directory platformInterfacePackage =
195+
createFakePlugin('plugin1_platform_interface', pluginGroup);
196+
final Directory implementationPackage =
197+
createFakePlugin('plugin1_web', pluginGroup);
198+
199+
await runCapturingPrint(
200+
runner, <String>['sample', '--base-sha=main', '--packages=plugin1']);
201+
202+
expect(
203+
command.plugins,
204+
unorderedEquals(<String>[
205+
appFacingPackage.path,
206+
platformInterfacePackage.path,
207+
implementationPackage.path
208+
]));
209+
});
210+
211+
test(
212+
'specifying the app-facing package of a federated plugin using its '
213+
'fully qualified name should include only that package', () async {
214+
processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
215+
MockProcess(stdout: '''
216+
packages/plugin1/plugin1/plugin1.dart
217+
'''),
218+
];
219+
final Directory pluginGroup = packagesDir.childDirectory('plugin1');
220+
final Directory appFacingPackage =
221+
createFakePlugin('plugin1', pluginGroup);
222+
createFakePlugin('plugin1_platform_interface', pluginGroup);
223+
createFakePlugin('plugin1_web', pluginGroup);
224+
225+
await runCapturingPrint(runner,
226+
<String>['sample', '--base-sha=main', '--packages=plugin1/plugin1']);
227+
228+
expect(command.plugins, unorderedEquals(<String>[appFacingPackage.path]));
229+
});
230+
231+
test(
232+
'specifying a package of a federated plugin by its name should '
233+
'include only that package', () async {
234+
processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
235+
MockProcess(stdout: '''
236+
packages/plugin1/plugin1/plugin1.dart
237+
'''),
238+
];
239+
final Directory pluginGroup = packagesDir.childDirectory('plugin1');
240+
241+
createFakePlugin('plugin1', pluginGroup);
242+
final Directory platformInterfacePackage =
243+
createFakePlugin('plugin1_platform_interface', pluginGroup);
244+
createFakePlugin('plugin1_web', pluginGroup);
245+
246+
await runCapturingPrint(runner, <String>[
247+
'sample',
248+
'--base-sha=main',
249+
'--packages=plugin1_platform_interface'
250+
]);
251+
252+
expect(command.plugins,
253+
unorderedEquals(<String>[platformInterfacePackage.path]));
254+
});
255+
183256
group('conflicting package selection', () {
184257
test('does not allow --packages with --run-on-changed-packages',
185258
() async {
@@ -442,7 +515,7 @@ packages/plugin1/plugin1_web/plugin1_web.dart
442515
});
443516

444517
test(
445-
'changing one plugin in a federated group should include all plugins in the group',
518+
'changing one plugin in a federated group should only include that plugin',
446519
() async {
447520
processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
448521
MockProcess(stdout: '''
@@ -451,17 +524,13 @@ packages/plugin1/plugin1/plugin1.dart
451524
];
452525
final Directory plugin1 =
453526
createFakePlugin('plugin1', packagesDir.childDirectory('plugin1'));
454-
final Directory plugin2 = createFakePlugin('plugin1_platform_interface',
527+
createFakePlugin('plugin1_platform_interface',
455528
packagesDir.childDirectory('plugin1'));
456-
final Directory plugin3 = createFakePlugin(
457-
'plugin1_web', packagesDir.childDirectory('plugin1'));
529+
createFakePlugin('plugin1_web', packagesDir.childDirectory('plugin1'));
458530
await runCapturingPrint(runner,
459531
<String>['sample', '--base-sha=main', '--run-on-changed-packages']);
460532

461-
expect(
462-
command.plugins,
463-
unorderedEquals(
464-
<String>[plugin1.path, plugin2.path, plugin3.path]));
533+
expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
465534
});
466535

467536
test('--exclude flag works with --run-on-changed-packages', () async {

0 commit comments

Comments
 (0)