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 1f096069d5a1..ba3f4d7322d0 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -199,8 +199,6 @@ class MakeDepsPathBasedCommand extends PackageCommand { .where( (String packageName) => localDependencies.containsKey(packageName)) .toList(); - // Sort the combined list to avoid sort_pub_dependencies lint violations. - packagesToOverride.sort(); if (packagesToOverride.isEmpty) { return false; @@ -219,34 +217,60 @@ class MakeDepsPathBasedCommand extends PackageCommand { final YamlEditor editablePubspec = YamlEditor(pubspecContents); final YamlNode root = editablePubspec.parseAt([]); const String dependencyOverridesKey = 'dependency_overrides'; - // Ensure that there's a `dependencyOverridesKey` entry to update. - if ((root as YamlMap)[dependencyOverridesKey] == null) { - editablePubspec.update([dependencyOverridesKey], YamlMap()); - } - 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: repoRootPath)); - editablePubspec.update([ - dependencyOverridesKey, - packageName - ], { - 'path': p.posix.joinAll([ + + // Add in any existing overrides, then sort the combined list to avoid + // sort_pub_dependencies lint violations. + final YamlMap? existingOverrides = + (root as YamlMap)[dependencyOverridesKey] as YamlMap?; + final List allDependencyOverrides = { + ...existingOverrides?.keys.cast() ?? [], + ...packagesToOverride, + }.toList(); + allDependencyOverrides.sort(); + + // Build a fresh overrides section, to ensure that new entries are sorted + // with any existing entries. This does mean comments will be removed, but + // since this command's changes will always be reverted anyway, that + // shouldn't be an issue, whereas lint violations will cause analysis + // failures. + // This uses string concatenation rather than YamlMap because the latter + // doesn't guarantee any particular output order for its entries. + final List newOverrideLines = []; + for (final String packageName in allDependencyOverrides) { + final String overrideValue; + if (packagesToOverride.contains(packageName)) { + // Create a new override. + + // Find the relative path from the common base to the local package. + final List repoRelativePathComponents = path.split( + path.relative(localDependencies[packageName]!.path, + from: repoRootPath)); + final String pathValue = p.posix.joinAll([ ...relativeBasePathComponents, ...repoRelativePathComponents, - ]) - }); + ]); + overrideValue = '{path: $pathValue}'; + } else { + // Re-use the pre-existing override. + overrideValue = existingOverrides![packageName].toString(); + } + newOverrideLines.add(' $packageName: $overrideValue'); } - - // Add the warning if it's not already there. + // Create an empty section as a placeholder to replace. + editablePubspec.update([dependencyOverridesKey], YamlMap()); String newContent = editablePubspec.toString(); - if (!newContent.contains(_dependencyOverrideWarningComment)) { - newContent = newContent.replaceFirst('$dependencyOverridesKey:', ''' -$_dependencyOverrideWarningComment -$dependencyOverridesKey: + // Add the warning if it's not already there. + final String warningComment = + newContent.contains(_dependencyOverrideWarningComment) + ? '' + : '$_dependencyOverrideWarningComment\n'; + // Replace the placeholder with the new section. + newContent = + newContent.replaceFirst(RegExp('$dependencyOverridesKey:\\s*{}\\n'), ''' +$warningComment$dependencyOverridesKey: +${newOverrideLines.join('\n')} '''); - } // Write the new pubspec. package.pubspecFile.writeAsStringSync(newContent); 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 532d56f89716..7e2625a49f34 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -93,6 +93,20 @@ ${devDependencies.map((String dep) => ' $dep: $constraint').join('\n')} '''); } + /// Adds a 'dependency_overrides:' section with entries for each package in + /// [overrides] to [package]. + void addDependencyOverridesSection( + RepositoryPackage package, Iterable overrides, + {String path = '../'}) { + final String originalContent = package.pubspecFile.readAsStringSync(); + package.pubspecFile.writeAsStringSync(''' +$originalContent + +dependency_overrides: +${overrides.map((String dep) => ' $dep:\n path: $path').join('\n')} +'''); + } + Map getDependencyOverrides(RepositoryPackage package) { final Pubspec pubspec = package.parsePubspec(); return pubspec.dependencyOverrides.map((String name, Dependency dep) => @@ -424,12 +438,57 @@ ${devDependencies.map((String dep) => ' $dep: $constraint').join('\n')} ' Modified packages/bar/bar_android/pubspec.yaml', ' Modified packages/foo/pubspec.yaml', ])); - expect(simplePackageUpdatedContent, - simplePackage.pubspecFile.readAsStringSync()); - expect(appFacingPackageUpdatedContent, - pluginAppFacing.pubspecFile.readAsStringSync()); - expect(implementationPackageUpdatedContent, - pluginImplementation.pubspecFile.readAsStringSync()); + expect(simplePackage.pubspecFile.readAsStringSync(), + simplePackageUpdatedContent); + expect(pluginAppFacing.pubspecFile.readAsStringSync(), + appFacingPackageUpdatedContent); + expect(pluginImplementation.pubspecFile.readAsStringSync(), + implementationPackageUpdatedContent); + }); + + test('sorts with existing overrides', () async { + final RepositoryPackage simplePackage = + createFakePackage('foo', packagesDir, isFlutter: true); + final Directory pluginGroup = packagesDir.childDirectory('bar'); + + createFakePackage('bar_platform_interface', pluginGroup, isFlutter: true); + final RepositoryPackage pluginImplementation = + createFakePlugin('bar_android', pluginGroup); + final RepositoryPackage pluginAppFacing = + createFakePlugin('bar', pluginGroup); + + addDependencies(simplePackage, [ + 'bar', + 'bar_android', + 'bar_platform_interface', + ]); + addDependencies(pluginAppFacing, [ + 'bar_platform_interface', + 'bar_android', + ]); + addDependencies(pluginImplementation, [ + 'bar_platform_interface', + ]); + addDependencyOverridesSection( + simplePackage, + ['bar_android'], + path: '../bar/bar_android', + ); + + await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies=bar,bar_platform_interface' + ]); + final String simplePackageUpdatedContent = + simplePackage.pubspecFile.readAsStringSync(); + + expect( + simplePackageUpdatedContent.split('\n'), + containsAllInOrder([ + contains(' bar:'), + contains(' bar_android:'), + contains(' bar_platform_interface:'), + ])); }); group('target-dependencies-with-non-breaking-updates', () {