[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,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`).

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
 

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) {

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 {