From 9ef5601e0e75fe1fdde95ac18931475d9b48adaf Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 20 Apr 2026 14:14:13 -0400 Subject: [PATCH 1/6] Extract most of the readme check into a separate validator class --- script/tool/lib/src/common/file_utils.dart | 18 + .../src/common/package_looping_command.dart | 5 +- .../tool/lib/src/license_check_command.dart | 9 +- .../lib/src/make_deps_path_based_command.dart | 12 +- script/tool/lib/src/readme_check_command.dart | 320 +---------------- .../lib/src/validators/readme_validator.dart | 339 ++++++++++++++++++ .../tool/test/license_check_command_test.dart | 6 +- 7 files changed, 383 insertions(+), 326 deletions(-) create mode 100644 script/tool/lib/src/validators/readme_validator.dart diff --git a/script/tool/lib/src/common/file_utils.dart b/script/tool/lib/src/common/file_utils.dart index 199e62196ab3..cbd88bf89588 100644 --- a/script/tool/lib/src/common/file_utils.dart +++ b/script/tool/lib/src/common/file_utils.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'package:file/file.dart'; +import 'package:path/path.dart' as p; /// Returns a [File] created by appending all but the last item in [components] /// to [base] as subdirectories, then appending the last as a file. @@ -31,3 +32,20 @@ Directory childDirectoryWithSubcomponents( } return dir; } + +/// Returns the relative path from [from] to [entity] using [platformContext] +/// as the path context, but always formatting the result as a POSIX path +/// (using forward slashes). +/// +/// This is useful for generating paths that will be used in configuration +/// files or command lines that expect POSIX paths, even when running on a +/// platform that uses a different path separator, or for display. +String relativePosixPath( + FileSystemEntity entity, { + required Directory from, + required p.Context platformContext, +}) => p.posix.joinAll( + platformContext.split( + platformContext.relative(entity.absolute.path, from: from.path), + ), +); diff --git a/script/tool/lib/src/common/package_looping_command.dart b/script/tool/lib/src/common/package_looping_command.dart index 3aa518a9012a..53c885bcf6e7 100644 --- a/script/tool/lib/src/common/package_looping_command.dart +++ b/script/tool/lib/src/common/package_looping_command.dart @@ -5,10 +5,10 @@ import 'dart:async'; import 'package:file/file.dart'; -import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'core.dart'; +import 'file_utils.dart'; import 'git_version_finder.dart'; import 'output_utils.dart'; import 'package_command.dart'; @@ -238,8 +238,7 @@ abstract class PackageLoopingCommand extends PackageCommand { String getRelativePosixPath( FileSystemEntity entity, { required Directory from, - }) => - p.posix.joinAll(path.split(path.relative(entity.path, from: from.path))); + }) => relativePosixPath(entity, from: from, platformContext: path); /// The suggested indentation for printed output. String get indentation => hasLongOutput ? '' : ' '; diff --git a/script/tool/lib/src/license_check_command.dart b/script/tool/lib/src/license_check_command.dart index b5e8a365cf28..f7b882b7de5a 100644 --- a/script/tool/lib/src/license_check_command.dart +++ b/script/tool/lib/src/license_check_command.dart @@ -9,6 +9,7 @@ import 'package:git/git.dart'; import 'package:path/path.dart' as p; import 'common/core.dart'; +import 'common/file_utils.dart'; import 'common/output_utils.dart'; import 'common/package_command.dart'; @@ -393,10 +394,10 @@ class LicenseCheckCommand extends PackageCommand { } String _repoRelativePath(File file) { - return p.posix.joinAll( - path.split( - path.relative(file.absolute.path, from: packagesDir.parent.path), - ), + return relativePosixPath( + file, + from: packagesDir.parent, + platformContext: path, ); } } 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 ed8119cb7c62..ed84f338bb30 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -10,6 +10,7 @@ import 'package:yaml/yaml.dart'; import 'package:yaml_edit/yaml_edit.dart'; import 'common/core.dart'; +import 'common/file_utils.dart'; import 'common/git_version_finder.dart'; import 'common/output_utils.dart'; import 'common/package_command.dart'; @@ -347,13 +348,10 @@ ${newOverrideLines.join('\n')} ); // Ignored deleted packages, as they won't be published. if (!package.pubspecFile.existsSync()) { - final String directoryName = p.posix.joinAll( - path.split( - path.relative( - package.directory.absolute.path, - from: packagesDir.parent.path, - ), - ), + final String directoryName = relativePosixPath( + package.directory, + from: packagesDir.parent, + platformContext: path, ); print(' Skipping $directoryName; deleted.'); continue; diff --git a/script/tool/lib/src/readme_check_command.dart b/script/tool/lib/src/readme_check_command.dart index ea290d52379c..452579055992 100644 --- a/script/tool/lib/src/readme_check_command.dart +++ b/script/tool/lib/src/readme_check_command.dart @@ -3,15 +3,12 @@ // found in the LICENSE file. import 'package:file/file.dart'; -import 'package:yaml/yaml.dart'; import 'common/core.dart'; import 'common/output_utils.dart'; import 'common/package_looping_command.dart'; import 'common/repository_package.dart'; - -const String _instructionUrl = - 'https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md'; +import 'validators/readme_validator.dart'; /// A command to enforce README conventions across the repository. class ReadmeCheckCommand extends PackageLoopingCommand { @@ -30,16 +27,6 @@ class ReadmeCheckCommand extends PackageLoopingCommand { static const String _requireExcerptsArg = 'require-excerpts'; - // Standardized capitalizations for platforms that a plugin can support. - static const Map _standardPlatformNames = { - 'android': 'Android', - 'ios': 'iOS', - 'linux': 'Linux', - 'macos': 'macOS', - 'web': 'Web', - 'windows': 'Windows', - }; - @override final String name = 'readme-check'; @@ -55,14 +42,21 @@ class ReadmeCheckCommand extends PackageLoopingCommand { @override Future runForPackage(RepositoryPackage package) async { - final List errors = _validateReadme( + final validator = ReadmeValidator( + path: path, + indentation: indentation, + warningLogger: printWarning, + requireCodeExcerpts: getBoolArg(_requireExcerptsArg), + ); + + final List errors = validator.validateReadme( package.readmeFile, mainPackage: package, isExample: false, ); for (final RepositoryPackage packageToCheck in package.getExamples()) { errors.addAll( - _validateReadme( + validator.validateReadme( packageToCheck.readmeFile, mainPackage: package, isExample: true, @@ -76,7 +70,7 @@ class ReadmeCheckCommand extends PackageLoopingCommand { final File exampleDirReadme = exampleDir.childFile('README.md'); if (exampleDir.existsSync() && !isPackage(exampleDir)) { errors.addAll( - _validateReadme( + validator.validateReadme( exampleDirReadme, mainPackage: package, isExample: true, @@ -88,296 +82,4 @@ class ReadmeCheckCommand extends PackageLoopingCommand { ? PackageResult.success() : PackageResult.fail(errors); } - - List _validateReadme( - File readme, { - required RepositoryPackage mainPackage, - required bool isExample, - }) { - if (!readme.existsSync()) { - if (isExample) { - print( - '${indentation}No README for ' - '${getRelativePosixPath(readme.parent, from: mainPackage.directory)}', - ); - return []; - } else { - printError( - '${indentation}No README found at ' - '${getRelativePosixPath(readme, from: mainPackage.directory)}', - ); - return ['Missing README.md']; - } - } - - print( - '${indentation}Checking ' - '${getRelativePosixPath(readme, from: mainPackage.directory)}...', - ); - - final List readmeLines = readme.readAsLinesSync(); - final errors = []; - - final String? blockValidationError = _validateCodeBlocks( - readmeLines, - mainPackage: mainPackage, - ); - if (blockValidationError != null) { - errors.add(blockValidationError); - } - - errors.addAll( - _validateBoilerplate( - readmeLines, - mainPackage: mainPackage, - isExample: isExample, - ), - ); - - // Check if this is the main readme for a plugin, and if so enforce extra - // checks. - if (!isExample) { - final Pubspec pubspec = mainPackage.parsePubspec(); - final isPlugin = pubspec.flutter?['plugin'] != null; - if (isPlugin && (!mainPackage.isFederated || mainPackage.isAppFacing)) { - final String? error = _validateSupportedPlatforms(readmeLines, pubspec); - if (error != null) { - errors.add(error); - } - } - } - - return errors; - } - - /// Validates that code blocks (``` ... ```) follow repository standards. - String? _validateCodeBlocks( - List readmeLines, { - required RepositoryPackage mainPackage, - }) { - final codeBlockDelimiterPattern = RegExp(r'^\s*```\s*([^ ]*)\s*'); - const excerptTagStart = '[]; - final missingExcerptLines = []; - var inBlock = false; - for (var i = 0; i < readmeLines.length; ++i) { - final RegExpMatch? match = codeBlockDelimiterPattern.firstMatch( - readmeLines[i], - ); - if (match == null) { - continue; - } - if (inBlock) { - inBlock = false; - continue; - } - inBlock = true; - - final int humanReadableLineNumber = i + 1; - - // Ensure that there's a language tag. - final String infoString = match[1] ?? ''; - if (infoString.isEmpty) { - missingLanguageLines.add(humanReadableLineNumber); - continue; - } - - // Check for code-excerpt usage if requested. - if (getBoolArg(_requireExcerptsArg) && infoString == 'dart') { - if (i == 0 || !readmeLines[i - 1].trim().startsWith(excerptTagStart)) { - missingExcerptLines.add(humanReadableLineNumber); - } - } - } - - String? errorSummary; - - if (missingLanguageLines.isNotEmpty) { - for (final lineNumber in missingLanguageLines) { - printError( - '${indentation}Code block at line $lineNumber is missing ' - 'a language identifier.', - ); - } - printError( - '\n${indentation}For each block listed above, add a language tag to ' - 'the opening block. For instance, for Dart code, use:\n' - '${indentation * 2}```dart\n', - ); - errorSummary = 'Missing language identifier for code block'; - } - - if (missingExcerptLines.isNotEmpty) { - for (final lineNumber in missingExcerptLines) { - printError( - '${indentation}Dart code block at line $lineNumber is not ' - 'managed by code-excerpt.', - ); - } - printError( - '\n${indentation}For each block listed above, add ' - 'tag on the previous line, as explained at\n' - '$_instructionUrl', - ); - errorSummary ??= 'Missing code-excerpt management for code block'; - } - - return errorSummary; - } - - /// Validates that the plugin has a supported platforms table following the - /// expected format, returning an error string if any issues are found. - String? _validateSupportedPlatforms( - List readmeLines, - Pubspec pubspec, - ) { - // Example table following expected format: - // | | Android | iOS | Web | - // |----------------|---------|----------|------------------------| - // | **Support** | SDK 21+ | iOS 10+* | [See `camera_web `][1] | - final int detailsLineNumber = readmeLines.indexWhere( - (String line) => line.startsWith('| **Support**'), - ); - if (detailsLineNumber == -1) { - return 'No OS support table found'; - } - final int osLineNumber = detailsLineNumber - 2; - if (osLineNumber < 0 || !readmeLines[osLineNumber].startsWith('|')) { - return 'OS support table does not have the expected header format'; - } - - // Utility method to convert an iterable of strings to a case-insensitive - // sorted, comma-separated string of its elements. - String sortedListString(Iterable entries) { - final List entryList = entries.toList(); - entryList.sort( - (String a, String b) => a.toLowerCase().compareTo(b.toLowerCase()), - ); - return entryList.join(', '); - } - - // Validate that the supported OS lists match. - final pluginSection = pubspec.flutter!['plugin'] as YamlMap; - final dynamic platformsEntry = pluginSection['platforms']; - if (platformsEntry == null) { - logWarning('Plugin not support any platforms'); - return null; - } - final platformSupportMaps = platformsEntry as YamlMap; - final Set actuallySupportedPlatform = platformSupportMaps.keys - .toSet() - .cast(); - final Iterable documentedPlatforms = readmeLines[osLineNumber] - .split('|') - .map((String entry) => entry.trim()) - .where((String entry) => entry.isNotEmpty); - final Set documentedPlatformsLowercase = documentedPlatforms - .map((String entry) => entry.toLowerCase()) - .toSet(); - if (actuallySupportedPlatform.length != documentedPlatforms.length || - actuallySupportedPlatform - .intersection(documentedPlatformsLowercase) - .length != - actuallySupportedPlatform.length) { - printError(''' -${indentation}OS support table does not match supported platforms: -${indentation * 2}Actual: ${sortedListString(actuallySupportedPlatform)} -${indentation * 2}Documented: ${sortedListString(documentedPlatformsLowercase)} -'''); - return 'Incorrect OS support table'; - } - - // Enforce a standard set of capitalizations for the OS headings. - final Iterable incorrectCapitalizations = documentedPlatforms - .toSet() - .difference(_standardPlatformNames.values.toSet()); - if (incorrectCapitalizations.isNotEmpty) { - final Iterable expectedVersions = incorrectCapitalizations.map( - (String name) => _standardPlatformNames[name.toLowerCase()]!, - ); - printError(''' -${indentation}Incorrect OS capitalization: ${sortedListString(incorrectCapitalizations)} -${indentation * 2}Please use standard capitalizations: ${sortedListString(expectedVersions)} -'''); - return 'Incorrect OS support formatting'; - } - - // TODO(stuartmorgan): Add validation that the minimums in the table are - // consistent with what the current implementations require. See - // https://github.com/flutter/flutter/issues/84200 - return null; - } - - /// Validates [readmeLines], outputing error messages for any issue and - /// returning an array of error summaries (if any). - /// - /// Returns an empty array if validation passes. - List _validateBoilerplate( - List readmeLines, { - required RepositoryPackage mainPackage, - required bool isExample, - }) { - final errors = []; - - if (_containsTemplateFlutterBoilerplate(readmeLines)) { - printError( - '${indentation}The boilerplate section about getting started ' - 'with Flutter should not be left in.', - ); - errors.add('Contains template boilerplate'); - } - - // Enforce a repository-standard message in implementation plugin examples, - // since they aren't typical examples, which has been a source of - // confusion for plugin clients who find them. - if (isExample && mainPackage.isPlatformImplementation) { - if (_containsExampleBoilerplate(readmeLines)) { - printError( - '${indentation}The boilerplate should not be left in for a ' - "federated plugin implementation package's example.", - ); - errors.add('Contains template boilerplate'); - } - if (!_containsImplementationExampleExplanation(readmeLines)) { - printError( - '${indentation}The example README for a platform ' - 'implementation package should warn readers about its intended ' - 'use. Please copy the example README from another implementation ' - 'package in this repository.', - ); - errors.add('Missing implementation package example warning'); - } - } - - return errors; - } - - /// Returns true if the README still has unwanted parts of the boilerplate - /// from the `flutter create` templates. - bool _containsTemplateFlutterBoilerplate(List readmeLines) { - return readmeLines.any( - (String line) => line.contains('For help getting started with Flutter'), - ); - } - - /// Returns true if the README still has the generic description of an - /// example from the `flutter create` templates. - bool _containsExampleBoilerplate(List readmeLines) { - return readmeLines.any( - (String line) => line.contains('Demonstrates how to use the'), - ); - } - - /// Returns true if the README contains the repository-standard explanation of - /// the purpose of a federated plugin implementation's example. - bool _containsImplementationExampleExplanation(List readmeLines) { - return (readmeLines.contains('# Platform Implementation Test App') && - readmeLines.any( - (String line) => line.contains('This is a test app for'), - )) || - (readmeLines.contains('# Platform Implementation Test Apps') && - readmeLines.any( - (String line) => line.contains('These are test apps for'), - )); - } } diff --git a/script/tool/lib/src/validators/readme_validator.dart b/script/tool/lib/src/validators/readme_validator.dart new file mode 100644 index 000000000000..23f6ef82624b --- /dev/null +++ b/script/tool/lib/src/validators/readme_validator.dart @@ -0,0 +1,339 @@ +// Copyright 2013 The Flutter Authors +// 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 'package:path/path.dart' as path; +import 'package:yaml/yaml.dart'; + +import '../common/file_utils.dart'; +import '../common/output_utils.dart'; +import '../common/repository_package.dart'; + +// Standardized capitalizations for platforms that a plugin can support. +const Map _standardPlatformNames = { + 'android': 'Android', + 'ios': 'iOS', + 'linux': 'Linux', + 'macos': 'macOS', + 'web': 'Web', + 'windows': 'Windows', +}; + +const String _instructionUrl = + 'https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md'; + +/// Validates that READMEs follow repository conventions. +class ReadmeValidator { + /// Creates a new instance of the validator with the given command context. + ReadmeValidator({ + required path.Context path, + required String indentation, + required void Function(String) warningLogger, + required bool requireCodeExcerpts, + }) : _path = path, + _indentation = indentation, + _logWarning = warningLogger, + _requireCodeExcerpts = requireCodeExcerpts; + + final path.Context _path; + final String _indentation; + final void Function(String) _logWarning; + final bool _requireCodeExcerpts; + + /// Validates the given README file, returning a list of resulting error + /// strings. + /// + /// If no errors are found, an empty list is returned. + List validateReadme( + File readme, { + required RepositoryPackage mainPackage, + required bool isExample, + }) { + if (!readme.existsSync()) { + if (isExample) { + print( + '${_indentation}No README for ' + '${relativePosixPath(readme.parent, from: mainPackage.directory, platformContext: _path)}', + ); + return []; + } else { + printError( + '${_indentation}No README found at ' + '${relativePosixPath(readme, from: mainPackage.directory, platformContext: _path)}', + ); + return ['Missing README.md']; + } + } + + print( + '${_indentation}Checking ' + '${relativePosixPath(readme, from: mainPackage.directory, platformContext: _path)}...', + ); + + final List readmeLines = readme.readAsLinesSync(); + final errors = []; + + final String? blockValidationError = _validateCodeBlocks( + readmeLines, + mainPackage: mainPackage, + ); + if (blockValidationError != null) { + errors.add(blockValidationError); + } + + errors.addAll( + _validateBoilerplate( + readmeLines, + mainPackage: mainPackage, + isExample: isExample, + ), + ); + + // Check if this is the main readme for a plugin, and if so enforce extra + // checks. + if (!isExample) { + final Pubspec pubspec = mainPackage.parsePubspec(); + final isPlugin = pubspec.flutter?['plugin'] != null; + if (isPlugin && (!mainPackage.isFederated || mainPackage.isAppFacing)) { + final String? error = _validateSupportedPlatforms(readmeLines, pubspec); + if (error != null) { + errors.add(error); + } + } + } + + return errors; + } + + /// Validates that code blocks (``` ... ```) follow repository standards. + String? _validateCodeBlocks( + List readmeLines, { + required RepositoryPackage mainPackage, + }) { + final codeBlockDelimiterPattern = RegExp(r'^\s*```\s*([^ ]*)\s*'); + const excerptTagStart = '[]; + final missingExcerptLines = []; + var inBlock = false; + for (var i = 0; i < readmeLines.length; ++i) { + final RegExpMatch? match = codeBlockDelimiterPattern.firstMatch( + readmeLines[i], + ); + if (match == null) { + continue; + } + if (inBlock) { + inBlock = false; + continue; + } + inBlock = true; + + final int humanReadableLineNumber = i + 1; + + // Ensure that there's a language tag. + final String infoString = match[1] ?? ''; + if (infoString.isEmpty) { + missingLanguageLines.add(humanReadableLineNumber); + continue; + } + + // Check for code-excerpt usage if requested. + if (_requireCodeExcerpts && infoString == 'dart') { + if (i == 0 || !readmeLines[i - 1].trim().startsWith(excerptTagStart)) { + missingExcerptLines.add(humanReadableLineNumber); + } + } + } + + String? errorSummary; + + if (missingLanguageLines.isNotEmpty) { + for (final lineNumber in missingLanguageLines) { + printError( + '${_indentation}Code block at line $lineNumber is missing ' + 'a language identifier.', + ); + } + printError( + '\n${_indentation}For each block listed above, add a language tag to ' + 'the opening block. For instance, for Dart code, use:\n' + '${_indentation * 2}```dart\n', + ); + errorSummary = 'Missing language identifier for code block'; + } + + if (missingExcerptLines.isNotEmpty) { + for (final lineNumber in missingExcerptLines) { + printError( + '${_indentation}Dart code block at line $lineNumber is not ' + 'managed by code-excerpt.', + ); + } + printError( + '\n${_indentation}For each block listed above, add ' + 'tag on the previous line, as explained at\n' + '$_instructionUrl', + ); + errorSummary ??= 'Missing code-excerpt management for code block'; + } + + return errorSummary; + } + + /// Validates that the plugin has a supported platforms table following the + /// expected format, returning an error string if any issues are found. + String? _validateSupportedPlatforms( + List readmeLines, + Pubspec pubspec, + ) { + // Example table following expected format: + // | | Android | iOS | Web | + // |----------------|---------|----------|------------------------| + // | **Support** | SDK 21+ | iOS 10+* | [See `camera_web `][1] | + final int detailsLineNumber = readmeLines.indexWhere( + (String line) => line.startsWith('| **Support**'), + ); + if (detailsLineNumber == -1) { + return 'No OS support table found'; + } + final int osLineNumber = detailsLineNumber - 2; + if (osLineNumber < 0 || !readmeLines[osLineNumber].startsWith('|')) { + return 'OS support table does not have the expected header format'; + } + + // Utility method to convert an iterable of strings to a case-insensitive + // sorted, comma-separated string of its elements. + String sortedListString(Iterable entries) { + final List entryList = entries.toList(); + entryList.sort( + (String a, String b) => a.toLowerCase().compareTo(b.toLowerCase()), + ); + return entryList.join(', '); + } + + // Validate that the supported OS lists match. + final pluginSection = pubspec.flutter!['plugin'] as YamlMap; + final dynamic platformsEntry = pluginSection['platforms']; + if (platformsEntry == null) { + _logWarning('Plugin not support any platforms'); + return null; + } + final platformSupportMaps = platformsEntry as YamlMap; + final Set actuallySupportedPlatform = platformSupportMaps.keys + .toSet() + .cast(); + final Iterable documentedPlatforms = readmeLines[osLineNumber] + .split('|') + .map((String entry) => entry.trim()) + .where((String entry) => entry.isNotEmpty); + final Set documentedPlatformsLowercase = documentedPlatforms + .map((String entry) => entry.toLowerCase()) + .toSet(); + if (actuallySupportedPlatform.length != documentedPlatforms.length || + actuallySupportedPlatform + .intersection(documentedPlatformsLowercase) + .length != + actuallySupportedPlatform.length) { + printError(''' +${_indentation}OS support table does not match supported platforms: +${_indentation * 2}Actual: ${sortedListString(actuallySupportedPlatform)} +${_indentation * 2}Documented: ${sortedListString(documentedPlatformsLowercase)} +'''); + return 'Incorrect OS support table'; + } + + // Enforce a standard set of capitalizations for the OS headings. + final Iterable incorrectCapitalizations = documentedPlatforms + .toSet() + .difference(_standardPlatformNames.values.toSet()); + if (incorrectCapitalizations.isNotEmpty) { + final Iterable expectedVersions = incorrectCapitalizations.map( + (String name) => _standardPlatformNames[name.toLowerCase()]!, + ); + printError(''' +${_indentation}Incorrect OS capitalization: ${sortedListString(incorrectCapitalizations)} +${_indentation * 2}Please use standard capitalizations: ${sortedListString(expectedVersions)} +'''); + return 'Incorrect OS support formatting'; + } + + // TODO(stuartmorgan): Add validation that the minimums in the table are + // consistent with what the current implementations require. See + // https://github.com/flutter/flutter/issues/84200 + return null; + } + + /// Validates [readmeLines], outputing error messages for any issue and + /// returning an array of error summaries (if any). + /// + /// Returns an empty array if validation passes. + List _validateBoilerplate( + List readmeLines, { + required RepositoryPackage mainPackage, + required bool isExample, + }) { + final errors = []; + + if (_containsTemplateFlutterBoilerplate(readmeLines)) { + printError( + '${_indentation}The boilerplate section about getting started ' + 'with Flutter should not be left in.', + ); + errors.add('Contains template boilerplate'); + } + + // Enforce a repository-standard message in implementation plugin examples, + // since they aren't typical examples, which has been a source of + // confusion for plugin clients who find them. + if (isExample && mainPackage.isPlatformImplementation) { + if (_containsExampleBoilerplate(readmeLines)) { + printError( + '${_indentation}The boilerplate should not be left in for a ' + "federated plugin implementation package's example.", + ); + errors.add('Contains template boilerplate'); + } + if (!_containsImplementationExampleExplanation(readmeLines)) { + printError( + '${_indentation}The example README for a platform ' + 'implementation package should warn readers about its intended ' + 'use. Please copy the example README from another implementation ' + 'package in this repository.', + ); + errors.add('Missing implementation package example warning'); + } + } + + return errors; + } + + /// Returns true if the README still has unwanted parts of the boilerplate + /// from the `flutter create` templates. + bool _containsTemplateFlutterBoilerplate(List readmeLines) { + return readmeLines.any( + (String line) => line.contains('For help getting started with Flutter'), + ); + } + + /// Returns true if the README still has the generic description of an + /// example from the `flutter create` templates. + bool _containsExampleBoilerplate(List readmeLines) { + return readmeLines.any( + (String line) => line.contains('Demonstrates how to use the'), + ); + } + + /// Returns true if the README contains the repository-standard explanation of + /// the purpose of a federated plugin implementation's example. + bool _containsImplementationExampleExplanation(List readmeLines) { + return (readmeLines.contains('# Platform Implementation Test App') && + readmeLines.any( + (String line) => line.contains('This is a test app for'), + )) || + (readmeLines.contains('# Platform Implementation Test Apps') && + readmeLines.any( + (String line) => line.contains('These are test apps for'), + )); + } +} diff --git a/script/tool/test/license_check_command_test.dart b/script/tool/test/license_check_command_test.dart index 510eeda62227..2c4455fbacc5 100644 --- a/script/tool/test/license_check_command_test.dart +++ b/script/tool/test/license_check_command_test.dart @@ -5,6 +5,7 @@ import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:flutter_plugin_tools/src/common/core.dart'; +import 'package:flutter_plugin_tools/src/common/file_utils.dart'; import 'package:flutter_plugin_tools/src/license_check_command.dart'; import 'package:git/git.dart'; import 'package:path/path.dart' as p; @@ -74,9 +75,8 @@ void main() { .listSync(recursive: true, followLinks: false) .whereType() .map( - (File f) => p.posix.joinAll( - p.split(p.relative(f.absolute.path, from: root.path)), - ), + (File f) => + relativePosixPath(f, from: root, platformContext: p.posix), ) .join('\n'); From 50be44ec056aad7795adb9c8268009d02cbbd057 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 21 Apr 2026 15:10:48 -0400 Subject: [PATCH 2/6] Extract most of pubuspec check into a validator --- .../tool/lib/src/pubspec_check_command.dart | 574 +---------------- .../lib/src/validators/pubspec_validator.dart | 579 ++++++++++++++++++ 2 files changed, 611 insertions(+), 542 deletions(-) create mode 100644 script/tool/lib/src/validators/pubspec_validator.dart diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 77431c23094c..016e89b6954c 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -4,15 +4,11 @@ import 'package:file/file.dart'; import 'package:path/path.dart' as p; -import 'package:pub_semver/pub_semver.dart'; -import 'package:pubspec_parse/pubspec_parse.dart'; -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'; +import 'validators/pubspec_validator.dart'; /// A command to enforce pubspec conventions across the repository. /// @@ -57,38 +53,12 @@ class PubspecCheckCommand extends PackageLoopingCommand { static const String _allowPinnedDependenciesFlag = 'allow-pinned-dependencies'; - // Section order for plugins. Because the 'flutter' section is critical - // information for plugins, and usually small, it goes near the top unlike in - // a normal app or package. - static const List _majorPluginSections = [ - 'environment:', - 'flutter:', - 'dependencies:', - 'dev_dependencies:', - 'topics:', - 'screenshots:', - 'false_secrets:', - ]; - - static const List _majorPackageSections = [ - 'environment:', - 'dependencies:', - 'dev_dependencies:', - 'flutter:', - 'topics:', - 'screenshots:', - 'false_secrets:', - ]; - - static const String _expectedIssueLinkFormat = - 'https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A'; - // The names of all published packages in the repository. - late final Set _localPackages = {}; - - // Packages on the explicit allow list. - late final Set _allowedUnpinnedPackages = {}; - late final Set _allowedPinnedPackages = {}; + final AllowPackageLists _allowedPackages = ( + local: {}, + pinned: {}, + unpinned: {}, + ); @override final String name = 'pubspec-check'; @@ -110,6 +80,31 @@ class PubspecCheckCommand extends PackageLoopingCommand { @override Future initializeRun() async { // Find all local, published packages. + _allowedPackages.local.addAll(await _findAllPublishedPackages().toList()); + // Load explicitly allowed packages. + _allowedPackages.unpinned.addAll(getYamlListArg(_allowDependenciesFlag)); + _allowedPackages.pinned.addAll( + getYamlListArg(_allowPinnedDependenciesFlag), + ); + } + + @override + Future runForPackage(RepositoryPackage package) async { + final validator = PubspecValidator( + path: path, + indentation: indentation, + warningLogger: printWarning, + allowedPackages: _allowedPackages, + repoRoot: packagesDir.parent, + minMinFlutterVersion: getStringArg(_minMinFlutterVersionFlag), + ); + final List errors = await validator.validatePubspec(package); + return errors.isEmpty + ? PackageResult.success() + : PackageResult.fail(errors); + } + + Stream _findAllPublishedPackages() async* { for (final File pubspecFile in (await packagesDir.parent .list(recursive: true, followLinks: false) @@ -120,141 +115,9 @@ class PubspecCheckCommand extends PackageLoopingCommand { )) { final Pubspec? pubspec = _tryParsePubspec(pubspecFile.readAsStringSync()); if (pubspec != null && pubspec.publishTo != 'none') { - _localPackages.add(pubspec.name); + yield pubspec.name; } } - // Load explicitly allowed packages. - _allowedUnpinnedPackages.addAll(getYamlListArg(_allowDependenciesFlag)); - _allowedPinnedPackages.addAll(getYamlListArg(_allowPinnedDependenciesFlag)); - } - - @override - Future runForPackage(RepositoryPackage package) async { - final File pubspec = package.pubspecFile; - final bool passesCheck = - !pubspec.existsSync() || await _checkPubspec(pubspec, package: package); - if (!passesCheck) { - return PackageResult.fail(); - } - return PackageResult.success(); - } - - Future _checkPubspec( - File pubspecFile, { - required RepositoryPackage package, - }) async { - final String contents = pubspecFile.readAsStringSync(); - final Pubspec? pubspec = _tryParsePubspec(contents); - if (pubspec == null) { - return false; - } - - final List pubspecLines = contents.split('\n'); - final bool isPlugin = pubspec.flutter?.containsKey('plugin') ?? false; - final List sectionOrder = isPlugin - ? _majorPluginSections - : _majorPackageSections; - bool passing = _checkSectionOrder(pubspecLines, sectionOrder); - if (!passing) { - printError( - '${indentation}Major sections should follow standard ' - 'repository ordering:', - ); - final String listIndentation = indentation * 2; - printError('$listIndentation${sectionOrder.join('\n$listIndentation')}'); - } - - final String minMinFlutterVersionString = getStringArg( - _minMinFlutterVersionFlag, - ); - final String? minVersionError = _checkForMinimumVersionError( - pubspec, - package, - minMinFlutterVersion: minMinFlutterVersionString.isEmpty - ? null - : Version.parse(minMinFlutterVersionString), - ); - if (minVersionError != null) { - printError('$indentation$minVersionError'); - passing = false; - } - - if (isPlugin) { - final String? implementsError = _checkForImplementsError( - pubspec, - package: package, - ); - if (implementsError != null) { - printError('$indentation$implementsError'); - passing = false; - } - - final String? defaultPackageError = _checkForDefaultPackageError( - pubspec, - package: package, - ); - if (defaultPackageError != null) { - printError('$indentation$defaultPackageError'); - passing = false; - } - } - - final String? dependenciesError = _checkDependencies(pubspec); - if (dependenciesError != null) { - printError( - dependenciesError - .split('\n') - .map((String line) => '$indentation$line') - .join('\n'), - ); - passing = false; - } - - // Ignore metadata that's only relevant for published packages if the - // packages is not intended for publishing. - if (pubspec.publishTo != 'none') { - final List repositoryErrors = _checkForRepositoryLinkErrors( - pubspec, - package: package, - ); - if (repositoryErrors.isNotEmpty) { - for (final error in repositoryErrors) { - printError('$indentation$error'); - } - passing = false; - } - - if (!_checkIssueLink(pubspec)) { - 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', - ); - 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. - if (!package.isPlatformInterface && !package.isPlatformImplementation) { - final String? descriptionError = _checkDescription( - pubspec, - package: package, - ); - if (descriptionError != null) { - printError('$indentation$descriptionError'); - passing = false; - } - } - } - - return passing; } Pubspec? _tryParsePubspec(String pubspecContents) { @@ -265,377 +128,4 @@ class PubspecCheckCommand extends PackageLoopingCommand { } return null; } - - bool _checkSectionOrder( - List pubspecLines, - List sectionOrder, - ) { - var previousSectionIndex = 0; - for (final line in pubspecLines) { - final int index = sectionOrder.indexOf(line); - if (index == -1) { - continue; - } - if (index < previousSectionIndex) { - return false; - } - previousSectionIndex = index; - } - return true; - } - - List _checkForRepositoryLinkErrors( - Pubspec pubspec, { - required RepositoryPackage package, - }) { - final errorMessages = []; - if (pubspec.repository == null) { - errorMessages.add('Missing "repository"'); - } else { - final String relativePackagePath = getRelativePosixPath( - package.directory, - from: packagesDir.parent, - ); - if (!pubspec.repository!.path.endsWith(relativePackagePath)) { - errorMessages.add( - 'The "repository" link should end with the package path.', - ); - } - - if (!pubspec.repository!.toString().startsWith( - 'https://github.com/flutter/packages/tree/main', - )) { - errorMessages.add( - 'The "repository" link should start with the repository\'s ' - 'main tree: "https://github.com/flutter/packages/tree/main".', - ); - } - } - - if (pubspec.homepage != null) { - errorMessages.add( - 'Found a "homepage" entry; only "repository" should be used.', - ); - } - - return errorMessages; - } - - // Validates the "description" field for a package, returning an error - // string if there are any issues. - String? _checkDescription( - Pubspec pubspec, { - required RepositoryPackage package, - }) { - final String? description = pubspec.description; - if (description == null) { - return 'Missing "description"'; - } - - if (description.length < 60) { - return '"description" is too short. pub.dev recommends package ' - 'descriptions of 60-180 characters.'; - } - if (description.length > 180) { - return '"description" is too long. pub.dev recommends package ' - 'descriptions of 60-180 characters.'; - } - return null; - } - - bool _checkIssueLink(Pubspec pubspec) { - return pubspec.issueTracker?.toString().startsWith( - _expectedIssueLinkFormat, - ) ?? - false; - } - - // Validates the "topics" keyword for a plugin, returning an error - // string if there are any issues. - String? _checkTopics(Pubspec pubspec, {required RepositoryPackage package}) { - final List topics = pubspec.topics ?? []; - if (topics.isEmpty) { - return 'A published package should include "topics". ' - 'See https://dart.dev/tools/pub/pubspec#topics.'; - } - if (topics.length > 5) { - return 'A published package should have maximum 5 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.'; - } - } - - // Validates topic names according to https://dart.dev/tools/pub/pubspec#topics - final expectedTopicFormat = RegExp(r'^[a-z](?:-?[a-z0-9]+)*$'); - final Iterable invalidTopics = topics.where( - (String topic) => - !expectedTopicFormat.hasMatch(topic) || - topic.length < 2 || - topic.length > 32, - ); - if (invalidTopics.isNotEmpty) { - return 'Invalid topic(s): ${invalidTopics.join(', ')} in "topics" section. ' - 'Topics must consist of lowercase alphanumerical characters or dash (but no double dash), ' - 'start with a-z and ending with a-z or 0-9, have a minimum of 2 characters ' - 'and have a maximum of 32 characters.'; - } - return null; - } - - // 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 RepositoryPackage package, - }) { - if (_isImplementationPackage(package)) { - final pluginSection = pubspec.flutter!['plugin'] as YamlMap; - final implements = pluginSection['implements'] as String?; - final String expectedImplements = package.directory.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; - } - - // Validates any "default_package" entries a plugin, returning an error - // string if there are any issues. - // - // Should only be called on plugin packages. - String? _checkForDefaultPackageError( - Pubspec pubspec, { - required RepositoryPackage package, - }) { - final pluginSection = pubspec.flutter!['plugin'] as YamlMap; - final platforms = pluginSection['platforms'] as YamlMap?; - if (platforms == null) { - logWarning('Does not implement any platforms'); - return null; - } - final String packageName = package.directory.basename; - - // Validate that the default_package entries look correct (e.g., no typos). - final defaultPackages = {}; - for (final MapEntry platformEntry in platforms.entries) { - final platformDetails = platformEntry.value! as YamlMap; - final defaultPackage = platformDetails['default_package'] as String?; - if (defaultPackage != null) { - defaultPackages.add(defaultPackage); - if (!defaultPackage.startsWith('${packageName}_')) { - return '"$defaultPackage" is not an expected implementation name ' - 'for "$packageName"'; - } - } - } - - // Validate that all default_packages are also dependencies. - final Iterable dependencies = pubspec.dependencies.keys; - final Iterable missingPackages = defaultPackages.where( - (String package) => !dependencies.contains(package), - ); - if (missingPackages.isNotEmpty) { - return 'The following default_packages are missing ' - 'corresponding dependencies:\n' - ' ${missingPackages.join('\n ')}'; - } - - return null; - } - - // Returns true if [packageName] appears to be an implementation package - // according to repository conventions. - bool _isImplementationPackage(RepositoryPackage package) { - if (!package.isFederated) { - return false; - } - final String packageName = package.directory.basename; - final String parentName = package.directory.parent.basename; - // 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 nonImplementationSuffixes = { - '', // App-facing package. - '_platform_interface', // Platform interface package. - }; - final String suffix = packageName.substring(parentName.length); - return !nonImplementationSuffixes.contains(suffix); - } - - /// Validates that a Flutter package has a minimum SDK version constraint of - /// at least [minMinFlutterVersion] (if provided), or that a non-Flutter - /// package has a minimum SDK version constraint of [minMinDartVersion] - /// (if provided). - /// - /// Returns an error string if validation fails. - String? _checkForMinimumVersionError( - Pubspec pubspec, - RepositoryPackage package, { - Version? minMinFlutterVersion, - }) { - String unknownDartVersionError(Version flutterVersion) { - return 'Dart SDK version for Flutter SDK version ' - '$flutterVersion is unknown. ' - 'Please update the map for getDartSdkForFlutterSdk with the ' - 'corresponding Dart version.'; - } - - Version? minMinDartVersion; - if (minMinFlutterVersion != null) { - minMinDartVersion = getDartSdkForFlutterSdk(minMinFlutterVersion); - if (minMinDartVersion == null) { - return unknownDartVersionError(minMinFlutterVersion); - } - } - - final Version? dartConstraintMin = _minimumForConstraint( - pubspec.environment['sdk'], - ); - final Version? flutterConstraintMin = _minimumForConstraint( - pubspec.environment['flutter'], - ); - - // Validate the Flutter constraint, if any. - if (flutterConstraintMin != null && minMinFlutterVersion != null) { - if (flutterConstraintMin < minMinFlutterVersion) { - return 'Minimum allowed Flutter version $flutterConstraintMin is less ' - 'than $minMinFlutterVersion'; - } - } - - // Validate the Dart constraint, if any. - if (dartConstraintMin != null) { - // Ensure that it satisfies the minimum. - if (minMinDartVersion != null) { - if (dartConstraintMin < minMinDartVersion) { - return 'Minimum allowed Dart version $dartConstraintMin is less than $minMinDartVersion'; - } - } - - // Ensure that if there is also a Flutter constraint, they are consistent. - if (flutterConstraintMin != null) { - final Version? dartVersionForFlutterMinimum = getDartSdkForFlutterSdk( - flutterConstraintMin, - ); - if (dartVersionForFlutterMinimum == null) { - return unknownDartVersionError(flutterConstraintMin); - } - if (dartVersionForFlutterMinimum != dartConstraintMin) { - return 'The minimum Dart version is $dartConstraintMin, but the ' - 'minimum Flutter version of $flutterConstraintMin shipped with ' - 'Dart $dartVersionForFlutterMinimum. Please use consistent lower ' - 'SDK bounds'; - } - } - } - - return null; - } - - /// Returns the minumum version allowed by [constraint], or null if the - /// constraint is null. - Version? _minimumForConstraint(VersionConstraint? constraint) { - if (constraint == null) { - return null; - } - Version? result; - if (constraint is VersionRange) { - result = constraint.min; - } - return result ?? Version.none; - } - - // Validates the dependencies for a package, returning an error string if - // there are any that aren't allowed. - String? _checkDependencies(Pubspec pubspec) { - final badDependencies = {}; - final misplacedDevDependencies = {}; - // Shipped dependencies. - for (final dependencies in >[ - pubspec.dependencies, - pubspec.devDependencies, - ]) { - dependencies.forEach((String name, Dependency dependency) { - if (!_shouldAllowDependency(name, dependency)) { - badDependencies.add(name); - } - }); - } - - // Ensure that dev-only dependencies aren't in `dependencies`. - const devOnlyDependencies = { - 'build_runner', - 'integration_test', - 'flutter_test', - 'leak_tracker_flutter_testing', - 'mockito', - 'pigeon', - 'test', - }; - // Non-published packages like pigeon subpackages are allowed to violate - // the dev only dependencies rule, as are packages that end in `_test` (as - // they are assumed to be intended to be used as dev_dependencies by - // clients). - if (pubspec.publishTo != 'none' && !pubspec.name.endsWith('_test')) { - pubspec.dependencies.forEach((String name, Dependency dependency) { - if (devOnlyDependencies.contains(name)) { - misplacedDevDependencies.add(name); - } - }); - } - - final errors = [ - if (badDependencies.isNotEmpty) - ''' -The following unexpected non-local dependencies were found: -${badDependencies.map((String name) => ' $name').join('\n')} -Please see https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#Dependencies -for more information and next steps. -''', - if (misplacedDevDependencies.isNotEmpty) - ''' -The following dev dependencies were found in the dependencies section: -${misplacedDevDependencies.map((String name) => ' $name').join('\n')} -Please move them to dev_dependencies. -''', - ]; - return errors.isEmpty ? null : errors.join('\n\n'); - } - - // Checks whether a given dependency is allowed. - // Defaults to false. - bool _shouldAllowDependency(String name, Dependency dependency) { - if (dependency is PathDependency || dependency is SdkDependency) { - return true; - } - if (_localPackages.contains(name) || - _allowedUnpinnedPackages.contains(name)) { - return true; - } - if (dependency is HostedDependency && - _allowedPinnedPackages.contains(name)) { - final VersionConstraint constraint = dependency.version; - if (constraint is VersionRange && - constraint.min != null && - constraint.max != null && - constraint.includeMin && - constraint.includeMax) { - return true; - } - } - return false; - } } diff --git a/script/tool/lib/src/validators/pubspec_validator.dart b/script/tool/lib/src/validators/pubspec_validator.dart new file mode 100644 index 000000000000..783bb3461a82 --- /dev/null +++ b/script/tool/lib/src/validators/pubspec_validator.dart @@ -0,0 +1,579 @@ +// Copyright 2013 The Flutter Authors +// 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 'package:path/path.dart' as path; +import 'package:pub_semver/pub_semver.dart'; +import 'package:pubspec_parse/pubspec_parse.dart'; +import 'package:yaml/yaml.dart'; + +import '../common/core.dart'; +import '../common/file_utils.dart'; +import '../common/output_utils.dart'; +import '../common/plugin_utils.dart'; +import '../common/repository_package.dart'; + +// Section order for plugins. Because the 'flutter' section is critical +// information for plugins, and usually small, it goes near the top unlike in +// a normal app or package. +const List _majorPluginSections = [ + 'environment:', + 'flutter:', + 'dependencies:', + 'dev_dependencies:', + 'topics:', + 'screenshots:', + 'false_secrets:', +]; + +const List _majorPackageSections = [ + 'environment:', + 'dependencies:', + 'dev_dependencies:', + 'flutter:', + 'topics:', + 'screenshots:', + 'false_secrets:', +]; + +const String _expectedIssueLinkFormat = + 'https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A'; + +/// A set of packages that are allowed as dependencies. +typedef AllowPackageLists = ({ + /// Packages local to the repository. + Set local, + + /// Packages that are allowed only as pinned dependencies. + Set pinned, + + /// Packages that are allowed as unpinned dependencies. + Set unpinned, +}); + +/// Validates that pubspecs follow repository conventions. +class PubspecValidator { + /// Creates a new instance of [PubspecValidator] with the given configuration. + /// + /// [allowedPackages] must contain all of the packages that are allowed as + /// published dependencies. + PubspecValidator({ + required path.Context path, + required String indentation, + required AllowPackageLists allowedPackages, + required void Function(String) warningLogger, + required Directory repoRoot, + required String minMinFlutterVersion, + }) : _path = path, + _indentation = indentation, + _allowedPackages = allowedPackages, + _logWarning = warningLogger, + _repoRoot = repoRoot, + _minMinFlutterVersion = minMinFlutterVersion; + + final path.Context _path; + final String _indentation; + final AllowPackageLists _allowedPackages; + final void Function(String) _logWarning; + final Directory _repoRoot; + final String _minMinFlutterVersion; + + /// Validates that the pubspec of a package follows repository conventions, + /// returning a list of errors. + /// + /// If no errors are found, an empty list is returned. + Future> validatePubspec(RepositoryPackage package) async { + if (!package.pubspecFile.existsSync()) { + return []; + } + final String contents = package.pubspecFile.readAsStringSync(); + Pubspec pubspec; + try { + pubspec = Pubspec.parse(contents); + } on Exception { + printError('${_indentation}Unable to parse pubspec.yaml'); + return ['pubspec.yaml parse failure']; + } + + final List pubspecLines = contents.split('\n'); + final bool isPlugin = pubspec.flutter?.containsKey('plugin') ?? false; + final List sectionOrder = isPlugin + ? _majorPluginSections + : _majorPackageSections; + bool passing = _checkSectionOrder(pubspecLines, sectionOrder); + if (!passing) { + printError( + '${_indentation}Major sections should follow standard ' + 'repository ordering:', + ); + final String listIndentation = _indentation * 2; + printError('$listIndentation${sectionOrder.join('\n$listIndentation')}'); + } + + final String? minVersionError = _checkForMinimumVersionError( + pubspec, + package, + minMinFlutterVersion: _minMinFlutterVersion.isEmpty + ? null + : Version.parse(_minMinFlutterVersion), + ); + if (minVersionError != null) { + printError('$_indentation$minVersionError'); + passing = false; + } + + if (isPlugin) { + final String? implementsError = _checkForImplementsError( + pubspec, + package: package, + ); + if (implementsError != null) { + printError('$_indentation$implementsError'); + passing = false; + } + + final String? defaultPackageError = _checkForDefaultPackageError( + pubspec, + package: package, + ); + if (defaultPackageError != null) { + printError('$_indentation$defaultPackageError'); + passing = false; + } + } + + final String? dependenciesError = _checkDependencies(pubspec); + if (dependenciesError != null) { + printError( + dependenciesError + .split('\n') + .map((String line) => '$_indentation$line') + .join('\n'), + ); + passing = false; + } + + // Ignore metadata that's only relevant for published packages if the + // packages is not intended for publishing. + if (pubspec.publishTo != 'none') { + final List repositoryErrors = _checkForRepositoryLinkErrors( + pubspec, + package: package, + ); + if (repositoryErrors.isNotEmpty) { + for (final error in repositoryErrors) { + printError('$_indentation$error'); + } + passing = false; + } + + if (!_checkIssueLink(pubspec)) { + 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', + ); + 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. + if (!package.isPlatformInterface && !package.isPlatformImplementation) { + final String? descriptionError = _checkDescription( + pubspec, + package: package, + ); + if (descriptionError != null) { + printError('$_indentation$descriptionError'); + passing = false; + } + } + } + + return passing + ? [] + : ['pubspec.yaml failed validation, see above for details']; + } + + bool _checkSectionOrder( + List pubspecLines, + List sectionOrder, + ) { + var previousSectionIndex = 0; + for (final line in pubspecLines) { + final int index = sectionOrder.indexOf(line); + if (index == -1) { + continue; + } + if (index < previousSectionIndex) { + return false; + } + previousSectionIndex = index; + } + return true; + } + + List _checkForRepositoryLinkErrors( + Pubspec pubspec, { + required RepositoryPackage package, + }) { + final errorMessages = []; + if (pubspec.repository == null) { + errorMessages.add('Missing "repository"'); + } else { + final String relativePackagePath = relativePosixPath( + package.directory, + from: _repoRoot, + platformContext: _path, + ); + if (!pubspec.repository!.path.endsWith(relativePackagePath)) { + errorMessages.add( + 'The "repository" link should end with the package path.', + ); + } + + if (!pubspec.repository!.toString().startsWith( + 'https://github.com/flutter/packages/tree/main', + )) { + errorMessages.add( + 'The "repository" link should start with the repository\'s ' + 'main tree: "https://github.com/flutter/packages/tree/main".', + ); + } + } + + if (pubspec.homepage != null) { + errorMessages.add( + 'Found a "homepage" entry; only "repository" should be used.', + ); + } + + return errorMessages; + } + + // Validates the "description" field for a package, returning an error + // string if there are any issues. + String? _checkDescription( + Pubspec pubspec, { + required RepositoryPackage package, + }) { + final String? description = pubspec.description; + if (description == null) { + return 'Missing "description"'; + } + + if (description.length < 60) { + return '"description" is too short. pub.dev recommends package ' + 'descriptions of 60-180 characters.'; + } + if (description.length > 180) { + return '"description" is too long. pub.dev recommends package ' + 'descriptions of 60-180 characters.'; + } + return null; + } + + bool _checkIssueLink(Pubspec pubspec) { + return pubspec.issueTracker?.toString().startsWith( + _expectedIssueLinkFormat, + ) ?? + false; + } + + // Validates the "topics" keyword for a plugin, returning an error + // string if there are any issues. + String? _checkTopics(Pubspec pubspec, {required RepositoryPackage package}) { + final List topics = pubspec.topics ?? []; + if (topics.isEmpty) { + return 'A published package should include "topics". ' + 'See https://dart.dev/tools/pub/pubspec#topics.'; + } + if (topics.length > 5) { + return 'A published package should have maximum 5 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.'; + } + } + + // Validates topic names according to https://dart.dev/tools/pub/pubspec#topics + final expectedTopicFormat = RegExp(r'^[a-z](?:-?[a-z0-9]+)*$'); + final Iterable invalidTopics = topics.where( + (String topic) => + !expectedTopicFormat.hasMatch(topic) || + topic.length < 2 || + topic.length > 32, + ); + if (invalidTopics.isNotEmpty) { + return 'Invalid topic(s): ${invalidTopics.join(', ')} in "topics" section. ' + 'Topics must consist of lowercase alphanumerical characters or dash (but no double dash), ' + 'start with a-z and ending with a-z or 0-9, have a minimum of 2 characters ' + 'and have a maximum of 32 characters.'; + } + return null; + } + + // 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 RepositoryPackage package, + }) { + if (_isImplementationPackage(package)) { + final pluginSection = pubspec.flutter!['plugin'] as YamlMap; + final implements = pluginSection['implements'] as String?; + final String expectedImplements = package.directory.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; + } + + // Validates any "default_package" entries a plugin, returning an error + // string if there are any issues. + // + // Should only be called on plugin packages. + String? _checkForDefaultPackageError( + Pubspec pubspec, { + required RepositoryPackage package, + }) { + final pluginSection = pubspec.flutter!['plugin'] as YamlMap; + final platforms = pluginSection['platforms'] as YamlMap?; + if (platforms == null) { + _logWarning('Does not implement any platforms'); + return null; + } + final String packageName = package.directory.basename; + + // Validate that the default_package entries look correct (e.g., no typos). + final defaultPackages = {}; + for (final MapEntry platformEntry in platforms.entries) { + final platformDetails = platformEntry.value! as YamlMap; + final defaultPackage = platformDetails['default_package'] as String?; + if (defaultPackage != null) { + defaultPackages.add(defaultPackage); + if (!defaultPackage.startsWith('${packageName}_')) { + return '"$defaultPackage" is not an expected implementation name ' + 'for "$packageName"'; + } + } + } + + // Validate that all default_packages are also dependencies. + final Iterable dependencies = pubspec.dependencies.keys; + final Iterable missingPackages = defaultPackages.where( + (String package) => !dependencies.contains(package), + ); + if (missingPackages.isNotEmpty) { + return 'The following default_packages are missing ' + 'corresponding dependencies:\n' + ' ${missingPackages.join('\n ')}'; + } + + return null; + } + + // Returns true if [packageName] appears to be an implementation package + // according to repository conventions. + bool _isImplementationPackage(RepositoryPackage package) { + if (!package.isFederated) { + return false; + } + final String packageName = package.directory.basename; + final String parentName = package.directory.parent.basename; + // 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 nonImplementationSuffixes = { + '', // App-facing package. + '_platform_interface', // Platform interface package. + }; + final String suffix = packageName.substring(parentName.length); + return !nonImplementationSuffixes.contains(suffix); + } + + /// Validates that a Flutter package has a minimum SDK version constraint of + /// at least [minMinFlutterVersion] (if provided), or that a non-Flutter + /// package has a minimum SDK version constraint of [minMinDartVersion] + /// (if provided). + /// + /// Returns an error string if validation fails. + String? _checkForMinimumVersionError( + Pubspec pubspec, + RepositoryPackage package, { + Version? minMinFlutterVersion, + }) { + String unknownDartVersionError(Version flutterVersion) { + return 'Dart SDK version for Flutter SDK version ' + '$flutterVersion is unknown. ' + 'Please update the map for getDartSdkForFlutterSdk with the ' + 'corresponding Dart version.'; + } + + Version? minMinDartVersion; + if (minMinFlutterVersion != null) { + minMinDartVersion = getDartSdkForFlutterSdk(minMinFlutterVersion); + if (minMinDartVersion == null) { + return unknownDartVersionError(minMinFlutterVersion); + } + } + + final Version? dartConstraintMin = _minimumForConstraint( + pubspec.environment['sdk'], + ); + final Version? flutterConstraintMin = _minimumForConstraint( + pubspec.environment['flutter'], + ); + + // Validate the Flutter constraint, if any. + if (flutterConstraintMin != null && minMinFlutterVersion != null) { + if (flutterConstraintMin < minMinFlutterVersion) { + return 'Minimum allowed Flutter version $flutterConstraintMin is less ' + 'than $minMinFlutterVersion'; + } + } + + // Validate the Dart constraint, if any. + if (dartConstraintMin != null) { + // Ensure that it satisfies the minimum. + if (minMinDartVersion != null) { + if (dartConstraintMin < minMinDartVersion) { + return 'Minimum allowed Dart version $dartConstraintMin is less than $minMinDartVersion'; + } + } + + // Ensure that if there is also a Flutter constraint, they are consistent. + if (flutterConstraintMin != null) { + final Version? dartVersionForFlutterMinimum = getDartSdkForFlutterSdk( + flutterConstraintMin, + ); + if (dartVersionForFlutterMinimum == null) { + return unknownDartVersionError(flutterConstraintMin); + } + if (dartVersionForFlutterMinimum != dartConstraintMin) { + return 'The minimum Dart version is $dartConstraintMin, but the ' + 'minimum Flutter version of $flutterConstraintMin shipped with ' + 'Dart $dartVersionForFlutterMinimum. Please use consistent lower ' + 'SDK bounds'; + } + } + } + + return null; + } + + /// Returns the minumum version allowed by [constraint], or null if the + /// constraint is null. + Version? _minimumForConstraint(VersionConstraint? constraint) { + if (constraint == null) { + return null; + } + Version? result; + if (constraint is VersionRange) { + result = constraint.min; + } + return result ?? Version.none; + } + + // Validates the dependencies for a package, returning an error string if + // there are any that aren't allowed. + String? _checkDependencies(Pubspec pubspec) { + final badDependencies = {}; + final misplacedDevDependencies = {}; + // Shipped dependencies. + for (final dependencies in >[ + pubspec.dependencies, + pubspec.devDependencies, + ]) { + dependencies.forEach((String name, Dependency dependency) { + if (!_shouldAllowDependency(name, dependency)) { + badDependencies.add(name); + } + }); + } + + // Ensure that dev-only dependencies aren't in `dependencies`. + const devOnlyDependencies = { + 'build_runner', + 'integration_test', + 'flutter_test', + 'leak_tracker_flutter_testing', + 'mockito', + 'pigeon', + 'test', + }; + // Non-published packages like pigeon subpackages are allowed to violate + // the dev only dependencies rule, as are packages that end in `_test` (as + // they are assumed to be intended to be used as dev_dependencies by + // clients). + if (pubspec.publishTo != 'none' && !pubspec.name.endsWith('_test')) { + pubspec.dependencies.forEach((String name, Dependency dependency) { + if (devOnlyDependencies.contains(name)) { + misplacedDevDependencies.add(name); + } + }); + } + + final errors = [ + if (badDependencies.isNotEmpty) + ''' +The following unexpected non-local dependencies were found: +${badDependencies.map((String name) => ' $name').join('\n')} +Please see https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#Dependencies +for more information and next steps. +''', + if (misplacedDevDependencies.isNotEmpty) + ''' +The following dev dependencies were found in the dependencies section: +${misplacedDevDependencies.map((String name) => ' $name').join('\n')} +Please move them to dev_dependencies. +''', + ]; + return errors.isEmpty ? null : errors.join('\n\n'); + } + + // Checks whether a given dependency is allowed. + // Defaults to false. + bool _shouldAllowDependency(String name, Dependency dependency) { + if (dependency is PathDependency || dependency is SdkDependency) { + return true; + } + if (_allowedPackages.local.contains(name) || + _allowedPackages.unpinned.contains(name)) { + return true; + } + if (dependency is HostedDependency && + _allowedPackages.pinned.contains(name)) { + final VersionConstraint constraint = dependency.version; + if (constraint is VersionRange && + constraint.min != null && + constraint.max != null && + constraint.includeMin && + constraint.includeMax) { + return true; + } + } + return false; + } +} From d8b475e3d3a82a88a0e00ad4a2c948cf8add5818 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 21 Apr 2026 16:15:43 -0400 Subject: [PATCH 3/6] Extract most of the repo info check into a validator --- .../src/repo_package_info_check_command.dart | 375 +--------------- .../src/validators/repo_info_validator.dart | 410 ++++++++++++++++++ 2 files changed, 428 insertions(+), 357 deletions(-) create mode 100644 script/tool/lib/src/validators/repo_info_validator.dart diff --git a/script/tool/lib/src/repo_package_info_check_command.dart b/script/tool/lib/src/repo_package_info_check_command.dart index bd2ae9cfd721..21d6b02ad590 100644 --- a/script/tool/lib/src/repo_package_info_check_command.dart +++ b/script/tool/lib/src/repo_package_info_check_command.dart @@ -3,15 +3,10 @@ // found in the LICENSE file. import 'package:file/file.dart'; -import 'package:yaml/yaml.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; +import 'validators/repo_info_validator.dart'; /// A command to verify repository-level metadata about packages, such as /// repo README and auto-label entries. @@ -26,7 +21,7 @@ class RepoPackageInfoCheckCommand extends PackageLoopingCommand { >{}; /// Packages with entries in labeler.yml. - final List _autoLabeledPackages = []; + final Set _autoLabeledPackages = {}; @override final String name = 'repo-package-info-check'; @@ -46,365 +41,31 @@ class RepoPackageInfoCheckCommand extends PackageLoopingCommand { _repoRoot = packagesDir.fileSystem.directory((await gitDir).path); // Extract all of the README.md table entries. - final 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 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); - } - } - } - + _readmeTableEntries.addAll( + RepoInfoValidator.loadReadmeTableEntries( + repoRoot: _repoRoot, + packagesDir: packagesDir, + thirdPartyPackagesDir: thirdPartyPackagesDir, + ), + ); // Extract all of the lebeler.yml package entries. - // Validate the match rules rather than the label itself, as the labels - // don't always correspond 1:1 to packages and package names. - final packageGlobPattern = RegExp( - r'^\s*-\s*(?:third_party/)?packages/([^*]*)/', + _autoLabeledPackages.addAll( + RepoInfoValidator.loadAutoLabeledPackages(repoRoot: _repoRoot), ); - for (final String line - in _repoRoot - .childDirectory('.github') - .childFile('labeler.yml') - .readAsLinesSync()) { - final RegExpMatch? match = packageGlobPattern.firstMatch(line); - if (match == null) { - continue; - } - final String name = match.group(1)!; - _autoLabeledPackages.add(name); - } } @override Future runForPackage(RepositoryPackage package) async { - final String packageName = package.directory.basename; - final errors = []; - - // All packages should have an auto-applied label. For plugins, only the - // group needs a rule, so check the app-facing package. - if (!(package.isFederated && !package.isAppFacing) && - !_autoLabeledPackages.contains(packageName)) { - printError('${indentation}Missing a rule in .github/labeler.yml.'); - errors.add('Missing auto-labeler entry'); - } - - // The content of ci_config.yaml must be valid if there is one. - try { - package.parseCIConfig(); - } on FormatException catch (e) { - printError('$indentation${e.message}'); - errors.add(e.message); - } - - errors.addAll(await _validateFilesBasedOnReleaseStrategy(package)); - - // All published packages should have a README.md entry. - if (package.isPublishable()) { - errors.addAll(_validateRootReadme(package)); - } + final validator = RepoInfoValidator( + readmeTableEntries: _readmeTableEntries, + autoLabeledPackages: _autoLabeledPackages, + gitDir: await gitDir, + indentation: indentation, + ); + final List errors = await validator.validatePackage(package); return errors.isEmpty ? PackageResult.success() : PackageResult.fail(errors); } - - List _validateRootReadme(RepositoryPackage package) { - final errors = []; - - // For federated plugins, only the app-facing package is listed. - if (package.isFederated && !package.isAppFacing) { - return errors; - } - - final String packageName = package.directory.basename; - final List? 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 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 packageLink = RegExp( - r'^!\[.*\]\(https://img.shields.io/pub/.*/' - '$packageName' - r'(?:\.svg)?\)$', - ); - final issueTagLink = RegExp( - r'^!\[.*\]\(https://img.shields.io/github/issues/flutter/flutter/' - '$encodedIssueTag' - r'\?label=\)$', - ); - final 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 pubDevLink = RegExp( - '^https://pub.dev/packages/$packageName(?:/score)?\$', - ); - final gitHubIssueLink = RegExp( - '^https://github.com/flutter/flutter/labels/$encodedIssueTag\$', - ); - final 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; - } - - 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'; - } - } - - Future> _validateFilesBasedOnReleaseStrategy( - RepositoryPackage package, - ) async { - final errors = []; - final bool isBatchRelease = - package.parseCIConfig()?.isBatchRelease ?? false; - final String packageName = package.directory.basename; - final Directory workflowDir = _repoRoot - .childDirectory('.github') - .childDirectory('workflows'); - - errors.addAll( - _validateSpecificBatchWorkflow( - packageName, - workflowDir: workflowDir, - isBatchRelease: isBatchRelease, - ), - ); - - errors.addAll( - _validateGlobalWorkflowTrigger( - 'release_from_branches.yml', - workflowDir: workflowDir, - isBatchRelease: isBatchRelease, - packageName: packageName, - ), - ); - errors.addAll( - _validateGlobalWorkflowTrigger( - 'sync_release_pr.yml', - workflowDir: workflowDir, - isBatchRelease: isBatchRelease, - packageName: packageName, - ), - ); - - if (isBatchRelease && - (package.parsePubspec().version?.isPreRelease ?? false)) { - errors.add( - 'Batch release packages must not have a pre-release version.\n' - 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', - ); - } - - return errors; - } - - List _validateSpecificBatchWorkflow( - String packageName, { - required Directory workflowDir, - required bool isBatchRelease, - }) { - final errors = []; - final File batchWorkflowFile = workflowDir.childFile( - '${packageName}_batch.yml', - ); - if (isBatchRelease) { - if (!batchWorkflowFile.existsSync()) { - errors.add( - 'Missing batch workflow file: .github/workflows/${packageName}_batch.yml\n' - 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', - ); - } else { - // Validate content. - final String content = batchWorkflowFile.readAsStringSync(); - final YamlMap yaml; - try { - yaml = loadYaml(content) as YamlMap; - } catch (e) { - errors.add('Invalid YAML in ${packageName}_batch.yml: $e'); - return errors; - } - - var foundDispatch = false; - final jobs = yaml['jobs'] as YamlMap?; - if (jobs != null) { - for (final Object? job in jobs.values) { - if (job is YamlMap && job['steps'] is YamlList) { - final steps = job['steps'] as YamlList; - for (final Object? step in steps) { - if (step is YamlMap && - step['uses'] is String && - (step['uses'] as String).startsWith( - 'peter-evans/repository-dispatch', - )) { - final withArgs = step['with'] as YamlMap?; - if (withArgs != null && - withArgs['event-type'] == 'batch-release-pr' && - withArgs['client-payload'] == - '{"package": "$packageName"}') { - foundDispatch = true; - } - } - } - } - } - } - - if (!foundDispatch) { - errors.add( - 'Invalid batch workflow content in ${packageName}_batch.yml. ' - 'Must contain a step using peter-evans/repository-dispatch with:\n' - ' event-type: batch-release-pr\n' - ' client-payload: \'{"package": "$packageName"}\'\n' - 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', - ); - } - } - } else { - if (batchWorkflowFile.existsSync()) { - errors.add( - 'Unexpected batch workflow file: .github/workflows/${packageName}_batch.yml\n', - ); - } - } - return errors; - } - - List _validateGlobalWorkflowTrigger( - String workflowName, { - required Directory workflowDir, - required bool isBatchRelease, - required String packageName, - }) { - final errors = []; - final File workflowFile = workflowDir.childFile(workflowName); - if (!workflowFile.existsSync()) { - if (isBatchRelease) { - errors.add( - 'Missing global workflow file: .github/workflows/$workflowName\n' - 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', - ); - } - return errors; - } - - final String content = workflowFile.readAsStringSync(); - final YamlMap yaml; - try { - yaml = loadYaml(content) as YamlMap; - } catch (e) { - errors.add('Invalid YAML in $workflowName: $e'); - return errors; - } - - var hasTrigger = false; - final on = yaml['on'] as YamlMap?; - if (on is YamlMap) { - final push = on['push'] as YamlMap?; - if (push is YamlMap) { - final branches = push['branches'] as YamlList?; - if (branches is YamlList) { - if (branches.contains('release-$packageName-*')) { - hasTrigger = true; - } - } - } - } - - if (isBatchRelease && !hasTrigger) { - errors.add( - 'Missing trigger for release-$packageName-* in .github/workflows/$workflowName\n' - 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', - ); - } else if (!isBatchRelease && hasTrigger) { - errors.add( - 'Unexpected trigger for release-$packageName-* in .github/workflows/$workflowName\n', - ); - } - return errors; - } } diff --git a/script/tool/lib/src/validators/repo_info_validator.dart b/script/tool/lib/src/validators/repo_info_validator.dart new file mode 100644 index 000000000000..d7bbc9b57710 --- /dev/null +++ b/script/tool/lib/src/validators/repo_info_validator.dart @@ -0,0 +1,410 @@ +// Copyright 2013 The Flutter Authors +// 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 'package:git/git.dart'; +import 'package:yaml/yaml.dart'; + +import '../common/core.dart'; +import '../common/output_utils.dart'; +import '../common/repository_package.dart'; + +const int _exitBadTableEntry = 3; +const int _exitUnknownPackageEntry = 4; + +/// Validates that a package has all of the expected repository-level entries. +class RepoInfoValidator { + RepoInfoValidator({ + required Map> readmeTableEntries, + required Set autoLabeledPackages, + required GitDir gitDir, + required String indentation, + }) : _readmeTableEntries = readmeTableEntries, + _autoLabeledPackages = autoLabeledPackages, + _gitDir = gitDir, + _indentation = indentation; + + final Map> _readmeTableEntries; + final Set _autoLabeledPackages; + final GitDir _gitDir; + final String _indentation; + + /// Returns all the data from the package table in the root README.md file. + /// + /// Throws a [ToolExit] if the table is malformed or contains unknown packages. + static Map> loadReadmeTableEntries({ + required Directory repoRoot, + required Directory packagesDir, + required Directory thirdPartyPackagesDir, + }) { + final entries = >{}; + final 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 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); + } + entries[name] = cells; + + if (!(packagesDir.childDirectory(name).existsSync() || + thirdPartyPackagesDir.childDirectory(name).existsSync())) { + printError('Unknown package "$name" in root README.md table.'); + throw ToolExit(_exitUnknownPackageEntry); + } + } + } + return entries; + } + + /// Returns all of the package names from the labeler.yml file. + static Set loadAutoLabeledPackages({required Directory repoRoot}) { + final entries = {}; + // Validate the match rules rather than the label itself, as the labels + // don't always correspond 1:1 to packages and package names. + final packageGlobPattern = RegExp( + r'^\s*-\s*(?:third_party/)?packages/([^*]*)/', + ); + for (final String line + in repoRoot + .childDirectory('.github') + .childFile('labeler.yml') + .readAsLinesSync()) { + final RegExpMatch? match = packageGlobPattern.firstMatch(line); + if (match == null) { + continue; + } + final String name = match.group(1)!; + entries.add(name); + } + return entries; + } + + Future> validatePackage(RepositoryPackage package) async { + final String packageName = package.directory.basename; + final errors = []; + + // All packages should have an auto-applied label. For plugins, only the + // group needs a rule, so check the app-facing package. + if (!(package.isFederated && !package.isAppFacing) && + !_autoLabeledPackages.contains(packageName)) { + printError('${_indentation}Missing a rule in .github/labeler.yml.'); + errors.add('Missing auto-labeler entry'); + } + + // The content of ci_config.yaml must be valid if there is one. + try { + package.parseCIConfig(); + } on FormatException catch (e) { + printError('$_indentation${e.message}'); + errors.add(e.message); + } + + errors.addAll(await _validateFilesBasedOnReleaseStrategy(package)); + + // All published packages should have a README.md entry. + if (package.isPublishable()) { + errors.addAll(_validateRootReadme(package)); + } + + return errors; + } + + List _validateRootReadme(RepositoryPackage package) { + final errors = []; + + // For federated plugins, only the app-facing package is listed. + if (package.isFederated && !package.isAppFacing) { + return errors; + } + + final String packageName = package.directory.basename; + final List? 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 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 packageLink = RegExp( + r'^!\[.*\]\(https://img.shields.io/pub/.*/' + '$packageName' + r'(?:\.svg)?\)$', + ); + final issueTagLink = RegExp( + r'^!\[.*\]\(https://img.shields.io/github/issues/flutter/flutter/' + '$encodedIssueTag' + r'\?label=\)$', + ); + final 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 pubDevLink = RegExp( + '^https://pub.dev/packages/$packageName(?:/score)?\$', + ); + final gitHubIssueLink = RegExp( + '^https://github.com/flutter/flutter/labels/$encodedIssueTag\$', + ); + final 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; + } + + String _prTagForPackage(String packageName) => 'p: $packageName'; + + String _issueTagForPackage(String packageName) { + // TODO(stuartmorgan): Move this to a config file. See + // https://github.com/flutter/flutter/issues/185364 + switch (packageName) { + case 'google_maps_flutter': + return 'p: maps'; + case 'webview_flutter': + return 'p: webview'; + default: + return 'p: $packageName'; + } + } + + Future> _validateFilesBasedOnReleaseStrategy( + RepositoryPackage package, + ) async { + final errors = []; + final bool isBatchRelease = + package.parseCIConfig()?.isBatchRelease ?? false; + final String packageName = package.directory.basename; + final Directory repoRoot = package.directory.fileSystem.directory( + _gitDir.path, + ); + final Directory workflowDir = repoRoot + .childDirectory('.github') + .childDirectory('workflows'); + + errors.addAll( + _validateSpecificBatchWorkflow( + packageName, + workflowDir: workflowDir, + isBatchRelease: isBatchRelease, + ), + ); + + errors.addAll( + _validateGlobalWorkflowTrigger( + 'release_from_branches.yml', + workflowDir: workflowDir, + isBatchRelease: isBatchRelease, + packageName: packageName, + ), + ); + errors.addAll( + _validateGlobalWorkflowTrigger( + 'sync_release_pr.yml', + workflowDir: workflowDir, + isBatchRelease: isBatchRelease, + packageName: packageName, + ), + ); + + if (isBatchRelease && + (package.parsePubspec().version?.isPreRelease ?? false)) { + errors.add( + 'Batch release packages must not have a pre-release version.\n' + 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', + ); + } + + return errors; + } + + List _validateSpecificBatchWorkflow( + String packageName, { + required Directory workflowDir, + required bool isBatchRelease, + }) { + final errors = []; + final File batchWorkflowFile = workflowDir.childFile( + '${packageName}_batch.yml', + ); + if (isBatchRelease) { + if (!batchWorkflowFile.existsSync()) { + errors.add( + 'Missing batch workflow file: .github/workflows/${packageName}_batch.yml\n' + 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', + ); + } else { + // Validate content. + final String content = batchWorkflowFile.readAsStringSync(); + final YamlMap yaml; + try { + yaml = loadYaml(content) as YamlMap; + } catch (e) { + errors.add('Invalid YAML in ${packageName}_batch.yml: $e'); + return errors; + } + + var foundDispatch = false; + final jobs = yaml['jobs'] as YamlMap?; + if (jobs != null) { + for (final Object? job in jobs.values) { + if (job is YamlMap && job['steps'] is YamlList) { + final steps = job['steps'] as YamlList; + for (final Object? step in steps) { + if (step is YamlMap && + step['uses'] is String && + (step['uses'] as String).startsWith( + 'peter-evans/repository-dispatch', + )) { + final withArgs = step['with'] as YamlMap?; + if (withArgs != null && + withArgs['event-type'] == 'batch-release-pr' && + withArgs['client-payload'] == + '{"package": "$packageName"}') { + foundDispatch = true; + } + } + } + } + } + } + + if (!foundDispatch) { + errors.add( + 'Invalid batch workflow content in ${packageName}_batch.yml. ' + 'Must contain a step using peter-evans/repository-dispatch with:\n' + ' event-type: batch-release-pr\n' + ' client-payload: \'{"package": "$packageName"}\'\n' + 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', + ); + } + } + } else { + if (batchWorkflowFile.existsSync()) { + errors.add( + 'Unexpected batch workflow file: .github/workflows/${packageName}_batch.yml\n', + ); + } + } + return errors; + } + + List _validateGlobalWorkflowTrigger( + String workflowName, { + required Directory workflowDir, + required bool isBatchRelease, + required String packageName, + }) { + final errors = []; + final File workflowFile = workflowDir.childFile(workflowName); + if (!workflowFile.existsSync()) { + if (isBatchRelease) { + errors.add( + 'Missing global workflow file: .github/workflows/$workflowName\n' + 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', + ); + } + return errors; + } + + final String content = workflowFile.readAsStringSync(); + final YamlMap yaml; + try { + yaml = loadYaml(content) as YamlMap; + } catch (e) { + errors.add('Invalid YAML in $workflowName: $e'); + return errors; + } + + var hasTrigger = false; + final on = yaml['on'] as YamlMap?; + if (on is YamlMap) { + final push = on['push'] as YamlMap?; + if (push is YamlMap) { + final branches = push['branches'] as YamlList?; + if (branches is YamlList) { + if (branches.contains('release-$packageName-*')) { + hasTrigger = true; + } + } + } + } + + if (isBatchRelease && !hasTrigger) { + errors.add( + 'Missing trigger for release-$packageName-* in .github/workflows/$workflowName\n' + 'See https://github.com/flutter/flutter/blob/master/docs/ecosystem/release/README.md#batch-release', + ); + } else if (!isBatchRelease && hasTrigger) { + errors.add( + 'Unexpected trigger for release-$packageName-* in .github/workflows/$workflowName\n', + ); + } + return errors; + } +} From 1286e726ae93a5b6db23efd60c93f84daeb260ae Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 21 Apr 2026 16:40:30 -0400 Subject: [PATCH 4/6] Extract most of the version/changelog check into a validator --- .../src/validators/repo_info_validator.dart | 5 + .../version_and_changelog_validator.dart | 680 ++++++++++++++++++ .../tool/lib/src/version_check_command.dart | 633 +--------------- .../tool/test/version_check_command_test.dart | 5 +- 4 files changed, 704 insertions(+), 619 deletions(-) create mode 100644 script/tool/lib/src/validators/version_and_changelog_validator.dart diff --git a/script/tool/lib/src/validators/repo_info_validator.dart b/script/tool/lib/src/validators/repo_info_validator.dart index d7bbc9b57710..523c27417fa1 100644 --- a/script/tool/lib/src/validators/repo_info_validator.dart +++ b/script/tool/lib/src/validators/repo_info_validator.dart @@ -15,6 +15,7 @@ const int _exitUnknownPackageEntry = 4; /// Validates that a package has all of the expected repository-level entries. class RepoInfoValidator { + /// Creates a new instance of the validator with the given command context. RepoInfoValidator({ required Map> readmeTableEntries, required Set autoLabeledPackages, @@ -95,6 +96,10 @@ class RepoInfoValidator { return entries; } + /// Validates that the repository information for a package is correct, + /// returning a list of resulting error strings. + /// + /// If no errors are found, an empty list is returned. Future> validatePackage(RepositoryPackage package) async { final String packageName = package.directory.basename; final errors = []; diff --git a/script/tool/lib/src/validators/version_and_changelog_validator.dart b/script/tool/lib/src/validators/version_and_changelog_validator.dart new file mode 100644 index 000000000000..0368cc72348f --- /dev/null +++ b/script/tool/lib/src/validators/version_and_changelog_validator.dart @@ -0,0 +1,680 @@ +// Copyright 2013 The Flutter Authors +// 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 'package:meta/meta.dart'; +import 'package:path/path.dart' as path; +import 'package:pub_semver/pub_semver.dart'; + +import '../common/file_utils.dart'; +import '../common/git_version_finder.dart'; +import '../common/output_utils.dart'; +import '../common/package_state_utils.dart'; +import '../common/repository_package.dart'; + +/// Categories of version change types. +enum NextVersionType { + /// A breaking change. + BREAKING_MAJOR, + + /// A minor change (e.g., added feature). + MINOR, + + /// A bugfix change. + PATCH, + + /// The release of an existing pre-1.0 version. + V1_RELEASE, +} + +/// Returns the set of allowed next non-prerelease versions, with their change +/// type, for [version]. +/// +/// [newVersion] is used to check whether this is a pre-1.0 version bump, as +/// those have different semver rules. +@visibleForTesting +Map getAllowedNextVersions( + Version version, { + required Version newVersion, +}) { + final allowedNextVersions = { + version.nextMajor: NextVersionType.BREAKING_MAJOR, + version.nextMinor: NextVersionType.MINOR, + version.nextPatch: NextVersionType.PATCH, + }; + + if (version.major < 1 && newVersion.major < 1) { + var nextBuildNumber = -1; + if (version.build.isEmpty) { + nextBuildNumber = 1; + } else { + final currentBuildNumber = version.build.first as int; + nextBuildNumber = currentBuildNumber + 1; + } + final nextBuildVersion = Version( + version.major, + version.minor, + version.patch, + build: nextBuildNumber.toString(), + ); + allowedNextVersions.clear(); + allowedNextVersions[version.nextMajor] = NextVersionType.V1_RELEASE; + allowedNextVersions[version.nextMinor] = NextVersionType.BREAKING_MAJOR; + allowedNextVersions[version.nextPatch] = NextVersionType.MINOR; + allowedNextVersions[nextBuildVersion] = NextVersionType.PATCH; + } + return allowedNextVersions; +} + +/// A validator that checks that the version and changelog of a package are +/// consistent, and match the policy for the files changed. +class VersionAndChangelogValidator { + /// Creates a new instance of the validator with the given command context. + VersionAndChangelogValidator({ + required path.Context path, + required String indentation, + required Directory repoRoot, + required void Function(String) warningLogger, + required GitVersionFinder gitVersionFinder, + required List changedFiles, + required Set prLabels, + }) : _path = path, + _indentation = indentation, + _repoRoot = repoRoot, + _logWarning = warningLogger, + _gitVersionFinder = gitVersionFinder, + _changedFiles = changedFiles, + _prLabels = prLabels; + + final path.Context _path; + final String _indentation; + final Directory _repoRoot; + final void Function(String) _logWarning; + final GitVersionFinder _gitVersionFinder; + final List _changedFiles; + final Set _prLabels; + + /// The label that must be on a PR to allow a breaking + /// change to a platform interface. + static const String _breakingChangeOverrideLabel = + 'override: allow breaking change'; + + /// The label that must be on a PR to allow skipping a version change for a PR + /// that would normally require one. + static const String _missingVersionChangeOverrideLabel = + 'override: no versioning needed'; + + /// The label that must be on a PR to allow skipping a CHANGELOG change for a + /// PR that would normally require one. + static const String _missingChangelogChangeOverrideLabel = + 'override: no changelog needed'; + + /// Validates that the version and changelog of a package are consistent, + /// and match the policy for the files changed, returning a list of resulting + /// error strings. + /// + /// If no errors are found, an empty list is returned. + Future> validateChangelogAndVersion( + RepositoryPackage package, { + required bool checkForMissingChanges, + required bool ignorePlatformInterfaceBreaks, + }) async { + final Pubspec pubspec = package.parsePubspec(); + + final Version? currentPubspecVersion = pubspec.version; + if (currentPubspecVersion == null) { + printError( + '${_indentation}No version found in pubspec.yaml. A package ' + 'that intentionally has no version should be marked ' + '"publish_to: none".', + ); + // No remaining checks make sense, so fail immediately. + return ['No pubspec.yaml version.']; + } + + final errors = []; + + final CIConfig? ciConfig = package.parseCIConfig(); + final bool usesBatchRelease = ciConfig?.isBatchRelease ?? false; + + // All packages with batch release enabled should have valid pending changelogs. + if (usesBatchRelease) { + try { + package.getPendingChangelogs(); + } on FormatException catch (e) { + printError('$_indentation${e.message}'); + errors.add(e.message); + } + } else { + if (package.pendingChangelogsDirectory.existsSync()) { + printError( + '${_indentation}Package does not use batch release but has pending changelogs.', + ); + errors.add( + 'Package does not use batch release but has pending changelogs.', + ); + } + } + + final _CurrentVersionState versionState = await _getVersionState( + package, + pubspec: pubspec, + ignorePlatformInterfaceBreaks: ignorePlatformInterfaceBreaks, + ); + // PR with post release label is going to sync changelog.md and pubspec.yaml + // change back to main branch. Proceed with regular version check. + final bool hasPostReleaseLabel = _prLabels.contains( + 'override: post-release-${pubspec.name}', + ); + bool versionChanged; + + if (usesBatchRelease && !hasPostReleaseLabel) { + versionChanged = await _validatePendingChangeForBatchReleasePackage( + package: package, + changedFiles: _changedFiles, + errors: errors, + versionState: versionState, + ); + if (errors.isNotEmpty) { + return errors; + } + } else { + switch (versionState) { + case _CurrentVersionState.unchanged: + versionChanged = false; + case _CurrentVersionState.validIncrease: + case _CurrentVersionState.validRevert: + case _CurrentVersionState.newPackage: + versionChanged = true; + case _CurrentVersionState.invalidChange: + versionChanged = true; + errors.add('Disallowed version change.'); + case _CurrentVersionState.unknown: + versionChanged = false; + errors.add('Unable to determine previous version.'); + } + + if (!(await _validateChangelogVersion( + package, + pubspec: pubspec, + pubspecVersionState: versionState, + ))) { + errors.add('CHANGELOG.md failed validation.'); + } + } + + // If there are no other issues, make sure that there isn't a missing + // change to the version and/or CHANGELOG. + if (checkForMissingChanges && !versionChanged && errors.isEmpty) { + final String? error = await _checkForMissingChangeError(package); + if (error != null) { + errors.add(error); + } + } + + return errors; + } + + /// Returns the version of [package] from git at the base comparison hash. + Future _getPreviousVersionFromGit(RepositoryPackage package) async { + final File pubspecFile = package.pubspecFile; + // Use Posix-style paths for git. + final String gitPath = relativePosixPath( + pubspecFile, + from: _repoRoot, + platformContext: _path, + ); + return _gitVersionFinder.getPackageVersion(gitPath); + } + + /// Returns the state of the verison of [package] relative to the comparison + /// base (git or pub, depending on flags). + Future<_CurrentVersionState> _getVersionState( + RepositoryPackage package, { + required Pubspec pubspec, + required bool ignorePlatformInterfaceBreaks, + }) async { + // This method isn't called unless `version` is non-null. + final Version currentVersion = pubspec.version!; + final Version previousVersion = + await _getPreviousVersionFromGit(package) ?? Version.none; + if (previousVersion == Version.none) { + print( + '${_indentation}Unable to find previous version ' + 'at git base.', + ); + _logWarning( + '${_indentation}If this package is not new, something has gone wrong.', + ); + return _CurrentVersionState.newPackage; + } + + if (previousVersion == currentVersion) { + print('${_indentation}No version change.'); + return _CurrentVersionState.unchanged; + } + + // Check for reverts when doing local validation. + if (currentVersion < previousVersion) { + // Since this skips validation, try to ensure that it really is likely + // to be a revert rather than a typo by checking that the transition + // from the lower version to the new version would have been valid. + if (_shouldAllowVersionChange( + oldVersion: currentVersion, + newVersion: previousVersion, + )) { + _logWarning( + '${_indentation}New version is lower than previous version. ' + 'This is assumed to be a revert.', + ); + return _CurrentVersionState.validRevert; + } + } + + final Map allowedNextVersions = + getAllowedNextVersions(previousVersion, newVersion: currentVersion); + + if (_shouldAllowVersionChange( + oldVersion: previousVersion, + newVersion: currentVersion, + )) { + print('$_indentation$previousVersion -> $currentVersion'); + } else { + final String baseSha = await _gitVersionFinder.getBaseSha(); + printError( + '${_indentation}Incorrectly updated version.\n' + '${_indentation}HEAD: $currentVersion, $baseSha: $previousVersion.\n' + '${_indentation}Allowed versions: $allowedNextVersions', + ); + return _CurrentVersionState.invalidChange; + } + + // Check whether the version (or for a pre-release, the version that + // pre-release would eventually be released as) is a breaking change, and + // if so, validate it. + final Version targetReleaseVersion = currentVersion.isPreRelease + ? currentVersion.nextPatch + : currentVersion; + if (allowedNextVersions[targetReleaseVersion] == + NextVersionType.BREAKING_MAJOR && + !_validateBreakingChange( + package, + ignorePlatformInterfaceBreaks: ignorePlatformInterfaceBreaks, + )) { + printError( + '${_indentation}Breaking change detected.\n' + '${_indentation}Breaking changes to platform interfaces are not ' + 'allowed without explicit justification.\n' + '${_indentation}See ' + 'https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md ' + 'for more information.', + ); + return _CurrentVersionState.invalidChange; + } + + return _CurrentVersionState.validIncrease; + } + + /// Checks whether or not [package]'s CHANGELOG's versioning is correct, + /// both that it matches [pubspec] and that NEXT is used correctly, printing + /// the results of its checks. + /// + /// Returns false if the CHANGELOG fails validation. + Future _validateChangelogVersion( + RepositoryPackage package, { + required Pubspec pubspec, + required _CurrentVersionState pubspecVersionState, + }) async { + // This method isn't called unless `version` is non-null. + final Version fromPubspec = pubspec.version!; + + // get first version from CHANGELOG + final File changelog = package.changelogFile; + final List lines = changelog.readAsLinesSync(); + String? firstLineWithText; + final Iterator iterator = lines.iterator; + while (iterator.moveNext()) { + if (iterator.current.trim().isNotEmpty) { + firstLineWithText = iterator.current.trim(); + break; + } + } + // Remove all leading mark down syntax from the version line. + String? versionString = firstLineWithText?.split(' ').last; + String? leadingMarkdown = firstLineWithText?.split(' ').first; + + final badNextErrorMessage = + '${_indentation}When bumping the version ' + 'for release, the NEXT section should be incorporated into the new ' + "version's release notes."; + + // Skip validation for the special NEXT version that's used to accumulate + // changes that don't warrant publishing on their own. + final hasNextSection = versionString == 'NEXT'; + if (hasNextSection) { + // NEXT should not be present in a commit that increases the version. + if (pubspecVersionState == _CurrentVersionState.validIncrease || + pubspecVersionState == _CurrentVersionState.invalidChange) { + printError(badNextErrorMessage); + return false; + } + print( + '${_indentation}Found NEXT; validating next version in the CHANGELOG.', + ); + // Ensure that the version in pubspec hasn't changed without updating + // CHANGELOG. That means the next version entry in the CHANGELOG should + // pass the normal validation. + versionString = null; + leadingMarkdown = null; + while (iterator.moveNext()) { + if (iterator.current.trim().startsWith('## ')) { + versionString = iterator.current.trim().split(' ').last; + leadingMarkdown = iterator.current.trim().split(' ').first; + break; + } + } + } + + final validLeadingMarkdown = leadingMarkdown == '##'; + if (versionString == null || !validLeadingMarkdown) { + printError('${_indentation}Unable to find a version in CHANGELOG.md'); + print( + '${_indentation}The current version should be on a line starting ' + 'with "## ", either on the first non-empty line or after a "## NEXT" ' + 'section.', + ); + return false; + } + + final Version fromChangeLog; + try { + fromChangeLog = Version.parse(versionString); + } on FormatException { + printError('"$versionString" could not be parsed as a version.'); + return false; + } + + if (fromPubspec != fromChangeLog) { + printError(''' +${_indentation}Versions in CHANGELOG.md and pubspec.yaml do not match. +${_indentation}The version in pubspec.yaml is $fromPubspec. +${_indentation}The first version listed in CHANGELOG.md is $fromChangeLog. +'''); + return false; + } + + // If NEXT wasn't the first section, it should not exist at all. + if (!hasNextSection) { + final nextRegex = RegExp(r'^#+\s*NEXT\s*$'); + if (lines.any((String line) => nextRegex.hasMatch(line))) { + printError(badNextErrorMessage); + return false; + } + } + + // Check for blank lines between list items in the version section. + var inList = false; + var seenBlankLineInList = false; + final listItemRegex = RegExp(r'^\s*[*+-]\s'); + while (iterator.moveNext()) { + final String line = iterator.current; + final bool isListItem = listItemRegex.hasMatch(line); + final bool isBlank = line.trim().isEmpty; + + if (isListItem) { + if (seenBlankLineInList) { + printError( + '${_indentation}Blank lines found between list items in CHANGELOG.\n' + '${_indentation}This creates multiple separate lists on pub.dev.\n' + '${_indentation}Remove blank lines to keep all items in a single list.', + ); + return false; + } + inList = true; + } else if (isBlank) { + if (inList) { + seenBlankLineInList = true; + } + } else { + // Any other non-blank, non-list line resets the state (e.g. new headers, text). + inList = false; + seenBlankLineInList = false; + } + } + + return true; + } + + /// Checks whether the current breaking change to [package] should be allowed, + /// logging extra information for auditing when allowing unusual cases. + bool _validateBreakingChange( + RepositoryPackage package, { + required bool ignorePlatformInterfaceBreaks, + }) { + // Only platform interfaces have breaking change restrictions. + if (!package.isPlatformInterface) { + return true; + } + + if (ignorePlatformInterfaceBreaks) { + _logWarning( + '${_indentation}Ignoring breaking change to ${package.displayName} ' + 'due to command configuration.', + ); + return true; + } + + if (_prLabels.contains(_breakingChangeOverrideLabel)) { + _logWarning( + '${_indentation}Allowing breaking change to ${package.displayName} ' + 'due to the "$_breakingChangeOverrideLabel" label.', + ); + return true; + } + + return false; + } + + /// Returns true if the given version transition should be allowed. + bool _shouldAllowVersionChange({ + required Version oldVersion, + required Version newVersion, + }) { + // Get the non-pre-release next version mapping. + final Map allowedNextVersions = + getAllowedNextVersions(oldVersion, newVersion: newVersion); + + if (allowedNextVersions.containsKey(newVersion)) { + return true; + } + // Allow a pre-release version of a version that would be a valid + // transition. + if (newVersion.isPreRelease) { + final Version targetReleaseVersion = newVersion.nextPatch; + if (allowedNextVersions.containsKey(targetReleaseVersion)) { + return true; + } + } + return false; + } + + /// Returns an error string if the changes to this package should have + /// resulted in a version change, or shoud have resulted in a CHANGELOG change + /// but didn't. + /// + /// This should only be called if the version did not change. + Future _checkForMissingChangeError(RepositoryPackage package) async { + // Find the relative path to the current package, as it would appear at the + // beginning of a path reported by changedFiles (which always uses + // Posix paths). + final String relativePackagePath = await _getRelativePackagePath(package); + + final PackageChangeState state = await checkPackageChangeState( + package, + changedPaths: _changedFiles, + relativePackagePath: relativePackagePath, + git: _gitVersionFinder, + ); + + if (!state.hasChanges) { + return null; + } + + var missingVersionChange = false; + var missingChangelogChange = false; + if (state.needsVersionChange) { + if (_prLabels.contains(_missingVersionChangeOverrideLabel)) { + _logWarning( + 'Ignoring lack of version change due to the ' + '"$_missingVersionChangeOverrideLabel" label.', + ); + } else { + missingVersionChange = true; + printError( + 'No version change found, but the change to this package could ' + 'not be verified to be exempt\n' + 'from version changes according to repository policy.\n' + 'If this is a false positive, please comment in ' + 'the PR to explain why the PR\n' + 'is exempt, and add (or ask your reviewer to add) the ' + '"$_missingVersionChangeOverrideLabel" label.\n', + ); + } + } + + if (!state.hasChangelogChange && state.needsChangelogChange) { + if (_prLabels.contains(_missingChangelogChangeOverrideLabel)) { + _logWarning( + 'Ignoring lack of CHANGELOG update due to the ' + '"$_missingChangelogChangeOverrideLabel" label.', + ); + } else { + missingChangelogChange = true; + final CIConfig? config = package.parseCIConfig(); + const useOverrideChangelogLabel = + 'If this PR needs an exemption from the standard policy of listing ' + 'all changes in the CHANGELOG,\n' + 'comment in the PR to explain why the PR is exempt, and add (or ' + 'ask your reviewer to add) the\n' + '"$_missingChangelogChangeOverrideLabel" label.\n'; + if (config?.isBatchRelease ?? false) { + printError( + 'No new changelog files found in the pending_changelogs folder.\n' + '$useOverrideChangelogLabel' + 'Otherwise, please add a changelog entry with version:skip in the pending_changelogs folder as described in ' + 'the contributing guide.\n', + ); + } else { + printError( + 'No CHANGELOG change found.\n' + '$useOverrideChangelogLabel' + 'Otherwise, please add a NEXT entry in the CHANGELOG as described in ' + 'the contributing guide.\n', + ); + } + } + } + + if (missingVersionChange && missingChangelogChange) { + printError( + 'If this PR is not exempt, you can update version and ' + 'CHANGELOG with the "update-release-info" command.\\\n' + 'See here for an example: ' + 'https://github.com/flutter/packages/blob/main/script/tool/README.md#update-changelog-and-version\\\n' + 'For more details on versioning, check the contributing guide.', + ); + } + if (missingVersionChange) { + return 'Missing version change'; + } + if (missingChangelogChange) { + return 'Missing CHANGELOG change'; + } + + return null; + } + + /// Validates that the pending changelog files are correct for a batch release. + /// + /// This should only be called for package that uses batch release. + Future _validatePendingChangeForBatchReleasePackage({ + required RepositoryPackage package, + required List changedFiles, + required List errors, + required _CurrentVersionState versionState, + }) async { + final String relativePackagePath = await _getRelativePackagePath(package); + final List changedFilesInPackage = changedFiles + .where((String path) => path.startsWith(relativePackagePath)) + .toList(); + + // For batch release, only check pending changelog files. + final List allChangelogs; + try { + allChangelogs = package.getPendingChangelogs(); + } on FormatException catch (e) { + errors.add(e.message); + return false; + } + + final List newEntries = allChangelogs + .where( + (PendingChangelogEntry entry) => changedFilesInPackage.any( + (String path) => path.endsWith(entry.file.path.split('/').last), + ), + ) + .toList(); + final bool versionChanged = newEntries.any( + (PendingChangelogEntry entry) => entry.version != VersionChange.skip, + ); + + // The changelog.md and pubspec.yaml's version should not be updated directly. + if (changedFilesInPackage.contains('$relativePackagePath/CHANGELOG.md')) { + printError( + 'This package uses batch release, so CHANGELOG.md should not be changed directly.\n' + 'Instead, create a pending changelog file in pending_changelogs folder.', + ); + errors.add('CHANGELOG.md changed'); + } + if (changedFilesInPackage.contains('$relativePackagePath/pubspec.yaml')) { + if (versionState != _CurrentVersionState.unchanged) { + printError( + 'This package uses batch release, so the version in pubspec.yaml should not be changed directly.\n' + 'Instead, create a pending changelog file in pending_changelogs folder.', + ); + errors.add('pubspec.yaml version changed'); + } + } + return versionChanged; + } + + Future _getRelativePackagePath(RepositoryPackage package) async { + return relativePosixPath( + package.directory, + from: _repoRoot, + platformContext: _path, + ); + } +} + +/// The state of a package's version relative to the comparison base. +enum _CurrentVersionState { + /// The version is unchanged. + unchanged, + + /// The version has increased, and the transition is valid. + validIncrease, + + /// The version has decrease, and the transition is a valid revert. + validRevert, + + /// The version has changed, and the transition is invalid. + invalidChange, + + /// The package is new. + newPackage, + + /// There was an error determining the version state. + unknown, +} diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart index 55d3ba13a597..e359cb25f6d6 100644 --- a/script/tool/lib/src/version_check_command.dart +++ b/script/tool/lib/src/version_check_command.dart @@ -3,90 +3,12 @@ // found in the LICENSE file. import 'package:file/file.dart'; -import 'package:meta/meta.dart'; -import 'package:path/path.dart' as p; -import 'package:pub_semver/pub_semver.dart'; import 'common/git_version_finder.dart'; import 'common/output_utils.dart'; import 'common/package_looping_command.dart'; -import 'common/package_state_utils.dart'; import 'common/repository_package.dart'; - -/// Categories of version change types. -enum NextVersionType { - /// A breaking change. - BREAKING_MAJOR, - - /// A minor change (e.g., added feature). - MINOR, - - /// A bugfix change. - PATCH, - - /// The release of an existing pre-1.0 version. - V1_RELEASE, -} - -/// The state of a package's version relative to the comparison base. -enum _CurrentVersionState { - /// The version is unchanged. - unchanged, - - /// The version has increased, and the transition is valid. - validIncrease, - - /// The version has decrease, and the transition is a valid revert. - validRevert, - - /// The version has changed, and the transition is invalid. - invalidChange, - - /// The package is new. - newPackage, - - /// There was an error determining the version state. - unknown, -} - -/// Returns the set of allowed next non-prerelease versions, with their change -/// type, for [version]. -/// -/// [newVersion] is used to check whether this is a pre-1.0 version bump, as -/// those have different semver rules. -@visibleForTesting -Map getAllowedNextVersions( - Version version, { - required Version newVersion, -}) { - final allowedNextVersions = { - version.nextMajor: NextVersionType.BREAKING_MAJOR, - version.nextMinor: NextVersionType.MINOR, - version.nextPatch: NextVersionType.PATCH, - }; - - if (version.major < 1 && newVersion.major < 1) { - var nextBuildNumber = -1; - if (version.build.isEmpty) { - nextBuildNumber = 1; - } else { - final currentBuildNumber = version.build.first as int; - nextBuildNumber = currentBuildNumber + 1; - } - final nextBuildVersion = Version( - version.major, - version.minor, - version.patch, - build: nextBuildNumber.toString(), - ); - allowedNextVersions.clear(); - allowedNextVersions[version.nextMajor] = NextVersionType.V1_RELEASE; - allowedNextVersions[version.nextMinor] = NextVersionType.BREAKING_MAJOR; - allowedNextVersions[version.nextPatch] = NextVersionType.MINOR; - allowedNextVersions[nextBuildVersion] = NextVersionType.PATCH; - } - return allowedNextVersions; -} +import 'validators/version_and_changelog_validator.dart'; /// A command to validate version changes to packages. class VersionCheckCommand extends PackageLoopingCommand { @@ -133,32 +55,10 @@ class VersionCheckCommand extends PackageLoopingCommand { static const String _ignorePlatformInterfaceBreaks = 'ignore-platform-interface-breaks'; - /// The label that must be on a PR to allow a breaking - /// change to a platform interface. - static const String _breakingChangeOverrideLabel = - 'override: allow breaking change'; - - /// The label that must be on a PR to allow skipping a version change for a PR - /// that would normally require one. - static const String _missingVersionChangeOverrideLabel = - 'override: no versioning needed'; - - /// The label that must be on a PR to allow skipping a CHANGELOG change for a - /// PR that would normally require one. - static const String _missingChangelogChangeOverrideLabel = - 'override: no changelog needed'; - late final GitVersionFinder _gitVersionFinder; late final Set _prLabels = _getPRLabels(); - Future _getRelativePackagePath(RepositoryPackage package) async { - final Directory gitRoot = packagesDir.fileSystem.directory( - (await gitDir).path, - ); - return getRelativePosixPath(package.directory, from: gitRoot); - } - @override final String name = 'version-check'; @@ -190,331 +90,30 @@ class VersionCheckCommand extends PackageLoopingCommand { if (pubspec.publishTo == 'none') { return PackageResult.skip('Found "publish_to: none".'); } + final Directory repoRoot = packagesDir.fileSystem.directory( + (await gitDir).path, + ); - final Version? currentPubspecVersion = pubspec.version; - if (currentPubspecVersion == null) { - printError( - '${indentation}No version found in pubspec.yaml. A package ' - 'that intentionally has no version should be marked ' - '"publish_to: none".', - ); - // No remaining checks make sense, so fail immediately. - return PackageResult.fail(['No pubspec.yaml version.']); - } - - final errors = []; - - final CIConfig? ciConfig = package.parseCIConfig(); - final bool usesBatchRelease = ciConfig?.isBatchRelease ?? false; - - // All packages with batch release enabled should have valid pending changelogs. - if (usesBatchRelease) { - try { - package.getPendingChangelogs(); - } on FormatException catch (e) { - printError('$indentation${e.message}'); - errors.add(e.message); - } - } else { - if (package.pendingChangelogsDirectory.existsSync()) { - printError( - '${indentation}Package does not use batch release but has pending changelogs.', - ); - errors.add( - 'Package does not use batch release but has pending changelogs.', - ); - } - } + final validator = VersionAndChangelogValidator( + path: path, + indentation: indentation, + warningLogger: logWarning, + gitVersionFinder: _gitVersionFinder, + repoRoot: repoRoot, + changedFiles: changedFiles, + prLabels: _prLabels, + ); - final _CurrentVersionState versionState = await _getVersionState( + final List errors = await validator.validateChangelogAndVersion( package, - pubspec: pubspec, + checkForMissingChanges: getBoolArg(_checkForMissingChanges), + ignorePlatformInterfaceBreaks: getBoolArg(_ignorePlatformInterfaceBreaks), ); - // PR with post release label is going to sync changelog.md and pubspec.yaml - // change back to main branch. Proceed with regular version check. - final bool hasPostReleaseLabel = _prLabels.contains( - 'override: post-release-${pubspec.name}', - ); - bool versionChanged; - - if (usesBatchRelease && !hasPostReleaseLabel) { - versionChanged = await _validatePendingChangeForBatchReleasePackage( - package: package, - changedFiles: changedFiles, - errors: errors, - versionState: versionState, - ); - if (errors.isNotEmpty) { - return PackageResult.fail(errors); - } - } else { - switch (versionState) { - case _CurrentVersionState.unchanged: - versionChanged = false; - case _CurrentVersionState.validIncrease: - case _CurrentVersionState.validRevert: - case _CurrentVersionState.newPackage: - versionChanged = true; - case _CurrentVersionState.invalidChange: - versionChanged = true; - errors.add('Disallowed version change.'); - case _CurrentVersionState.unknown: - versionChanged = false; - errors.add('Unable to determine previous version.'); - } - - if (!(await _validateChangelogVersion( - package, - pubspec: pubspec, - pubspecVersionState: versionState, - ))) { - errors.add('CHANGELOG.md failed validation.'); - } - } - - // If there are no other issues, make sure that there isn't a missing - // change to the version and/or CHANGELOG. - if (getBoolArg(_checkForMissingChanges) && - !versionChanged && - errors.isEmpty) { - final String? error = await _checkForMissingChangeError(package); - if (error != null) { - errors.add(error); - } - } - return errors.isEmpty ? PackageResult.success() : PackageResult.fail(errors); } - /// Returns the version of [package] from git at the base comparison hash. - Future _getPreviousVersionFromGit(RepositoryPackage package) async { - final File pubspecFile = package.pubspecFile; - final String relativePath = path.relative( - pubspecFile.absolute.path, - from: (await gitDir).path, - ); - // Use Posix-style paths for git. - final String gitPath = path.style == p.Style.windows - ? p.posix.joinAll(path.split(relativePath)) - : relativePath; - return _gitVersionFinder.getPackageVersion(gitPath, gitRef: baseSha); - } - - /// Returns the state of the verison of [package] relative to the comparison - /// base (git or pub, depending on flags). - Future<_CurrentVersionState> _getVersionState( - RepositoryPackage package, { - required Pubspec pubspec, - }) async { - // This method isn't called unless `version` is non-null. - final Version currentVersion = pubspec.version!; - final Version previousVersion = - await _getPreviousVersionFromGit(package) ?? Version.none; - if (previousVersion == Version.none) { - print( - '${indentation}Unable to find previous version ' - 'at git base.', - ); - logWarning( - '${indentation}If this package is not new, something has gone wrong.', - ); - return _CurrentVersionState.newPackage; - } - - if (previousVersion == currentVersion) { - print('${indentation}No version change.'); - return _CurrentVersionState.unchanged; - } - - // Check for reverts when doing local validation. - if (currentVersion < previousVersion) { - // Since this skips validation, try to ensure that it really is likely - // to be a revert rather than a typo by checking that the transition - // from the lower version to the new version would have been valid. - if (_shouldAllowVersionChange( - oldVersion: currentVersion, - newVersion: previousVersion, - )) { - logWarning( - '${indentation}New version is lower than previous version. ' - 'This is assumed to be a revert.', - ); - return _CurrentVersionState.validRevert; - } - } - - final Map allowedNextVersions = - getAllowedNextVersions(previousVersion, newVersion: currentVersion); - - if (_shouldAllowVersionChange( - oldVersion: previousVersion, - newVersion: currentVersion, - )) { - print('$indentation$previousVersion -> $currentVersion'); - } else { - printError( - '${indentation}Incorrectly updated version.\n' - '${indentation}HEAD: $currentVersion, $baseSha: $previousVersion.\n' - '${indentation}Allowed versions: $allowedNextVersions', - ); - return _CurrentVersionState.invalidChange; - } - - // Check whether the version (or for a pre-release, the version that - // pre-release would eventually be released as) is a breaking change, and - // if so, validate it. - final Version targetReleaseVersion = currentVersion.isPreRelease - ? currentVersion.nextPatch - : currentVersion; - if (allowedNextVersions[targetReleaseVersion] == - NextVersionType.BREAKING_MAJOR && - !_validateBreakingChange(package)) { - printError( - '${indentation}Breaking change detected.\n' - '${indentation}Breaking changes to platform interfaces are not ' - 'allowed without explicit justification.\n' - '${indentation}See ' - 'https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md ' - 'for more information.', - ); - return _CurrentVersionState.invalidChange; - } - - return _CurrentVersionState.validIncrease; - } - - /// Checks whether or not [package]'s CHANGELOG's versioning is correct, - /// both that it matches [pubspec] and that NEXT is used correctly, printing - /// the results of its checks. - /// - /// Returns false if the CHANGELOG fails validation. - Future _validateChangelogVersion( - RepositoryPackage package, { - required Pubspec pubspec, - required _CurrentVersionState pubspecVersionState, - }) async { - // This method isn't called unless `version` is non-null. - final Version fromPubspec = pubspec.version!; - - // get first version from CHANGELOG - final File changelog = package.changelogFile; - final List lines = changelog.readAsLinesSync(); - String? firstLineWithText; - final Iterator iterator = lines.iterator; - while (iterator.moveNext()) { - if (iterator.current.trim().isNotEmpty) { - firstLineWithText = iterator.current.trim(); - break; - } - } - // Remove all leading mark down syntax from the version line. - String? versionString = firstLineWithText?.split(' ').last; - String? leadingMarkdown = firstLineWithText?.split(' ').first; - - final badNextErrorMessage = - '${indentation}When bumping the version ' - 'for release, the NEXT section should be incorporated into the new ' - "version's release notes."; - - // Skip validation for the special NEXT version that's used to accumulate - // changes that don't warrant publishing on their own. - final hasNextSection = versionString == 'NEXT'; - if (hasNextSection) { - // NEXT should not be present in a commit that increases the version. - if (pubspecVersionState == _CurrentVersionState.validIncrease || - pubspecVersionState == _CurrentVersionState.invalidChange) { - printError(badNextErrorMessage); - return false; - } - print( - '${indentation}Found NEXT; validating next version in the CHANGELOG.', - ); - // Ensure that the version in pubspec hasn't changed without updating - // CHANGELOG. That means the next version entry in the CHANGELOG should - // pass the normal validation. - versionString = null; - leadingMarkdown = null; - while (iterator.moveNext()) { - if (iterator.current.trim().startsWith('## ')) { - versionString = iterator.current.trim().split(' ').last; - leadingMarkdown = iterator.current.trim().split(' ').first; - break; - } - } - } - - final validLeadingMarkdown = leadingMarkdown == '##'; - if (versionString == null || !validLeadingMarkdown) { - printError('${indentation}Unable to find a version in CHANGELOG.md'); - print( - '${indentation}The current version should be on a line starting ' - 'with "## ", either on the first non-empty line or after a "## NEXT" ' - 'section.', - ); - return false; - } - - final Version fromChangeLog; - try { - fromChangeLog = Version.parse(versionString); - } on FormatException { - printError('"$versionString" could not be parsed as a version.'); - return false; - } - - if (fromPubspec != fromChangeLog) { - printError(''' -${indentation}Versions in CHANGELOG.md and pubspec.yaml do not match. -${indentation}The version in pubspec.yaml is $fromPubspec. -${indentation}The first version listed in CHANGELOG.md is $fromChangeLog. -'''); - return false; - } - - // If NEXT wasn't the first section, it should not exist at all. - if (!hasNextSection) { - final nextRegex = RegExp(r'^#+\s*NEXT\s*$'); - if (lines.any((String line) => nextRegex.hasMatch(line))) { - printError(badNextErrorMessage); - return false; - } - } - - // Check for blank lines between list items in the version section. - var inList = false; - var seenBlankLineInList = false; - final listItemRegex = RegExp(r'^\s*[*+-]\s'); - while (iterator.moveNext()) { - final String line = iterator.current; - final bool isListItem = listItemRegex.hasMatch(line); - final bool isBlank = line.trim().isEmpty; - - if (isListItem) { - if (seenBlankLineInList) { - printError( - '${indentation}Blank lines found between list items in CHANGELOG.\n' - '${indentation}This creates multiple separate lists on pub.dev.\n' - '${indentation}Remove blank lines to keep all items in a single list.', - ); - return false; - } - inList = true; - } else if (isBlank) { - if (inList) { - seenBlankLineInList = true; - } - } else { - // Any other non-blank, non-list line resets the state (e.g. new headers, text). - inList = false; - seenBlankLineInList = false; - } - } - - return true; - } - Pubspec? _tryParsePubspec(RepositoryPackage package) { try { final Pubspec pubspec = package.parsePubspec(); @@ -525,33 +124,6 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog. } } - /// Checks whether the current breaking change to [package] should be allowed, - /// logging extra information for auditing when allowing unusual cases. - bool _validateBreakingChange(RepositoryPackage package) { - // Only platform interfaces have breaking change restrictions. - if (!package.isPlatformInterface) { - return true; - } - - if (getBoolArg(_ignorePlatformInterfaceBreaks)) { - logWarning( - '${indentation}Allowing breaking change to ${package.displayName} ' - 'due to --$_ignorePlatformInterfaceBreaks', - ); - return true; - } - - if (_prLabels.contains(_breakingChangeOverrideLabel)) { - logWarning( - '${indentation}Allowing breaking change to ${package.displayName} ' - 'due to the "$_breakingChangeOverrideLabel" label.', - ); - return true; - } - - return false; - } - /// Returns the labels associated with this PR, if any, or an empty set /// if that flag is not provided. Set _getPRLabels() { @@ -561,177 +133,4 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog. } return labels.split(',').map((String label) => label.trim()).toSet(); } - - /// Returns true if the given version transition should be allowed. - bool _shouldAllowVersionChange({ - required Version oldVersion, - required Version newVersion, - }) { - // Get the non-pre-release next version mapping. - final Map allowedNextVersions = - getAllowedNextVersions(oldVersion, newVersion: newVersion); - - if (allowedNextVersions.containsKey(newVersion)) { - return true; - } - // Allow a pre-release version of a version that would be a valid - // transition. - if (newVersion.isPreRelease) { - final Version targetReleaseVersion = newVersion.nextPatch; - if (allowedNextVersions.containsKey(targetReleaseVersion)) { - return true; - } - } - return false; - } - - /// Returns an error string if the changes to this package should have - /// resulted in a version change, or shoud have resulted in a CHANGELOG change - /// but didn't. - /// - /// This should only be called if the version did not change. - Future _checkForMissingChangeError(RepositoryPackage package) async { - // Find the relative path to the current package, as it would appear at the - // beginning of a path reported by changedFiles (which always uses - // Posix paths). - final String relativePackagePath = await _getRelativePackagePath(package); - - final PackageChangeState state = await checkPackageChangeState( - package, - changedPaths: changedFiles, - relativePackagePath: relativePackagePath, - git: await retrieveVersionFinder(), - ); - - if (!state.hasChanges) { - return null; - } - - var missingVersionChange = false; - var missingChangelogChange = false; - if (state.needsVersionChange) { - if (_prLabels.contains(_missingVersionChangeOverrideLabel)) { - logWarning( - 'Ignoring lack of version change due to the ' - '"$_missingVersionChangeOverrideLabel" label.', - ); - } else { - missingVersionChange = true; - printError( - 'No version change found, but the change to this package could ' - 'not be verified to be exempt\n' - 'from version changes according to repository policy.\n' - 'If this is a false positive, please comment in ' - 'the PR to explain why the PR\n' - 'is exempt, and add (or ask your reviewer to add) the ' - '"$_missingVersionChangeOverrideLabel" label.\n', - ); - } - } - - if (!state.hasChangelogChange && state.needsChangelogChange) { - if (_prLabels.contains(_missingChangelogChangeOverrideLabel)) { - logWarning( - 'Ignoring lack of CHANGELOG update due to the ' - '"$_missingChangelogChangeOverrideLabel" label.', - ); - } else { - missingChangelogChange = true; - final CIConfig? config = package.parseCIConfig(); - const useOverrideChangelogLabel = - 'If this PR needs an exemption from the standard policy of listing ' - 'all changes in the CHANGELOG,\n' - 'comment in the PR to explain why the PR is exempt, and add (or ' - 'ask your reviewer to add) the\n' - '"$_missingChangelogChangeOverrideLabel" label.\n'; - if (config?.isBatchRelease ?? false) { - printError( - 'No new changelog files found in the pending_changelogs folder.\n' - '$useOverrideChangelogLabel' - 'Otherwise, please add a changelog entry with version:skip in the pending_changelogs folder as described in ' - 'the contributing guide.\n', - ); - } else { - printError( - 'No CHANGELOG change found.\n' - '$useOverrideChangelogLabel' - 'Otherwise, please add a NEXT entry in the CHANGELOG as described in ' - 'the contributing guide.\n', - ); - } - } - } - - if (missingVersionChange && missingChangelogChange) { - printError( - 'If this PR is not exempt, you can update version and ' - 'CHANGELOG with the "update-release-info" command.\\\n' - 'See here for an example: ' - 'https://github.com/flutter/packages/blob/main/script/tool/README.md#update-changelog-and-version\\\n' - 'For more details on versioning, check the contributing guide.', - ); - } - if (missingVersionChange) { - return 'Missing version change'; - } - if (missingChangelogChange) { - return 'Missing CHANGELOG change'; - } - - return null; - } - - /// Validates that the pending changelog files are correct for a batch release. - /// - /// This should only be called for package that uses batch release. - Future _validatePendingChangeForBatchReleasePackage({ - required RepositoryPackage package, - required List changedFiles, - required List errors, - required _CurrentVersionState versionState, - }) async { - final String relativePackagePath = await _getRelativePackagePath(package); - final List changedFilesInPackage = changedFiles - .where((String path) => path.startsWith(relativePackagePath)) - .toList(); - - // For batch release, only check pending changelog files. - final List allChangelogs; - try { - allChangelogs = package.getPendingChangelogs(); - } on FormatException catch (e) { - errors.add(e.message); - return false; - } - - final List newEntries = allChangelogs - .where( - (PendingChangelogEntry entry) => changedFilesInPackage.any( - (String path) => path.endsWith(entry.file.path.split('/').last), - ), - ) - .toList(); - final bool versionChanged = newEntries.any( - (PendingChangelogEntry entry) => entry.version != VersionChange.skip, - ); - - // The changelog.md and pubspec.yaml's version should not be updated directly. - if (changedFilesInPackage.contains('$relativePackagePath/CHANGELOG.md')) { - printError( - 'This package uses batch release, so CHANGELOG.md should not be changed directly.\n' - 'Instead, create a pending changelog file in pending_changelogs folder.', - ); - errors.add('CHANGELOG.md changed'); - } - if (changedFilesInPackage.contains('$relativePackagePath/pubspec.yaml')) { - if (versionState != _CurrentVersionState.unchanged) { - printError( - 'This package uses batch release, so the version in pubspec.yaml should not be changed directly.\n' - 'Instead, create a pending changelog file in pending_changelogs folder.', - ); - errors.add('pubspec.yaml version changed'); - } - } - return versionChanged; - } } diff --git a/script/tool/test/version_check_command_test.dart b/script/tool/test/version_check_command_test.dart index 75bf007eaec1..a5f8230b1f1d 100644 --- a/script/tool/test/version_check_command_test.dart +++ b/script/tool/test/version_check_command_test.dart @@ -5,6 +5,7 @@ import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:flutter_plugin_tools/src/common/core.dart'; +import 'package:flutter_plugin_tools/src/validators/version_and_changelog_validator.dart'; import 'package:flutter_plugin_tools/src/version_check_command.dart'; import 'package:git/git.dart'; import 'package:pub_semver/pub_semver.dart'; @@ -371,8 +372,8 @@ void main() { output, containsAllInOrder([ contains( - 'Allowing breaking change to plugin_platform_interface due ' - 'to --ignore-platform-interface-breaks', + 'Ignoring breaking change to plugin_platform_interface due ' + 'to command configuration', ), contains('Ran for 1 package(s) (1 with warnings)'), ]), From b6ee45f639ca727f3f57ba7f056c66d4780db56d Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 22 Apr 2026 13:24:49 -0400 Subject: [PATCH 5/6] Extract most of dependabot check into a validator, removing the skip result --- .../lib/src/dependabot_check_command.dart | 108 ++-------------- .../src/validators/dependabot_validator.dart | 115 ++++++++++++++++++ .../test/dependabot_check_command_test.dart | 4 +- 3 files changed, 130 insertions(+), 97 deletions(-) create mode 100644 script/tool/lib/src/validators/dependabot_validator.dart diff --git a/script/tool/lib/src/dependabot_check_command.dart b/script/tool/lib/src/dependabot_check_command.dart index 6ac09e56f2da..e2b2bf5d8ec6 100644 --- a/script/tool/lib/src/dependabot_check_command.dart +++ b/script/tool/lib/src/dependabot_check_command.dart @@ -3,29 +3,20 @@ // found in the LICENSE file. import 'package:file/file.dart'; -import 'package:yaml/yaml.dart'; -import 'common/output_utils.dart'; import 'common/package_looping_command.dart'; import 'common/repository_package.dart'; +import 'validators/dependabot_validator.dart'; /// A command to verify Dependabot configuration coverage of packages. class DependabotCheckCommand extends PackageLoopingCommand { /// Creates Dependabot check command instance. - DependabotCheckCommand(super.packagesDir, {super.gitDir}) { - argParser.addOption( - _configPathFlag, - help: 'Path to the Dependabot configuration file', - defaultsTo: '.github/dependabot.yml', - ); - } - - static const String _configPathFlag = 'config'; + DependabotCheckCommand(super.packagesDir, {super.gitDir}); late Directory _repoRoot; - // The set of directories covered by "gradle" entries in the config. - Set _gradleDirs = const {}; + // The set of directories covered by the repo's Dependabot configuration. + late DependabotCoverage _coverage; @override final String name = 'dependabot-check'; @@ -48,99 +39,26 @@ class DependabotCheckCommand extends PackageLoopingCommand { Future initializeRun() async { _repoRoot = packagesDir.fileSystem.directory((await gitDir).path); - final config = - loadYaml( - _repoRoot - .childFile(getStringArg(_configPathFlag)) - .readAsStringSync(), - ) - as YamlMap; - final dynamic entries = config['updates']; - if (entries is! YamlList) { - return; - } - - const typeKey = 'package-ecosystem'; - const dirKey = 'directory'; - const dirsKey = 'directories'; - final Iterable gradleEntries = entries.cast().where( - (YamlMap entry) => entry[typeKey] == 'gradle', - ); - final Iterable directoryEntries = gradleEntries.map( - (YamlMap entry) => entry[dirKey] as String?, - ); - final Iterable directoriesEntries = gradleEntries - .map((YamlMap entry) => entry[dirsKey] as YamlList?) - .expand((YamlList? list) => list?.nodes ?? []) - .cast() - .map((YamlScalar entry) => entry.value as String); - _gradleDirs = directoryEntries - .followedBy(directoriesEntries) - .whereType() - .toSet(); + _coverage = DependabotValidator.loadConfig(repoRoot: _repoRoot); } @override Future runForPackage(RepositoryPackage package) async { - var skipped = true; + final validator = DependabotValidator( + coverage: _coverage, + path: path, + repoRoot: _repoRoot, + indentation: indentation, + ); + final errors = []; - final _GradleCoverageResult gradleResult = - _validateDependabotGradleCoverage(package); - skipped = skipped && gradleResult.runState == RunState.skipped; - if (gradleResult.runState == RunState.failed) { - printError('${indentation}Missing Gradle coverage.'); - print( - '${indentation}Add a "gradle" entry to ' - '${getStringArg(_configPathFlag)} for ${gradleResult.missingPath}', - ); - errors.add('Missing Gradle coverage'); - } + errors.addAll(validator.validateDependabotCoverage(package)); // TODO(stuartmorgan): Add other ecosystem checks here as more are enabled. - if (skipped) { - return PackageResult.skip('No supported package ecosystems'); - } return errors.isEmpty ? PackageResult.success() : PackageResult.fail(errors); } - - /// Returns the state for the Dependabot coverage of the Gradle ecosystem for - /// [package]: - /// - succeeded if it includes gradle and is covered. - /// - failed if it includes gradle and is not covered. - /// - skipped if it doesn't include gradle. - _GradleCoverageResult _validateDependabotGradleCoverage( - RepositoryPackage package, - ) { - final Directory androidDir = package.platformDirectory( - FlutterPlatform.android, - ); - final Directory appDir = androidDir.childDirectory('app'); - if (appDir.existsSync()) { - // It's an app, so only check for the app directory to be covered. - final dependabotPath = - '/${getRelativePosixPath(appDir, from: _repoRoot)}'; - return _gradleDirs.contains(dependabotPath) - ? _GradleCoverageResult(RunState.succeeded) - : _GradleCoverageResult(RunState.failed, missingPath: dependabotPath); - } else if (androidDir.existsSync()) { - // It's a library, so only check for the android directory to be covered. - final dependabotPath = - '/${getRelativePosixPath(androidDir, from: _repoRoot)}'; - return _gradleDirs.contains(dependabotPath) - ? _GradleCoverageResult(RunState.succeeded) - : _GradleCoverageResult(RunState.failed, missingPath: dependabotPath); - } - return _GradleCoverageResult(RunState.skipped); - } -} - -class _GradleCoverageResult { - _GradleCoverageResult(this.runState, {this.missingPath}); - - final RunState runState; - final String? missingPath; } diff --git a/script/tool/lib/src/validators/dependabot_validator.dart b/script/tool/lib/src/validators/dependabot_validator.dart new file mode 100644 index 000000000000..efa36d488bf2 --- /dev/null +++ b/script/tool/lib/src/validators/dependabot_validator.dart @@ -0,0 +1,115 @@ +// Copyright 2013 The Flutter Authors +// 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 'package:path/path.dart' as path; +import 'package:yaml/yaml.dart'; + +import '../common/file_utils.dart'; +import '../common/output_utils.dart'; +import '../common/repository_package.dart'; + +/// Directories covered by Dependabot, for various ecosystems. +// TODO(stuartmorgan): Add coverage for other ecosystems. +typedef DependabotCoverage = ({Set gradleDirs}); + +/// A validator that checks that the Dependabot configuration for a package is +/// correct. +class DependabotValidator { + /// Creates a new instance of the validator. + DependabotValidator({ + required DependabotCoverage coverage, + required path.Context path, + required Directory repoRoot, + required String indentation, + }) : _coverage = coverage, + _path = path, + _repoRoot = repoRoot, + _indentation = indentation; + + final DependabotCoverage _coverage; + final String _indentation; + final path.Context _path; + final Directory _repoRoot; + + /// The path to the Dependabot config file, relative to the repository root. + static const String _configFilePath = '.github/dependabot.yml'; + + /// Loads Dependabot coverage from the repository's configuration file. + static DependabotCoverage loadConfig({required Directory repoRoot}) { + final DependabotCoverage coverage = (gradleDirs: {}); + final config = + loadYaml(repoRoot.childFile(_configFilePath).readAsStringSync()) + as YamlMap; + final dynamic entries = config['updates']; + if (entries is YamlList) { + const typeKey = 'package-ecosystem'; + const dirKey = 'directory'; + const dirsKey = 'directories'; + final Iterable gradleEntries = entries.cast().where( + (YamlMap entry) => entry[typeKey] == 'gradle', + ); + final Iterable directoryEntries = gradleEntries.map( + (YamlMap entry) => entry[dirKey] as String?, + ); + final Iterable directoriesEntries = gradleEntries + .map((YamlMap entry) => entry[dirsKey] as YamlList?) + .expand((YamlList? list) => list?.nodes ?? []) + .cast() + .map((YamlScalar entry) => entry.value as String); + coverage.gradleDirs.addAll( + directoryEntries.followedBy(directoriesEntries).whereType(), + ); + } + return coverage; + } + + /// Validates that the Dependabot coverage for a package is correct, returning + /// a list of resulting error strings. + /// + /// If no errors are found, an empty list is returned. + List validateDependabotCoverage(RepositoryPackage package) { + final errors = []; + + final String? missingGradlePath = _validateDependabotGradleCoverage( + package, + ); + if (missingGradlePath != null) { + printError('${_indentation}Missing Gradle coverage.'); + print( + '${_indentation}Add a "gradle" entry to $_configFilePath for ' + '$missingGradlePath', + ); + errors.add('Missing Gradle coverage'); + } + + // TODO(stuartmorgan): Add other ecosystem checks here as more are enabled. + + return errors; + } + + /// Returns the path of a file that is missing dependabot coverage, if any. + String? _validateDependabotGradleCoverage(RepositoryPackage package) { + final Directory androidDir = package.platformDirectory( + FlutterPlatform.android, + ); + final Directory appDir = androidDir.childDirectory('app'); + if (appDir.existsSync()) { + // It's an app, so only check for the app directory to be covered. + final dependabotPath = + '/${relativePosixPath(appDir, from: _repoRoot, platformContext: _path)}'; + return _coverage.gradleDirs.contains(dependabotPath) + ? null + : dependabotPath; + } else if (androidDir.existsSync()) { + // It's a library, so only check for the android directory to be covered. + final dependabotPath = + '/${relativePosixPath(androidDir, from: _repoRoot, platformContext: _path)}'; + return _coverage.gradleDirs.contains(dependabotPath) + ? null + : dependabotPath; + } + return null; + } +} diff --git a/script/tool/test/dependabot_check_command_test.dart b/script/tool/test/dependabot_check_command_test.dart index 630c6f535805..09d95f932981 100644 --- a/script/tool/test/dependabot_check_command_test.dart +++ b/script/tool/test/dependabot_check_command_test.dart @@ -68,7 +68,7 @@ $gradleEntries '''); } - test('skips with no supported ecosystems', () async { + test('passes with no supported ecosystems', () async { setDependabotCoverage(); createFakePackage('a_package', packagesDir); @@ -79,7 +79,7 @@ $gradleEntries expect( output, containsAllInOrder([ - contains('SKIPPING: No supported package ecosystems'), + contains('No issues found!'), ]), ); }); From 99af76e2e23da45cecf7b17bc12b6f4df58dd7d1 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 22 Apr 2026 15:02:16 -0400 Subject: [PATCH 6/6] Extract most of the gradle check into a validator --- script/tool/lib/src/gradle_check_command.dart | 754 +---------------- .../lib/src/validators/gradle_validator.dart | 783 ++++++++++++++++++ .../tool/test/gradle_check_command_test.dart | 17 +- 3 files changed, 798 insertions(+), 756 deletions(-) create mode 100644 script/tool/lib/src/validators/gradle_validator.dart diff --git a/script/tool/lib/src/gradle_check_command.dart b/script/tool/lib/src/gradle_check_command.dart index 16fb59e4b106..395239a5002d 100644 --- a/script/tool/lib/src/gradle_check_command.dart +++ b/script/tool/lib/src/gradle_check_command.dart @@ -2,28 +2,15 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:math' as math; - -import 'package:collection/collection.dart'; -import 'package:file/file.dart'; -import 'package:meta/meta.dart'; -import 'package:pub_semver/pub_semver.dart'; - -import 'common/output_utils.dart'; import 'common/package_looping_command.dart'; import 'common/repository_package.dart'; - -/// The lowest `ext.kotlin_version` that example apps are allowed to use. -@visibleForTesting -final Version minKotlinVersion = Version(1, 7, 10); +import 'validators/gradle_validator.dart'; /// A command to enforce gradle file conventions and best practices. class GradleCheckCommand extends PackageLoopingCommand { /// Creates an instance of the gradle check command. GradleCheckCommand(super.packagesDir, {super.gitDir}); - static const int _minimumJavaVersion = 17; - @override final String name = 'gradle-check'; @@ -47,741 +34,12 @@ class GradleCheckCommand extends PackageLoopingCommand { return PackageResult.skip('No android/ directory.'); } - const exampleDirName = 'example'; - final bool isExample = - package.directory.basename == exampleDirName || - package.directory.parent.basename == exampleDirName; - if (!_validateBuildGradles(package, isExample: isExample)) { - return PackageResult.fail(); - } - return PackageResult.success(); - } - - bool _validateBuildGradles( - RepositoryPackage package, { - required bool isExample, - }) { - final Directory androidDir = package.platformDirectory( - FlutterPlatform.android, - ); - final File topLevelGradleFile = _getBuildGradleFile(androidDir); - - // This is tracked as a variable rather than a sequence of &&s so that all - // failures are reported at once, not just the first one. - var succeeded = true; - if (isExample) { - if (!_validateExampleTopLevelBuildGradle(package, topLevelGradleFile)) { - succeeded = false; - } - final File topLevelSettingsGradleFile = _getSettingsGradleFile( - androidDir, - ); - if (!_validateExampleTopLevelSettingsGradle( - package, - topLevelSettingsGradleFile, - )) { - succeeded = false; - } - - final File appGradleFile = _getBuildGradleFile( - androidDir.childDirectory('app'), - ); - if (!_validateExampleAppBuildGradle(package, appGradleFile)) { - succeeded = false; - } - } else { - succeeded = _validatePluginBuildGradle(package, topLevelGradleFile); - } - - return succeeded; - } - - // Returns the gradle file in the given directory. - File _getBuildGradleFile(Directory dir) { - const buildGradleBaseName = 'build.gradle'; - const buildGradleKtsBaseName = '$buildGradleBaseName.kts'; - if (dir.childFile(buildGradleKtsBaseName).existsSync()) { - return dir.childFile(buildGradleKtsBaseName); - } - return dir.childFile(buildGradleBaseName); - } - - // Returns the settings gradle file in the given directory. - File _getSettingsGradleFile(Directory dir) { - const settingsGradleBaseName = 'settings.gradle'; - const settingsGradleKtsBaseName = '$settingsGradleBaseName.kts'; - if (dir.childFile(settingsGradleKtsBaseName).existsSync()) { - return dir.childFile(settingsGradleKtsBaseName); - } - return dir.childFile(settingsGradleBaseName); - } - - // Returns the main/AndroidManifest.xml file for the given package. - File _getMainAndroidManifest( - RepositoryPackage package, { - required bool isExample, - }) { - final Directory androidDir = package.platformDirectory( - FlutterPlatform.android, + final validator = GradleValidator(path: path, indentation: indentation, ); - final Directory baseDir = isExample - ? androidDir.childDirectory('app') - : androidDir; - return baseDir - .childDirectory('src') - .childDirectory('main') - .childFile('AndroidManifest.xml'); - } - - bool _isCommented(String line) => line.trim().startsWith('//'); - - /// Validates the build.gradle file for a plugin - /// (some_plugin/android/build.gradle). - bool _validatePluginBuildGradle(RepositoryPackage package, File gradleFile) { - print( - '${indentation}Validating ' - '${getRelativePosixPath(gradleFile, from: package.directory)}.', - ); - final String contents = gradleFile.readAsStringSync(); - final List lines = contents.split('\n'); - - // This is tracked as a variable rather than a sequence of &&s so that all - // failures are reported at once, not just the first one. - var succeeded = true; - if (!_validateNamespace(package, contents, isExample: false)) { - succeeded = false; - } - if (!_validateCompatibilityVersions(lines)) { - succeeded = false; - } - if (!_validateKotlinJvmCompatibility(lines)) { - succeeded = false; - } - if (!_validateJavaKotlinCompileOptionsAlignment(lines)) { - succeeded = false; - } - if (!_validateGradleDrivenLintConfig(lines)) { - succeeded = false; - } - if (!_validateCompileSdkUsage(package, lines)) { - succeeded = false; - } - return succeeded; - } - - /// Documentation url for Artifact hub implementation in flutter repo's. - @visibleForTesting - static const String artifactHubDocumentationString = - r'https://github.com/flutter/flutter/blob/master/docs/ecosystem/Plugins-and-Packages-repository-structure.md#gradle-structure'; - - /// String printed as example of valid example root build.gradle.kts - /// repository configuration that enables artifact hub env variable. - @visibleForTesting - static const String exampleRootGradleArtifactHubString = - ''' - // See $artifactHubDocumentationString for more info. - val artifactRepoKey = "ARTIFACT_HUB_REPOSITORY" - val artifactRepoUrl = System.getenv(artifactRepoKey) - if (artifactRepoUrl != null) { - println("Using artifact hub") - maven { - url = uri(artifactRepoUrl) - } - } -'''; + final List errors = validator.validateGradle(package); - /// Validates that [gradleLines] reads and uses a artifiact hub repository - /// when ARTIFACT_HUB_REPOSITORY is set. - /// - /// Required in root gradle file. - bool _validateArtifactHubUsage( - RepositoryPackage example, - List gradleLines, - ) { - // Gradle variable name used to hold environment variable string. - const keyVariable = 'artifactRepoKey'; - const urlVariable = 'artifactRepoUrl'; - final keyPresentRegex = RegExp( - '$keyVariable' - r'''\s+=\s+["']ARTIFACT_HUB_REPOSITORY["']''', - ); - final documentationPresentRegex = RegExp( - r'github\.com.*flutter.*blob.*Plugins-and-Packages-repository-structure.*gradle-structure', - ); - final keyReadRegex = RegExp( - '$urlVariable' - r'\s*=\s*System\.getenv\(' - '$keyVariable' - r'\)', - ); - final keyUsedRegex = RegExp( - r'url = uri\(' - '$urlVariable' - r'\)', - ); - - final bool keyPresent = gradleLines.any( - (String line) => keyPresentRegex.hasMatch(line), - ); - final bool documentationPresent = gradleLines.any( - (String line) => documentationPresentRegex.hasMatch(line), - ); - final bool keyRead = gradleLines.any( - (String line) => keyReadRegex.hasMatch(line), - ); - final bool keyUsed = gradleLines.any( - (String line) => keyUsedRegex.hasMatch(line), - ); - - if (!(documentationPresent && keyPresent && keyRead && keyUsed)) { - printError( - 'Failed Artifact Hub validation. Include the following in ' - 'example root build.gradle:\n$exampleRootGradleArtifactHubString', - ); - } - - return keyPresent && documentationPresent && keyRead && keyUsed; - } - - /// Validates the top-level settings.gradle for an example app (e.g., - /// some_package/example/android/settings.gradle). - bool _validateExampleTopLevelSettingsGradle( - RepositoryPackage package, - File gradleSettingsFile, - ) { - print( - '${indentation}Validating ' - '${getRelativePosixPath(gradleSettingsFile, from: package.directory)}.', - ); - final String contents = gradleSettingsFile.readAsStringSync(); - final List lines = contents.split('\n'); - // This is tracked as a variable rather than a sequence of &&s so that all - // failures are reported at once, not just the first one. - var succeeded = true; - if (!_validateArtifactHubSettingsUsage(package, lines)) { - succeeded = false; - } - if (!_validateKotlinVersion(package, lines)) { - succeeded = false; - } - return succeeded; - } - - /// String printed as a valid example of settings.gradle.kts repository - /// configuration that enables artifact hub env variable. - @visibleForTesting - static const String exampleSettingsArtifactHubString = ''' -plugins { - id("dev.flutter.flutter-plugin-loader") version "1.0.0" - // ...other plugins - id("com.google.cloud.artifactregistry.gradle-plugin") version "2.2.1" -} - '''; - - /// Validates that [gradleLines] reads and uses a artifiact hub repository - /// when ARTIFACT_HUB_REPOSITORY is set. - /// - /// Required in root gradle file. - bool _validateArtifactHubSettingsUsage( - RepositoryPackage example, - List gradleLines, - ) { - final documentationPresentRegex = RegExp( - r'github\.com.*flutter.*blob.*Plugins-and-Packages-repository-structure.*gradle-structure', - ); - final artifactRegistryPluginApplyRegex = RegExp( - r'id.*com\.google\.cloud\.artifactregistry\.gradle-plugin.*version.*\b\d+\.\d+\.\d+\b', - ); - - final bool documentationPresent = gradleLines.any( - (String line) => documentationPresentRegex.hasMatch(line), - ); - final bool declarativeArtifactRegistryApplied = gradleLines.any( - (String line) => artifactRegistryPluginApplyRegex.hasMatch(line), - ); - final bool validArtifactConfiguration = - documentationPresent && declarativeArtifactRegistryApplied; - - if (!validArtifactConfiguration) { - printError('Failed Artifact Hub validation.'); - if (!documentationPresent) { - printError( - 'The link to the Artifact Hub documentation is missing. Include the following in ' - 'example root settings.gradle:\n// See $artifactHubDocumentationString for more info.', - ); - } - if (!declarativeArtifactRegistryApplied) { - printError( - 'Include the following in ' - 'example root settings.gradle:\n$exampleSettingsArtifactHubString', - ); - } - } - return validArtifactConfiguration; - } - - /// Validates the top-level build.gradle(.kts) for an example app (e.g., - /// some_package/example/android/build.gradle(.kts)). - bool _validateExampleTopLevelBuildGradle( - RepositoryPackage package, - File gradleFile, - ) { - print( - '${indentation}Validating ' - '${getRelativePosixPath(gradleFile, from: package.directory)}.', - ); - final String contents = gradleFile.readAsStringSync(); - final List lines = contents.split('\n'); - - // This is tracked as a variable rather than a sequence of &&s so that all - // failures are reported at once, not just the first one. - var succeeded = true; - if (!_validateJavacLintConfig(package, lines)) { - succeeded = false; - } - if (!_validateArtifactHubUsage(package, lines)) { - succeeded = false; - } - return succeeded; - } - - /// Validates the app-level build.gradle(.kts) for an example app (e.g., - /// some_package/example/android/app/build.gradle(.kts)). - bool _validateExampleAppBuildGradle( - RepositoryPackage package, - File gradleFile, - ) { - print( - '${indentation}Validating ' - '${getRelativePosixPath(gradleFile, from: package.directory)}.', - ); - final String contents = gradleFile.readAsStringSync(); - - // This is tracked as a variable rather than a sequence of &&s so that all - // failures are reported at once, not just the first one. - var succeeded = true; - if (!_validateNamespace(package, contents, isExample: true)) { - succeeded = false; - } - return succeeded; - } - - /// Validates that [gradleContents] sets a namespace, which is required for - /// compatibility with apps that use AGP 8+. - bool _validateNamespace( - RepositoryPackage package, - String gradleContents, { - required bool isExample, - }) { - // Regex to validate that the following namespace definition - // is found (where the single quotes can be single or double): - // - namespace = 'dev.flutter.foo' - final nameSpaceRegex = RegExp( - '^\\s*namespace\\s+=\\s*[\'"](.*?)[\'"]', - multiLine: true, - ); - final RegExpMatch? nameSpaceRegexMatch = nameSpaceRegex.firstMatch( - gradleContents, - ); - - if (nameSpaceRegexMatch == null) { - const errorMessage = ''' -build.gradle.kts must set a "namespace": - - android { - namespace = "dev.flutter.foo" - } - -The value must match the "package" attribute in AndroidManifest.xml, if one is -present. For more information, see: -https://developer.android.com/build/publish-library/prep-lib-release#choose-namespace -'''; - - printError( - '$indentation${errorMessage.split('\n').join('\n$indentation')}', - ); - return false; - } else { - return _validateNamespaceMatchesManifest( - package, - isExample: isExample, - namespace: nameSpaceRegexMatch.group(1)!, - ); - } - } - - /// Validates that the given namespace matches the manifest package of - /// [package] (if any; a package does not need to be in the manifest in cases - /// where compatibility with AGP <7 is no longer required). - /// - /// Prints an error and returns false if validation fails. - bool _validateNamespaceMatchesManifest( - RepositoryPackage package, { - required bool isExample, - required String namespace, - }) { - final manifestPackageRegex = RegExp(r'package\s*=\s*"(.*?)"'); - final String manifestContents = _getMainAndroidManifest( - package, - isExample: isExample, - ).readAsStringSync(); - final RegExpMatch? packageMatch = manifestPackageRegex.firstMatch( - manifestContents, - ); - if (packageMatch != null && namespace != packageMatch.group(1)) { - const buildGradleName = 'build.gradle.kts'; - final errorMessage = - ''' -$buildGradleName "namespace" must match the "package" attribute in AndroidManifest.xml, if one is present. - $buildGradleName namespace: "$namespace" - AndroidMastifest.xml package: "${packageMatch.group(1)}" -'''; - printError( - '$indentation${errorMessage.split('\n').join('\n$indentation')}', - ); - return false; - } - return true; - } - - /// Checks for a source compatibiltiy version, so that it's explicit rather - /// than using whatever the client's local toolchaing defaults to (which can - /// lead to compile errors that show up for clients, but not in CI). - bool _validateCompatibilityVersions(List gradleLines) { - final bool hasLanguageVersion = gradleLines.any( - (String line) => line.contains('languageVersion') && !_isCommented(line), - ); - final bool hasCompabilityVersions = - gradleLines.any( - (String line) => - line.contains('sourceCompatibility = JavaVersion.VERSION_') && - !_isCommented(line), - ) && - // Newer toolchains default targetCompatibility to the same value as - // sourceCompatibility, but older toolchains require it to be set - // explicitly. The exact version cutoff (and of which piece of the - // toolchain; likely AGP) is unknown; for context see - // https://github.com/flutter/flutter/issues/125482 - gradleLines.any( - (String line) => - line.contains('targetCompatibility = JavaVersion.VERSION_') && - !_isCommented(line), - ); - if (!hasLanguageVersion && !hasCompabilityVersions) { - const javaErrorMessage = - ''' -build.gradle.kts must set an explicit Java compatibility version. - -This can be done either via "sourceCompatibility"/"targetCompatibility": - android { - compileOptions { - sourceCompatibility = JavaVersion.VERSION_$_minimumJavaVersion - targetCompatibility = JavaVersion.VERSION_$_minimumJavaVersion - } - } - -or "toolchain": - java { - toolchain { - languageVersion = JavaLanguageVersion.of($_minimumJavaVersion) - } - } - -See: -https://docs.gradle.org/current/userguide/java_plugin.html#toolchain_and_compatibility -for more details.'''; - - printError( - '$indentation${javaErrorMessage.split('\n').join('\n$indentation')}', - ); - return false; - } - - return true; - } - - bool _validateKotlinJvmCompatibility(List gradleLines) { - bool isKotlinOptions(String line) => - line.contains('kotlinOptions') && !_isCommented(line); - final bool hasKotlinOptions = gradleLines.any(isKotlinOptions); - final bool kotlinOptionsUsesJavaVersion = gradleLines.any( - (String line) => - line.contains('jvmTarget = JavaVersion.VERSION_') && - !_isCommented(line), - ); - // Either does not set kotlinOptions or does and uses non-string based syntax. - if (hasKotlinOptions && !kotlinOptionsUsesJavaVersion) { - // Bad lines contains the first 4 lines including the kotlinOptions section. - var badLines = ''; - final int startIndex = gradleLines.indexOf( - gradleLines.firstWhere(isKotlinOptions), - ); - for ( - var i = startIndex; - i < math.min(startIndex + 4, gradleLines.length); - i++ - ) { - badLines += '${gradleLines[i]}\n'; - } - final kotlinErrorMessage = - ''' -If build.gradle.kts sets jvmTarget then it must use JavaVersion syntax. - Good: - android { - kotlinOptions { - jvmTarget = JavaVersion.VERSION_$_minimumJavaVersion.toString() - } - } - BAD: - $badLines -'''; - printError( - '$indentation${kotlinErrorMessage.split('\n').join('\n$indentation')}', - ); - return false; - } - // No error condition. - return true; - } - - bool _validateJavaKotlinCompileOptionsAlignment(List gradleLines) { - final javaVersions = []; - // Some java versions have the format VERSION_1_8 but we dont need to handle those - // because they are below the minimum. - final javaVersionMatcher = RegExp( - r'JavaVersion.VERSION_(?\d+)', - ); - for (final line in gradleLines) { - final RegExpMatch? match = javaVersionMatcher.firstMatch(line); - if (!_isCommented(line) && match != null) { - final String? foundVersion = match.namedGroup('javaVersion'); - if (foundVersion != null) { - javaVersions.add(foundVersion); - } - } - } - if (javaVersions.isNotEmpty) { - final int version = int.parse(javaVersions.first); - if (!javaVersions.every((String element) => element == '$version')) { - const javaVersionAlignmentError = ''' -If build.gradle.kts uses JavaVersion.* versions must be the same. -'''; - printError( - '$indentation${javaVersionAlignmentError.split('\n').join('\n$indentation')}', - ); - return false; - } - - if (version < _minimumJavaVersion) { - final minimumJavaVersionError = - ''' -build.gradle.kts uses "JavaVersion.VERSION_$version". -Which is below the minimum required. Use at least "JavaVersion.VERSION_$_minimumJavaVersion". -'''; - printError( - '$indentation${minimumJavaVersionError.split('\n').join('\n$indentation')}', - ); - return false; - } - } - - return true; - } - - /// Returns whether the given gradle content is configured to enable all - /// Gradle-driven lints (those checked by ./gradlew lint) and treat them as - /// errors. - bool _validateGradleDrivenLintConfig(List gradleLines) { - if (!gradleLines.any( - (String line) => - line.contains('checkAllWarnings = true') && !_isCommented(line), - ) || - !gradleLines.any( - (String line) => - line.contains('warningsAsErrors = true') && !_isCommented(line), - )) { - printError( - '${indentation}This package is not configured to enable all ' - 'Gradle-driven lint warnings and treat them as errors. ' - 'Please add the following to the lintOptions section of ' - 'android/build.gradle.kts:', - ); - print(''' - checkAllWarnings = true - warningsAsErrors = true -'''); - return false; - } - return true; - } - - bool _validateCompileSdkUsage( - RepositoryPackage package, - List gradleLines, - ) { - final linePattern = RegExp(r'^\s*compileSdk.*\s+='); - final legacySettingPattern = RegExp(r'^\s*compileSdkVersion'); - final String? compileSdkLine = gradleLines.firstWhereOrNull( - (String line) => linePattern.hasMatch(line), - ); - - if (compileSdkLine == null) { - // Equals regex not found check for method pattern. - final compileSpacePattern = RegExp(r'^\s*compileSdk'); - final String? methodAssignmentLine = gradleLines.firstWhereOrNull( - (String line) => compileSpacePattern.hasMatch(line), - ); - if (methodAssignmentLine == null) { - printError('${indentation}No compileSdk or compileSdkVersion found.'); - } else { - printError( - '${indentation}No "compileSdk =" found. Please use property assignment.', - ); - } - return false; - } - if (legacySettingPattern.hasMatch(compileSdkLine)) { - printError( - '${indentation}Please replace the deprecated ' - '"compileSdkVersion" setting with the newer "compileSdk"', - ); - return false; - } - if (compileSdkLine.contains('flutter.compileSdkVersion')) { - final Pubspec pubspec = package.parsePubspec(); - final VersionConstraint? flutterConstraint = - pubspec.environment['flutter']; - final Version? minFlutterVersion = - flutterConstraint != null && flutterConstraint is VersionRange - ? flutterConstraint.min - : null; - if (minFlutterVersion == null) { - printError( - '${indentation}Unable to find a Flutter SDK version ' - 'constraint. Use of flutter.compileSdkVersion requires a minimum ' - 'Flutter version of 3.27', - ); - return false; - } - if (minFlutterVersion < Version(3, 27, 0)) { - printError( - '${indentation}Use of flutter.compileSdkVersion requires a ' - 'minimum Flutter version of 3.27, but this package currently ' - 'supports $minFlutterVersion.\n' - "${indentation}Please update the package's minimum Flutter SDK " - 'version to at least 3.27.', - ); - return false; - } - } else { - // Extract compileSdkVersion and check if it is higher than flutter.compileSdkVersion. - final numericVersionPattern = RegExp(r'=\s*(\d+)'); - final RegExpMatch? versionMatch = numericVersionPattern.firstMatch( - compileSdkLine, - ); - - if (versionMatch != null) { - final int compileSdkVersion = int.parse(versionMatch.group(1)!); - const minCompileSdkVersion = 36; - - if (compileSdkVersion < minCompileSdkVersion) { - printError( - '${indentation}compileSdk version $compileSdkVersion is too low. ' - 'Minimum required version is $minCompileSdkVersion.\n' - "${indentation}Please update this package's compileSdkVersion to at least " - '$minCompileSdkVersion or use flutter.compileSdkVersion.', - ); - return false; - } - } else { - printError('${indentation}Unable to parse compileSdk version number.'); - return false; - } - } - return true; - } - - /// Validates whether the given [example]'s gradle content is configured to - /// build its plugin target with javac lints enabled and treated as errors, - /// if the enclosing package is a plugin. - /// - /// This can only be called on example packages. (Plugin packages should not - /// be configured this way, since it would affect clients.) - /// - /// If [example]'s enclosing package is not a plugin package, this just - /// returns true. - bool _validateJavacLintConfig( - RepositoryPackage example, - List gradleLines, - ) { - final RepositoryPackage enclosingPackage = example.getEnclosingPackage()!; - // This checks for android/ rather than using pluginSupportsPlatform because - // Dart-only implementations (e.g., usin jnigen) won't have this - // configuration since there's no native plugin code to check. - if (!enclosingPackage - .platformDirectory(FlutterPlatform.android) - .existsSync()) { - return true; - } - final String enclosingPackageName = enclosingPackage.directory.basename; - - // The check here is intentionally somewhat loose, to allow for the - // possibility of variations (e.g., not using Xlint:all in some cases, or - // passing other arguments). - if (!(gradleLines.any( - (String line) => line.contains('project(":$enclosingPackageName")'), - ) && - gradleLines.any( - (String line) => - line.contains('options.compilerArgs') && - line.contains('-Xlint') && - line.contains('-Werror'), - ))) { - printError( - 'The example ' - '"${getRelativePosixPath(example.directory, from: enclosingPackage.directory)}" ' - 'is not configured to treat javac lints and warnings as errors. ' - 'Please add the following to its build.gradle:', - ); - print(''' -gradle.projectsEvaluated { - project(":$enclosingPackageName") { - tasks.withType(JavaCompile) { - options.compilerArgs << "-Xlint:all" << "-Werror" - } - } -} -'''); - return false; - } - return true; - } - - /// Validates whether the given [example] has its Kotlin version set to at - /// least a minimum value, if it is set at all. - bool _validateKotlinVersion( - RepositoryPackage example, - List gradleLines, - ) { - final kotlinVersionRegex = RegExp( - r'id\("org\.jetbrains\.kotlin\.android"\) version "([\d.]+)"', - ); - RegExpMatch? match; - if (gradleLines.any((String line) { - match = kotlinVersionRegex.firstMatch(line); - return match != null; - })) { - final version = Version.parse(match!.group(1)!); - if (version < minKotlinVersion) { - printError( - 'settings.gradle.kts sets the "org.jetbrains.kotlin.android" ' - 'plugin version to "$version". The minimum Kotlin version that can ' - 'be specified is $minKotlinVersion, for compatibility with modern ' - 'dependencies.', - ); - return false; - } - } - return true; + // TODO(stuartmorgan): When combining this with other commands, use the + // errors. For now they are dropped to keep the existing behavior. + return errors.isEmpty ? PackageResult.success() : PackageResult.fail(); } } diff --git a/script/tool/lib/src/validators/gradle_validator.dart b/script/tool/lib/src/validators/gradle_validator.dart new file mode 100644 index 000000000000..9797f313c8df --- /dev/null +++ b/script/tool/lib/src/validators/gradle_validator.dart @@ -0,0 +1,783 @@ +// Copyright 2013 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:math' as math; + +import 'package:collection/collection.dart'; +import 'package:file/file.dart'; +import 'package:meta/meta.dart'; +import 'package:path/path.dart' as path; +import 'package:pub_semver/pub_semver.dart'; + +import '../common/file_utils.dart'; +import '../common/output_utils.dart'; +import '../common/repository_package.dart'; + +/// The lowest `ext.kotlin_version` that example apps are allowed to use. +@visibleForTesting +final Version minKotlinVersion = Version(1, 7, 10); + +/// A validator that checks that Gradle files follow repository conventions. +class GradleValidator { + /// Creates a new validator with the given command context. + GradleValidator({required path.Context path, required String indentation}) { + _path = path; + _indentation = indentation; + } + + late path.Context _path; + late String _indentation; + + static const int _minimumJavaVersion = 17; + + /// Documentation url for Artifact hub implementation in flutter repo's. + @visibleForTesting + static const String artifactHubDocumentationString = + r'https://github.com/flutter/flutter/blob/master/docs/ecosystem/Plugins-and-Packages-repository-structure.md#gradle-structure'; + + /// String printed as example of valid example root build.gradle.kts + /// repository configuration that enables artifact hub env variable. + @visibleForTesting + static const String exampleRootGradleArtifactHubString = + ''' + // See $artifactHubDocumentationString for more info. + val artifactRepoKey = "ARTIFACT_HUB_REPOSITORY" + val artifactRepoUrl = System.getenv(artifactRepoKey) + if (artifactRepoUrl != null) { + println("Using artifact hub") + maven { + url = uri(artifactRepoUrl) + } + } +'''; + + /// Validates the Gradle configuration for the given package, returning a list + /// of error messages. + /// + /// Returns an empty list if validation succeeds. + List validateGradle(RepositoryPackage package) { + const exampleDirName = 'example'; + final bool isExample = + package.directory.basename == exampleDirName || + package.directory.parent.basename == exampleDirName; + if (!_validateBuildGradles(package, isExample: isExample)) { + return ['Failed Gradle validation']; + } + return []; + } + + bool _validateBuildGradles( + RepositoryPackage package, { + required bool isExample, + }) { + final Directory androidDir = package.platformDirectory( + FlutterPlatform.android, + ); + final File topLevelGradleFile = _getBuildGradleFile(androidDir); + + // This is tracked as a variable rather than a sequence of &&s so that all + // failures are reported at once, not just the first one. + var succeeded = true; + if (isExample) { + if (!_validateExampleTopLevelBuildGradle(package, topLevelGradleFile)) { + succeeded = false; + } + final File topLevelSettingsGradleFile = _getSettingsGradleFile( + androidDir, + ); + if (!_validateExampleTopLevelSettingsGradle( + package, + topLevelSettingsGradleFile, + )) { + succeeded = false; + } + + final File appGradleFile = _getBuildGradleFile( + androidDir.childDirectory('app'), + ); + if (!_validateExampleAppBuildGradle(package, appGradleFile)) { + succeeded = false; + } + } else { + succeeded = _validatePluginBuildGradle(package, topLevelGradleFile); + } + + return succeeded; + } + + // Returns the gradle file in the given directory. + File _getBuildGradleFile(Directory dir) { + const buildGradleBaseName = 'build.gradle'; + const buildGradleKtsBaseName = '$buildGradleBaseName.kts'; + if (dir.childFile(buildGradleKtsBaseName).existsSync()) { + return dir.childFile(buildGradleKtsBaseName); + } + return dir.childFile(buildGradleBaseName); + } + + // Returns the settings gradle file in the given directory. + File _getSettingsGradleFile(Directory dir) { + const settingsGradleBaseName = 'settings.gradle'; + const settingsGradleKtsBaseName = '$settingsGradleBaseName.kts'; + if (dir.childFile(settingsGradleKtsBaseName).existsSync()) { + return dir.childFile(settingsGradleKtsBaseName); + } + return dir.childFile(settingsGradleBaseName); + } + + // Returns the main/AndroidManifest.xml file for the given package. + File _getMainAndroidManifest( + RepositoryPackage package, { + required bool isExample, + }) { + final Directory androidDir = package.platformDirectory( + FlutterPlatform.android, + ); + final Directory baseDir = isExample + ? androidDir.childDirectory('app') + : androidDir; + return baseDir + .childDirectory('src') + .childDirectory('main') + .childFile('AndroidManifest.xml'); + } + + bool _isCommented(String line) => line.trim().startsWith('//'); + + /// Validates the build.gradle file for a plugin + /// (some_plugin/android/build.gradle). + bool _validatePluginBuildGradle(RepositoryPackage package, File gradleFile) { + print( + '${_indentation}Validating ' + '${_getRelativePosixPath(gradleFile, from: package.directory)}.', + ); + final String contents = gradleFile.readAsStringSync(); + final List lines = contents.split('\n'); + + // This is tracked as a variable rather than a sequence of &&s so that all + // failures are reported at once, not just the first one. + var succeeded = true; + if (!_validateNamespace(package, contents, isExample: false)) { + succeeded = false; + } + if (!_validateCompatibilityVersions(lines)) { + succeeded = false; + } + if (!_validateKotlinJvmCompatibility(lines)) { + succeeded = false; + } + if (!_validateJavaKotlinCompileOptionsAlignment(lines)) { + succeeded = false; + } + if (!_validateGradleDrivenLintConfig(lines)) { + succeeded = false; + } + if (!_validateCompileSdkUsage(package, lines)) { + succeeded = false; + } + return succeeded; + } + + /// Validates that [gradleLines] reads and uses a artifiact hub repository + /// when ARTIFACT_HUB_REPOSITORY is set. + /// + /// Required in root gradle file. + bool _validateArtifactHubUsage( + RepositoryPackage example, + List gradleLines, + ) { + // Gradle variable name used to hold environment variable string. + const keyVariable = 'artifactRepoKey'; + const urlVariable = 'artifactRepoUrl'; + final keyPresentRegex = RegExp( + '$keyVariable' + r'''\s+=\s+["']ARTIFACT_HUB_REPOSITORY["']''', + ); + final documentationPresentRegex = RegExp( + r'github\.com.*flutter.*blob.*Plugins-and-Packages-repository-structure.*gradle-structure', + ); + final keyReadRegex = RegExp( + '$urlVariable' + r'\s*=\s*System\.getenv\(' + '$keyVariable' + r'\)', + ); + final keyUsedRegex = RegExp( + r'url = uri\(' + '$urlVariable' + r'\)', + ); + + final bool keyPresent = gradleLines.any( + (String line) => keyPresentRegex.hasMatch(line), + ); + final bool documentationPresent = gradleLines.any( + (String line) => documentationPresentRegex.hasMatch(line), + ); + final bool keyRead = gradleLines.any( + (String line) => keyReadRegex.hasMatch(line), + ); + final bool keyUsed = gradleLines.any( + (String line) => keyUsedRegex.hasMatch(line), + ); + + if (!(documentationPresent && keyPresent && keyRead && keyUsed)) { + printError( + 'Failed Artifact Hub validation. Include the following in ' + 'example root build.gradle:\n$exampleRootGradleArtifactHubString', + ); + } + + return keyPresent && documentationPresent && keyRead && keyUsed; + } + + /// Validates the top-level settings.gradle for an example app (e.g., + /// some_package/example/android/settings.gradle). + bool _validateExampleTopLevelSettingsGradle( + RepositoryPackage package, + File gradleSettingsFile, + ) { + print( + '${_indentation}Validating ' + '${_getRelativePosixPath(gradleSettingsFile, from: package.directory)}.', + ); + final String contents = gradleSettingsFile.readAsStringSync(); + final List lines = contents.split('\n'); + // This is tracked as a variable rather than a sequence of &&s so that all + // failures are reported at once, not just the first one. + var succeeded = true; + if (!_validateArtifactHubSettingsUsage(package, lines)) { + succeeded = false; + } + if (!_validateKotlinVersion(package, lines)) { + succeeded = false; + } + return succeeded; + } + + /// String printed as a valid example of settings.gradle.kts repository + /// configuration that enables artifact hub env variable. + @visibleForTesting + static const String exampleSettingsArtifactHubString = ''' +plugins { + id("dev.flutter.flutter-plugin-loader") version "1.0.0" + // ...other plugins + id("com.google.cloud.artifactregistry.gradle-plugin") version "2.2.1" +} + '''; + + /// Validates that [gradleLines] reads and uses a artifiact hub repository + /// when ARTIFACT_HUB_REPOSITORY is set. + /// + /// Required in root gradle file. + bool _validateArtifactHubSettingsUsage( + RepositoryPackage example, + List gradleLines, + ) { + final documentationPresentRegex = RegExp( + r'github\.com.*flutter.*blob.*Plugins-and-Packages-repository-structure.*gradle-structure', + ); + final artifactRegistryPluginApplyRegex = RegExp( + r'id.*com\.google\.cloud\.artifactregistry\.gradle-plugin.*version.*\b\d+\.\d+\.\d+\b', + ); + + final bool documentationPresent = gradleLines.any( + (String line) => documentationPresentRegex.hasMatch(line), + ); + final bool declarativeArtifactRegistryApplied = gradleLines.any( + (String line) => artifactRegistryPluginApplyRegex.hasMatch(line), + ); + final bool validArtifactConfiguration = + documentationPresent && declarativeArtifactRegistryApplied; + + if (!validArtifactConfiguration) { + printError('Failed Artifact Hub validation.'); + if (!documentationPresent) { + printError( + 'The link to the Artifact Hub documentation is missing. Include the following in ' + 'example root settings.gradle:\n// See $artifactHubDocumentationString for more info.', + ); + } + if (!declarativeArtifactRegistryApplied) { + printError( + 'Include the following in ' + 'example root settings.gradle:\n$exampleSettingsArtifactHubString', + ); + } + } + return validArtifactConfiguration; + } + + /// Validates the top-level build.gradle(.kts) for an example app (e.g., + /// some_package/example/android/build.gradle(.kts)). + bool _validateExampleTopLevelBuildGradle( + RepositoryPackage package, + File gradleFile, + ) { + print( + '${_indentation}Validating ' + '${_getRelativePosixPath(gradleFile, from: package.directory)}.', + ); + final String contents = gradleFile.readAsStringSync(); + final List lines = contents.split('\n'); + + // This is tracked as a variable rather than a sequence of &&s so that all + // failures are reported at once, not just the first one. + var succeeded = true; + if (!_validateJavacLintConfig(package, lines)) { + succeeded = false; + } + if (!_validateArtifactHubUsage(package, lines)) { + succeeded = false; + } + return succeeded; + } + + /// Validates the app-level build.gradle(.kts) for an example app (e.g., + /// some_package/example/android/app/build.gradle(.kts)). + bool _validateExampleAppBuildGradle( + RepositoryPackage package, + File gradleFile, + ) { + print( + '${_indentation}Validating ' + '${_getRelativePosixPath(gradleFile, from: package.directory)}.', + ); + final String contents = gradleFile.readAsStringSync(); + + // This is tracked as a variable rather than a sequence of &&s so that all + // failures are reported at once, not just the first one. + var succeeded = true; + if (!_validateNamespace(package, contents, isExample: true)) { + succeeded = false; + } + return succeeded; + } + + /// Validates that [gradleContents] sets a namespace, which is required for + /// compatibility with apps that use AGP 8+. + bool _validateNamespace( + RepositoryPackage package, + String gradleContents, { + required bool isExample, + }) { + // Regex to validate that the following namespace definition + // is found (where the single quotes can be single or double): + // - namespace = 'dev.flutter.foo' + final nameSpaceRegex = RegExp( + '^\\s*namespace\\s+=\\s*[\'"](.*?)[\'"]', + multiLine: true, + ); + final RegExpMatch? nameSpaceRegexMatch = nameSpaceRegex.firstMatch( + gradleContents, + ); + + if (nameSpaceRegexMatch == null) { + const errorMessage = ''' +build.gradle.kts must set a "namespace": + + android { + namespace = "dev.flutter.foo" + } + +The value must match the "package" attribute in AndroidManifest.xml, if one is +present. For more information, see: +https://developer.android.com/build/publish-library/prep-lib-release#choose-namespace +'''; + + printError( + '$_indentation${errorMessage.split('\n').join('\n$_indentation')}', + ); + return false; + } else { + return _validateNamespaceMatchesManifest( + package, + isExample: isExample, + namespace: nameSpaceRegexMatch.group(1)!, + ); + } + } + + /// Validates that the given namespace matches the manifest package of + /// [package] (if any; a package does not need to be in the manifest in cases + /// where compatibility with AGP <7 is no longer required). + /// + /// Prints an error and returns false if validation fails. + bool _validateNamespaceMatchesManifest( + RepositoryPackage package, { + required bool isExample, + required String namespace, + }) { + final manifestPackageRegex = RegExp(r'package\s*=\s*"(.*?)"'); + final String manifestContents = _getMainAndroidManifest( + package, + isExample: isExample, + ).readAsStringSync(); + final RegExpMatch? packageMatch = manifestPackageRegex.firstMatch( + manifestContents, + ); + if (packageMatch != null && namespace != packageMatch.group(1)) { + const buildGradleName = 'build.gradle.kts'; + final errorMessage = + ''' +$buildGradleName "namespace" must match the "package" attribute in AndroidManifest.xml, if one is present. + $buildGradleName namespace: "$namespace" + AndroidMastifest.xml package: "${packageMatch.group(1)}" +'''; + printError( + '$_indentation${errorMessage.split('\n').join('\n$_indentation')}', + ); + return false; + } + return true; + } + + /// Checks for a source compatibiltiy version, so that it's explicit rather + /// than using whatever the client's local toolchaing defaults to (which can + /// lead to compile errors that show up for clients, but not in CI). + bool _validateCompatibilityVersions(List gradleLines) { + final bool hasLanguageVersion = gradleLines.any( + (String line) => line.contains('languageVersion') && !_isCommented(line), + ); + final bool hasCompabilityVersions = + gradleLines.any( + (String line) => + line.contains('sourceCompatibility = JavaVersion.VERSION_') && + !_isCommented(line), + ) && + // Newer toolchains default targetCompatibility to the same value as + // sourceCompatibility, but older toolchains require it to be set + // explicitly. The exact version cutoff (and of which piece of the + // toolchain; likely AGP) is unknown; for context see + // https://github.com/flutter/flutter/issues/125482 + gradleLines.any( + (String line) => + line.contains('targetCompatibility = JavaVersion.VERSION_') && + !_isCommented(line), + ); + if (!hasLanguageVersion && !hasCompabilityVersions) { + const javaErrorMessage = + ''' +build.gradle.kts must set an explicit Java compatibility version. + +This can be done either via "sourceCompatibility"/"targetCompatibility": + android { + compileOptions { + sourceCompatibility = JavaVersion.VERSION_$_minimumJavaVersion + targetCompatibility = JavaVersion.VERSION_$_minimumJavaVersion + } + } + +or "toolchain": + java { + toolchain { + languageVersion = JavaLanguageVersion.of($_minimumJavaVersion) + } + } + +See: +https://docs.gradle.org/current/userguide/java_plugin.html#toolchain_and_compatibility +for more details.'''; + + printError( + '$_indentation${javaErrorMessage.split('\n').join('\n$_indentation')}', + ); + return false; + } + + return true; + } + + bool _validateKotlinJvmCompatibility(List gradleLines) { + bool isKotlinOptions(String line) => + line.contains('kotlinOptions') && !_isCommented(line); + final bool hasKotlinOptions = gradleLines.any(isKotlinOptions); + final bool kotlinOptionsUsesJavaVersion = gradleLines.any( + (String line) => + line.contains('jvmTarget = JavaVersion.VERSION_') && + !_isCommented(line), + ); + // Either does not set kotlinOptions or does and uses non-string based syntax. + if (hasKotlinOptions && !kotlinOptionsUsesJavaVersion) { + // Bad lines contains the first 4 lines including the kotlinOptions section. + var badLines = ''; + final int startIndex = gradleLines.indexOf( + gradleLines.firstWhere(isKotlinOptions), + ); + for ( + var i = startIndex; + i < math.min(startIndex + 4, gradleLines.length); + i++ + ) { + badLines += '${gradleLines[i]}\n'; + } + final kotlinErrorMessage = + ''' +If build.gradle.kts sets jvmTarget then it must use JavaVersion syntax. + Good: + android { + kotlinOptions { + jvmTarget = JavaVersion.VERSION_$_minimumJavaVersion.toString() + } + } + BAD: + $badLines +'''; + printError( + '$_indentation${kotlinErrorMessage.split('\n').join('\n$_indentation')}', + ); + return false; + } + // No error condition. + return true; + } + + bool _validateJavaKotlinCompileOptionsAlignment(List gradleLines) { + final javaVersions = []; + // Some java versions have the format VERSION_1_8 but we dont need to handle those + // because they are below the minimum. + final javaVersionMatcher = RegExp( + r'JavaVersion.VERSION_(?\d+)', + ); + for (final line in gradleLines) { + final RegExpMatch? match = javaVersionMatcher.firstMatch(line); + if (!_isCommented(line) && match != null) { + final String? foundVersion = match.namedGroup('javaVersion'); + if (foundVersion != null) { + javaVersions.add(foundVersion); + } + } + } + if (javaVersions.isNotEmpty) { + final int version = int.parse(javaVersions.first); + if (!javaVersions.every((String element) => element == '$version')) { + const javaVersionAlignmentError = ''' +If build.gradle.kts uses JavaVersion.* versions must be the same. +'''; + printError( + '$_indentation${javaVersionAlignmentError.split('\n').join('\n$_indentation')}', + ); + return false; + } + + if (version < _minimumJavaVersion) { + final minimumJavaVersionError = + ''' +build.gradle.kts uses "JavaVersion.VERSION_$version". +Which is below the minimum required. Use at least "JavaVersion.VERSION_$_minimumJavaVersion". +'''; + printError( + '$_indentation${minimumJavaVersionError.split('\n').join('\n$_indentation')}', + ); + return false; + } + } + + return true; + } + + /// Returns whether the given gradle content is configured to enable all + /// Gradle-driven lints (those checked by ./gradlew lint) and treat them as + /// errors. + bool _validateGradleDrivenLintConfig(List gradleLines) { + if (!gradleLines.any( + (String line) => + line.contains('checkAllWarnings = true') && !_isCommented(line), + ) || + !gradleLines.any( + (String line) => + line.contains('warningsAsErrors = true') && !_isCommented(line), + )) { + printError( + '${_indentation}This package is not configured to enable all ' + 'Gradle-driven lint warnings and treat them as errors. ' + 'Please add the following to the lintOptions section of ' + 'android/build.gradle.kts:', + ); + print(''' + checkAllWarnings = true + warningsAsErrors = true +'''); + return false; + } + return true; + } + + bool _validateCompileSdkUsage( + RepositoryPackage package, + List gradleLines, + ) { + final linePattern = RegExp(r'^\s*compileSdk.*\s+='); + final legacySettingPattern = RegExp(r'^\s*compileSdkVersion'); + final String? compileSdkLine = gradleLines.firstWhereOrNull( + (String line) => linePattern.hasMatch(line), + ); + + if (compileSdkLine == null) { + // Equals regex not found check for method pattern. + final compileSpacePattern = RegExp(r'^\s*compileSdk'); + final String? methodAssignmentLine = gradleLines.firstWhereOrNull( + (String line) => compileSpacePattern.hasMatch(line), + ); + if (methodAssignmentLine == null) { + printError('${_indentation}No compileSdk or compileSdkVersion found.'); + } else { + printError( + '${_indentation}No "compileSdk =" found. Please use property assignment.', + ); + } + return false; + } + if (legacySettingPattern.hasMatch(compileSdkLine)) { + printError( + '${_indentation}Please replace the deprecated ' + '"compileSdkVersion" setting with the newer "compileSdk"', + ); + return false; + } + if (compileSdkLine.contains('flutter.compileSdkVersion')) { + final Pubspec pubspec = package.parsePubspec(); + final VersionConstraint? flutterConstraint = + pubspec.environment['flutter']; + final Version? minFlutterVersion = + flutterConstraint != null && flutterConstraint is VersionRange + ? flutterConstraint.min + : null; + if (minFlutterVersion == null) { + printError( + '${_indentation}Unable to find a Flutter SDK version ' + 'constraint. Use of flutter.compileSdkVersion requires a minimum ' + 'Flutter version of 3.27', + ); + return false; + } + if (minFlutterVersion < Version(3, 27, 0)) { + printError( + '${_indentation}Use of flutter.compileSdkVersion requires a ' + 'minimum Flutter version of 3.27, but this package currently ' + 'supports $minFlutterVersion.\n' + "${_indentation}Please update the package's minimum Flutter SDK " + 'version to at least 3.27.', + ); + return false; + } + } else { + // Extract compileSdkVersion and check if it is higher than flutter.compileSdkVersion. + final numericVersionPattern = RegExp(r'=\s*(\d+)'); + final RegExpMatch? versionMatch = numericVersionPattern.firstMatch( + compileSdkLine, + ); + + if (versionMatch != null) { + final int compileSdkVersion = int.parse(versionMatch.group(1)!); + const minCompileSdkVersion = 36; + + if (compileSdkVersion < minCompileSdkVersion) { + printError( + '${_indentation}compileSdk version $compileSdkVersion is too low. ' + 'Minimum required version is $minCompileSdkVersion.\n' + "${_indentation}Please update this package's compileSdkVersion to at least " + '$minCompileSdkVersion or use flutter.compileSdkVersion.', + ); + return false; + } + } else { + printError('${_indentation}Unable to parse compileSdk version number.'); + return false; + } + } + return true; + } + + /// Validates whether the given [example]'s gradle content is configured to + /// build its plugin target with javac lints enabled and treated as errors, + /// if the enclosing package is a plugin. + /// + /// This can only be called on example packages. (Plugin packages should not + /// be configured this way, since it would affect clients.) + /// + /// If [example]'s enclosing package is not a plugin package, this just + /// returns true. + bool _validateJavacLintConfig( + RepositoryPackage example, + List gradleLines, + ) { + final RepositoryPackage enclosingPackage = example.getEnclosingPackage()!; + // This checks for android/ rather than using pluginSupportsPlatform because + // Dart-only implementations (e.g., usin jnigen) won't have this + // configuration since there's no native plugin code to check. + if (!enclosingPackage + .platformDirectory(FlutterPlatform.android) + .existsSync()) { + return true; + } + final String enclosingPackageName = enclosingPackage.directory.basename; + + // The check here is intentionally somewhat loose, to allow for the + // possibility of variations (e.g., not using Xlint:all in some cases, or + // passing other arguments). + if (!(gradleLines.any( + (String line) => line.contains('project(":$enclosingPackageName")'), + ) && + gradleLines.any( + (String line) => + line.contains('options.compilerArgs') && + line.contains('-Xlint') && + line.contains('-Werror'), + ))) { + printError( + 'The example ' + '"${_getRelativePosixPath(example.directory, from: enclosingPackage.directory)}" ' + 'is not configured to treat javac lints and warnings as errors. ' + 'Please add the following to its build.gradle:', + ); + print(''' +gradle.projectsEvaluated { + project(":$enclosingPackageName") { + tasks.withType(JavaCompile) { + options.compilerArgs << "-Xlint:all" << "-Werror" + } + } +} +'''); + return false; + } + return true; + } + + /// Validates whether the given [example] has its Kotlin version set to at + /// least a minimum value, if it is set at all. + bool _validateKotlinVersion( + RepositoryPackage example, + List gradleLines, + ) { + final kotlinVersionRegex = RegExp( + r'id\("org\.jetbrains\.kotlin\.android"\) version "([\d.]+)"', + ); + RegExpMatch? match; + if (gradleLines.any((String line) { + match = kotlinVersionRegex.firstMatch(line); + return match != null; + })) { + final version = Version.parse(match!.group(1)!); + if (version < minKotlinVersion) { + printError( + 'settings.gradle.kts sets the "org.jetbrains.kotlin.android" ' + 'plugin version to "$version". The minimum Kotlin version that can ' + 'be specified is $minKotlinVersion, for compatibility with modern ' + 'dependencies.', + ); + return false; + } + } + return true; + } + + String _getRelativePosixPath( + FileSystemEntity file, { + required Directory from, + }) { + return relativePosixPath(file, from: from, platformContext: _path); + } +} diff --git a/script/tool/test/gradle_check_command_test.dart b/script/tool/test/gradle_check_command_test.dart index 6ce23bb334ca..c038fa189015 100644 --- a/script/tool/test/gradle_check_command_test.dart +++ b/script/tool/test/gradle_check_command_test.dart @@ -7,6 +7,7 @@ import 'package:file/file.dart'; import 'package:flutter_plugin_tools/src/common/core.dart'; import 'package:flutter_plugin_tools/src/common/plugin_utils.dart'; import 'package:flutter_plugin_tools/src/gradle_check_command.dart'; +import 'package:flutter_plugin_tools/src/validators/gradle_validator.dart'; import 'package:git/git.dart'; import 'package:test/test.dart'; @@ -165,7 +166,7 @@ gradle.projectsEvaluated { buildGradle.writeAsStringSync(''' allprojects { repositories { - ${includeArtifactHub ? GradleCheckCommand.exampleRootGradleArtifactHubString : ''} + ${includeArtifactHub ? GradleValidator.exampleRootGradleArtifactHubString : ''} google() mavenCentral() } @@ -226,7 +227,7 @@ pluginManagement { } } -${includeArtifactDocumentation ? '// See ${GradleCheckCommand.artifactHubDocumentationString} for more info.' : ''} +${includeArtifactDocumentation ? '// See ${GradleValidator.artifactHubDocumentationString} for more info.' : ''} plugins { id("dev.flutter.flutter-plugin-loader") version "1.0.0" id("com.android.application") version "8.11.1" apply false @@ -989,8 +990,8 @@ flutter { expect( output, containsAllInOrder([ - contains(GradleCheckCommand.exampleRootGradleArtifactHubString), - contains(GradleCheckCommand.exampleSettingsArtifactHubString), + contains(GradleValidator.exampleRootGradleArtifactHubString), + contains(GradleValidator.exampleSettingsArtifactHubString), ]), ); }, @@ -1025,7 +1026,7 @@ flutter { expect( output, containsAllInOrder([ - contains(GradleCheckCommand.exampleRootGradleArtifactHubString), + contains(GradleValidator.exampleRootGradleArtifactHubString), ]), ); }); @@ -1059,12 +1060,12 @@ flutter { expect( output, containsAllInOrder([ - contains(GradleCheckCommand.exampleSettingsArtifactHubString), + contains(GradleValidator.exampleSettingsArtifactHubString), ]), ); expect( output, - isNot(contains(GradleCheckCommand.exampleRootGradleArtifactHubString)), + isNot(contains(GradleValidator.exampleRootGradleArtifactHubString)), ); }); @@ -1101,7 +1102,7 @@ flutter { expect( output, containsAllInOrder([ - contains(GradleCheckCommand.artifactHubDocumentationString), + contains(GradleValidator.artifactHubDocumentationString), ]), ); },