[flutter_plugin_tool] Allow re-pathifying dependencies (#5376)

script/tool/CHANGELOG.md

@@ -1,3 +1,8 @@
+## NEXT
+
+- Allows `make-deps-path-based` to skip packages it has alredy rewritten, so
+  that running multiple times won't fail after the first time.
+
 ## 0.8.2+1
 
 - Adds a new `readme-check` command.

script/tool/lib/src/make_deps_path_based_command.dart

@@ -16,6 +16,8 @@ import 'common/plugin_command.dart';
 const int _exitPackageNotFound = 3;
 const int _exitCannotUpdatePubspec = 4;
 
+enum _RewriteOutcome { changed, noChangesNeeded, alreadyChanged }
+
 /// Converts all dependencies on target packages to path-based dependencies.
 ///
 /// This is to allow for pre-publish testing of changes that could affect other
@@ -48,6 +50,10 @@ class MakeDepsPathBasedCommand extends PluginCommand {
   static const String _targetDependenciesWithNonBreakingUpdatesArg =
       'target-dependencies-with-non-breaking-updates';
 
+  // The comment to add to temporary dependency overrides.
+  static const String _dependencyOverrideWarningComment =
+      '# FOR TESTING ONLY. DO NOT MERGE.';
+
   @override
   final String name = 'make-deps-path-based';
 
@@ -73,12 +79,19 @@ class MakeDepsPathBasedCommand extends PluginCommand {
 
     final String repoRootPath = (await gitDir).path;
     for (final File pubspec in await _getAllPubspecs()) {
-      if (await _addDependencyOverridesIfNecessary(
+      final String displayPath = p.posix.joinAll(
-          pubspec, localDependencyPackages)) {
+          path.split(path.relative(pubspec.absolute.path, from: repoRootPath)));
-        // Print the relative path of the changed pubspec.
+      final _RewriteOutcome outcome = await _addDependencyOverridesIfNecessary(
-        final String displayPath = p.posix.joinAll(path
+          pubspec, localDependencyPackages);
-            .split(path.relative(pubspec.absolute.path, from: repoRootPath)));
+      switch (outcome) {
-        print('  Modified $displayPath');
+        case _RewriteOutcome.changed:
+          print('  Modified $displayPath');
+          break;
+        case _RewriteOutcome.alreadyChanged:
+          print('  Skipped $displayPath - Already rewritten');
+          break;
+        case _RewriteOutcome.noChangesNeeded:
+          break;
       }
     }
   }
@@ -125,16 +138,18 @@ class MakeDepsPathBasedCommand extends PluginCommand {
   /// 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,
-  /// Returns true if any changes were made.
-  Future<bool> _addDependencyOverridesIfNecessary(File pubspecFile,
       Map<String, RepositoryPackage> localDependencies) async {
     final String pubspecContents = pubspecFile.readAsStringSync();
     final Pubspec pubspec = Pubspec.parse(pubspecContents);
-    // Fail if there are any dependency overrides already. If support for that
+    // Fail if there are any dependency overrides already, other than ones
-    // is needed at some point, it can be added, but currently it's not and
+    // created by this script. If support for that is needed at some point, it
-    // relying on that makes the logic here much simpler.
+    // can be added, but currently it's not and relying on that makes the logic
+    // here much simpler.
     if (pubspec.dependencyOverrides.isNotEmpty) {
+      if (pubspecContents.contains(_dependencyOverrideWarningComment)) {
+        return _RewriteOutcome.alreadyChanged;
+      }
       printError(
           'Plugins with dependency overrides are not currently supported.');
       throw ToolExit(_exitCannotUpdatePubspec);
@@ -158,7 +173,7 @@ class MakeDepsPathBasedCommand extends PluginCommand {
       String newPubspecContents = pubspecContents +
           '''
 
-# FOR TESTING ONLY. DO NOT MERGE.
+$_dependencyOverrideWarningComment
 dependency_overrides:
 ''';
       for (final String packageName in packagesToOverride) {
@@ -175,9 +190,9 @@ dependency_overrides:
 ''';
       }
       pubspecFile.writeAsStringSync(newPubspecContents);
-      return true;
+      return _RewriteOutcome.changed;
     }
-    return false;
+    return _RewriteOutcome.noChangesNeeded;
   }
 
   /// Returns all pubspecs anywhere under the packages directory.

script/tool/test/make_deps_path_based_command_test.dart

@@ -144,6 +144,52 @@ void main() {
         ]));
   });
 
+  // This test case ensures that running CI using this command on an interim
+  // PR that itself used this command won't fail on the rewrite step.
+  test('running a second time no-ops without failing', () async {
+    final RepositoryPackage simplePackage = RepositoryPackage(
+        createFakePackage('foo', packagesDir, isFlutter: true));
+    final Directory pluginGroup = packagesDir.childDirectory('bar');
+
+    RepositoryPackage(createFakePackage('bar_platform_interface', pluginGroup,
+        isFlutter: true));
+    final RepositoryPackage pluginImplementation =
+        RepositoryPackage(createFakePlugin('bar_android', pluginGroup));
+    final RepositoryPackage pluginAppFacing =
+        RepositoryPackage(createFakePlugin('bar', pluginGroup));
+
+    _addDependencies(simplePackage, <String>[
+      'bar',
+      'bar_android',
+      'bar_platform_interface',
+    ]);
+    _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,bar_platform_interface'
+    ]);
+    final List<String> output = await runCapturingPrint(runner, <String>[
+      'make-deps-path-based',
+      '--target-dependencies=bar,bar_platform_interface'
+    ]);
+
+    expect(
+        output,
+        containsAll(<String>[
+          'Rewriting references to: bar, bar_platform_interface...',
+          '  Skipped packages/bar/bar/pubspec.yaml - Already rewritten',
+          '  Skipped packages/bar/bar_android/pubspec.yaml - Already rewritten',
+          '  Skipped packages/foo/pubspec.yaml - Already rewritten',
+        ]));
+  });
+
   group('target-dependencies-with-non-breaking-updates', () {
     test('no-ops for no published changes', () async {
       final Directory package = createFakePackage('foo', packagesDir);