Fix and test for 'implements' pubspec entry (#4242)

The federated plugin spec calls for implementation packages to include an `implements` entry in the `plugins` section of the `pubspec.yaml` indicating what app-facing package it implements. Most of the described behaviors of the `flutter` tool aren't implemented yet, and the pub.dev features have `default_plugin` as a backstop, so we haven't noticed that they are mostly missing (or in one case, incorrect).

To better future-proof the plugins, and to provide a better example to people looking at our plugins as examples of federation, this adds a CI check to make sure that we are correctly adding it, and fixes all of the missing/incorrect values it turned up.

Fixes https://github.com/flutter/flutter/issues/88222
This commit is contained in:
stuartmorgan
2021-08-20 07:58:24 -07:00
committed by GitHub
parent 721421a091
commit 0f6d559f10
3 changed files with 301 additions and 53 deletions

View File

@ -7,6 +7,7 @@ import 'package:git/git.dart';
import 'package:platform/platform.dart';
import 'package:pubspec_parse/pubspec_parse.dart';
import 'common/core.dart';
import 'common/package_looping_command.dart';
import 'common/process_runner.dart';
@ -65,8 +66,8 @@ class PubspecCheckCommand extends PackageLoopingCommand {
@override
Future<PackageResult> runForPackage(Directory package) async {
final File pubspec = package.childFile('pubspec.yaml');
final bool passesCheck = !pubspec.existsSync() ||
await _checkPubspec(pubspec, packageName: package.basename);
final bool passesCheck =
!pubspec.existsSync() || await _checkPubspec(pubspec, package: package);
if (!passesCheck) {
return PackageResult.fail();
}
@ -75,7 +76,7 @@ class PubspecCheckCommand extends PackageLoopingCommand {
Future<bool> _checkPubspec(
File pubspecFile, {
required String packageName,
required Directory package,
}) async {
final String contents = pubspecFile.readAsStringSync();
final Pubspec? pubspec = _tryParsePubspec(contents);
@ -84,34 +85,43 @@ class PubspecCheckCommand extends PackageLoopingCommand {
}
final List<String> pubspecLines = contents.split('\n');
final List<String> sectionOrder = pubspecLines.contains(' plugin:')
? _majorPluginSections
: _majorPackageSections;
final bool isPlugin = pubspec.flutter?.containsKey('plugin') ?? false;
final List<String> sectionOrder =
isPlugin ? _majorPluginSections : _majorPackageSections;
bool passing = _checkSectionOrder(pubspecLines, sectionOrder);
if (!passing) {
print('${indentation}Major sections should follow standard '
printError('${indentation}Major sections should follow standard '
'repository ordering:');
final String listIndentation = indentation * 2;
print('$listIndentation${sectionOrder.join('\n$listIndentation')}');
printError('$listIndentation${sectionOrder.join('\n$listIndentation')}');
}
if (pubspec.publishTo != 'none') {
final List<String> repositoryErrors =
_checkForRepositoryLinkErrors(pubspec, packageName: packageName);
_checkForRepositoryLinkErrors(pubspec, packageName: package.basename);
if (repositoryErrors.isNotEmpty) {
for (final String error in repositoryErrors) {
print('$indentation$error');
printError('$indentation$error');
}
passing = false;
}
if (!_checkIssueLink(pubspec)) {
print(
printError(
'${indentation}A package should have an "issue_tracker" link to a '
'search for open flutter/flutter bugs with the relevant label:\n'
'${indentation * 2}$_expectedIssueLinkFormat<package label>');
passing = false;
}
if (isPlugin) {
final String? error =
_checkForImplementsError(pubspec, package: package);
if (error != null) {
printError('$indentation$error');
passing = false;
}
}
}
return passing;
@ -168,4 +178,52 @@ class PubspecCheckCommand extends PackageLoopingCommand {
.startsWith(_expectedIssueLinkFormat) ==
true;
}
// Validates the "implements" keyword for a plugin, returning an error
// string if there are any issues.
//
// Should only be called on plugin packages.
String? _checkForImplementsError(
Pubspec pubspec, {
required Directory package,
}) {
if (_isImplementationPackage(package)) {
final String? implements =
pubspec.flutter!['plugin']!['implements'] as String?;
final String expectedImplements = package.parent.basename;
if (implements == null) {
return 'Missing "implements: $expectedImplements" in "plugin" section.';
} else if (implements != expectedImplements) {
return 'Expecetd "implements: $expectedImplements"; '
'found "implements: $implements".';
}
}
return null;
}
// Returns true if [packageName] appears to be an implementation package
// according to repository conventions.
bool _isImplementationPackage(Directory package) {
// An implementation package should be in a group folder...
final Directory parentDir = package.parent;
if (parentDir.path == packagesDir.path) {
return false;
}
final String packageName = package.basename;
final String parentName = parentDir.basename;
// ... whose name is a prefix of the package name.
if (!packageName.startsWith(parentName)) {
return false;
}
// A few known package names are not implementation packages; assume
// anything else is. (This is done instead of listing known implementation
// suffixes to allow for non-standard suffixes; e.g., to put several
// platforms in one package for code-sharing purposes.)
const Set<String> nonImplementationSuffixes = <String>{
'', // App-facing package.
'_platform_interface', // Platform interface package.
};
final String suffix = packageName.substring(parentName.length);
return !nonImplementationSuffixes.contains(suffix);
}
}