diff --git a/script/tool/lib/src/common.dart b/script/tool/lib/src/common.dart index fc1fdca6a1..d8826c7930 100644 --- a/script/tool/lib/src/common.dart +++ b/script/tool/lib/src/common.dart @@ -596,10 +596,18 @@ class GitVersionFinder { return changedFiles.toList(); } - /// Get the package version specified in the pubspec file in `pubspecPath` and at the revision of `gitRef`. - Future getPackageVersion(String pubspecPath, String gitRef) async { - final io.ProcessResult gitShow = - await baseGitDir.runCommand(['show', '$gitRef:$pubspecPath']); + /// Get the package version specified in the pubspec file in `pubspecPath` and + /// at the revision of `gitRef` (defaulting to the base if not provided). + Future getPackageVersion(String pubspecPath, {String gitRef}) async { + final String ref = gitRef ?? (await _getBaseSha()); + + io.ProcessResult gitShow; + try { + gitShow = + await baseGitDir.runCommand(['show', '$ref:$pubspecPath']); + } on io.ProcessException { + return null; + } final String fileContent = gitShow.stdout as String; final String versionString = loadYaml(fileContent)['version'] as String; return versionString == null ? null : Version.parse(versionString); diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart index 086fa54888..14dc8fb28e 100644 --- a/script/tool/lib/src/version_check_command.dart +++ b/script/tool/lib/src/version_check_command.dart @@ -13,8 +13,6 @@ import 'package:pubspec_parse/pubspec_parse.dart'; import 'common.dart'; -const String _kBaseSha = 'base-sha'; - /// Categories of version change types. enum NextVersionType { /// A breaking change. @@ -96,48 +94,59 @@ class VersionCheckCommand extends PluginCommand { final List changedPubspecs = await gitVersionFinder.getChangedPubSpecs(); - final String baseSha = argResults[_kBaseSha] as String; + const String indentation = ' '; for (final String pubspecPath in changedPubspecs) { - try { - final File pubspecFile = fileSystem.file(pubspecPath); - if (!pubspecFile.existsSync()) { - continue; - } - final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); - if (pubspec.publishTo == 'none') { - continue; - } + print('Checking versions for $pubspecPath...'); + final File pubspecFile = fileSystem.file(pubspecPath); + if (!pubspecFile.existsSync()) { + print('${indentation}Deleted; skipping.'); + continue; + } + final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); + if (pubspec.publishTo == 'none') { + print('${indentation}Found "publish_to: none"; skipping.'); + continue; + } - final Version masterVersion = - await gitVersionFinder.getPackageVersion(pubspecPath, baseSha); - final Version headVersion = - await gitVersionFinder.getPackageVersion(pubspecPath, 'HEAD'); - if (headVersion == null) { - continue; // Example apps don't have versions - } + final Version headVersion = + await gitVersionFinder.getPackageVersion(pubspecPath, gitRef: 'HEAD'); + if (headVersion == null) { + printErrorAndExit( + errorMessage: '${indentation}No version found. A package that ' + 'intentionally has no version should be marked ' + '"publish_to: none".'); + } + final Version masterVersion = + await gitVersionFinder.getPackageVersion(pubspecPath); + if (masterVersion == null) { + print('${indentation}Unable to find pubspec in master. ' + 'Safe to ignore if the project is new.'); + } - final Map allowedNextVersions = - getAllowedNextVersions(masterVersion, headVersion); + if (masterVersion == headVersion) { + print('${indentation}No version change.'); + continue; + } - if (!allowedNextVersions.containsKey(headVersion)) { - final String error = '$pubspecPath incorrectly updated version.\n' - 'HEAD: $headVersion, master: $masterVersion.\n' - 'Allowed versions: $allowedNextVersions'; - printErrorAndExit(errorMessage: error); - } + final Map allowedNextVersions = + getAllowedNextVersions(masterVersion, headVersion); - final bool isPlatformInterface = - pubspec.name.endsWith('_platform_interface'); - if (isPlatformInterface && - allowedNextVersions[headVersion] == - NextVersionType.BREAKING_MAJOR) { - final String error = '$pubspecPath breaking change detected.\n' - 'Breaking changes to platform interfaces are strongly discouraged.\n'; - printErrorAndExit(errorMessage: error); - } - } on io.ProcessException { - print('Unable to find pubspec in master for $pubspecPath.' - ' Safe to ignore if the project is new.'); + if (!allowedNextVersions.containsKey(headVersion)) { + final String error = '${indentation}Incorrectly updated version.\n' + '${indentation}HEAD: $headVersion, master: $masterVersion.\n' + '${indentation}Allowed versions: $allowedNextVersions'; + printErrorAndExit(errorMessage: error); + } else { + print('$indentation$headVersion -> $masterVersion'); + } + + final bool isPlatformInterface = + pubspec.name.endsWith('_platform_interface'); + if (isPlatformInterface && + allowedNextVersions[headVersion] == NextVersionType.BREAKING_MAJOR) { + final String error = '$pubspecPath breaking change detected.\n' + 'Breaking changes to platform interfaces are strongly discouraged.\n'; + printErrorAndExit(errorMessage: error); } } @@ -153,7 +162,7 @@ class VersionCheckCommand extends PluginCommand { final String packageName = plugin.basename; print('-----------------------------------------'); print( - 'Checking the first version listed in CHANGELOG.MD matches the version in pubspec.yaml for $packageName.'); + 'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for $packageName.'); final Pubspec pubspec = _tryParsePubspec(plugin); if (pubspec == null) { @@ -169,12 +178,29 @@ class VersionCheckCommand extends PluginCommand { final Iterator iterator = lines.iterator; while (iterator.moveNext()) { if (iterator.current.trim().isNotEmpty) { - firstLineWithText = iterator.current; + firstLineWithText = iterator.current.trim(); break; } } // Remove all leading mark down syntax from the version line. - final String versionString = firstLineWithText.split(' ').last; + String versionString = firstLineWithText.split(' ').last; + + // Skip validation for the special NEXT version that's used to accumulate + // changes that don't warrant publishing on their own. + bool hasNextSection = versionString == 'NEXT'; + if (hasNextSection) { + print('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 pass the + // normal validation. + while (iterator.moveNext()) { + if (iterator.current.trim().startsWith('## ')) { + versionString = iterator.current.trim().split(' ').last; + break; + } + } + } + final Version fromChangeLog = Version.parse(versionString); if (fromChangeLog == null) { final String error = @@ -190,6 +216,18 @@ The first version listed in CHANGELOG.md is $fromChangeLog. '''; printErrorAndExit(errorMessage: error); } + + // If NEXT wasn't the first section, it should not exist at all. + if (!hasNextSection) { + final RegExp nextRegex = RegExp(r'^#+\s*NEXT\s*$'); + if (lines.any((String line) => nextRegex.hasMatch(line))) { + printErrorAndExit(errorMessage: ''' +When bumping the version for release, the NEXT section should be incorporated +into the new version's release notes. + '''); + } + } + print('$packageName passed version check'); } diff --git a/script/tool/pubspec.yaml b/script/tool/pubspec.yaml index e471239590..9260e8f48f 100644 --- a/script/tool/pubspec.yaml +++ b/script/tool/pubspec.yaml @@ -1,6 +1,6 @@ name: flutter_plugin_tools description: Productivity utils for hosting multiple plugins within one repository. -publish_to: 'none' +publish_to: none dependencies: args: "^1.4.3" diff --git a/script/tool/test/version_check_test.dart b/script/tool/test/version_check_test.dart index 96a460d7e7..ed953e0b7f 100644 --- a/script/tool/test/version_check_test.dart +++ b/script/tool/test/version_check_test.dart @@ -62,6 +62,8 @@ void main() { final String response = gitShowResponses[invocation.positionalArguments[0][1]]; when(mockProcessResult.stdout as String).thenReturn(response); + } else if (invocation.positionalArguments[0][0] == 'merge-base') { + when(mockProcessResult.stdout as String).thenReturn('abc123'); } return Future.value(mockProcessResult); }); @@ -98,11 +100,12 @@ void main() { ); expect(gitDirCommands.length, equals(3)); expect( - gitDirCommands[0].join(' '), equals('diff --name-only master HEAD')); - expect(gitDirCommands[1].join(' '), - equals('show master:packages/plugin/pubspec.yaml')); - expect(gitDirCommands[2].join(' '), - equals('show HEAD:packages/plugin/pubspec.yaml')); + gitDirCommands, + containsAll([ + equals(['diff', '--name-only', 'master', 'HEAD']), + equals(['show', 'master:packages/plugin/pubspec.yaml']), + equals(['show', 'HEAD:packages/plugin/pubspec.yaml']), + ])); }); test('denies invalid version', () async { @@ -117,15 +120,50 @@ void main() { await expectLater( result, - throwsA(const TypeMatcher()), + throwsA(const TypeMatcher()), ); expect(gitDirCommands.length, equals(3)); expect( - gitDirCommands[0].join(' '), equals('diff --name-only master HEAD')); - expect(gitDirCommands[1].join(' '), - equals('show master:packages/plugin/pubspec.yaml')); - expect(gitDirCommands[2].join(' '), - equals('show HEAD:packages/plugin/pubspec.yaml')); + gitDirCommands, + containsAll([ + equals(['diff', '--name-only', 'master', 'HEAD']), + equals(['show', 'master:packages/plugin/pubspec.yaml']), + equals(['show', 'HEAD:packages/plugin/pubspec.yaml']), + ])); + }); + + test('allows valid version without explicit base-sha', () async { + createFakePlugin('plugin', includeChangeLog: true, includeVersion: true); + gitDiffResponse = 'packages/plugin/pubspec.yaml'; + gitShowResponses = { + 'abc123:packages/plugin/pubspec.yaml': 'version: 1.0.0', + 'HEAD:packages/plugin/pubspec.yaml': 'version: 2.0.0', + }; + final List output = + await runCapturingPrint(runner, ['version-check']); + + expect( + output, + containsAllInOrder([ + 'No version check errors found!', + ]), + ); + }); + + test('denies invalid version without explicit base-sha', () async { + createFakePlugin('plugin', includeChangeLog: true, includeVersion: true); + gitDiffResponse = 'packages/plugin/pubspec.yaml'; + gitShowResponses = { + 'abc123:packages/plugin/pubspec.yaml': 'version: 0.0.1', + 'HEAD:packages/plugin/pubspec.yaml': 'version: 0.2.0', + }; + final Future> result = + runCapturingPrint(runner, ['version-check']); + + await expectLater( + result, + throwsA(const TypeMatcher()), + ); }); test('gracefully handles missing pubspec.yaml', () async { @@ -143,6 +181,8 @@ void main() { output, orderedEquals([ 'Determine diff with base sha: master', + 'Checking versions for packages/plugin/pubspec.yaml...', + ' Deleted; skipping.', 'No version check errors found!', ]), ); @@ -171,13 +211,18 @@ void main() { ); expect(gitDirCommands.length, equals(3)); expect( - gitDirCommands[0].join(' '), equals('diff --name-only master HEAD')); - expect( - gitDirCommands[1].join(' '), - equals( - 'show master:packages/plugin_platform_interface/pubspec.yaml')); - expect(gitDirCommands[2].join(' '), - equals('show HEAD:packages/plugin_platform_interface/pubspec.yaml')); + gitDirCommands, + containsAll([ + equals(['diff', '--name-only', 'master', 'HEAD']), + equals([ + 'show', + 'master:packages/plugin_platform_interface/pubspec.yaml' + ]), + equals([ + 'show', + 'HEAD:packages/plugin_platform_interface/pubspec.yaml' + ]), + ])); }); test('disallows breaking changes to platform interfaces', () async { @@ -194,17 +239,22 @@ void main() { runner, ['version-check', '--base-sha=master']); await expectLater( output, - throwsA(const TypeMatcher()), + throwsA(const TypeMatcher()), ); expect(gitDirCommands.length, equals(3)); expect( - gitDirCommands[0].join(' '), equals('diff --name-only master HEAD')); - expect( - gitDirCommands[1].join(' '), - equals( - 'show master:packages/plugin_platform_interface/pubspec.yaml')); - expect(gitDirCommands[2].join(' '), - equals('show HEAD:packages/plugin_platform_interface/pubspec.yaml')); + gitDirCommands, + containsAll([ + equals(['diff', '--name-only', 'master', 'HEAD']), + equals([ + 'show', + 'master:packages/plugin_platform_interface/pubspec.yaml' + ]), + equals([ + 'show', + 'HEAD:packages/plugin_platform_interface/pubspec.yaml' + ]), + ])); }); test('Allow empty lines in front of the first version in CHANGELOG', @@ -230,7 +280,7 @@ void main() { expect( output, containsAllInOrder([ - 'Checking the first version listed in CHANGELOG.MD matches the version in pubspec.yaml for plugin.', + 'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for plugin.', 'plugin passed version check', 'No version check errors found!' ]), @@ -255,7 +305,7 @@ void main() { runner, ['version-check', '--base-sha=master']); await expectLater( output, - throwsA(const TypeMatcher()), + throwsA(const TypeMatcher()), ); try { final List outputValue = await output; @@ -291,7 +341,7 @@ void main() { expect( output, containsAllInOrder([ - 'Checking the first version listed in CHANGELOG.MD matches the version in pubspec.yaml for plugin.', + 'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for plugin.', 'plugin passed version check', 'No version check errors found!' ]), @@ -322,7 +372,121 @@ void main() { runner, ['version-check', '--base-sha=master']); await expectLater( output, - throwsA(const TypeMatcher()), + throwsA(const TypeMatcher()), + ); + try { + final List outputValue = await output; + await expectLater( + outputValue, + containsAllInOrder([ + ''' + versions for plugin in CHANGELOG.md and pubspec.yaml do not match. + The version in pubspec.yaml is 1.0.0. + The first version listed in CHANGELOG.md is 1.0.1. + ''', + ]), + ); + } on ToolExit catch (_) {} + }); + + test('Allow NEXT as a placeholder for gathering CHANGELOG entries', + () async { + createFakePlugin('plugin', includeChangeLog: true, includeVersion: true); + + final Directory pluginDirectory = + mockPackagesDir.childDirectory('plugin'); + + createFakePubspec(pluginDirectory, + isFlutter: true, includeVersion: true, version: '1.0.0'); + const String changelog = ''' +## NEXT + +* Some changes that won't be published until the next time there's a release. + +## 1.0.0 + +* Some other changes. +'''; + createFakeCHANGELOG(pluginDirectory, changelog); + final List output = await runCapturingPrint( + runner, ['version-check', '--base-sha=master']); + await expectLater( + output, + containsAllInOrder([ + 'Found NEXT; validating next version in the CHANGELOG.', + 'plugin passed version check', + 'No version check errors found!', + ]), + ); + }); + + test('Fail if NEXT is left in the CHANGELOG when adding a version bump', + () async { + createFakePlugin('plugin', includeChangeLog: true, includeVersion: true); + + final Directory pluginDirectory = + mockPackagesDir.childDirectory('plugin'); + + createFakePubspec(pluginDirectory, + isFlutter: true, includeVersion: true, version: '1.0.1'); + const String changelog = ''' +## 1.0.1 + +* Some changes. + +## NEXT + +* Some changes that should have been folded in 1.0.1. + +## 1.0.0 + +* Some other changes. +'''; + createFakeCHANGELOG(pluginDirectory, changelog); + final Future> output = runCapturingPrint( + runner, ['version-check', '--base-sha=master']); + await expectLater( + output, + throwsA(const TypeMatcher()), + ); + try { + final List outputValue = await output; + await expectLater( + outputValue, + containsAllInOrder([ + ''' + versions for plugin in CHANGELOG.md and pubspec.yaml do not match. + The version in pubspec.yaml is 1.0.0. + The first version listed in CHANGELOG.md is 1.0.1. + ''', + ]), + ); + } on ToolExit catch (_) {} + }); + + test('Fail if the version changes without replacing NEXT', () async { + createFakePlugin('plugin', includeChangeLog: true, includeVersion: true); + + final Directory pluginDirectory = + mockPackagesDir.childDirectory('plugin'); + + createFakePubspec(pluginDirectory, + isFlutter: true, includeVersion: true, version: '1.0.1'); + const String changelog = ''' +## NEXT + +* Some changes that should be listed as part of 1.0.1. + +## 1.0.0 + +* Some other changes. +'''; + createFakeCHANGELOG(pluginDirectory, changelog); + final Future> output = runCapturingPrint( + runner, ['version-check', '--base-sha=master']); + await expectLater( + output, + throwsA(const TypeMatcher()), ); try { final List outputValue = await output;