Skip to content

Commit

Permalink
[ci] Check repo-level package metadata (flutter#5811)
Browse files Browse the repository at this point in the history
Adds a new tool command (and runs it in CI) to check that each package:
- is listed correctly in the repo-level README.md table
- has a CODEOWNERS entry

In the future we could add other things (e.g., auto-label), but these were the main things we've had issues with recently.

Updates README.md and CODEOWNERS to fix failures it found:
- Adds a couple of missing CODEOWNERS
- Expands the web implementation CODEOWNERS to individual packages so that we don't have to special-case handling in the tool
- Fixes some minor mistakes in README.md
- URL-encodes all `:`s in the README.md links (which is why ever line shows as changed); it worked without that in practice, but it should really be encoded, and having it consistently encoded made things easier for the tooling.
  • Loading branch information
stuartmorgan authored and arc-yong committed Jun 14, 2024
1 parent 5d45445 commit b156b17
Show file tree
Hide file tree
Showing 7 changed files with 690 additions and 44 deletions.
4 changes: 4 additions & 0 deletions .ci/targets/repo_checks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ tasks:
script: script/tool_runner.sh
args: ["gradle-check"]
always: true
- name: Repository-level package metadata validation
script: script/tool_runner.sh
args: ["check-repo-package-info"]
always: true
- name: Dependabot coverage validation
script: script/tool_runner.sh
args: ["dependabot-check"]
Expand Down
16 changes: 14 additions & 2 deletions CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,16 @@ third_party/packages/cupertino_icons/** @MitchellGoodwin
# matching entry takes precedence.

# - Web
packages/**/*_web/** @ditman
packages/camera/camera_web/** @ditman
packages/file_selector/file_selector_web/** @ditman
packages/google_maps_flutter/google_maps_flutter_web/** @ditman
packages/google_sign_in/google_sign_in_web/** @ditman
packages/image_picker/image_picker_for_web/** @ditman
packages/pointer_interceptor/pointer_interceptor_web/** @ditman
packages/shared_preferences/shared_preferences_web/** @ditman
packages/url_launcher/url_launcher_web/** @ditman
packages/video_player/video_player_web/** @ditman
packages/webview_flutter/webview_flutter_web/** @ditman

# - Android
packages/camera/camera_android/** @camsim99
Expand All @@ -68,6 +77,8 @@ packages/quick_actions/quick_actions_android/** @camsim99
packages/shared_preferences/shared_preferences_android/** @reidbaker
packages/url_launcher/url_launcher_android/** @gmackall
packages/video_player/video_player_android/** @camsim99
# Owned by ecosystem team for now during the wrapper evaluation.
packages/webview_flutter/webview_flutter_android/** @bparrishMines

# - iOS
packages/camera/camera_avfoundation/** @hellohuanlin
Expand All @@ -76,9 +87,10 @@ packages/google_maps_flutter/google_maps_flutter_ios/** @hellohuanlin
packages/google_sign_in/google_sign_in_ios/** @vashworth
packages/image_picker/image_picker_ios/** @vashworth
packages/in_app_purchase/in_app_purchase_storekit/** @louisehsu
packages/ios_platform_images/ios/** @jmagman
packages/ios_platform_images/** @jmagman
packages/local_auth/local_auth_ios/** @louisehsu
packages/path_provider/path_provider_foundation/** @jmagman
packages/pointer_interceptor/pointer_interceptor_ios/** @ditman
packages/quick_actions/quick_actions_ios/** @hellohuanlin
packages/shared_preferences/shared_preferences_foundation/** @tarrinneal
packages/url_launcher/url_launcher_ios/** @jmagman
Expand Down
86 changes: 44 additions & 42 deletions README.md

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions script/tool/lib/src/common/repository_package.dart
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,9 @@ class RepositoryPackage {
}
return null;
}

/// Returns true if the package is not marked as "publish_to: none".
bool isPublishable() {
return parsePubspec().publishTo != 'none';
}
}
2 changes: 2 additions & 0 deletions script/tool/lib/src/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import 'publish_command.dart';
import 'pubspec_check_command.dart';
import 'readme_check_command.dart';
import 'remove_dev_dependencies_command.dart';
import 'repo_package_info_check_command.dart';
import 'update_dependency_command.dart';
import 'update_excerpts_command.dart';
import 'update_min_sdk_command.dart';
Expand Down Expand Up @@ -82,6 +83,7 @@ void main(List<String> args) {
..addCommand(PubspecCheckCommand(packagesDir))
..addCommand(ReadmeCheckCommand(packagesDir))
..addCommand(RemoveDevDependenciesCommand(packagesDir))
..addCommand(RepoPackageInfoCheckCommand(packagesDir))
..addCommand(DartTestCommand(packagesDir))
..addCommand(UpdateDependencyCommand(packagesDir))
..addCommand(UpdateExcerptsCommand(packagesDir))
Expand Down
205 changes: 205 additions & 0 deletions script/tool/lib/src/repo_package_info_check_command.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:file/file.dart';

import 'common/core.dart';
import 'common/output_utils.dart';
import 'common/package_looping_command.dart';
import 'common/repository_package.dart';

const int _exitBadTableEntry = 3;
const int _exitUnknownPackageEntry = 4;

/// A command to verify repository-level metadata about packages, such as
/// repo README and CODEOWNERS entries.
class RepoPackageInfoCheckCommand extends PackageLoopingCommand {
/// Creates Dependabot check command instance.
RepoPackageInfoCheckCommand(super.packagesDir, {super.gitDir});

late Directory _repoRoot;

/// Data from the root README.md table of packages.
final Map<String, List<String>> _readmeTableEntries =
<String, List<String>>{};

/// Packages with entries in CODEOWNERS.
final List<String> _ownedPackages = <String>[];

@override
final String name = 'repo-package-info-check';

@override
List<String> get aliases => <String>['check-repo-package-info'];

@override
final String description =
'Checks that all packages are listed correctly in the repo README.';

@override
final bool hasLongOutput = false;

@override
Future<void> initializeRun() async {
_repoRoot = packagesDir.fileSystem.directory((await gitDir).path);

// Extract all of the README.md table entries.
final RegExp namePattern = RegExp(r'\[(.*?)\]\(');
for (final String line
in _repoRoot.childFile('README.md').readAsLinesSync()) {
// Find all the table entries, skipping the header.
if (line.startsWith('|') &&
!line.startsWith('| Package') &&
!line.startsWith('|-')) {
final List<String> cells = line
.split('|')
.map((String s) => s.trim())
.where((String s) => s.isNotEmpty)
.toList();
// Extract the name, removing any markdown escaping.
final String? name =
namePattern.firstMatch(cells[0])?.group(1)?.replaceAll(r'\_', '_');
if (name == null) {
printError('Unexpected README table line:\n $line');
throw ToolExit(_exitBadTableEntry);
}
_readmeTableEntries[name] = cells;

if (!(packagesDir.childDirectory(name).existsSync() ||
thirdPartyPackagesDir.childDirectory(name).existsSync())) {
printError('Unknown package "$name" in root README.md table.');
throw ToolExit(_exitUnknownPackageEntry);
}
}
}

// Extract all of the CODEOWNERS package entries.
final RegExp packageOwnershipPattern =
RegExp(r'^((?:third_party/)?packages/(?:[^/]*/)?([^/]*))/\*\*');
for (final String line
in _repoRoot.childFile('CODEOWNERS').readAsLinesSync()) {
final RegExpMatch? match = packageOwnershipPattern.firstMatch(line);
if (match == null) {
continue;
}
final String path = match.group(1)!;
final String name = match.group(2)!;
if (!_repoRoot.childDirectory(path).existsSync()) {
printError('Unknown directory "$path" in CODEOWNERS');
throw ToolExit(_exitUnknownPackageEntry);
}
_ownedPackages.add(name);
}
}

@override
Future<PackageResult> runForPackage(RepositoryPackage package) async {
final String packageName = package.directory.basename;
final List<String> errors = <String>[];

// All packages should have an owner.
// Platform interface packages are considered to be owned by the app-facing
// package owner.
if (!(_ownedPackages.contains(packageName) ||
package.isPlatformInterface &&
_ownedPackages.contains(package.directory.parent.basename))) {
printError('${indentation}Missing CODEOWNERS entry.');
errors.add('Missing CODEOWNERS entry');
}

// Any published package should be in the README table.
// For federated plugins, only the app-facing package is listed.
if (package.isPublishable() &&
(!package.isFederated || package.isAppFacing)) {
final List<String>? cells = _readmeTableEntries[packageName];

if (cells == null) {
printError('${indentation}Missing repo root README.md table entry');
errors.add('Missing repo root README.md table entry');
} else {
// Extract the two parts of a "[label](link)" .md link.
final RegExp mdLinkPattern = RegExp(r'^\[(.*)\]\((.*)\)$');
// Possible link targets.
for (final String cell in cells) {
final RegExpMatch? match = mdLinkPattern.firstMatch(cell);
if (match == null) {
printError(
'${indentation}Invalid repo root README.md table entry: "$cell"');
errors.add('Invalid root README.md table entry');
} else {
final String encodedIssueTag =
Uri.encodeComponent(_issueTagForPackage(packageName));
final String encodedPRTag =
Uri.encodeComponent(_prTagForPackage(packageName));
final String anchor = match.group(1)!;
final String target = match.group(2)!;

// The anchor should be one of:
// - The package name (optionally with any underscores escaped)
// - An image with a name-based link
// - An image with a tag-based link
final RegExp packageLink =
RegExp(r'^!\[.*\]\(https://img.shields.io/pub/.*/'
'$packageName'
r'(?:\.svg)?\)$');
final RegExp issueTagLink = RegExp(
r'^!\[.*\]\(https://img.shields.io/github/issues/flutter/flutter/'
'$encodedIssueTag'
r'\?label=\)$');
final RegExp prTagLink = RegExp(
r'^!\[.*\]\(https://img.shields.io/github/issues-pr/flutter/packages/'
'$encodedPRTag'
r'\?label=\)$');
if (!(anchor == packageName ||
anchor == packageName.replaceAll('_', r'\_') ||
packageLink.hasMatch(anchor) ||
issueTagLink.hasMatch(anchor) ||
prTagLink.hasMatch(anchor))) {
printError(
'${indentation}Incorrect anchor in root README.md table: "$anchor"');
errors.add('Incorrect anchor in root README.md table');
}

// The link should be one of:
// - a relative link to the in-repo package
// - a pub.dev link to the package
// - a github label link to the package's label
final RegExp pubDevLink =
RegExp('^https://pub.dev/packages/$packageName(?:/score)?\$');
final RegExp gitHubIssueLink = RegExp(
'^https://github.com/flutter/flutter/labels/$encodedIssueTag\$');
final RegExp gitHubPRLink = RegExp(
'^https://github.com/flutter/packages/labels/$encodedPRTag\$');
if (!(target == './packages/$packageName/' ||
target == './third_party/packages/$packageName/' ||
pubDevLink.hasMatch(target) ||
gitHubIssueLink.hasMatch(target) ||
gitHubPRLink.hasMatch(target))) {
printError(
'${indentation}Incorrect link in root README.md table: "$target"');
errors.add('Incorrect link in root README.md table');
}
}
}
}
}

return errors.isEmpty
? PackageResult.success()
: PackageResult.fail(errors);
}

String _prTagForPackage(String packageName) => 'p: $packageName';

String _issueTagForPackage(String packageName) {
switch (packageName) {
case 'google_maps_flutter':
return 'p: maps';
case 'webview_flutter':
return 'p: webview';
default:
return 'p: $packageName';
}
}
}
Loading

0 comments on commit b156b17

Please sign in to comment.