From e5978f7c361bb500823b88fca16a5d88758f2a98 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 27 Aug 2021 15:54:27 -0400 Subject: [PATCH] [flutter_plugin_tool] Migrate 'publish' to new base command Moves `publish` to PackageLoopingCommand, giving it the same standardized output and summary that is used by most other commands in the tool. Adds minor new functionality to the base command to allow it to handle the specific needs of publish: - Allows fully customizing the set of packages to loop over, to support --all-changed - Allows customization of a few more strings so the output better matches the functionality (e.g., using 'published' instead of 'ran' in the summary lines). Fixes https://github.com/flutter/flutter/issues/83413 --- .../src/common/package_looping_command.dart | 22 +- .../tool/lib/src/publish_plugin_command.dart | 239 ++++++------------ .../test/publish_plugin_command_test.dart | 140 +++++----- 3 files changed, 163 insertions(+), 238 deletions(-) diff --git a/script/tool/lib/src/common/package_looping_command.dart b/script/tool/lib/src/common/package_looping_command.dart index 00caeb30ef42..96dd881bfe00 100644 --- a/script/tool/lib/src/common/package_looping_command.dart +++ b/script/tool/lib/src/common/package_looping_command.dart @@ -92,6 +92,18 @@ abstract class PackageLoopingCommand extends PluginCommand { /// arguments are invalid), and to set up any run-level state. Future initializeRun() async {} + /// Returns the packages to process. By default, this returns the packages + /// defined by the standard tooling flags and the [inculdeSubpackages] option, + /// but can be overridden for custom package enumeration. + /// + /// Note: Consistent behavior across commands whenever possibel is a goal for + /// this tool, so this should be overridden only in rare cases. + Stream getPackagesToProcess() async* { + yield* includeSubpackages + ? getTargetPackagesAndSubpackages(filterExcluded: false) + : getTargetPackages(filterExcluded: false); + } + /// Runs the command for [package], returning a list of errors. /// /// Errors may either be an empty string if there is no context that should @@ -138,6 +150,9 @@ abstract class PackageLoopingCommand extends PluginCommand { /// context. String get failureListFooter => 'See above for full details.'; + /// The summary string used for a successful run in the final overview output. + String get successSummaryMessage => 'ran'; + /// If true, all printing (including the summary) will be redirected to a /// buffer, and provided in a call to [handleCapturedOutput] at the end of /// the run. @@ -206,9 +221,8 @@ abstract class PackageLoopingCommand extends PluginCommand { await initializeRun(); - final List targetPackages = includeSubpackages - ? await getTargetPackagesAndSubpackages(filterExcluded: false).toList() - : await getTargetPackages(filterExcluded: false).toList(); + final List targetPackages = + await getPackagesToProcess().toList(); final Map results = {}; @@ -346,7 +360,7 @@ abstract class PackageLoopingCommand extends PluginCommand { summary = 'skipped'; style = hadWarning ? Styles.LIGHT_YELLOW : Styles.DARK_GRAY; } else { - summary = 'ran'; + summary = successSummaryMessage; style = hadWarning ? Styles.YELLOW : Styles.GREEN; } if (hadWarning) { diff --git a/script/tool/lib/src/publish_plugin_command.dart b/script/tool/lib/src/publish_plugin_command.dart index aafe7868d8d0..0db400a1d32a 100644 --- a/script/tool/lib/src/publish_plugin_command.dart +++ b/script/tool/lib/src/publish_plugin_command.dart @@ -7,7 +7,6 @@ import 'dart:convert'; import 'dart:io' as io; import 'package:file/file.dart'; -import 'package:flutter_plugin_tools/src/common/repository_package.dart'; import 'package:git/git.dart'; import 'package:http/http.dart' as http; import 'package:meta/meta.dart'; @@ -19,9 +18,11 @@ import 'package:yaml/yaml.dart'; import 'common/core.dart'; import 'common/git_version_finder.dart'; +import 'common/package_looping_command.dart'; import 'common/plugin_command.dart'; import 'common/process_runner.dart'; import 'common/pub_version_finder.dart'; +import 'common/repository_package.dart'; @immutable class _RemoteInfo { @@ -45,7 +46,7 @@ class _RemoteInfo { /// usage information. /// /// [processRunner], [print], and [stdin] can be overriden for easier testing. -class PublishPluginCommand extends PluginCommand { +class PublishPluginCommand extends PackageLoopingCommand { /// Creates an instance of the publish command. PublishPluginCommand( Directory packagesDir, { @@ -116,38 +117,45 @@ class PublishPluginCommand extends PluginCommand { StreamSubscription? _stdinSubscription; final PubVersionFinder _pubVersionFinder; + // Tags that already exist in the repository. + List _existingGitTags = []; + // The remote to push tags to. + late _RemoteInfo _remote; + + @override + String get successSummaryMessage => 'published'; + + @override + String get failureListHeader => + 'The following packages had failures during publishing:'; + @override - Future run() async { + Future initializeRun() async { print('Checking local repo...'); - final GitDir repository = await gitDir; + + // Ensure that the requested remote is present. final String remoteName = getStringArg(_remoteOption); final String? remoteUrl = await _verifyRemote(remoteName); if (remoteUrl == null) { printError('Unable to find URL for remote $remoteName; cannot push tags'); throw ToolExit(1); } - final _RemoteInfo remote = _RemoteInfo(name: remoteName, url: remoteUrl); + _remote = _RemoteInfo(name: remoteName, url: remoteUrl); + + // Pre-fetch all the repository's tags, to check against when publishing. + final GitDir repository = await gitDir; + final io.ProcessResult existingTagsResult = + await repository.runCommand(['tag', '--sort=-committerdate']); + _existingGitTags = (existingTagsResult.stdout as String).split('\n') + ..removeWhere((String element) => element.isEmpty); - print('Local repo is ready!'); if (getBoolArg(_dryRunFlag)) { - print('=============== DRY RUN ==============='); + print('=============== DRY RUN ==============='); } - - final List packages = await _getPackagesToProcess() - .where((PackageEnumerationEntry entry) => !entry.excluded) - .toList(); - bool successful = true; - - successful = await _publishPackages( - packages, - baseGitDir: repository, - remoteForTagPush: remote, - ); - - await _finish(successful); } - Stream _getPackagesToProcess() async* { + @override + Stream getPackagesToProcess() async* { if (getBoolArg(_allChangedFlag)) { final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); final List changedPubspecs = @@ -169,92 +177,52 @@ class PublishPluginCommand extends PluginCommand { } } - Future _publishPackages( - List packages, { - required GitDir baseGitDir, - required _RemoteInfo remoteForTagPush, - }) async { - if (packages.isEmpty) { - print('No version updates in this commit.'); - return true; + @override + Future runForPackage(RepositoryPackage package) async { + final PackageResult? checkResult = await _checkNeedsRelease(package); + if (checkResult != null) { + return checkResult; } - final io.ProcessResult existingTagsResult = - await baseGitDir.runCommand(['tag', '--sort=-committerdate']); - final List existingTags = (existingTagsResult.stdout as String) - .split('\n') - ..removeWhere((String element) => element.isEmpty); - - final List packagesReleased = []; - final List packagesFailed = []; - - for (final PackageEnumerationEntry entry in packages) { - final RepositoryPackage package = entry.package; - - final _CheckNeedsReleaseResult result = await _checkNeedsRelease( - package: package, - existingTags: existingTags, - ); - switch (result) { - case _CheckNeedsReleaseResult.release: - break; - case _CheckNeedsReleaseResult.noRelease: - continue; - case _CheckNeedsReleaseResult.failure: - packagesFailed.add(package.displayName); - continue; - } - print('\n'); - if (await _publishAndTagPackage(package, - remoteForTagPush: remoteForTagPush)) { - packagesReleased.add(package.displayName); - } else { - packagesFailed.add(package.displayName); - } - print('\n'); + if (!await _checkGitStatus(package)) { + return PackageResult.fail(['uncommitted changes']); } - if (packagesReleased.isNotEmpty) { - print('Packages released: ${packagesReleased.join(', ')}'); + + if (!await _publish(package)) { + return PackageResult.fail(['publish failed']); } - if (packagesFailed.isNotEmpty) { - printError( - 'Failed to release the following packages: ${packagesFailed.join(', ')}, see above for details.'); + + if (!await _tagRelease(package)) { + return PackageResult.fail(['tagging failed']); } - return packagesFailed.isEmpty; + + print('\nPublished ${package.directory.basename} successfully!'); + return PackageResult.success(); } - // Publish the package to pub with `pub publish`, then git tag the release - // and push the tag to [remoteForTagPush]. - // Returns `true` if publishing and tagging are successful. - Future _publishAndTagPackage( - RepositoryPackage package, { - _RemoteInfo? remoteForTagPush, - }) async { - if (!await _publishPackage(package)) { - return false; - } - if (!await _tagRelease( - package, - remoteForPush: remoteForTagPush, - )) { - return false; - } - print('Published ${package.directory.basename} successfully.'); - return true; + @override + Future completeRun() async { + _pubVersionFinder.httpClient.close(); + await _stdinSubscription?.cancel(); + _stdinSubscription = null; } - // Returns a [_CheckNeedsReleaseResult] that indicates the result. - Future<_CheckNeedsReleaseResult> _checkNeedsRelease({ - required RepositoryPackage package, - required List existingTags, - }) async { + /// Checks whether [package] needs to be released, printing check status and + /// returning one of: + /// - PackageResult.fail if the check could not be completed + /// - PackageResult.skip if no release is necessary + /// - null if releasing should proceed + /// + /// In cases where a non-null result is returned, that should be returned + /// as the final result for the package, without further processing. + Future _checkNeedsRelease(RepositoryPackage package) async { final File pubspecFile = package.pubspecFile; if (!pubspecFile.existsSync()) { - print(''' + logWarning(''' The pubspec file at ${pubspecFile.path} does not exist. Publishing will not happen for ${pubspecFile.parent.basename}. Safe to ignore if the package is deleted in this commit. '''); - return _CheckNeedsReleaseResult.noRelease; + return PackageResult.skip('package deleted'); } final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); @@ -263,17 +231,18 @@ Safe to ignore if the package is deleted in this commit. // Ignore flutter_plugin_tools package when running publishing through flutter_plugin_tools. // TODO(cyanglaz): Make the tool also auto publish flutter_plugin_tools package. // https://github.com/flutter/flutter/issues/85430 - return _CheckNeedsReleaseResult.noRelease; + return PackageResult.skip( + 'publishing flutter_plugin_tools via the tool is not supported'); } if (pubspec.publishTo == 'none') { - return _CheckNeedsReleaseResult.noRelease; + return PackageResult.skip('publish_to: none'); } if (pubspec.version == null) { printError( 'No version found. A package that intentionally has no version should be marked "publish_to: none"'); - return _CheckNeedsReleaseResult.failure; + return PackageResult.fail(['no version']); } // Check if the package named `packageName` with `version` has already @@ -282,49 +251,29 @@ Safe to ignore if the package is deleted in this commit. final PubVersionFinderResponse pubVersionFinderResponse = await _pubVersionFinder.getPackageVersion(packageName: pubspec.name); if (pubVersionFinderResponse.versions.contains(version)) { - final String tagsForPackageWithSameVersion = existingTags.firstWhere( + final String tagsForPackageWithSameVersion = _existingGitTags.firstWhere( (String tag) => tag.split('-v').first == pubspec.name && tag.split('-v').last == version.toString(), orElse: () => ''); - print( - 'The version $version of ${pubspec.name} has already been published'); if (tagsForPackageWithSameVersion.isEmpty) { printError( - 'However, the git release tag for this version (${pubspec.name}-v$version) is not found. Please manually fix the tag then run the command again.'); - return _CheckNeedsReleaseResult.failure; + '${pubspec.name} $version has already been published, however ' + 'the git release tag (${pubspec.name}-v$version) was not found. ' + 'Please manually fix the tag then run the command again.'); + return PackageResult.fail(['published but untagged']); } else { - print('skip.'); - return _CheckNeedsReleaseResult.noRelease; + print('${pubspec.name} $version has already been published.'); + return PackageResult.skip('already published'); } } - return _CheckNeedsReleaseResult.release; - } - - // Publish the package. - // - // Returns `true` if successful, `false` otherwise. - Future _publishPackage(RepositoryPackage package) async { - final bool gitStatusOK = await _checkGitStatus(package); - if (!gitStatusOK) { - return false; - } - final bool publishOK = await _publish(package); - if (!publishOK) { - return false; - } - print('Package published!'); - return true; + return null; } - // Tag the release with -v, and, if [remoteForTagPush] - // is provided, push it to that remote. + // Tag the release with -v, and push it to the remote. // // Return `true` if successful, `false` otherwise. - Future _tagRelease( - RepositoryPackage package, { - _RemoteInfo? remoteForPush, - }) async { + Future _tagRelease(RepositoryPackage package) async { final String tag = _getTag(package); print('Tagging release $tag...'); if (!getBoolArg(_dryRunFlag)) { @@ -337,27 +286,15 @@ Safe to ignore if the package is deleted in this commit. } } - if (remoteForPush == null) { - return true; - } - - print('Pushing tag to ${remoteForPush.name}...'); - return await _pushTagToRemote( + print('Pushing tag to ${_remote.name}...'); + final bool success = await _pushTagToRemote( tag: tag, - remote: remoteForPush, + remote: _remote, ); - } - - Future _finish(bool successful) async { - _pubVersionFinder.httpClient.close(); - await _stdinSubscription?.cancel(); - _stdinSubscription = null; - if (successful) { - print('Done!'); - } else { - printError('Failed, see above for details.'); - throw ToolExit(1); + if (success) { + print('Release tagged!'); } + return success; } Future _checkGitStatus(RepositoryPackage package) async { @@ -396,6 +333,7 @@ Safe to ignore if the package is deleted in this commit. } Future _publish(RepositoryPackage package) async { + print('Publishing...'); final List publishFlags = getStringListArg(_pubFlagsOption); print('Running `pub publish ${publishFlags.join(' ')}` in ' '${package.directory.absolute.path}...\n'); @@ -423,6 +361,8 @@ Safe to ignore if the package is deleted in this commit. printError('Publishing ${package.directory.basename} failed.'); return false; } + + print('Package published!'); return true; } @@ -518,14 +458,3 @@ final String _credentialsPath = () { return p.join(cacheDir, 'credentials.json'); }(); - -enum _CheckNeedsReleaseResult { - // The package needs to be released. - release, - - // The package does not need to be released. - noRelease, - - // There's an error when trying to determine whether the package needs to be released. - failure, -} diff --git a/script/tool/test/publish_plugin_command_test.dart b/script/tool/test/publish_plugin_command_test.dart index ae3d768fcc70..2ea4fc753460 100644 --- a/script/tool/test/publish_plugin_command_test.dart +++ b/script/tool/test/publish_plugin_command_test.dart @@ -108,7 +108,8 @@ void main() { '?? /packages/foo/tmp\n\n' 'If the directory should be clean, you can run `git clean -xdf && ' 'git reset --hard HEAD` to wipe all local changes.'), - contains('Failed, see above for details.'), + contains('foo:\n' + ' uncommitted changes'), ])); }); @@ -264,10 +265,13 @@ void main() { isNot(contains('git-push'))); expect( output, - containsAllInOrder([ - '=============== DRY RUN ===============', - 'Running `pub publish ` in ${pluginDir.path}...\n', - 'Done!' + containsAllInOrder([ + contains('=============== DRY RUN ==============='), + contains('Running for foo'), + contains('Running `pub publish ` in ${pluginDir.path}...'), + contains('Tagging release foo-v0.0.1...'), + contains('Pushing tag to upstream...'), + contains('Published foo successfully!'), ])); }); @@ -353,7 +357,8 @@ void main() { expect( output, containsAllInOrder([ - contains('Published foo successfully.'), + contains('Pushing tag to upstream...'), + contains('Published foo successfully!'), ])); }); @@ -376,7 +381,7 @@ void main() { expect( output, containsAllInOrder([ - contains('Published foo successfully.'), + contains('Published foo successfully!'), ])); }); @@ -395,12 +400,12 @@ void main() { isNot(contains('git-push'))); expect( output, - containsAllInOrder([ - '=============== DRY RUN ===============', - 'Running `pub publish ` in ${pluginDir.path}...\n', - 'Tagging release foo-v0.0.1...', - 'Pushing tag to upstream...', - 'Done!' + containsAllInOrder([ + contains('=============== DRY RUN ==============='), + contains('Running `pub publish ` in ${pluginDir.path}...'), + contains('Tagging release foo-v0.0.1...'), + contains('Pushing tag to upstream...'), + contains('Published foo successfully!'), ])); }); @@ -424,7 +429,7 @@ void main() { expect( output, containsAllInOrder([ - contains('Published foo successfully.'), + contains('Published foo successfully!'), ])); }); }); @@ -460,13 +465,11 @@ void main() { expect( output, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - 'Running `pub publish ` in ${pluginDir1.path}...\n', - 'Running `pub publish ` in ${pluginDir2.path}...\n', - 'Packages released: plugin1, plugin2/plugin2', - 'Done!' + containsAllInOrder([ + contains('Running `pub publish ` in ${pluginDir1.path}...'), + contains('Running `pub publish ` in ${pluginDir2.path}...'), + contains('plugin1 - \x1B[32mpublished\x1B[0m'), + contains('plugin2/plugin2 - \x1B[32mpublished\x1B[0m'), ])); expect( processRunner.recordedCalls, @@ -522,12 +525,8 @@ void main() { expect( output, containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', 'Running `pub publish ` in ${pluginDir1.path}...\n', 'Running `pub publish ` in ${pluginDir2.path}...\n', - 'Packages released: plugin1, plugin2/plugin2', - 'Done!' ])); expect( processRunner.recordedCalls, @@ -573,18 +572,16 @@ void main() { expect( output, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - '=============== DRY RUN ===============', - 'Running `pub publish ` in ${pluginDir1.path}...\n', - 'Tagging release plugin1-v0.0.1...', - 'Pushing tag to upstream...', - 'Running `pub publish ` in ${pluginDir2.path}...\n', - 'Tagging release plugin2-v0.0.1...', - 'Pushing tag to upstream...', - 'Packages released: plugin1, plugin2/plugin2', - 'Done!' + containsAllInOrder([ + contains('=============== DRY RUN ==============='), + contains('Running `pub publish ` in ${pluginDir1.path}...'), + contains('Tagging release plugin1-v0.0.1...'), + contains('Pushing tag to upstream...'), + contains('Published plugin1 successfully!'), + contains('Running `pub publish ` in ${pluginDir2.path}...'), + contains('Tagging release plugin2-v0.0.1...'), + contains('Pushing tag to upstream...'), + contains('Published plugin2 successfully!'), ])); expect( processRunner.recordedCalls @@ -623,13 +620,11 @@ void main() { ['publish-plugin', '--all-changed', '--base-sha=HEAD~']); expect( output2, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - 'Running `pub publish ` in ${pluginDir1.path}...\n', - 'Running `pub publish ` in ${pluginDir2.path}...\n', - 'Packages released: plugin1, plugin2/plugin2', - 'Done!' + containsAllInOrder([ + contains('Running `pub publish ` in ${pluginDir1.path}...'), + contains('Published plugin1 successfully!'), + contains('Running `pub publish ` in ${pluginDir2.path}...'), + contains('Published plugin2 successfully!'), ])); expect( processRunner.recordedCalls, @@ -642,7 +637,7 @@ void main() { }); test( - 'delete package will not trigger publish but exit the command successfully.', + 'delete package will not trigger publish but exit the command successfully!', () async { mockHttpResponses['plugin1'] = { 'name': 'plugin1', @@ -674,13 +669,13 @@ void main() { ['publish-plugin', '--all-changed', '--base-sha=HEAD~']); expect( output2, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - 'Running `pub publish ` in ${pluginDir1.path}...\n', - 'The pubspec file at ${pluginDir2.childFile('pubspec.yaml').path} does not exist. Publishing will not happen for plugin2.\nSafe to ignore if the package is deleted in this commit.\n', - 'Packages released: plugin1', - 'Done!' + containsAllInOrder([ + contains('Running `pub publish ` in ${pluginDir1.path}...'), + contains('Published plugin1 successfully!'), + contains( + 'The pubspec file at ${pluginDir2.childFile('pubspec.yaml').path} does not exist. Publishing will not happen for plugin2.\nSafe to ignore if the package is deleted in this commit.\n'), + contains('SKIPPING: package deleted'), + contains('skipped (with warning)'), ])); expect( processRunner.recordedCalls, @@ -724,14 +719,11 @@ void main() { expect( output, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - 'The version 0.0.2 of plugin1 has already been published', - 'skip.', - 'The version 0.0.2 of plugin2 has already been published', - 'skip.', - 'Done!' + containsAllInOrder([ + contains('plugin1 0.0.2 has already been published'), + contains('SKIPPING: already published'), + contains('plugin2 0.0.2 has already been published'), + contains('SKIPPING: already published'), ])); expect( @@ -778,12 +770,10 @@ void main() { expect( output, containsAllInOrder([ - contains('The version 0.0.2 of plugin1 has already been published'), - contains( - 'However, the git release tag for this version (plugin1-v0.0.2) is not found.'), - contains('The version 0.0.2 of plugin2 has already been published'), - contains( - 'However, the git release tag for this version (plugin2-v0.0.2) is not found.'), + contains('plugin1 0.0.2 has already been published, ' + 'however the git release tag (plugin1-v0.0.2) was not found.'), + contains('plugin2 0.0.2 has already been published, ' + 'however the git release tag (plugin2-v0.0.2) was not found.'), ])); expect( processRunner.recordedCalls @@ -807,14 +797,7 @@ void main() { final List output = await runCapturingPrint(commandRunner, ['publish-plugin', '--all-changed', '--base-sha=HEAD~']); - expect( - output, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - 'No version updates in this commit.', - 'Done!' - ])); + expect(output, containsAllInOrder(['Ran for 0 package(s)'])); expect( processRunner.recordedCalls .map((ProcessCall call) => call.executable), @@ -838,14 +821,13 @@ void main() { expect( output, - containsAllInOrder([ - 'Checking local repo...', - 'Local repo is ready!', - 'Done!' + containsAllInOrder([ + contains( + 'SKIPPING: publishing flutter_plugin_tools via the tool is not supported') ])); expect( output.contains( - 'Running `pub publish ` in ${flutterPluginTools.path}...\n', + 'Running `pub publish ` in ${flutterPluginTools.path}...', ), isFalse); expect(