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