[tool] Ensure that injected dependency overrides are sorted (#8542)

Updates `make-deps-path-based` to manually output dependency overrides in sorted order, as `YamlMap` apparently doesn't have a defined output ordering, leading to CI analysis failures in some cases.

Fixes flutter/flutter#160810

Author: stuartmorgan-g

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(<String>[]);
     const String dependencyOverridesKey = 'dependency_overrides';
-    // Ensure that there's a `dependencyOverridesKey` entry to update.
-    if ((root as YamlMap)[dependencyOverridesKey] == null) {
-      editablePubspec.update(<String>[dependencyOverridesKey], YamlMap());
-    }
-    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: repoRootPath));
-      editablePubspec.update(<String>[
-        dependencyOverridesKey,
-        packageName
-      ], <String, String>{
-        'path': p.posix.joinAll(<String>[
+
+    // 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<String> allDependencyOverrides = <String>{
+      ...existingOverrides?.keys.cast<String>() ?? <String>[],
+      ...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<String> newOverrideLines = <String>[];
+    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<String> repoRelativePathComponents = path.split(
+            path.relative(localDependencies[packageName]!.path,
+                from: repoRootPath));
+        final String pathValue = p.posix.joinAll(<String>[
           ...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(<String>[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);

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<String> 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<String, String> 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, <String>[
+      'bar',
+      'bar_android',
+      'bar_platform_interface',
+    ]);
+    addDependencies(pluginAppFacing, <String>[
+      'bar_platform_interface',
+      'bar_android',
+    ]);
+    addDependencies(pluginImplementation, <String>[
+      'bar_platform_interface',
+    ]);
+    addDependencyOverridesSection(
+      simplePackage,
+      <String>['bar_android'],
+      path: '../bar/bar_android',
+    );
+
+    await runCapturingPrint(runner, <String>[
+      'make-deps-path-based',
+      '--target-dependencies=bar,bar_platform_interface'
+    ]);
+    final String simplePackageUpdatedContent =
+        simplePackage.pubspecFile.readAsStringSync();
+
+    expect(
+        simplePackageUpdatedContent.split('\n'),
+        containsAllInOrder(<Matcher>[
+          contains('  bar:'),
+          contains('  bar_android:'),
+          contains('  bar_platform_interface:'),
+        ]));
   });
 
   group('target-dependencies-with-non-breaking-updates', () {