[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.
diff --git a/script/tool/lib/src/make_deps_path_based_command.dart b/script/tool/lib/src/make_deps_path_based_command.dart
index b3682ea..1f09606 100644
--- a/script/tool/lib/src/make_deps_path_based_command.dart
+++ b/script/tool/lib/src/make_deps_path_based_command.dart
@@ -153,9 +153,9 @@
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,12 +183,14 @@
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),
...additionalPackagesToOverride,
@@ -205,10 +207,10 @@
}
// 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 @@
}
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 @@
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;
}
diff --git a/script/tool/test/make_deps_path_based_command_test.dart b/script/tool/test/make_deps_path_based_command_test.dart
index 25b0aba..532d56f 100644
--- a/script/tool/test/make_deps_path_based_command_test.dart
+++ b/script/tool/test/make_deps_path_based_command_test.dart
@@ -64,6 +64,21 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
expect(
output,
containsAllInOrder(<Matcher>[
- contains('Skipping foo; deleted.'),
+ contains('Skipping packages/foo; deleted.'),
contains('No target dependencies'),
]),
);
