diff --git a/script/tool/lib/src/common/core.dart b/script/tool/lib/src/common/core.dart index 476106c924ae..63208d23f7e4 100644 --- a/script/tool/lib/src/common/core.dart +++ b/script/tool/lib/src/common/core.dart @@ -33,6 +33,10 @@ const String platformWindows = 'windows'; /// Key for enable experiment. const String kEnableExperiment = 'enable-experiment'; +/// A String to add to comments on temporarily-added changes that should not +/// land (e.g., dependency overrides in federated plugin combination PRs). +const String kDoNotLandWarning = 'DO NOT MERGE'; + /// Target platforms supported by Flutter. // ignore: public_member_api_docs enum FlutterPlatform { android, ios, linux, macos, web, windows } diff --git a/script/tool/lib/src/federation_safety_check_command.dart b/script/tool/lib/src/federation_safety_check_command.dart index 804ef8e3abc9..477fe4a2f61c 100644 --- a/script/tool/lib/src/federation_safety_check_command.dart +++ b/script/tool/lib/src/federation_safety_check_command.dart @@ -6,6 +6,7 @@ import 'package:file/file.dart'; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; +import 'common/core.dart'; import 'common/file_utils.dart'; import 'common/git_version_finder.dart'; import 'common/output_utils.dart'; @@ -112,6 +113,21 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand { 'Platform interface changes are not validated.'); } + // Special-case combination PRs that are following repo process, so that + // they don't get an error that makes it sound like something is wrong with + // the PR (but is still an error so that the PR can't land without following + // the resolution process). + if (package.getExamples().any(_hasTemporaryDependencyOverrides)) { + printError('"$kDoNotLandWarning" found in pubspec.yaml, so this is ' + 'assumed to be the initial combination PR for a federated change, ' + 'following the standard repository procedure. This failure is ' + 'expected, in order to prevent accidentally landing the temporary ' + 'overrides, and will automatically be resolved when the temporary ' + 'overrides are replaced by dependency version bumps later in the ' + 'process.'); + return PackageResult.fail(['Unresolved combo PR.']); + } + // Uses basename to match _changedPackageFiles. final String basePackageName = package.directory.parent.basename; final String platformInterfacePackageName = @@ -216,4 +232,9 @@ class FederationSafetyCheckCommand extends PackageLoopingCommand { // against having the wrong (e.g., incorrectly empty) diff output. return foundComment; } + + bool _hasTemporaryDependencyOverrides(RepositoryPackage package) { + final String pubspecContents = package.pubspecFile.readAsStringSync(); + return pubspecContents.contains(kDoNotLandWarning); + } } diff --git a/script/tool/lib/src/make_deps_path_based_command.dart b/script/tool/lib/src/make_deps_path_based_command.dart index 7e5c095fb53a..b3682ea6f132 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -57,7 +57,7 @@ class MakeDepsPathBasedCommand extends PackageCommand { // Includes a reference to the docs so that reviewers who aren't familiar with // the federated plugin change process don't think it's a mistake. static const String _dependencyOverrideWarningComment = - '# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.\n' + '# FOR TESTING AND INITIAL REVIEW ONLY. $kDoNotLandWarning.\n' '# See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins'; @override diff --git a/script/tool/test/federation_safety_check_command_test.dart b/script/tool/test/federation_safety_check_command_test.dart index ac80173cde15..ca4b018a811b 100644 --- a/script/tool/test/federation_safety_check_command_test.dart +++ b/script/tool/test/federation_safety_check_command_test.dart @@ -262,6 +262,89 @@ index abc123..def456 100644 ); }); + test('fails with specific text for combo PRs using the recommended tooling', + () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final RepositoryPackage appFacing = createFakePlugin('foo', pluginGroupDir); + final RepositoryPackage implementation = + createFakePlugin('foo_bar', pluginGroupDir); + final RepositoryPackage platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + void addFakeTempPubspecOverrides(RepositoryPackage package) { + final String contents = package.pubspecFile.readAsStringSync(); + package.pubspecFile.writeAsStringSync(''' +$contents + +# FOR TESTING AND INITIAL REVIEW ONLY. $kDoNotLandWarning. +dependency_overrides: + foo_platform_interface: + path: ../../../foo/foo_platform_interface +'''); + } + + addFakeTempPubspecOverrides(appFacing.getExamples().first); + addFakeTempPubspecOverrides(implementation.getExamples().first); + + const String appFacingChanges = ''' +diff --git a/packages/foo/foo/lib/foo.dart b/packages/foo/foo/lib/foo.dart +index abc123..def456 100644 +--- a/packages/foo/foo/lib/foo.dart ++++ b/packages/foo/foo/lib/foo.dart +@@ -51,6 +51,9 @@ Future launchUrl( + return true; + } + ++// This is a new method ++bool foo() => true; ++ + // This in an existing method + void aMethod() { + // Do things. +'''; + + final String changedFileOutput = [ + appFacing.libDirectory.childFile('foo.dart'), + implementation.libDirectory.childFile('foo.dart'), + platformInterface.pubspecFile, + platformInterface.libDirectory.childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo(MockProcess(stdout: changedFileOutput)), + FakeProcessInfo(MockProcess(stdout: appFacingChanges), + ['', 'HEAD', '--', '/packages/foo/foo/lib/foo.dart']), + // The others diffs don't need to be specified, since empty diff is also + // treated as a non-comment change. + ]; + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['federation-safety-check'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Running for foo/foo...'), + contains('"DO NOT MERGE" found in pubspec.yaml, so this is assumed to ' + 'be the initial combination PR for a federated change, following ' + 'the standard repository procedure. This failure is expected, in ' + 'order to prevent accidentally landing the temporary overrides, ' + 'and will automatically be resolved when the temporary overrides ' + 'are replaced by dependency version bumps later in the process.'), + contains('Running for foo_bar...'), + contains('"DO NOT MERGE" found in pubspec.yaml'), + contains('The following packages had errors:'), + contains('foo/foo:\n' + ' Unresolved combo PR.'), + contains('foo_bar:\n' + ' Unresolved combo PR.'), + ]), + ); + }); + test('ignores test-only changes to interface packages', () async { final Directory pluginGroupDir = packagesDir.childDirectory('foo'); final RepositoryPackage appFacing = createFakePlugin('foo', pluginGroupDir);