[all] Add topics to pubspecs (#4771)

Adds [topics](https://dart.dev/tools/pub/pubspec#topics) to all packages, supporting the new pub feature for categorizing packages. The heuristics I used were: - Try to use existing topics from https://pub.dev/topics where applicable - Add new topics as necessary to cover things that seemed like obvious relevant topics - Include the plugin name as a topic for all federated plugin packages, for grouping (since pub doesn't inherently group or cross-link implementations) This is not an attempt to be exhaustive; as topics evolve I expect we will add more or adjust. Also updates the repo tooling to enforce topics, so that we don't forget to add them to new packages. The enforced rule is: - All packages must have at least one topic. We could potentially change this to allow an empty `topics` section so that we are enforcing that we didn't just forget to add the section, but in practice even for packages that we don't expect people to be likely to use, I didn't have any issue coming up with at least one relevant topic. - Federated plugin packages must contain the plugin name as a topic. While this isn't time-critical, I chose to include version bumps so that we aren't rolling out topics in a piecemeal way (e.g., with only a random subset of a federated plugin's packages having topics on pub.dev based on what has happened to have a bugfix).
2025-06-28 13:47:29 +08:00 · 2023-08-29 10:31:23 -07:00
parent 2fe1961828
commit b4985e25fe
216 changed files with 1029 additions and 207 deletions
--- a/script/tool/lib/src/common/package_command.dart
+++ b/script/tool/lib/src/common/package_command.dart
@ -43,6 +43,10 @@ abstract class PackageCommand extends Command<void> {
    this.platform = const LocalPlatform(),
    GitDir? gitDir,
  }) : _gitDir = gitDir {
+    thirdPartyPackagesDir = packagesDir.parent
+        .childDirectory('third_party')
+        .childDirectory('packages');
+
    argParser.addMultiOption(
      _packagesArg,
      help:
@ -137,6 +141,9 @@ abstract class PackageCommand extends Command<void> {
  /// The directory containing the packages.
  final Directory packagesDir;

+  /// The directory containing packages wrapping third-party code.
+  late Directory thirdPartyPackagesDir;
+
  /// The process runner.
  ///
  /// This can be overridden for testing.
@ -413,14 +420,10 @@ abstract class PackageCommand extends Command<void> {
      packages = <String>{currentPackageName};
    }

-    final Directory thirdPartyPackagesDirectory = packagesDir.parent
-        .childDirectory('third_party')
-        .childDirectory('packages');
-
    final Set<String> excludedPackageNames = getExcludedPackageNames();
    for (final Directory dir in <Directory>[
      packagesDir,
-      if (thirdPartyPackagesDirectory.existsSync()) thirdPartyPackagesDirectory,
+      if (thirdPartyPackagesDir.existsSync()) thirdPartyPackagesDir,
    ]) {
      await for (final FileSystemEntity entity
          in dir.list(followLinks: false)) {
--- a/script/tool/lib/src/make_deps_path_based_command.dart
+++ b/script/tool/lib/src/make_deps_path_based_command.dart
@ -124,6 +124,13 @@ class MakeDepsPathBasedCommand extends PackageCommand {
            : topLevelCandidate);
        continue;
      }
+      // Check for a match in the third-party packages directory.
+      final Directory thirdPartyCandidate =
+          thirdPartyPackagesDir.childDirectory(packageName);
+      if (thirdPartyCandidate.existsSync()) {
+        targets[packageName] = RepositoryPackage(thirdPartyCandidate);
+        continue;
+      }
      // If there is no packages/<packageName> directory, then either the
      // packages doesn't exist, or it is a sub-package of a federated plugin.
      // If it's the latter, it will be a directory whose name is a prefix.
--- a/script/tool/lib/src/pubspec_check_command.dart
+++ b/script/tool/lib/src/pubspec_check_command.dart
@ -11,6 +11,7 @@ import 'package:yaml/yaml.dart';
 import 'common/core.dart';
 import 'common/output_utils.dart';
 import 'common/package_looping_command.dart';
+import 'common/plugin_utils.dart';
 import 'common/repository_package.dart';

 /// A command to enforce pubspec conventions across the repository.
@ -58,6 +59,8 @@ class PubspecCheckCommand extends PackageLoopingCommand {
    'flutter:',
    'dependencies:',
    'dev_dependencies:',
+    'topics:',
+    'screenshots:',
    'false_secrets:',
  ];

@ -66,6 +69,8 @@ class PubspecCheckCommand extends PackageLoopingCommand {
    'dependencies:',
    'dev_dependencies:',
    'flutter:',
+    'topics:',
+    'screenshots:',
    'false_secrets:',
  ];

@ -219,6 +224,12 @@ class PubspecCheckCommand extends PackageLoopingCommand {
        passing = false;
      }

+      final String? topicsError = _checkTopics(pubspec, package: package);
+      if (topicsError != null) {
+        printError('$indentation$topicsError');
+        passing = false;
+      }
+
      // Don't check descriptions for federated package components other than
      // the app-facing package, since they are unlisted, and are expected to
      // have short descriptions.
@ -321,6 +332,29 @@ class PubspecCheckCommand extends PackageLoopingCommand {
        false;
  }

+  // Validates the "implements" keyword for a plugin, returning an error
+  // string if there are any issues.
+  String? _checkTopics(
+    Pubspec pubspec, {
+    required RepositoryPackage package,
+  }) {
+    final List<String> topics = pubspec.topics ?? <String>[];
+    if (topics.isEmpty) {
+      return 'A published package should include "topics". '
+          'See https://dart.dev/tools/pub/pubspec#topics.';
+    }
+    if (isFlutterPlugin(package) && package.isFederated) {
+      final String pluginName = package.directory.parent.basename;
+      // '_' isn't allowed in topics, so convert to '-'.
+      final String topicName = pluginName.replaceAll('_', '-');
+      if (!topics.contains(topicName)) {
+        return 'A federated plugin package should include its plugin name as '
+            'a topic. Add "$topicName" to the "topics" section.';
+      }
+    }
+    return null;
+  }
+
  // Validates the "implements" keyword for a plugin, returning an error
  // string if there are any issues.
  //
--- a/script/tool/test/make_deps_path_based_command_test.dart
+++ b/script/tool/test/make_deps_path_based_command_test.dart
@ -17,12 +17,16 @@ import 'util.dart';
 void main() {
  FileSystem fileSystem;
  late Directory packagesDir;
+  late Directory thirdPartyPackagesDir;
  late CommandRunner<void> runner;
  late RecordingProcessRunner processRunner;

  setUp(() {
    fileSystem = MemoryFileSystem();
    packagesDir = createPackagesDirectory(fileSystem: fileSystem);
+    thirdPartyPackagesDir = packagesDir.parent
+        .childDirectory('third_party')
+        .childDirectory('packages');

    final MockGitDir gitDir = MockGitDir();
    when(gitDir.path).thenReturn(packagesDir.parent.path);
@ -288,6 +292,31 @@ ${devDependencies.map((String dep) => '  $dep: $constraint').join('\n')}
            multiLine: true, dotAll: true)));
  });

+  test('finds third_party packages', () async {
+    createFakePackage('bar', thirdPartyPackagesDir, isFlutter: true);
+    final RepositoryPackage firstPartyPackge =
+        createFakePlugin('foo', packagesDir);
+
+    addDependencies(firstPartyPackge, <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 packages/foo/pubspec.yaml',
+        ]));
+
+    final Map<String, String?> simplePackageOverrides =
+        getDependencyOverrides(firstPartyPackge);
+    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 {
--- a/script/tool/test/pubspec_check_command_test.dart
+++ b/script/tool/test/pubspec_check_command_test.dart
@ -119,6 +119,13 @@ ${extraDependencies.map((String dep) => '  $dep').join('\n')}
 ''';
 }

+String _topicsSection([List<String> topics = const <String>['a-topic']]) {
+  return '''
+topics:
+${topics.map((String topic) => '  - $topic').join('\n')}
+''';
+}
+
 String _falseSecretsSection() {
  return '''
 false_secrets:
@ -160,6 +167,7 @@ ${_environmentSection()}
 ${_flutterSection(isPlugin: true)}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection()}
 ${_falseSecretsSection()}
 ''');

@ -199,6 +207,7 @@ ${_environmentSection()}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
 ${_flutterSection()}
+${_topicsSection()}
 ${_falseSecretsSection()}
 ''');

@ -236,6 +245,7 @@ ${_flutterSection()}
 ${_headerSection('package')}
 ${_environmentSection()}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      final List<String> output = await runCapturingPrint(runner, <String>[
@ -536,6 +546,93 @@ ${_devDependenciesSection()}
      );
    });

+    test('fails when topics section is missing', () async {
+      final RepositoryPackage plugin =
+          createFakePlugin('plugin', packagesDir, examples: <String>[]);
+
+      plugin.pubspecFile.writeAsStringSync('''
+${_headerSection('plugin')}
+${_environmentSection()}
+${_flutterSection(isPlugin: true)}
+${_dependenciesSection()}
+${_devDependenciesSection()}
+''');
+
+      Error? commandError;
+      final List<String> output = await runCapturingPrint(
+          runner, <String>['pubspec-check'], errorHandler: (Error e) {
+        commandError = e;
+      });
+
+      expect(commandError, isA<ToolExit>());
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('A published package should include "topics".'),
+        ]),
+      );
+    });
+
+    test('fails when topics section is empty', () async {
+      final RepositoryPackage plugin =
+          createFakePlugin('plugin', packagesDir, examples: <String>[]);
+
+      plugin.pubspecFile.writeAsStringSync('''
+${_headerSection('plugin')}
+${_environmentSection()}
+${_flutterSection(isPlugin: true)}
+${_dependenciesSection()}
+${_devDependenciesSection()}
+${_topicsSection(<String>[])}
+''');
+
+      Error? commandError;
+      final List<String> output = await runCapturingPrint(
+          runner, <String>['pubspec-check'], errorHandler: (Error e) {
+        commandError = e;
+      });
+
+      expect(commandError, isA<ToolExit>());
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('A published package should include "topics".'),
+        ]),
+      );
+    });
+
+    test('fails when federated plugin topics do not include plugin name',
+        () async {
+      final RepositoryPackage plugin = createFakePlugin(
+          'some_plugin_ios', packagesDir.childDirectory('some_plugin'),
+          examples: <String>[]);
+
+      plugin.pubspecFile.writeAsStringSync('''
+${_headerSection('plugin')}
+${_environmentSection()}
+${_flutterSection(isPlugin: true)}
+${_dependenciesSection()}
+${_devDependenciesSection()}
+${_topicsSection()}
+''');
+
+      Error? commandError;
+      final List<String> output = await runCapturingPrint(
+          runner, <String>['pubspec-check'], errorHandler: (Error e) {
+        commandError = e;
+      });
+
+      expect(commandError, isA<ToolExit>());
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains(
+              'A federated plugin package should include its plugin name as a topic. '
+              'Add "some-plugin" to the "topics" section.'),
+        ]),
+      );
+    });
+
    test('fails when environment section is out of order', () async {
      final RepositoryPackage plugin =
          createFakePlugin('plugin', packagesDir, examples: <String>[]);
@ -658,6 +755,7 @@ ${_flutterSection(isPlugin: true)}
 ${_dependenciesSection()}
 ${_falseSecretsSection()}
 ${_devDependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -688,6 +786,7 @@ ${_environmentSection()}
 ${_flutterSection(isPlugin: true)}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -717,6 +816,7 @@ ${_environmentSection()}
 ${_flutterSection(isPlugin: true, implementedPackage: 'plugin_a_foo')}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -749,6 +849,7 @@ ${_environmentSection()}
 ${_flutterSection(isPlugin: true, implementedPackage: 'plugin_a')}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection(<String>['plugin-a'])}
 ''');

      final List<String> output =
@ -782,6 +883,7 @@ ${_flutterSection(
      )}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -821,6 +923,7 @@ ${_flutterSection(
      )}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -853,6 +956,7 @@ ${_environmentSection()}
 ${_flutterSection(isPlugin: true)}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection(<String>['plugin-a'])}
 ''');

      final List<String> output =
@ -883,6 +987,7 @@ ${_environmentSection()}
 ${_flutterSection(isPlugin: true)}
 ${_dependenciesSection()}
 ${_devDependenciesSection()}
+${_topicsSection(<String>['plugin-a'])}
 ''');

      final List<String> output =
@ -970,6 +1075,7 @@ ${_devDependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(flutterConstraint: '>=2.10.0')}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -1001,6 +1107,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(flutterConstraint: '>=3.3.0', dartConstraint: '>=2.18.0 <4.0.0')}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      final List<String> output = await runCapturingPrint(runner,
@ -1026,6 +1133,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(flutterConstraint: '>=3.7.0', dartConstraint: '>=2.19.0 <4.0.0')}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      final List<String> output = await runCapturingPrint(runner,
@ -1049,6 +1157,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(dartConstraint: '>=2.14.0 <4.0.0', flutterConstraint: null)}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -1080,6 +1189,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(dartConstraint: '>=2.18.0 <4.0.0', flutterConstraint: null)}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      final List<String> output = await runCapturingPrint(runner,
@ -1105,6 +1215,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(dartConstraint: '>=2.18.0 <4.0.0', flutterConstraint: null)}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      final List<String> output = await runCapturingPrint(runner,
@ -1127,6 +1238,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -1158,6 +1270,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection(flutterConstraint: '>=3.3.0', dartConstraint: '>=2.16.0 <4.0.0')}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      Error? commandError;
@ -1190,11 +1303,13 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection(<String>['local_dependency: ^1.0.0'])}
+${_topicsSection()}
 ''');
        dependencyPackage.pubspecFile.writeAsStringSync('''
 ${_headerSection('local_dependency')}
 ${_environmentSection()}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

        final List<String> output =
@ -1217,6 +1332,7 @@ ${_dependenciesSection()}
 ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection(<String>['bad_dependency: ^1.0.0'])}
+${_topicsSection()}
 ''');

        Error? commandError;
@ -1248,6 +1364,7 @@ ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection()}
 ${_devDependenciesSection(<String>['bad_dependency: ^1.0.0'])}
+${_topicsSection()}
 ''');

        Error? commandError;
@ -1278,6 +1395,7 @@ ${_devDependenciesSection(<String>['bad_dependency: ^1.0.0'])}
 ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection(<String>['allowed: ^1.0.0'])}
+${_topicsSection()}
 ''');

        final List<String> output = await runCapturingPrint(runner,
@ -1301,6 +1419,7 @@ ${_dependenciesSection(<String>['allowed: ^1.0.0'])}
 ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection(<String>['allow_pinned: 1.0.0'])}
+${_topicsSection()}
 ''');

        final List<String> output = await runCapturingPrint(runner, <String>[
@ -1327,6 +1446,7 @@ ${_dependenciesSection(<String>['allow_pinned: 1.0.0'])}
 ${_headerSection('a_package')}
 ${_environmentSection()}
 ${_dependenciesSection(<String>['allow_pinned: ^1.0.0'])}
+${_topicsSection()}
 ''');

        Error? commandError;
@ -1385,6 +1505,7 @@ ${_dependenciesSection(<String>['allow_pinned: ^1.0.0'])}
 ${_headerSection('package')}
 ${_environmentSection()}
 ${_dependenciesSection()}
+${_topicsSection()}
 ''');

      final List<String> output =