[flutter_plugin_tools] Allow overriding breaking change check (#4369)

Author: stuartmorgan-g

.cirrus.yml

@@ -68,7 +68,21 @@ task:
         - cd script/tool
         - dart pub run test
     - name: publishable
-      version_check_script: ./script/tool_runner.sh version-check
+      env:
+        CHANGE_DESC: "$TMPDIR/change-description.txt"
+      version_check_script:
+        # For pre-submit, pass the PR description to the script to allow for
+        # platform version breaking version change justifications.
+        # For post-submit, ignore platform version breaking version changes.
+        # The PR description isn't reliably part of the commit message, so using
+        # the same flags as for presubmit would likely result in false-positive
+        # post-submit failures.
+        - if [[ $CIRRUS_PR == "" ]]; then
+        -   ./script/tool_runner.sh version-check --ignore-platform-interface-breaks
+        - else
+        -   echo "$CIRRUS_CHANGE_MESSAGE" > "$CHANGE_DESC"
+        -   ./script/tool_runner.sh version-check --change-description-file="$CHANGE_DESC"
+        - fi
       publish_check_script: ./script/tool_runner.sh publish-check
     - name: format
       format_script: ./script/tool_runner.sh format --fail-on-change

script/tool/CHANGELOG.md

@@ -6,6 +6,8 @@
   federated packages that have been done in such a way that they will pass in
   CI, but fail once the change is landed and published.
 - `publish-check` now validates that there is an `AUTHORS` file.
+- Added flags to `version-check` to allow overriding the platform interface
+  major version change restriction.
 
 ## 0.7.1
 

script/tool/lib/src/common/repository_package.dart

@@ -53,6 +53,11 @@ class RepositoryPackage {
       directory.parent.basename != 'packages' &&
       directory.basename.startsWith(directory.parent.basename);
 
+  /// True if this appears to be a platform interface package, according to
+  /// repository conventions.
+  bool get isPlatformInterface =>
+      directory.basename.endsWith('_platform_interface');
+
   /// Returns the Flutter example packages contained in the package, if any.
   Iterable<RepositoryPackage> getExamples() {
     final Directory exampleDirectory = directory.childDirectory('example');

script/tool/lib/src/drive_examples_command.dart

@@ -133,7 +133,7 @@ class DriveExamplesCommand extends PackageLoopingCommand {
 
   @override
   Future<PackageResult> runForPackage(RepositoryPackage package) async {
-    if (package.directory.basename.endsWith('_platform_interface') &&
+    if (package.isPlatformInterface &&
         !package.getSingleExampleDeprecated().directory.existsSync()) {
       // Platform interface packages generally aren't intended to have
       // examples, and don't need integration tests, so skip rather than fail.

script/tool/lib/src/federation_safety_check_command.dart

@@ -109,7 +109,7 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand {
       return PackageResult.skip('Not a federated plugin.');
     }
 
-    if (package.directory.basename.endsWith('_platform_interface')) {
+    if (package.isPlatformInterface) {
       // As the leaf nodes in the graph, a published package interface change is
       // assumed to be correct, and other changes are validated against that.
       return PackageResult.skip(

script/tool/lib/src/version_check_command.dart

@@ -18,6 +18,8 @@ import 'common/process_runner.dart';
 import 'common/pub_version_finder.dart';
 import 'common/repository_package.dart';
 
+const int _exitMissingChangeDescriptionFile = 3;
+
 /// Categories of version change types.
 enum NextVersionType {
   /// A breaking change.
@@ -108,13 +110,36 @@ class VersionCheckCommand extends PackageLoopingCommand {
     argParser.addFlag(
       _againstPubFlag,
       help: 'Whether the version check should run against the version on pub.\n'
-          'Defaults to false, which means the version check only run against the previous version in code.',
+          'Defaults to false, which means the version check only run against '
+          'the previous version in code.',
       defaultsTo: false,
       negatable: true,
     );
+    argParser.addOption(_changeDescriptionFile,
+        help: 'The path to a file containing the description of the change '
+            '(e.g., PR description or commit message).\n\n'
+            'If supplied, this is used to allow overrides to some version '
+            'checks.');
+    argParser.addFlag(_ignorePlatformInterfaceBreaks,
+        help: 'Bypasses the check that platform interfaces do not contain '
+            'breaking changes.\n\n'
+            'This is only intended for use in post-submit CI checks, to '
+            'prevent the possibility of post-submit breakage if a change '
+            'description justification is not transferred into the commit '
+            'message. Pre-submit checks should always use '
+            '--$_changeDescriptionFile instead.',
+        hide: true);
   }
 
   static const String _againstPubFlag = 'against-pub';
+  static const String _changeDescriptionFile = 'change-description-file';
+  static const String _ignorePlatformInterfaceBreaks =
+      'ignore-platform-interface-breaks';
+
+  /// The string that must be in [_changeDescriptionFile] to allow a breaking
+  /// change to a platform interface.
+  static const String _breakingChangeJustificationMarker =
+      '## Breaking change justification';
 
   final PubVersionFinder _pubVersionFinder;
 
@@ -292,16 +317,17 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
       return _CurrentVersionState.invalidChange;
     }
 
-    final bool isPlatformInterface =
-        pubspec.name.endsWith('_platform_interface');
-    // TODO(stuartmorgan): Relax this check. See
-    // https://github.com/flutter/flutter/issues/85391
-    if (isPlatformInterface &&
-        allowedNextVersions[currentVersion] == NextVersionType.BREAKING_MAJOR) {
+    if (allowedNextVersions[currentVersion] == NextVersionType.BREAKING_MAJOR &&
+        !_validateBreakingChange(package)) {
       printError('${indentation}Breaking change detected.\n'
-          '${indentation}Breaking changes to platform interfaces are strongly discouraged.\n');
+          '${indentation}Breaking changes to platform interfaces are not '
+          'allowed without explicit justification.\n'
+          '${indentation}See '
+          'https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages '
+          'for more information.');
       return _CurrentVersionState.invalidChange;
     }
+
     return _CurrentVersionState.validChange;
   }
 
@@ -398,4 +424,45 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
       return null;
     }
   }
+
+  /// 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 (_getChangeDescription().contains(_breakingChangeJustificationMarker)) {
+      logWarning(
+          '${indentation}Allowing breaking change to ${package.displayName} '
+          'due to "$_breakingChangeJustificationMarker" in the change '
+          'description.');
+      return true;
+    }
+
+    return false;
+  }
+
+  /// Returns the contents of the file pointed to by [_changeDescriptionFile],
+  /// or an empty string if that flag is not provided.
+  String _getChangeDescription() {
+    final String path = getStringArg(_changeDescriptionFile);
+    if (path.isEmpty) {
+      return '';
+    }
+    final File file = packagesDir.fileSystem.file(path);
+    if (!file.existsSync()) {
+      printError('${indentation}No such file: $path');
+      throw ToolExit(_exitMissingChangeDescriptionFile);
+    }
+    return file.readAsStringSync();
+  }
 }

script/tool/test/version_check_command_test.dart

@@ -46,7 +46,7 @@ class MockProcessResult extends Mock implements io.ProcessResult {}
 void main() {
   const String indentation = '  ';
   group('$VersionCheckCommand', () {
-    FileSystem fileSystem;
+    late FileSystem fileSystem;
     late MockPlatform mockPlatform;
     late Directory packagesDir;
     late CommandRunner<void> runner;
@@ -252,7 +252,8 @@ void main() {
           ]));
     });
 
-    test('disallows breaking changes to platform interfaces', () async {
+    test('disallows breaking changes to platform interfaces by default',
+        () async {
       createFakePlugin('plugin_platform_interface', packagesDir,
           version: '2.0.0');
       gitShowResponses = <String, String>{
@@ -276,6 +277,92 @@ void main() {
           ]));
     });
 
+    test('allows breaking changes to platform interfaces with explanation',
+        () async {
+      createFakePlugin('plugin_platform_interface', packagesDir,
+          version: '2.0.0');
+      gitShowResponses = <String, String>{
+        'master:packages/plugin_platform_interface/pubspec.yaml':
+            'version: 1.0.0',
+      };
+      final File changeDescriptionFile =
+          fileSystem.file('change_description.txt');
+      changeDescriptionFile.writeAsStringSync('''
+Some general PR description
+
+## Breaking change justification
+
+This is necessary because of X, Y, and Z
+
+## Another section''');
+      final List<String> output = await runCapturingPrint(runner, <String>[
+        'version-check',
+        '--base-sha=master',
+        '--change-description-file=${changeDescriptionFile.path}'
+      ]);
+
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Allowing breaking change to plugin_platform_interface '
+              'due to "## Breaking change justification" in the change '
+              'description.'),
+          contains('Ran for 1 package(s) (1 with warnings)'),
+        ]),
+      );
+    });
+
+    test('throws if a nonexistent change description file is specified',
+        () async {
+      createFakePlugin('plugin_platform_interface', packagesDir,
+          version: '2.0.0');
+      gitShowResponses = <String, String>{
+        'master:packages/plugin_platform_interface/pubspec.yaml':
+            'version: 1.0.0',
+      };
+
+      Error? commandError;
+      final List<String> output = await runCapturingPrint(runner, <String>[
+        'version-check',
+        '--base-sha=master',
+        '--change-description-file=a_missing_file.txt'
+      ], errorHandler: (Error e) {
+        commandError = e;
+      });
+
+      expect(commandError, isA<ToolExit>());
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('No such file: a_missing_file.txt'),
+        ]),
+      );
+    });
+
+    test('allows breaking changes to platform interfaces with bypass flag',
+        () async {
+      createFakePlugin('plugin_platform_interface', packagesDir,
+          version: '2.0.0');
+      gitShowResponses = <String, String>{
+        'master:packages/plugin_platform_interface/pubspec.yaml':
+            'version: 1.0.0',
+      };
+      final List<String> output = await runCapturingPrint(runner, <String>[
+        'version-check',
+        '--base-sha=master',
+        '--ignore-platform-interface-breaks'
+      ]);
+
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Allowing breaking change to plugin_platform_interface due '
+              'to --ignore-platform-interface-breaks'),
+          contains('Ran for 1 package(s) (1 with warnings)'),
+        ]),
+      );
+    });
+
     test('Allow empty lines in front of the first version in CHANGELOG',
         () async {
       const String version = '1.0.1';