diff --git a/.cirrus.yml b/.cirrus.yml index 0b3f07da48e1..8dcb4d96d2be 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -74,6 +74,11 @@ task: format_script: ./script/tool_runner.sh format --fail-on-change pubspec_script: ./script/tool_runner.sh pubspec-check license_script: dart $PLUGIN_TOOL license-check + - name: federated_safety + # This check is only meaningful for PRs, as it validates changes + # rather than state. + only_if: $CIRRUS_PR != "" + script: ./script/tool_runner.sh federation-safety-check - name: dart_unit_tests env: matrix: diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 7df6913db7d1..3be9173a505b 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -2,6 +2,9 @@ - `native-test --android`, `--ios`, and `--macos` now fail plugins that don't have unit tests, rather than skipping them. +- Added a new `federation-safety-check` command to help catch changes to + 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. ## 0.7.1 diff --git a/script/tool/lib/src/common/git_version_finder.dart b/script/tool/lib/src/common/git_version_finder.dart index a0a7a32b5e0f..1cdd2fcc409b 100644 --- a/script/tool/lib/src/common/git_version_finder.dart +++ b/script/tool/lib/src/common/git_version_finder.dart @@ -58,6 +58,9 @@ class GitVersionFinder { return null; } final String fileContent = gitShow.stdout as String; + if (fileContent.trim().isEmpty) { + return null; + } final String? versionString = loadYaml(fileContent)['version'] as String?; return versionString == null ? null : Version.parse(versionString); } diff --git a/script/tool/lib/src/common/repository_package.dart b/script/tool/lib/src/common/repository_package.dart index f6601d39b79e..feece7c1cdff 100644 --- a/script/tool/lib/src/common/repository_package.dart +++ b/script/tool/lib/src/common/repository_package.dart @@ -47,6 +47,12 @@ class RepositoryPackage { /// The package's top-level pubspec.yaml. File get pubspecFile => directory.childFile('pubspec.yaml'); + /// True if this appears to be a federated plugin package, according to + /// repository conventions. + bool get isFederated => + directory.parent.basename != 'packages' && + directory.basename.startsWith(directory.parent.basename); + /// 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/federation_safety_check_command.dart b/script/tool/lib/src/federation_safety_check_command.dart new file mode 100644 index 000000000000..cb0da162e604 --- /dev/null +++ b/script/tool/lib/src/federation_safety_check_command.dart @@ -0,0 +1,188 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:file/file.dart'; +import 'package:flutter_plugin_tools/src/common/plugin_utils.dart'; +import 'package:git/git.dart'; +import 'package:path/path.dart' as p; +import 'package:platform/platform.dart'; +import 'package:pub_semver/pub_semver.dart'; +import 'package:pubspec_parse/pubspec_parse.dart'; + +import 'common/core.dart'; +import 'common/file_utils.dart'; +import 'common/git_version_finder.dart'; +import 'common/package_looping_command.dart'; +import 'common/process_runner.dart'; +import 'common/repository_package.dart'; + +/// A command to check that PRs don't violate repository best practices that +/// have been established to avoid breakages that building and testing won't +/// catch. +class FederationSafetyCheckCommand extends PackageLoopingCommand { + /// Creates an instance of the safety check command. + FederationSafetyCheckCommand( + Directory packagesDir, { + ProcessRunner processRunner = const ProcessRunner(), + Platform platform = const LocalPlatform(), + GitDir? gitDir, + }) : super( + packagesDir, + processRunner: processRunner, + platform: platform, + gitDir: gitDir, + ); + + // A map of package name (as defined by the directory name of the package) + // to a list of changed Dart files in that package, as Posix paths relative to + // the package root. + // + // This only considers top-level packages, not subpackages such as example/. + final Map> _changedDartFiles = >{}; + + // The set of *_platform_interface packages that will have public code changes + // published. + final Set _modifiedAndPublishedPlatformInterfacePackages = {}; + + // The set of conceptual plugins (not packages) that have changes. + final Set _changedPlugins = {}; + + static const String _platformInterfaceSuffix = '_platform_interface'; + + @override + final String name = 'federation-safety-check'; + + @override + final String description = + 'Checks that the change does not violate repository rules around changes ' + 'to federated plugin packages.'; + + @override + bool get hasLongOutput => false; + + @override + Future initializeRun() async { + final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); + final String baseSha = await gitVersionFinder.getBaseSha(); + print('Validating changes relative to "$baseSha"\n'); + for (final String path in await gitVersionFinder.getChangedFiles()) { + // Git output always uses Posix paths. + final List allComponents = p.posix.split(path); + final int packageIndex = allComponents.indexOf('packages'); + if (packageIndex == -1) { + continue; + } + final List relativeComponents = + allComponents.sublist(packageIndex + 1); + // The package name is either the directory directly under packages/, or + // the directory under that in the case of a federated plugin. + String packageName = relativeComponents.removeAt(0); + // Count the top-level plugin as changed. + _changedPlugins.add(packageName); + if (relativeComponents[0] == packageName || + relativeComponents[0].startsWith('${packageName}_')) { + packageName = relativeComponents.removeAt(0); + } + + if (relativeComponents.last.endsWith('.dart')) { + _changedDartFiles[packageName] ??= []; + _changedDartFiles[packageName]! + .add(p.posix.joinAll(relativeComponents)); + } + + if (packageName.endsWith(_platformInterfaceSuffix) && + relativeComponents.first == 'pubspec.yaml' && + await _packageWillBePublished(path)) { + _modifiedAndPublishedPlatformInterfacePackages.add(packageName); + } + } + } + + @override + Future runForPackage(RepositoryPackage package) async { + if (!isFlutterPlugin(package)) { + return PackageResult.skip('Not a plugin.'); + } + + if (!package.isFederated) { + return PackageResult.skip('Not a federated plugin.'); + } + + if (package.directory.basename.endsWith('_platform_interface')) { + // 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( + 'Platform interface changes are not validated.'); + } + + // Uses basename to match _changedPackageFiles. + final String basePackageName = package.directory.parent.basename; + final String platformInterfacePackageName = + '$basePackageName$_platformInterfaceSuffix'; + final List changedPlatformInterfaceFiles = + _changedDartFiles[platformInterfacePackageName] ?? []; + + if (!_modifiedAndPublishedPlatformInterfacePackages + .contains(platformInterfacePackageName)) { + print('No published changes for $platformInterfacePackageName.'); + return PackageResult.success(); + } + + if (!changedPlatformInterfaceFiles + .any((String path) => path.startsWith('lib/'))) { + print('No public code changes for $platformInterfacePackageName.'); + return PackageResult.success(); + } + + // If the change would be flagged, but it appears to be a mass change + // rather than a plugin-specific change, allow it with a warning. + // + // This is a tradeoff between safety and convenience; forcing mass changes + // to be split apart is not ideal, and the assumption is that reviewers are + // unlikely to accidentally approve a PR that is supposed to be changing a + // single plugin, but touches other plugins (vs accidentally approving a + // PR that changes multiple parts of a single plugin, which is a relatively + // easy mistake to make). + // + // 3 is chosen to minimize the chances of accidentally letting something + // through (vs 2, which could be a single-plugin change with one stray + // change to another file accidentally included), while not setting too + // high a bar for detecting mass changes. This can be tuned if there are + // issues with false positives or false negatives. + const int massChangePluginThreshold = 3; + if (_changedPlugins.length >= massChangePluginThreshold) { + logWarning('Ignoring potentially dangerous change, as this appears ' + 'to be a mass change.'); + return PackageResult.success(); + } + + printError('Dart changes are not allowed to other packages in ' + '$basePackageName in the same PR as changes to public Dart code in ' + '$platformInterfacePackageName, as this can cause accidental breaking ' + 'changes to be missed by automated checks. Please split the changes to ' + 'these two packages into separate PRs.\n\n' + 'If you believe that this is a false positive, please file a bug.'); + return PackageResult.fail( + ['$platformInterfacePackageName changed.']); + } + + Future _packageWillBePublished( + String pubspecRepoRelativePosixPath) async { + final File pubspecFile = childFileWithSubcomponents( + packagesDir.parent, p.posix.split(pubspecRepoRelativePosixPath)); + final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); + if (pubspec.publishTo == 'none') { + return false; + } + + final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); + final Version? previousVersion = + await gitVersionFinder.getPackageVersion(pubspecRepoRelativePosixPath); + if (previousVersion == null) { + // The plugin is new, so it will be published. + return true; + } + return pubspec.version != previousVersion; + } +} diff --git a/script/tool/lib/src/main.dart b/script/tool/lib/src/main.dart index e70cba24cc5e..70a6ab516037 100644 --- a/script/tool/lib/src/main.dart +++ b/script/tool/lib/src/main.dart @@ -13,6 +13,7 @@ import 'build_examples_command.dart'; import 'common/core.dart'; import 'create_all_plugins_app_command.dart'; import 'drive_examples_command.dart'; +import 'federation_safety_check_command.dart'; import 'firebase_test_lab_command.dart'; import 'format_command.dart'; import 'license_check_command.dart'; @@ -49,6 +50,7 @@ void main(List args) { ..addCommand(BuildExamplesCommand(packagesDir)) ..addCommand(CreateAllPluginsAppCommand(packagesDir)) ..addCommand(DriveExamplesCommand(packagesDir)) + ..addCommand(FederationSafetyCheckCommand(packagesDir)) ..addCommand(FirebaseTestLabCommand(packagesDir)) ..addCommand(FormatCommand(packagesDir)) ..addCommand(LicenseCheckCommand(packagesDir)) diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart index 29f9ea733a03..605a8aa83a30 100644 --- a/script/tool/lib/src/pubspec_check_command.dart +++ b/script/tool/lib/src/pubspec_check_command.dart @@ -210,17 +210,11 @@ class PubspecCheckCommand extends PackageLoopingCommand { // Returns true if [packageName] appears to be an implementation package // according to repository conventions. bool _isImplementationPackage(RepositoryPackage package) { - // An implementation package should be in a group folder... - final Directory parentDir = package.directory.parent; - if (parentDir.path == packagesDir.path) { + if (!package.isFederated) { return false; } final String packageName = package.directory.basename; - final String parentName = parentDir.basename; - // ... whose name is a prefix of the package name. - if (!packageName.startsWith(parentName)) { - return false; - } + final String parentName = package.directory.parent.basename; // A few known package names are not implementation packages; assume // anything else is. (This is done instead of listing known implementation // suffixes to allow for non-standard suffixes; e.g., to put several diff --git a/script/tool/test/federation_safety_check_command_test.dart b/script/tool/test/federation_safety_check_command_test.dart new file mode 100644 index 000000000000..4ae3ec5c76d0 --- /dev/null +++ b/script/tool/test/federation_safety_check_command_test.dart @@ -0,0 +1,314 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:args/command_runner.dart'; +import 'package:file/file.dart'; +import 'package:file/memory.dart'; +import 'package:flutter_plugin_tools/src/common/core.dart'; +import 'package:flutter_plugin_tools/src/common/repository_package.dart'; +import 'package:flutter_plugin_tools/src/federation_safety_check_command.dart'; +import 'package:mockito/mockito.dart'; +import 'package:test/test.dart'; + +import 'common/plugin_command_test.mocks.dart'; +import 'mocks.dart'; +import 'util.dart'; + +void main() { + FileSystem fileSystem; + late MockPlatform mockPlatform; + late Directory packagesDir; + late CommandRunner runner; + late RecordingProcessRunner processRunner; + + setUp(() { + fileSystem = MemoryFileSystem(); + mockPlatform = MockPlatform(); + packagesDir = createPackagesDirectory(fileSystem: fileSystem); + + final MockGitDir gitDir = MockGitDir(); + when(gitDir.path).thenReturn(packagesDir.parent.path); + when(gitDir.runCommand(any, throwOnError: anyNamed('throwOnError'))) + .thenAnswer((Invocation invocation) { + final List arguments = + invocation.positionalArguments[0]! as List; + // Route git calls through the process runner, to make mock output + // consistent with other processes. Attach the first argument to the + // command to make targeting the mock results easier. + final String gitCommand = arguments.removeAt(0); + return processRunner.run('git-$gitCommand', arguments); + }); + + processRunner = RecordingProcessRunner(); + final FederationSafetyCheckCommand command = FederationSafetyCheckCommand( + packagesDir, + processRunner: processRunner, + platform: mockPlatform, + gitDir: gitDir); + + runner = CommandRunner('federation_safety_check_command', + 'Test for $FederationSafetyCheckCommand'); + runner.addCommand(command); + }); + + test('skips non-plugin packages', () async { + final Directory package = createFakePackage('foo', packagesDir); + + final String changedFileOutput = [ + package.childDirectory('lib').childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo...'), + contains('Not a plugin'), + contains('Skipped 1 package(s)'), + ]), + ); + }); + + test('skips unfederated plugins', () async { + final Directory package = createFakePlugin('foo', packagesDir); + + final String changedFileOutput = [ + package.childDirectory('lib').childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo...'), + contains('Not a federated plugin'), + contains('Skipped 1 package(s)'), + ]), + ); + }); + + test('skips interface packages', () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final Directory platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + final String changedFileOutput = [ + platformInterface.childDirectory('lib').childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo_platform_interface...'), + contains('Platform interface changes are not validated.'), + contains('Skipped 1 package(s)'), + ]), + ); + }); + + test('allows changes to multiple non-interface packages', () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final Directory appFacing = createFakePlugin('foo', pluginGroupDir); + final Directory implementation = + createFakePlugin('foo_bar', pluginGroupDir); + createFakePlugin('foo_platform_interface', pluginGroupDir); + + final String changedFileOutput = [ + appFacing.childFile('foo.dart'), + implementation.childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo/foo...'), + contains('No published changes for foo_platform_interface.'), + contains('Running for foo_bar...'), + contains('No published changes for foo_platform_interface.'), + ]), + ); + }); + + test( + 'fails on changes to interface and non-interface packages in the same plugin', + () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final Directory appFacing = createFakePlugin('foo', pluginGroupDir); + final Directory implementation = + createFakePlugin('foo_bar', pluginGroupDir); + final Directory platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + final String changedFileOutput = [ + appFacing.childFile('foo.dart'), + implementation.childFile('foo.dart'), + platformInterface.childFile('pubspec.yaml'), + platformInterface.childDirectory('lib').childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + 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('Dart changes are not allowed to other packages in foo in the ' + 'same PR as changes to public Dart code in foo_platform_interface, ' + 'as this can cause accidental breaking changes to be missed by ' + 'automated checks. Please split the changes to these two packages ' + 'into separate PRs.'), + contains('Running for foo_bar...'), + contains('Dart changes are not allowed to other packages in foo'), + contains('The following packages had errors:'), + contains('foo/foo:\n' + ' foo_platform_interface changed.'), + contains('foo_bar:\n' + ' foo_platform_interface changed.'), + ]), + ); + }); + + test('ignores test-only changes to interface packages', () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final Directory appFacing = createFakePlugin('foo', pluginGroupDir); + final Directory implementation = + createFakePlugin('foo_bar', pluginGroupDir); + final Directory platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + final String changedFileOutput = [ + appFacing.childFile('foo.dart'), + implementation.childFile('foo.dart'), + platformInterface.childFile('pubspec.yaml'), + platformInterface.childDirectory('test').childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo/foo...'), + contains('No public code changes for foo_platform_interface.'), + contains('Running for foo_bar...'), + contains('No public code changes for foo_platform_interface.'), + ]), + ); + }); + + test('ignores unpublished changes to interface packages', () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final Directory appFacing = createFakePlugin('foo', pluginGroupDir); + final Directory implementation = + createFakePlugin('foo_bar', pluginGroupDir); + final Directory platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + final String changedFileOutput = [ + appFacing.childFile('foo.dart'), + implementation.childFile('foo.dart'), + platformInterface.childFile('pubspec.yaml'), + platformInterface.childDirectory('lib').childFile('foo.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + // Simulate no change to the version in the interface's pubspec.yaml. + processRunner.mockProcessesForExecutable['git-show'] = [ + MockProcess( + stdout: RepositoryPackage(platformInterface) + .pubspecFile + .readAsStringSync()), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo/foo...'), + contains('No published changes for foo_platform_interface.'), + contains('Running for foo_bar...'), + contains('No published changes for foo_platform_interface.'), + ]), + ); + }); + + test('allows things that look like mass changes, with warning', () async { + final Directory pluginGroupDir = packagesDir.childDirectory('foo'); + final Directory appFacing = createFakePlugin('foo', pluginGroupDir); + final Directory implementation = + createFakePlugin('foo_bar', pluginGroupDir); + final Directory platformInterface = + createFakePlugin('foo_platform_interface', pluginGroupDir); + + final Directory otherPlugin1 = createFakePlugin('bar', packagesDir); + final Directory otherPlugin2 = createFakePlugin('baz', packagesDir); + + final String changedFileOutput = [ + appFacing.childFile('foo.dart'), + implementation.childFile('foo.dart'), + platformInterface.childFile('pubspec.yaml'), + platformInterface.childDirectory('lib').childFile('foo.dart'), + otherPlugin1.childFile('bar.dart'), + otherPlugin2.childFile('baz.dart'), + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + + final List output = + await runCapturingPrint(runner, ['federation-safety-check']); + + expect( + output, + containsAllInOrder([ + contains('Running for foo/foo...'), + contains( + 'Ignoring potentially dangerous change, as this appears to be a mass change.'), + contains('Running for foo_bar...'), + contains( + 'Ignoring potentially dangerous change, as this appears to be a mass change.'), + contains('Ran for 2 package(s) (2 with warnings)'), + ]), + ); + }); +}