From 0bb527ddb216429f4df306950575cf4a16a79c53 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 30 Oct 2024 14:16:26 -0400 Subject: [PATCH] [tool] Fix third_party dependency overrides `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 sometimes longer 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. --- .../lib/src/make_deps_path_based_command.dart | 23 +++--- .../make_deps_path_based_command_test.dart | 74 +++++++++++++++++-- 2 files changed, 78 insertions(+), 19 deletions(-) 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 b3682ea6f13..1f096069d5a 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 @@ 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 combinedDependencies = [ // 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. ...>[ ...pubspec.dependencies.entries, ...pubspec.devDependencies.entries, ] + .where((MapEntry element) => + element.value is! PathDependency) .where((MapEntry element) => allowsVersion(element.value, versions[element.key])) .map((MapEntry 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 relativeBasePathComponents = List.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 repoRelativePathComponents = path.split(path.relative( - localDependencies[packageName]!.path, - from: commonBasePath)); + final List repoRelativePathComponents = path.split(path + .relative(localDependencies[packageName]!.path, from: repoRootPath)); editablePubspec.update([ 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; } 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 25b0aba2831..532d56f8971 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 @@ 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 dependencies, + {required String relativePathBase}) { + final List lines = package.pubspecFile.readAsLinesSync(); + final int dependenciesStartIndex = lines.indexOf('dependencies:'); + assert(dependenciesStartIndex != -1); + lines.insertAll(dependenciesStartIndex + 1, [ + 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 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 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 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 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 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, ['foo'], relativePathBase: '../'); + + await runCapturingPrint( + runner, ['make-deps-path-based', '--target-dependencies=foo']); + + final Map 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, [ + 'bar', + ]); + + final List output = await runCapturingPrint( + runner, ['make-deps-path-based', '--target-dependencies=bar']); + + expect( + output, + containsAll([ + 'Rewriting references to: bar...', + ' Modified third_party/packages/foo/pubspec.yaml', + ])); + + final Map 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([ - contains('Skipping foo; deleted.'), + contains('Skipping packages/foo; deleted.'), contains('No target dependencies'), ]), );