diff --git a/.cirrus.yml b/.cirrus.yml index 8dcb4d96d2be..67343ee15f88 100644 --- a/.cirrus.yml +++ b/.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 diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 7e9cd3bec938..2bc7a901a9a2 100644 --- a/script/tool/CHANGELOG.md +++ b/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 diff --git a/script/tool/lib/src/common/repository_package.dart b/script/tool/lib/src/common/repository_package.dart index feece7c1cdff..cb586afb4dfe 100644 --- a/script/tool/lib/src/common/repository_package.dart +++ b/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 getExamples() { final Directory exampleDirectory = directory.childDirectory('example'); diff --git a/script/tool/lib/src/drive_examples_command.dart b/script/tool/lib/src/drive_examples_command.dart index b3434b0659f3..593e557fa395 100644 --- a/script/tool/lib/src/drive_examples_command.dart +++ b/script/tool/lib/src/drive_examples_command.dart @@ -133,7 +133,7 @@ class DriveExamplesCommand extends PackageLoopingCommand { @override Future 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. diff --git a/script/tool/lib/src/federation_safety_check_command.dart b/script/tool/lib/src/federation_safety_check_command.dart index fd53d6cbaa67..200f9c3f48cb 100644 --- a/script/tool/lib/src/federation_safety_check_command.dart +++ b/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( diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart index 6b49c40d66bb..528251fbf80d 100644 --- a/script/tool/lib/src/version_check_command.dart +++ b/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(); + } } diff --git a/script/tool/test/version_check_command_test.dart b/script/tool/test/version_check_command_test.dart index 9ab7c57089a3..7d59dbb3ee7e 100644 --- a/script/tool/test/version_check_command_test.dart +++ b/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 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 = { @@ -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 = { + '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 output = await runCapturingPrint(runner, [ + 'version-check', + '--base-sha=master', + '--change-description-file=${changeDescriptionFile.path}' + ]); + + expect( + output, + containsAllInOrder([ + 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 = { + 'master:packages/plugin_platform_interface/pubspec.yaml': + 'version: 1.0.0', + }; + + Error? commandError; + final List output = await runCapturingPrint(runner, [ + 'version-check', + '--base-sha=master', + '--change-description-file=a_missing_file.txt' + ], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + 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 = { + 'master:packages/plugin_platform_interface/pubspec.yaml': + 'version: 1.0.0', + }; + final List output = await runCapturingPrint(runner, [ + 'version-check', + '--base-sha=master', + '--ignore-platform-interface-breaks' + ]); + + expect( + output, + containsAllInOrder([ + 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';