Fix and update version checks (#3792)

Currently our version update checks aren't actually working; the script doesn't work correctly if no explicit --base-sha is passed, but that's always how CI is calling it.

Fixes https://github.com/flutter/flutter/issues/79823 (and version checks in general)

This makes a number of changes:
- Fixes it to work without --base-sha
- Adds tests that it works in that mode
  - And tightens existing tests to require ToolExit, not just any error, to reduce false-positive test success
- Adds verbose logging of the checks being done, to make it easier to debug this kind of issue in the future
- Tightens the exception handling for missing previous versions to just the line that's expected to fail in that case
- Only allows missing versions when "publish_to: none" is set
  - Adds that everywhere it's missing
  - Standardize the format in the repo to "none" (instead of also having "'none'").
- Allows the use of NEXT in CHANGELOG as a way of gathering changes that are worth noting, but not
  doing a publish cycle for. (Replaces the plan of using -dev versions, since that's actually harder to implement,
  and more confusing.)
  - Ensures that we don't forget to clean up NEXT entries when bumping versions

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`.
+  /// Get the package version specified in the pubspec file in `pubspecPath` and
-  Future<Version> getPackageVersion(String pubspecPath, String gitRef) async {
+  /// at the revision of `gitRef` (defaulting to the base if not provided).
-    final io.ProcessResult gitShow =
+  Future<Version> getPackageVersion(String pubspecPath, {String gitRef}) async {
-        await baseGitDir.runCommand(<String>['show', '$gitRef:$pubspecPath']);
+    final String ref = gitRef ?? (await _getBaseSha());
+
+    io.ProcessResult gitShow;
+    try {
+      gitShow =
+          await baseGitDir.runCommand(<String>['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);

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,49 +94,60 @@ class VersionCheckCommand extends PluginCommand {
     final List<String> changedPubspecs =
         await gitVersionFinder.getChangedPubSpecs();
 
-    final String baseSha = argResults[_kBaseSha] as String;
+    const String indentation = '  ';
     for (final String pubspecPath in changedPubspecs) {
-      try {
+      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');
+          await gitVersionFinder.getPackageVersion(pubspecPath, gitRef: 'HEAD');
       if (headVersion == null) {
-          continue; // Example apps don't have versions
+        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.');
+      }
+
+      if (masterVersion == headVersion) {
+        print('${indentation}No version change.');
+        continue;
       }
 
       final Map<Version, NextVersionType> allowedNextVersions =
           getAllowedNextVersions(masterVersion, headVersion);
 
       if (!allowedNextVersions.containsKey(headVersion)) {
-          final String error = '$pubspecPath incorrectly updated version.\n'
+        final String error = '${indentation}Incorrectly updated version.\n'
-              'HEAD: $headVersion, master: $masterVersion.\n'
+            '${indentation}HEAD: $headVersion, master: $masterVersion.\n'
-              'Allowed versions: $allowedNextVersions';
+            '${indentation}Allowed versions: $allowedNextVersions';
         printErrorAndExit(errorMessage: error);
+      } else {
+        print('$indentation$headVersion -> $masterVersion');
       }
 
       final bool isPlatformInterface =
           pubspec.name.endsWith('_platform_interface');
       if (isPlatformInterface &&
-            allowedNextVersions[headVersion] ==
+          allowedNextVersions[headVersion] == NextVersionType.BREAKING_MAJOR) {
-                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.');
-      }
     }
 
     await for (final Directory plugin in getPlugins()) {
@@ -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<String> 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');
   }
 

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"

script/tool/test/version_check_test.dart

@@ -62,6 +62,8 @@ void main() {
           final String response =
               gitShowResponses[invocation.positionalArguments[0][1]];
           when<String>(mockProcessResult.stdout as String).thenReturn(response);
+        } else if (invocation.positionalArguments[0][0] == 'merge-base') {
+          when<String>(mockProcessResult.stdout as String).thenReturn('abc123');
         }
         return Future<io.ProcessResult>.value(mockProcessResult);
       });
@@ -98,11 +100,12 @@ void main() {
       );
       expect(gitDirCommands.length, equals(3));
       expect(
-          gitDirCommands[0].join(' '), equals('diff --name-only master HEAD'));
+          gitDirCommands,
-      expect(gitDirCommands[1].join(' '),
+          containsAll(<Matcher>[
-          equals('show master:packages/plugin/pubspec.yaml'));
+            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
-      expect(gitDirCommands[2].join(' '),
+            equals(<String>['show', 'master:packages/plugin/pubspec.yaml']),
-          equals('show HEAD:packages/plugin/pubspec.yaml'));
+            equals(<String>['show', 'HEAD:packages/plugin/pubspec.yaml']),
+          ]));
     });
 
     test('denies invalid version', () async {
@@ -117,15 +120,50 @@ void main() {
 
       await expectLater(
         result,
-        throwsA(const TypeMatcher<Error>()),
+        throwsA(const TypeMatcher<ToolExit>()),
       );
       expect(gitDirCommands.length, equals(3));
       expect(
-          gitDirCommands[0].join(' '), equals('diff --name-only master HEAD'));
+          gitDirCommands,
-      expect(gitDirCommands[1].join(' '),
+          containsAll(<Matcher>[
-          equals('show master:packages/plugin/pubspec.yaml'));
+            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
-      expect(gitDirCommands[2].join(' '),
+            equals(<String>['show', 'master:packages/plugin/pubspec.yaml']),
-          equals('show HEAD:packages/plugin/pubspec.yaml'));
+            equals(<String>['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 = <String, String>{
+        'abc123:packages/plugin/pubspec.yaml': 'version: 1.0.0',
+        'HEAD:packages/plugin/pubspec.yaml': 'version: 2.0.0',
+      };
+      final List<String> output =
+          await runCapturingPrint(runner, <String>['version-check']);
+
+      expect(
+        output,
+        containsAllInOrder(<String>[
+          '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 = <String, String>{
+        'abc123:packages/plugin/pubspec.yaml': 'version: 0.0.1',
+        'HEAD:packages/plugin/pubspec.yaml': 'version: 0.2.0',
+      };
+      final Future<List<String>> result =
+          runCapturingPrint(runner, <String>['version-check']);
+
+      await expectLater(
+        result,
+        throwsA(const TypeMatcher<ToolExit>()),
+      );
     });
 
     test('gracefully handles missing pubspec.yaml', () async {
@@ -143,6 +181,8 @@ void main() {
         output,
         orderedEquals(<String>[
           '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'));
+          gitDirCommands,
-      expect(
+          containsAll(<Matcher>[
-          gitDirCommands[1].join(' '),
+            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
-          equals(
+            equals(<String>[
-              'show master:packages/plugin_platform_interface/pubspec.yaml'));
+              'show',
-      expect(gitDirCommands[2].join(' '),
+              'master:packages/plugin_platform_interface/pubspec.yaml'
-          equals('show HEAD:packages/plugin_platform_interface/pubspec.yaml'));
+            ]),
+            equals(<String>[
+              'show',
+              'HEAD:packages/plugin_platform_interface/pubspec.yaml'
+            ]),
+          ]));
     });
 
     test('disallows breaking changes to platform interfaces', () async {
@@ -194,17 +239,22 @@ void main() {
           runner, <String>['version-check', '--base-sha=master']);
       await expectLater(
         output,
-        throwsA(const TypeMatcher<Error>()),
+        throwsA(const TypeMatcher<ToolExit>()),
       );
       expect(gitDirCommands.length, equals(3));
       expect(
-          gitDirCommands[0].join(' '), equals('diff --name-only master HEAD'));
+          gitDirCommands,
-      expect(
+          containsAll(<Matcher>[
-          gitDirCommands[1].join(' '),
+            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
-          equals(
+            equals(<String>[
-              'show master:packages/plugin_platform_interface/pubspec.yaml'));
+              'show',
-      expect(gitDirCommands[2].join(' '),
+              'master:packages/plugin_platform_interface/pubspec.yaml'
-          equals('show HEAD:packages/plugin_platform_interface/pubspec.yaml'));
+            ]),
+            equals(<String>[
+              '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(<String>[
-          '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, <String>['version-check', '--base-sha=master']);
       await expectLater(
         output,
-        throwsA(const TypeMatcher<Error>()),
+        throwsA(const TypeMatcher<ToolExit>()),
       );
       try {
         final List<String> outputValue = await output;
@@ -291,7 +341,7 @@ void main() {
       expect(
         output,
         containsAllInOrder(<String>[
-          '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, <String>['version-check', '--base-sha=master']);
       await expectLater(
         output,
-        throwsA(const TypeMatcher<Error>()),
+        throwsA(const TypeMatcher<ToolExit>()),
+      );
+      try {
+        final List<String> outputValue = await output;
+        await expectLater(
+          outputValue,
+          containsAllInOrder(<String>[
+            '''
+  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<String> output = await runCapturingPrint(
+          runner, <String>['version-check', '--base-sha=master']);
+      await expectLater(
+        output,
+        containsAllInOrder(<String>[
+          '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<List<String>> output = runCapturingPrint(
+          runner, <String>['version-check', '--base-sha=master']);
+      await expectLater(
+        output,
+        throwsA(const TypeMatcher<ToolExit>()),
+      );
+      try {
+        final List<String> outputValue = await output;
+        await expectLater(
+          outputValue,
+          containsAllInOrder(<String>[
+            '''
+  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<List<String>> output = runCapturingPrint(
+          runner, <String>['version-check', '--base-sha=master']);
+      await expectLater(
+        output,
+        throwsA(const TypeMatcher<ToolExit>()),
       );
       try {
         final List<String> outputValue = await output;