Skip to content

Commit

Permalink
[tool] Fix third_party dependency overrides (#7966)
Browse files Browse the repository at this point in the history 
`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.
  • Loading branch information
@stuartmorgan
stuartmorgan authored Oct 30, 2024
1 parent 4feddff commit 030dd4e
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 19 deletions.
23 changes: 12 additions & 11 deletions script/tool/lib/src/make_deps_path_based_command.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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),
Expand All @@ -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, '..');
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down
74 changes: 66 additions & 8 deletions script/tool/test/make_deps_path_based_command_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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',
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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'),
]),
);
Expand Down

0 comments on commit 030dd4e

Please sign in to comment.