[tool] Fix third_party dependency overrides (#7966)

`made-deps-path-based` would sometimes create invalid relative paths when `third_party/packages` was involved because it was using the sibling directory of `packages` as the base. This updates the logic to always make the paths relative to the repository root; this is often a longer relative path than necessary, but that's harmless, and always using the repo root makes it easier to reason about.

Also fixes the fact that paths that were already path based (which is always the case for `some_package/example`'s dependency on `some_package`) were being overridden, causing CI to do some unnecessary duplicate analysis work.

Author: stuartmorgan-g

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'),
         ]),
       );