[tool] Include examples when pathifying deps (#3393)

Improves the `make-deps-path-based` command:
- When adding an override to a package, also adds it to that package's examples. This is needed for integration tests of app-facing packages of federated plugins when adding features to implementation packages, for instance.
- Switches from string-based rewrites to `yaml_edit` to make it more robust (necessary to do the above without major restructuring)
- Improves the auto-added comment since reviewers new to the repository are often confused by the overrides the first time they encounter them and think their inclusion in the PR is a mistake.

Fixes https://github.com/flutter/flutter/issues/111091

script/tool/lib/src/make_deps_path_based_command.dart

@@ -6,6 +6,8 @@ import 'package:file/file.dart';
 import 'package:git/git.dart';
 import 'package:path/path.dart' as p;
 import 'package:pub_semver/pub_semver.dart';
+import 'package:yaml/yaml.dart';
+import 'package:yaml_edit/yaml_edit.dart';
 
 import 'common/core.dart';
 import 'common/git_version_finder.dart';
@@ -13,9 +15,6 @@ import 'common/package_command.dart';
 import 'common/repository_package.dart';
 
 const int _exitPackageNotFound = 3;
-const int _exitCannotUpdatePubspec = 4;
-
-enum _RewriteOutcome { changed, noChangesNeeded, alreadyChanged }
 
 /// Converts all dependencies on target packages to path-based dependencies.
 ///
@@ -50,8 +49,12 @@ class MakeDepsPathBasedCommand extends PackageCommand {
       'target-dependencies-with-non-breaking-updates';
 
   // The comment to add to temporary dependency overrides.
+  //
+  // Includes a reference to the docs so that reviewers who aren't familiar with
+  // the federated plugin change process don't think it's a mistake.
   static const String _dependencyOverrideWarningComment =
-      '# FOR TESTING ONLY. DO NOT MERGE.';
+      '# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.\n'
+      '# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins';
 
   @override
   final String name = 'make-deps-path-based';
@@ -80,17 +83,10 @@ class MakeDepsPathBasedCommand extends PackageCommand {
     for (final File pubspec in await _getAllPubspecs()) {
       final String displayPath = p.posix.joinAll(
           path.split(path.relative(pubspec.absolute.path, from: repoRootPath)));
-      final _RewriteOutcome outcome = await _addDependencyOverridesIfNecessary(
+      final bool changed = await _addDependencyOverridesIfNecessary(
-          pubspec, localDependencyPackages);
+          RepositoryPackage(pubspec.parent), localDependencyPackages);
-      switch (outcome) {
+      if (changed) {
-        case _RewriteOutcome.changed:
+        print('  Modified $displayPath');
-          print('  Modified $displayPath');
-          break;
-        case _RewriteOutcome.alreadyChanged:
-          print('  Skipped $displayPath - Already rewritten');
-          break;
-        case _RewriteOutcome.noChangesNeeded:
-          break;
       }
     }
   }
@@ -137,26 +133,25 @@ class MakeDepsPathBasedCommand extends PackageCommand {
   /// If [pubspecFile] has any dependencies on packages in [localDependencies],
   /// adds dependency_overrides entries to redirect them to the local version
   /// using path-based dependencies.
-  Future<_RewriteOutcome> _addDependencyOverridesIfNecessary(File pubspecFile,
+  ///
-      Map<String, RepositoryPackage> localDependencies) async {
+  /// Returns true if any overrides were added.
-    final String pubspecContents = pubspecFile.readAsStringSync();
+  ///
-    final Pubspec pubspec = Pubspec.parse(pubspecContents);
+  /// If [additionalPackagesToOverride] are provided, they will get
-    // Fail if there are any dependency overrides already, other than ones
+  /// dependency_overrides even if there is no direct dependency. This is
-    // created by this script. If support for that is needed at some point, it
+  /// useful for overriding transitive dependencies.
-    // can be added, but currently it's not and relying on that makes the logic
+  Future<bool> _addDependencyOverridesIfNecessary(
-    // here much simpler.
+    RepositoryPackage package,
-    if (pubspec.dependencyOverrides.isNotEmpty) {
+    Map<String, RepositoryPackage> localDependencies, {
-      if (pubspecContents.contains(_dependencyOverrideWarningComment)) {
+    Iterable<String> additionalPackagesToOverride = const <String>{},
-        return _RewriteOutcome.alreadyChanged;
+  }) async {
-      }
+    final String pubspecContents = package.pubspecFile.readAsStringSync();
-      printError(
-          'Packages with dependency overrides are not currently supported.');
-      throw ToolExit(_exitCannotUpdatePubspec);
-    }
 
+    // Determine the dependencies to be overridden.
+    final Pubspec pubspec = Pubspec.parse(pubspecContents);
     final Iterable<String> combinedDependencies = <String>[
       ...pubspec.dependencies.keys,
       ...pubspec.devDependencies.keys,
+      ...additionalPackagesToOverride,
     ];
     final List<String> packagesToOverride = combinedDependencies
         .where(
@@ -164,42 +159,68 @@ class MakeDepsPathBasedCommand extends PackageCommand {
         .toList();
     // Sort the combined list to avoid sort_pub_dependencies lint violations.
     packagesToOverride.sort();
-    if (packagesToOverride.isNotEmpty) {
+
-      final String commonBasePath = packagesDir.path;
+    if (packagesToOverride.isEmpty) {
-      // Find the relative path to the common base.
+      return false;
-      final int packageDepth = path
+    }
-          .split(path.relative(pubspecFile.parent.absolute.path,
+
-              from: commonBasePath))
+    // Find the relative path to the common base.
-          .length;
+    final String commonBasePath = packagesDir.path;
-      final List<String> relativeBasePathComponents =
+    final int packageDepth = path
-          List<String>.filled(packageDepth, '..');
+        .split(path.relative(package.directory.absolute.path,
-      // This is done via strings rather than by manipulating the Pubspec and
+            from: commonBasePath))
-      // then re-serialiazing so that it's a localized change, rather than
+        .length;
-      // rewriting the whole file (e.g., destroying comments), which could be
+    final List<String> relativeBasePathComponents =
-      // more disruptive for local use.
+        List<String>.filled(packageDepth, '..');
-      String newPubspecContents = '''
+
-$pubspecContents
+    // Add the overrides.
+    final YamlEditor editablePubspec = YamlEditor(pubspecContents);
+    final YamlNode root = editablePubspec.parseAt(<String>[]);
+    const String dependencyOverridesKey = 'dependency_overrides';
+    // Ensure that there's a `dependencyOverridesKey` entry to update.
+    if ((root as YamlMap)[dependencyOverridesKey] == null) {
+      editablePubspec.update(<String>[dependencyOverridesKey], YamlMap());
+    }
+    for (final String packageName in packagesToOverride) {
+      // Find the relative path from the common base to the local package.
+      final List<String> repoRelativePathComponents = path.split(path.relative(
+          localDependencies[packageName]!.path,
+          from: commonBasePath));
+      editablePubspec.update(<String>[
+        dependencyOverridesKey,
+        packageName
+      ], <String, String>{
+        'path': p.posix.joinAll(<String>[
+          ...relativeBasePathComponents,
+          ...repoRelativePathComponents,
+        ])
+      });
+    }
+
+    // Add the warning if it's not already there.
+    String newContent = editablePubspec.toString();
+    if (!newContent.contains(_dependencyOverrideWarningComment)) {
+      newContent = newContent.replaceFirst('$dependencyOverridesKey:', '''
 
 $_dependencyOverrideWarningComment
-dependency_overrides:
+$dependencyOverridesKey:
-''';
+''');
-      for (final String packageName in packagesToOverride) {
-        // Find the relative path from the common base to the local package.
-        final List<String> repoRelativePathComponents = path.split(
-            path.relative(localDependencies[packageName]!.path,
-                from: commonBasePath));
-        newPubspecContents += '''
-  $packageName:
-    path: ${p.posix.joinAll(<String>[
-              ...relativeBasePathComponents,
-              ...repoRelativePathComponents,
-            ])}
-''';
-      }
-      pubspecFile.writeAsStringSync(newPubspecContents);
-      return _RewriteOutcome.changed;
     }
-    return _RewriteOutcome.noChangesNeeded;
+
+    // Write the new pubspec.
+    package.pubspecFile.writeAsStringSync(newContent);
+
+    // Update any examples. This is important for cases like integration tests
+    // of app-facing packages in federated plugins, where the app-facing
+    // package depends directly on the implementation packages, but the
+    // example app doesn't. Since integration tests are run in the example app,
+    // it needs the overrides in order for tests to pass.
+    for (final RepositoryPackage example in package.getExamples()) {
+      _addDependencyOverridesIfNecessary(example, localDependencies,
+          additionalPackagesToOverride: packagesToOverride);
+    }
+
+    return true;
   }
 
   /// Returns all pubspecs anywhere under the packages directory.

script/tool/test/make_deps_path_based_command_test.dart

@@ -9,6 +9,7 @@ import 'package:file/file.dart';
 import 'package:file/memory.dart';
 import 'package:flutter_plugin_tools/src/make_deps_path_based_command.dart';
 import 'package:mockito/mockito.dart';
+import 'package:pubspec_parse/pubspec_parse.dart';
 import 'package:test/test.dart';
 
 import 'common/package_command_test.mocks.dart';
@@ -73,6 +74,13 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
 ''');
   }
 
+  Map<String, String> getDependencyOverrides(RepositoryPackage package) {
+    final Pubspec pubspec = package.parsePubspec();
+    return pubspec.dependencyOverrides.map((String name, Dependency dep) =>
+        MapEntry<String, String>(
+            name, (dep is PathDependency) ? dep.path : dep.toString()));
+  }
+
   test('no-ops for no plugins', () async {
     createFakePackage('foo', packagesDir, isFlutter: true);
     final RepositoryPackage packageBar =
@@ -94,6 +102,27 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
     expect(packageBar.pubspecFile.readAsStringSync(), originalPubspecContents);
   });
 
+  test('includes explanatory comment', () async {
+    final RepositoryPackage packageA =
+        createFakePackage('package_a', packagesDir, isFlutter: true);
+    createFakePackage('package_b', packagesDir, isFlutter: true);
+
+    addDependencies(packageA, <String>[
+      'package_b',
+    ]);
+
+    await runCapturingPrint(runner,
+        <String>['make-deps-path-based', '--target-dependencies=package_b']);
+
+    expect(
+        packageA.pubspecFile.readAsLinesSync(),
+        containsAllInOrder(<String>[
+          '# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.',
+          '# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins',
+          'dependency_overrides:',
+        ]));
+  });
+
   test('rewrites "dependencies" references', () async {
     final RepositoryPackage simplePackage =
         createFakePackage('foo', packagesDir, isFlutter: true);
@@ -136,23 +165,18 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
         isNot(contains(
             '  Modified packages/bar/bar_platform_interface/pubspec.yaml')));
 
-    expect(
+    final Map<String, String?> simplePackageOverrides =
-        simplePackage.pubspecFile.readAsLinesSync(),
+        getDependencyOverrides(simplePackage);
-        containsAllInOrder(<String>[
+    expect(simplePackageOverrides.length, 2);
-          '# FOR TESTING ONLY. DO NOT MERGE.',
+    expect(simplePackageOverrides['bar'], '../bar/bar');
-          'dependency_overrides:',
+    expect(simplePackageOverrides['bar_platform_interface'],
-          '  bar:',
+        '../bar/bar_platform_interface');
-          '    path: ../bar/bar',
+
-          '  bar_platform_interface:',
+    final Map<String, String?> appFacingPackageOverrides =
-          '    path: ../bar/bar_platform_interface',
+        getDependencyOverrides(pluginAppFacing);
-        ]));
+    expect(appFacingPackageOverrides.length, 1);
-    expect(
+    expect(appFacingPackageOverrides['bar_platform_interface'],
-        pluginAppFacing.pubspecFile.readAsLinesSync(),
+        '../../bar/bar_platform_interface');
-        containsAllInOrder(<String>[
-          'dependency_overrides:',
-          '  bar_platform_interface:',
-          '    path: ../../bar/bar_platform_interface',
-        ]));
   });
 
   test('rewrites "dev_dependencies" references', () async {
@@ -174,18 +198,68 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
           '  Modified packages/foo_builder/pubspec.yaml',
         ]));
 
-    expect(
+    final Map<String, String?> overrides =
-        builderPackage.pubspecFile.readAsLinesSync(),
+        getDependencyOverrides(builderPackage);
-        containsAllInOrder(<String>[
+    expect(overrides.length, 1);
-          '# FOR TESTING ONLY. DO NOT MERGE.',
+    expect(overrides['foo'], '../foo');
-          'dependency_overrides:',
+  });
-          '  foo:',
+
-          '    path: ../foo',
+  test('rewrites examples when rewriting the main package', () async {
-        ]));
+    final Directory pluginGroup = packagesDir.childDirectory('bar');
+    createFakePackage('bar_platform_interface', pluginGroup, isFlutter: true);
+    final RepositoryPackage pluginImplementation =
+        createFakePlugin('bar_android', pluginGroup);
+    final RepositoryPackage pluginAppFacing =
+        createFakePlugin('bar', pluginGroup);
+
+    addDependencies(pluginAppFacing, <String>[
+      'bar_platform_interface',
+      'bar_android',
+    ]);
+    addDependencies(pluginImplementation, <String>[
+      'bar_platform_interface',
+    ]);
+
+    await runCapturingPrint(runner,
+        <String>['make-deps-path-based', '--target-dependencies=bar_android']);
+
+    final Map<String, String?> exampleOverrides =
+        getDependencyOverrides(pluginAppFacing.getExamples().first);
+    expect(exampleOverrides.length, 1);
+    expect(exampleOverrides['bar_android'], '../../../bar/bar_android');
+  });
+
+  test('example overrides include both local and main-package dependencies',
+      () async {
+    final Directory pluginGroup = packagesDir.childDirectory('bar');
+    createFakePackage('bar_platform_interface', pluginGroup, isFlutter: true);
+    createFakePlugin('bar_android', pluginGroup);
+    final RepositoryPackage pluginAppFacing =
+        createFakePlugin('bar', pluginGroup);
+    createFakePackage('another_package', packagesDir);
+
+    addDependencies(pluginAppFacing, <String>[
+      'bar_platform_interface',
+      'bar_android',
+    ]);
+    addDependencies(pluginAppFacing.getExamples().first, <String>[
+      'another_package',
+    ]);
+
+    await runCapturingPrint(runner, <String>[
+      'make-deps-path-based',
+      '--target-dependencies=bar_android,another_package'
+    ]);
+
+    final Map<String, String?> exampleOverrides =
+        getDependencyOverrides(pluginAppFacing.getExamples().first);
+    expect(exampleOverrides.length, 2);
+    expect(exampleOverrides['another_package'], '../../../another_package');
+    expect(exampleOverrides['bar_android'], '../../../bar/bar_android');
   });
 
   test(
-      'alphabetizes overrides from different sectinos to avoid lint warnings in analysis',
+      'alphabetizes overrides from different sections to avoid lint warnings in analysis',
       () async {
     createFakePackage('a', packagesDir);
     createFakePackage('b', packagesDir);
@@ -206,18 +280,12 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
           '  Modified packages/target/pubspec.yaml',
         ]));
 
+    // This matches with a regex in order to all for either flow style or
+    // expanded style output.
     expect(
-        targetPackage.pubspecFile.readAsLinesSync(),
+        targetPackage.pubspecFile.readAsStringSync(),
-        containsAllInOrder(<String>[
+        matches(RegExp(r'dependency_overrides:.*a:.*b:.*c:.*',
-          '# FOR TESTING ONLY. DO NOT MERGE.',
+            multiLine: true, dotAll: true)));
-          'dependency_overrides:',
-          '  a:',
-          '    path: ../a',
-          '  b:',
-          '    path: ../b',
-          '  c:',
-          '    path: ../c',
-        ]));
   });
 
   // This test case ensures that running CI using this command on an interim
@@ -250,6 +318,12 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
       'make-deps-path-based',
       '--target-dependencies=bar,bar_platform_interface'
     ]);
+    final String simplePackageUpdatedContent =
+        simplePackage.pubspecFile.readAsStringSync();
+    final String appFacingPackageUpdatedContent =
+        pluginAppFacing.pubspecFile.readAsStringSync();
+    final String implementationPackageUpdatedContent =
+        pluginImplementation.pubspecFile.readAsStringSync();
     final List<String> output = await runCapturingPrint(runner, <String>[
       'make-deps-path-based',
       '--target-dependencies=bar,bar_platform_interface'
@@ -259,10 +333,16 @@ ${devDependencies.map((String dep) => '  $dep: ^1.0.0').join('\n')}
         output,
         containsAll(<String>[
           'Rewriting references to: bar, bar_platform_interface...',
-          '  Skipped packages/bar/bar/pubspec.yaml - Already rewritten',
+          '  Modified packages/bar/bar/pubspec.yaml',
-          '  Skipped packages/bar/bar_android/pubspec.yaml - Already rewritten',
+          '  Modified packages/bar/bar_android/pubspec.yaml',
-          '  Skipped packages/foo/pubspec.yaml - Already rewritten',
+          '  Modified packages/foo/pubspec.yaml',
         ]));
+    expect(simplePackageUpdatedContent,
+        simplePackage.pubspecFile.readAsStringSync());
+    expect(appFacingPackageUpdatedContent,
+        pluginAppFacing.pubspecFile.readAsStringSync());
+    expect(implementationPackageUpdatedContent,
+        pluginImplementation.pubspecFile.readAsStringSync());
   });
 
   group('target-dependencies-with-non-breaking-updates', () {