[tool] Fix third_party dependency overrides

script/tool/lib/src/make_deps_path_based_command.dart

@@ -153,9 +153,9 @@ class MakeDepsPathBasedCommand extends PackageCommand {
     return targets;
   }
 
-  /// If [pubspecFile] has any dependencies on packages in [localDependencies],
-  /// adds dependency_overrides entries to redirect them to the local version
-  /// using path-based dependencies.
+  /// If [pubspecFile] has any non-path dependencies on packages in
+  /// [localDependencies], adds dependency_overrides entries to redirect them to
+  /// the local version using path-based dependencies.
   ///
   /// Returns true if any overrides were added.
   ///
@@ -183,11 +183,13 @@ class MakeDepsPathBasedCommand extends PackageCommand {
     final Pubspec pubspec = Pubspec.parse(pubspecContents);
     final Iterable<String> combinedDependencies = <String>[
       // Filter out any dependencies with version constraint that wouldn't allow
-      // the target if published.
+      // the target if published, and anything that is already path-based.
       ...<MapEntry<String, Dependency>>[
         ...pubspec.dependencies.entries,
         ...pubspec.devDependencies.entries,
       ]
+          .where((MapEntry<String, Dependency> element) =>
+              element.value is! PathDependency)
           .where((MapEntry<String, Dependency> element) =>
               allowsVersion(element.value, versions[element.key]))
           .map((MapEntry<String, Dependency> entry) => entry.key),
@@ -205,10 +207,10 @@ class MakeDepsPathBasedCommand extends PackageCommand {
     }
 
     // Find the relative path to the common base.
-    final String commonBasePath = packagesDir.path;
+    final String repoRootPath = packagesDir.parent.path;
     final int packageDepth = path
-        .split(path.relative(package.directory.absolute.path,
-            from: commonBasePath))
+        .split(
+            path.relative(package.directory.absolute.path, from: repoRootPath))
         .length;
     final List<String> relativeBasePathComponents =
         List<String>.filled(packageDepth, '..');
@@ -223,9 +225,8 @@ class MakeDepsPathBasedCommand extends PackageCommand {
     }
     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));
+      final List<String> repoRelativePathComponents = path.split(path
+          .relative(localDependencies[packageName]!.path, from: repoRootPath));
       editablePubspec.update(<String>[
         dependencyOverridesKey,
         packageName
@@ -301,7 +302,7 @@ $dependencyOverridesKey:
       if (!package.pubspecFile.existsSync()) {
         final String directoryName = p.posix.joinAll(path.split(path.relative(
             package.directory.absolute.path,
-            from: packagesDir.path)));
+            from: packagesDir.parent.path)));
         print('  Skipping $directoryName; deleted.');
         continue;
       }

script/tool/test/make_deps_path_based_command_test.dart

@@ -64,6 +64,21 @@ void main() {
     package.pubspecFile.writeAsStringSync(lines.join('\n'));
   }
 
+  /// Adds dummy 'dependencies:' entries for each package in [dependencies]
+  /// to [package], using a path-based dependency.
+  void addPathDependencies(
+      RepositoryPackage package, Iterable<String> dependencies,
+      {required String relativePathBase}) {
+    final List<String> lines = package.pubspecFile.readAsLinesSync();
+    final int dependenciesStartIndex = lines.indexOf('dependencies:');
+    assert(dependenciesStartIndex != -1);
+    lines.insertAll(dependenciesStartIndex + 1, <String>[
+      for (final String dependency in dependencies)
+        '  $dependency: { path: $relativePathBase$dependency }',
+    ]);
+    package.pubspecFile.writeAsStringSync(lines.join('\n'));
+  }
+
   /// Adds a 'dev_dependencies:' section with entries for each package in
   /// [dependencies] to [package].
   void addDevDependenciesSection(
@@ -172,15 +187,15 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
     final Map<String, String?> simplePackageOverrides =
         getDependencyOverrides(simplePackage);
     expect(simplePackageOverrides.length, 2);
-    expect(simplePackageOverrides['bar'], '../bar/bar');
+    expect(simplePackageOverrides['bar'], '../../packages/bar/bar');
     expect(simplePackageOverrides['bar_platform_interface'],
-        '../bar/bar_platform_interface');
+        '../../packages/bar/bar_platform_interface');
 
     final Map<String, String?> appFacingPackageOverrides =
         getDependencyOverrides(pluginAppFacing);
     expect(appFacingPackageOverrides.length, 1);
     expect(appFacingPackageOverrides['bar_platform_interface'],
-        '../../bar/bar_platform_interface');
+        '../../../packages/bar/bar_platform_interface');
   });
 
   test('rewrites "dev_dependencies" references', () async {
@@ -205,7 +220,7 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
     final Map<String, String?> overrides =
         getDependencyOverrides(builderPackage);
     expect(overrides.length, 1);
-    expect(overrides['foo'], '../foo');
+    expect(overrides['foo'], '../../packages/foo');
   });
 
   test('rewrites examples when rewriting the main package', () async {
@@ -230,7 +245,8 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
     final Map<String, String?> exampleOverrides =
         getDependencyOverrides(pluginAppFacing.getExamples().first);
     expect(exampleOverrides.length, 1);
-    expect(exampleOverrides['bar_android'], '../../../bar/bar_android');
+    expect(exampleOverrides['bar_android'],
+        '../../../../packages/bar/bar_android');
   });
 
   test('example overrides include both local and main-package dependencies',
@@ -258,8 +274,24 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
     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');
+    expect(exampleOverrides['another_package'],
+        '../../../../packages/another_package');
+    expect(exampleOverrides['bar_android'],
+        '../../../../packages/bar/bar_android');
+  });
+
+  test('does not rewrite path-based dependencies that are already path based',
+      () async {
+    final RepositoryPackage package = createFakePlugin('foo', packagesDir);
+    final RepositoryPackage example = package.getExamples().first;
+    addPathDependencies(example, <String>['foo'], relativePathBase: '../');
+
+    await runCapturingPrint(
+        runner, <String>['make-deps-path-based', '--target-dependencies=foo']);
+
+    final Map<String, String?> exampleOverrides =
+        getDependencyOverrides(example);
+    expect(exampleOverrides.length, 0);
   });
 
   test(
@@ -317,6 +349,32 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
     expect(simplePackageOverrides['bar'], '../../third_party/packages/bar');
   });
 
+  test('handles third_party target package references in third_party',
+      () async {
+    createFakePackage('bar', thirdPartyPackagesDir, isFlutter: true);
+    final RepositoryPackage otherThirdPartyPackge =
+        createFakePlugin('foo', thirdPartyPackagesDir);
+
+    addDependencies(otherThirdPartyPackge, <String>[
+      'bar',
+    ]);
+
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['make-deps-path-based', '--target-dependencies=bar']);
+
+    expect(
+        output,
+        containsAll(<String>[
+          'Rewriting references to: bar...',
+          '  Modified third_party/packages/foo/pubspec.yaml',
+        ]));
+
+    final Map<String, String?> simplePackageOverrides =
+        getDependencyOverrides(otherThirdPartyPackge);
+    expect(simplePackageOverrides.length, 1);
+    expect(simplePackageOverrides['bar'], '../../../third_party/packages/bar');
+  });
+
   // 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 {
@@ -420,7 +478,7 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
       expect(
         output,
         containsAllInOrder(<Matcher>[
-          contains('Skipping foo; deleted.'),
+          contains('Skipping packages/foo; deleted.'),
           contains('No target dependencies'),
         ]),
       );