diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index d7e58dcfba..6250e2a727 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.1.4 + +- Add a `pubspec-check` command + ## 0.1.3 - Cosmetic fix to `publish-check` output diff --git a/script/tool/lib/src/main.dart b/script/tool/lib/src/main.dart index 4b0e85704b..a3fbc34ae8 100644 --- a/script/tool/lib/src/main.dart +++ b/script/tool/lib/src/main.dart @@ -9,8 +9,6 @@ import 'dart:io' as io; import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:file/local.dart'; -import 'package:flutter_plugin_tools/src/publish_check_command.dart'; -import 'package:flutter_plugin_tools/src/publish_plugin_command.dart'; import 'package:path/path.dart' as p; import 'analyze_command.dart'; @@ -24,6 +22,9 @@ import 'java_test_command.dart'; import 'license_check_command.dart'; import 'lint_podspecs_command.dart'; import 'list_command.dart'; +import 'publish_check_command.dart'; +import 'publish_plugin_command.dart'; +import 'pubspec_check_command.dart'; import 'test_command.dart'; import 'version_check_command.dart'; import 'xctest_command.dart'; @@ -58,12 +59,20 @@ void main(List args) { ..addCommand(ListCommand(packagesDir, fileSystem)) ..addCommand(PublishCheckCommand(packagesDir, fileSystem)) ..addCommand(PublishPluginCommand(packagesDir, fileSystem)) + ..addCommand(PubspecCheckCommand(packagesDir, fileSystem)) ..addCommand(TestCommand(packagesDir, fileSystem)) ..addCommand(VersionCheckCommand(packagesDir, fileSystem)) ..addCommand(XCTestCommand(packagesDir, fileSystem)); commandRunner.run(args).catchError((Object e) { final ToolExit toolExit = e as ToolExit; - io.exit(toolExit.exitCode); + int exitCode = toolExit.exitCode; + // This should never happen; this check is here to guarantee that a ToolExit + // never accidentally has code 0 thus causing CI to pass. + if (exitCode == 0) { + assert(false); + exitCode = 255; + } + io.exit(exitCode); }, test: (Object e) => e is ToolExit); } diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart new file mode 100644 index 0000000000..fadcfbc56d --- /dev/null +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -0,0 +1,178 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; + +import 'package:file/file.dart'; +import 'package:git/git.dart'; +import 'package:path/path.dart' as p; +import 'package:pubspec_parse/pubspec_parse.dart'; + +import 'common.dart'; + +/// A command to enforce pubspec conventions across the repository. +/// +/// This both ensures that repo best practices for which optional fields are +/// used are followed, and that the structure is consistent to make edits +/// across multiple pubspec files easier. +class PubspecCheckCommand extends PluginCommand { + /// Creates an instance of the version check command. + PubspecCheckCommand( + Directory packagesDir, + FileSystem fileSystem, { + ProcessRunner processRunner = const ProcessRunner(), + GitDir? gitDir, + }) : super(packagesDir, fileSystem, + processRunner: processRunner, gitDir: gitDir); + + // 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:', + ]; + + static const List _majorPackageSections = [ + 'environment:', + 'dependencies:', + 'dev_dependencies:', + 'flutter:', + ]; + + static const String _expectedIssueLinkFormat = + 'https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A'; + + @override + final String name = 'pubspec-check'; + + @override + final String description = + 'Checks that pubspecs follow repository conventions.'; + + @override + Future run() async { + final List failingPackages = []; + await for (final Directory package in getPackages()) { + final String relativePackagePath = + p.relative(package.path, from: packagesDir.path); + print('Checking $relativePackagePath...'); + final File pubspec = package.childFile('pubspec.yaml'); + final bool passesCheck = !pubspec.existsSync() || + await _checkPubspec(pubspec, packageName: package.basename); + if (!passesCheck) { + failingPackages.add(relativePackagePath); + } + } + + if (failingPackages.isNotEmpty) { + print('The following packages have pubspec issues:'); + for (final String package in failingPackages) { + print(' $package'); + } + throw ToolExit(1); + } + + print('\nNo pubspec issues found!'); + } + + Future _checkPubspec( + File pubspecFile, { + required String packageName, + }) async { + const String indentation = ' '; + final String contents = pubspecFile.readAsStringSync(); + final Pubspec? pubspec = _tryParsePubspec(contents); + if (pubspec == null) { + return false; + } + + final List pubspecLines = contents.split('\n'); + final List sectionOrder = pubspecLines.contains(' plugin:') + ? _majorPluginSections + : _majorPackageSections; + bool passing = _checkSectionOrder(pubspecLines, sectionOrder); + if (!passing) { + print('${indentation}Major sections should follow standard ' + 'repository ordering:'); + final String listIndentation = indentation * 2; + print('$listIndentation${sectionOrder.join('\n$listIndentation')}'); + } + + if (pubspec.publishTo != 'none') { + final List repositoryErrors = + _checkForRepositoryLinkErrors(pubspec, packageName: packageName); + if (repositoryErrors.isNotEmpty) { + for (final String error in repositoryErrors) { + print('$indentation$error'); + } + passing = false; + } + + if (!_checkIssueLink(pubspec)) { + print( + '${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; + } + } + + return passing; + } + + Pubspec? _tryParsePubspec(String pubspecContents) { + try { + return Pubspec.parse(pubspecContents); + } on Exception catch (exception) { + print(' Cannot parse pubspec.yaml: $exception'); + } + return null; + } + + bool _checkSectionOrder( + List pubspecLines, List sectionOrder) { + int previousSectionIndex = 0; + for (final String 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 String packageName, + }) { + final List errorMessages = []; + if (pubspec.repository == null) { + errorMessages.add('Missing "repository"'); + } else if (!pubspec.repository!.path.endsWith(packageName)) { + errorMessages + .add('The "repository" link should end with the package name.'); + } + + if (pubspec.homepage != null) { + errorMessages + .add('Found a "homepage" entry; only "repository" should be used.'); + } + + return errorMessages; + } + + bool _checkIssueLink(Pubspec pubspec) { + return pubspec.issueTracker + ?.toString() + .startsWith(_expectedIssueLinkFormat) == + true; + } +} diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml index 8ea735f2b6..ab422daf8e 100644 --- a/script/tool/pubspec.yaml +++ b/script/tool/pubspec.yaml @@ -1,7 +1,7 @@ name: flutter_plugin_tools description: Productivity utils for flutter/plugins and flutter/packages repository: https://github.com/flutter/plugins/tree/master/script/tool -version: 0.1.3 +version: 0.1.4 dependencies: args: ^2.1.0 diff --git a/script/tool/test/build_examples_command_test.dart b/script/tool/test/build_examples_command_test.dart index 7a8aa85fc8..44634c2517 100644 --- a/script/tool/test/build_examples_command_test.dart +++ b/script/tool/test/build_examples_command_test.dart @@ -59,7 +59,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running build-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, orderedEquals([])); @@ -96,7 +95,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -142,7 +140,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running build-examples --linux with no // Linux implementation is a no-op. expect(processRunner.recordedCalls, orderedEquals([])); @@ -175,7 +172,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -211,7 +207,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running build-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, orderedEquals([])); @@ -245,7 +240,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -280,7 +274,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running build-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, orderedEquals([])); @@ -314,7 +307,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -353,7 +345,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running build-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, orderedEquals([])); @@ -386,7 +377,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -425,7 +415,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running build-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, orderedEquals([])); @@ -462,7 +451,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -492,7 +480,6 @@ void main() { '--enable-experiment=exp1' ]); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -522,7 +509,6 @@ void main() { '--no-macos', '--enable-experiment=exp1' ]); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ diff --git a/script/tool/test/drive_examples_command_test.dart b/script/tool/test/drive_examples_command_test.dart index 6fb8b8d335..b4d6a25154 100644 --- a/script/tool/test/drive_examples_command_test.dart +++ b/script/tool/test/drive_examples_command_test.dart @@ -63,7 +63,6 @@ void main() { final String deviceTestPath = p.join('test', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -109,7 +108,6 @@ void main() { final String deviceTestPath = p.join('test_driver', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -197,7 +195,6 @@ void main() { final String driverTestPath = p.join('test_driver', 'integration_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -252,7 +249,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running drive-examples --linux on a non-Linux // plugin is a no-op. expect(processRunner.recordedCalls, []); @@ -287,7 +283,6 @@ void main() { final String deviceTestPath = p.join('test_driver', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -332,7 +327,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running drive-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, []); @@ -367,7 +361,6 @@ void main() { final String deviceTestPath = p.join('test_driver', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -414,7 +407,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running drive-examples --web on a non-web // plugin is a no-op. expect(processRunner.recordedCalls, []); @@ -449,7 +441,6 @@ void main() { final String deviceTestPath = p.join('test_driver', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -498,7 +489,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running drive-examples --windows on a // non-Windows plugin is a no-op. expect(processRunner.recordedCalls, []); @@ -533,7 +523,6 @@ void main() { final String deviceTestPath = p.join('test_driver', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ @@ -579,7 +568,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running drive-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, []); @@ -600,7 +588,6 @@ void main() { ]), ); - print(processRunner.recordedCalls); // Output should be empty since running drive-examples --macos with no macos // implementation is a no-op. expect(processRunner.recordedCalls, []); @@ -627,7 +614,6 @@ void main() { final String deviceTestPath = p.join('test', 'plugin.dart'); final String driverTestPath = p.join('test_driver', 'plugin_test.dart'); - print(processRunner.recordedCalls); expect( processRunner.recordedCalls, orderedEquals([ diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart new file mode 100644 index 0000000000..aee3f7a203 --- /dev/null +++ b/script/tool/test/pubspec_check_command_test.dart @@ -0,0 +1,334 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:args/command_runner.dart'; +import 'package:file/file.dart'; +import 'package:file/memory.dart'; +import 'package:flutter_plugin_tools/src/common.dart'; +import 'package:flutter_plugin_tools/src/pubspec_check_command.dart'; +import 'package:test/test.dart'; + +import 'util.dart'; + +void main() { + group('test pubspec_check_command', () { + late CommandRunner runner; + late RecordingProcessRunner processRunner; + late FileSystem fileSystem; + late Directory packagesDir; + + setUp(() { + fileSystem = MemoryFileSystem(); + packagesDir = fileSystem.currentDirectory.childDirectory('packages'); + initializeFakePackages(parentDir: packagesDir.parent); + processRunner = RecordingProcessRunner(); + final PubspecCheckCommand command = PubspecCheckCommand( + packagesDir, fileSystem, + processRunner: processRunner); + + runner = CommandRunner( + 'pubspec_check_command', 'Test for pubspec_check_command'); + runner.addCommand(command); + }); + + String headerSection( + String name, { + bool isPlugin = false, + bool includeRepository = true, + bool includeHomepage = false, + bool includeIssueTracker = true, + }) { + final String repoLink = 'https://github.com/flutter/' + '${isPlugin ? 'plugins' : 'packages'}/tree/master/packages/$name'; + final String issueTrackerLink = + 'https://github.com/flutter/flutter/issues?' + 'q=is%3Aissue+is%3Aopen+label%3A%22p%3A+$name%22'; + return ''' +name: $name +${includeRepository ? 'repository: $repoLink' : ''} +${includeHomepage ? 'homepage: $repoLink' : ''} +${includeIssueTracker ? 'issue_tracker: $issueTrackerLink' : ''} +version: 1.0.0 +'''; + } + + String environmentSection() { + return ''' +environment: + sdk: ">=2.12.0 <3.0.0" + flutter: ">=2.0.0" +'''; + } + + String flutterSection({bool isPlugin = false}) { + const String pluginEntry = ''' + plugin: + platforms: +'''; + return ''' +flutter: +${isPlugin ? pluginEntry : ''} +'''; + } + + String dependenciesSection() { + return ''' +dependencies: + flutter: + sdk: flutter +'''; + } + + String devDependenciesSection() { + return ''' +dev_dependencies: + flutter_test: + sdk: flutter +'''; + } + + test('passes for a plugin following conventions', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true)} +${environmentSection()} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +${devDependenciesSection()} +'''); + + final List output = await runCapturingPrint(runner, [ + 'pubspec-check', + ]); + + expect( + output, + containsAllInOrder([ + 'Checking plugin...', + 'Checking plugin/example...', + '\nNo pubspec issues found!', + ]), + ); + }); + + test('passes for a Flutter package following conventions', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin')} +${environmentSection()} +${dependenciesSection()} +${devDependenciesSection()} +${flutterSection()} +'''); + + final List output = await runCapturingPrint(runner, [ + 'pubspec-check', + ]); + + expect( + output, + containsAllInOrder([ + 'Checking plugin...', + 'Checking plugin/example...', + '\nNo pubspec issues found!', + ]), + ); + }); + + test('passes for a minimal package following conventions', () async { + final Directory packageDirectory = packagesDir.childDirectory('package'); + packageDirectory.createSync(recursive: true); + + packageDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('package')} +${environmentSection()} +${dependenciesSection()} +'''); + + final List output = await runCapturingPrint(runner, [ + 'pubspec-check', + ]); + + expect( + output, + containsAllInOrder([ + 'Checking package...', + '\nNo pubspec issues found!', + ]), + ); + }); + + test('fails when homepage is included', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true, includeHomepage: true)} +${environmentSection()} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +${devDependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when repository is missing', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true, includeRepository: false)} +${environmentSection()} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +${devDependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when homepage is given instead of repository', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true, includeHomepage: true, includeRepository: false)} +${environmentSection()} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +${devDependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when issue tracker is missing', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true, includeIssueTracker: false)} +${environmentSection()} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +${devDependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when environment section is out of order', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true)} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +${devDependenciesSection()} +${environmentSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when flutter section is out of order', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true)} +${flutterSection(isPlugin: true)} +${environmentSection()} +${dependenciesSection()} +${devDependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when dependencies section is out of order', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true)} +${environmentSection()} +${flutterSection(isPlugin: true)} +${devDependenciesSection()} +${dependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + + test('fails when devDependencies section is out of order', () async { + final Directory pluginDirectory = createFakePlugin('plugin', + withSingleExample: true, packagesDirectory: packagesDir); + + pluginDirectory.childFile('pubspec.yaml').writeAsStringSync(''' +${headerSection('plugin', isPlugin: true)} +${environmentSection()} +${devDependenciesSection()} +${flutterSection(isPlugin: true)} +${dependenciesSection()} +'''); + + final Future> result = + runCapturingPrint(runner, ['pubspec-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); + }); + }); +}