From 2c9fad3dcbb8d7260077575d030590f6ac21b929 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Thu, 2 Dec 2021 10:49:20 -0500 Subject: [PATCH 01/11] Implement pathify --- .../ios_platform_images/analysis_options.yaml | 1 - .../lib/src/common/repository_package.dart | 9 + .../src/create_all_plugins_app_command.dart | 3 +- script/tool/lib/src/main.dart | 2 + script/tool/lib/src/pathify_command.dart | 232 ++++++++++++++++++ .../tool/lib/src/publish_check_command.dart | 7 +- .../tool/lib/src/publish_plugin_command.dart | 7 +- .../tool/lib/src/version_check_command.dart | 4 +- .../test/common/repository_package_test.dart | 20 ++ 9 files changed, 271 insertions(+), 14 deletions(-) delete mode 100644 packages/ios_platform_images/analysis_options.yaml create mode 100644 script/tool/lib/src/pathify_command.dart diff --git a/packages/ios_platform_images/analysis_options.yaml b/packages/ios_platform_images/analysis_options.yaml deleted file mode 100644 index cda4f6e153e6..000000000000 --- a/packages/ios_platform_images/analysis_options.yaml +++ /dev/null @@ -1 +0,0 @@ -include: ../../analysis_options_legacy.yaml diff --git a/script/tool/lib/src/common/repository_package.dart b/script/tool/lib/src/common/repository_package.dart index 3b4417ac8182..e0c4e4a83bfe 100644 --- a/script/tool/lib/src/common/repository_package.dart +++ b/script/tool/lib/src/common/repository_package.dart @@ -4,6 +4,7 @@ import 'package:file/file.dart'; import 'package:path/path.dart' as p; +import 'package:pubspec_parse/pubspec_parse.dart'; import 'core.dart'; @@ -47,6 +48,14 @@ class RepositoryPackage { /// The package's top-level pubspec.yaml. File get pubspecFile => directory.childFile('pubspec.yaml'); + late final Pubspec _parsedPubspec = + Pubspec.parse(pubspecFile.readAsStringSync()); + + /// Returns the parsed [pubspecFile]. + /// + /// Caches for future use. + Pubspec parsePubspec() => _parsedPubspec; + /// True if this appears to be a federated plugin package, according to /// repository conventions. bool get isFederated => diff --git a/script/tool/lib/src/create_all_plugins_app_command.dart b/script/tool/lib/src/create_all_plugins_app_command.dart index 5d9b4ed9c728..82f29bd501f3 100644 --- a/script/tool/lib/src/create_all_plugins_app_command.dart +++ b/script/tool/lib/src/create_all_plugins_app_command.dart @@ -178,8 +178,7 @@ class CreateAllPluginsAppCommand extends PluginCommand { final RepositoryPackage package = entry.package; final Directory pluginDirectory = package.directory; final String pluginName = pluginDirectory.basename; - final File pubspecFile = package.pubspecFile; - final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); + final Pubspec pubspec = package.parsePubspec(); if (pubspec.publishTo != 'none') { pathDependencies[pluginName] = PathDependency(pluginDirectory.path); diff --git a/script/tool/lib/src/main.dart b/script/tool/lib/src/main.dart index 70a6ab516037..b529e9ae6b04 100644 --- a/script/tool/lib/src/main.dart +++ b/script/tool/lib/src/main.dart @@ -21,6 +21,7 @@ import 'lint_android_command.dart'; import 'lint_podspecs_command.dart'; import 'list_command.dart'; import 'native_test_command.dart'; +import 'pathify_command.dart'; import 'publish_check_command.dart'; import 'publish_plugin_command.dart'; import 'pubspec_check_command.dart'; @@ -58,6 +59,7 @@ void main(List args) { ..addCommand(LintPodspecsCommand(packagesDir)) ..addCommand(ListCommand(packagesDir)) ..addCommand(NativeTestCommand(packagesDir)) + ..addCommand(PathifyCommand(packagesDir)) ..addCommand(PublishCheckCommand(packagesDir)) ..addCommand(PublishPluginCommand(packagesDir)) ..addCommand(PubspecCheckCommand(packagesDir)) diff --git a/script/tool/lib/src/pathify_command.dart b/script/tool/lib/src/pathify_command.dart new file mode 100644 index 000000000000..7bd4eab1dba1 --- /dev/null +++ b/script/tool/lib/src/pathify_command.dart @@ -0,0 +1,232 @@ +// 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/repository_package.dart'; +import 'package:path/path.dart' as p; +import 'package:pub_semver/pub_semver.dart'; +import 'package:pubspec_parse/pubspec_parse.dart'; + +import 'common/core.dart'; +import 'common/git_version_finder.dart'; +import 'common/plugin_command.dart'; + +const int _exitPackageNotFound = 3; +const int _exitCannotUpdatePubspec = 4; + +/// Converts all dependencies on target packages to path-based dependencies. +/// +/// This is to allow for pre-publish testing of changes that could affect other +/// packages in the repository. For instance, this allows for catching cases +/// where a non-breaking change to a platform interface package of a federated +/// plugin would cause post-publish analyzer failures in another package of that +/// plugin. +class PathifyCommand extends PluginCommand { + /// Creates an instance of the pathify command. + PathifyCommand(Directory packagesDir) : super(packagesDir) { + argParser.addMultiOption(_targetPackagesArg, + help: 'The names of the packages to convert to dependencies.\n' + 'Ignored if --$_targetNonBreakingUpdatePackagesArg is passed.', + valueHelp: 'some_package'); + argParser.addFlag( + _targetNonBreakingUpdatePackagesArg, + help: 'Causes all pacakges that have non-breaking version changes ' + 'relative to the git base to be treated as target packages.', + ); + } + + static const String _targetPackagesArg = 'target-packages'; + static const String _targetNonBreakingUpdatePackagesArg = + 'target-non-breaking-update-packages'; + + @override + final String name = 'pathify'; + + @override + final String description = + 'Converts package dependencies to path-based references.'; + + @override + Future run() async { + final Set targetPackages = + getBoolArg(_targetNonBreakingUpdatePackagesArg) + ? await _getNonBreakingUpdatePackages() + : getStringListArg(_targetPackagesArg).toSet(); + + if (targetPackages.isEmpty) { + print('No target packages; nothing to do.'); + return; + } + print('Rewriting references to: ${targetPackages.join(', ')}...'); + + final Map localPackageTargets = + _findLocalPackages(targetPackages); + + final String repoRootPath = (await gitDir).path; + for (final File pubspec in await _getAllPubspecs()) { + if (await _addDependencyOverridesIfNecessary( + pubspec, localPackageTargets)) { + // Print the relative path of the changed pubspec. + final String displayPath = p.posix.joinAll( + path.split(path.relative(pubspec.path, from: repoRootPath))); + print(' Modified $displayPath'); + } + } + } + + Map _findLocalPackages(Set packageNames) { + final Map targets = + {}; + for (final String packageName in packageNames) { + final Directory topLevelCandidate = + packagesDir.childDirectory(packageName); + // If packages// exists, then either that directory is the + // package, or packages/// exists and is the + // package (in the case of a federated plugin). + if (topLevelCandidate.existsSync()) { + final Directory appFacingCandidate = + topLevelCandidate.childDirectory(packageName); + targets[packageName] = RepositoryPackage(appFacingCandidate.existsSync() + ? appFacingCandidate + : topLevelCandidate); + continue; + } + // If there is no packages/ directory, then either the + // packages doesn't exist, or it is a sub-package of a federated plugin. + // If it's the latter, it will be a directory whose name is a prefix. + for (final FileSystemEntity entity in packagesDir.listSync()) { + if (entity is Directory && packageName.startsWith(entity.basename)) { + final Directory subPackageCandidate = + entity.childDirectory(packageName); + if (subPackageCandidate.existsSync()) { + targets[packageName] = RepositoryPackage(subPackageCandidate); + break; + } + } + } + + if (!targets.containsKey(packageName)) { + printError('Unable to find package "$packageName"'); + throw ToolExit(_exitPackageNotFound); + } + } + return targets; + } + + /// If [pubspecFile] has any dependencies on packages in [packageTargets], + /// adds dependency_overrides entries to redirect them to packageTargets. + /// + /// Returns true if any changes were made. + Future _addDependencyOverridesIfNecessary( + File pubspecFile, Map packageTargets) async { + final String pubspecContents = pubspecFile.readAsStringSync(); + final Pubspec pubspec = Pubspec.parse(pubspecContents); + // Fail if there are any dependency overrides already. If support for that + // is needed at some point, it can be added, but currently it's not and + // relying on that makes the logic here much simpler. + if (pubspec.dependencyOverrides.isNotEmpty) { + printError( + 'Plugins with dependency overrides are not currently supported.'); + throw ToolExit(_exitCannotUpdatePubspec); + } + + final Iterable packagesToOverride = pubspec.dependencies.keys + .where((String packageName) => packageTargets.containsKey(packageName)); + if (packagesToOverride.isNotEmpty) { + final String commonBasePath = packagesDir.path; + // Find the relative path to the common base. + final int packageDepth = path + .split(path.relative(pubspecFile.parent.path, from: commonBasePath)) + .length; + final List relativeBasePathComponents = + List.filled(packageDepth, '..'); + // This is done via strings rather than by manipulating the Pubspec and + // then re-serialiazing so that it's a localized change, rather than + // rewriting the whole file (e.g., destroying comments), which could be + // more disruptive for local use. + String newPubspecContents = pubspecContents + + ''' + +# FOR TESTING ONLY. DO NOT MERGE. +dependency_overrides: +'''; + for (final String packageName in packagesToOverride) { + // Find the relative path from the common base to the local pacakge. + final List repoRelativePathComponents = path.split( + path.relative(packageTargets[packageName]!.directory.path, + from: commonBasePath)); + newPubspecContents += ''' + $packageName: + path: + ${p.posix.joinAll([ + ...relativeBasePathComponents, + ...repoRelativePathComponents, + ])} +'''; + } + pubspecFile.writeAsStringSync(newPubspecContents); + return true; + } + return false; + } + + /// Returns all pubspecs anywhere under the packages directory. + Future> _getAllPubspecs() => packagesDir.parent + .list(recursive: true, followLinks: false) + .where((FileSystemEntity entity) => + entity is File && p.basename(entity.path) == 'pubspec.yaml') + .map((FileSystemEntity file) => file as File) + .toList(); + + /// Returns all packages that have non-breaking published changes (i.e., a + /// minor or bugfix version change) relative to the git comparison base. + /// + /// Prints status information about what was checked for ease of auditing logs + /// in CI. + Future> _getNonBreakingUpdatePackages() async { + final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); + final String baseSha = await gitVersionFinder.getBaseSha(); + print('Finding changed packages relative to "$baseSha"\n'); + + final Set changedPackages = {}; + for (final String path in await gitVersionFinder.getChangedFiles()) { + // Git output always uses Posix paths. + final List allComponents = p.posix.split(path); + // Only pubspec changes are potential publishing events. + if (allComponents.last != 'pubspec.yaml' || + allComponents.contains('example')) { + continue; + } + final RepositoryPackage package = + RepositoryPackage(packagesDir.fileSystem.file(path).parent); + final String packageName = package.parsePubspec().name; + if (!await _hasNonBreakingVersionChange(package)) { + // Log packages that had pubspec changes but weren't included for ease + // of auditing CI. + print(' Skipping $packageName; no non-breaking version change.'); + continue; + } + changedPackages.add(packageName); + } + return changedPackages; + } + + Future _hasNonBreakingVersionChange(RepositoryPackage package) async { + final Pubspec pubspec = package.parsePubspec(); + if (pubspec.publishTo == 'none') { + return false; + } + + final String pubspecGitPath = p.posix.joinAll(path.split( + path.relative(package.pubspecFile.path, from: (await gitDir).path))); + final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); + final Version? previousVersion = + await gitVersionFinder.getPackageVersion(pubspecGitPath); + if (previousVersion == null) { + // The plugin is new, so nothing can be depending on it yet. + return false; + } + return pubspec.version != previousVersion; + } +} diff --git a/script/tool/lib/src/publish_check_command.dart b/script/tool/lib/src/publish_check_command.dart index 563e0904552a..8fd96b818c1d 100644 --- a/script/tool/lib/src/publish_check_command.dart +++ b/script/tool/lib/src/publish_check_command.dart @@ -123,13 +123,12 @@ class PublishCheckCommand extends PackageLoopingCommand { } Pubspec? _tryParsePubspec(RepositoryPackage package) { - final File pubspecFile = package.pubspecFile; - try { - return Pubspec.parse(pubspecFile.readAsStringSync()); + return package.parsePubspec(); } on Exception catch (exception) { print( - 'Failed to parse `pubspec.yaml` at ${pubspecFile.path}: $exception}', + 'Failed to parse `pubspec.yaml` at ${package.pubspecFile.path}: ' + '$exception', ); return null; } diff --git a/script/tool/lib/src/publish_plugin_command.dart b/script/tool/lib/src/publish_plugin_command.dart index 4fdecf603eec..28d17a3a2487 100644 --- a/script/tool/lib/src/publish_plugin_command.dart +++ b/script/tool/lib/src/publish_plugin_command.dart @@ -217,16 +217,15 @@ class PublishPluginCommand extends PackageLoopingCommand { /// In cases where a non-null result is returned, that should be returned /// as the final result for the package, without further processing. Future _checkNeedsRelease(RepositoryPackage package) async { - final File pubspecFile = package.pubspecFile; - if (!pubspecFile.existsSync()) { + if (!package.pubspecFile.existsSync()) { logWarning(''' -The pubspec file at ${pubspecFile.path} does not exist. Publishing will not happen for ${pubspecFile.parent.basename}. +The pubspec file for ${package.displayName} does not exist, so no publishing will happen. Safe to ignore if the package is deleted in this commit. '''); return PackageResult.skip('package deleted'); } - final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); + final Pubspec pubspec = package.parsePubspec(); if (pubspec.name == 'flutter_plugin_tools') { // Ignore flutter_plugin_tools package when running publishing through flutter_plugin_tools. diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart index 1ec5dc4f2490..fcaea335920f 100644 --- a/script/tool/lib/src/version_check_command.dart +++ b/script/tool/lib/src/version_check_command.dart @@ -463,10 +463,8 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog. } Pubspec? _tryParsePubspec(RepositoryPackage package) { - final File pubspecFile = package.pubspecFile; - try { - final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync()); + final Pubspec pubspec = package.parsePubspec(); return pubspec; } on Exception catch (exception) { printError('${indentation}Failed to parse `pubspec.yaml`: $exception}'); diff --git a/script/tool/test/common/repository_package_test.dart b/script/tool/test/common/repository_package_test.dart index 4c20389ae4be..29e3b5832127 100644 --- a/script/tool/test/common/repository_package_test.dart +++ b/script/tool/test/common/repository_package_test.dart @@ -5,6 +5,7 @@ import 'package:file/file.dart'; import 'package:file/memory.dart'; import 'package:flutter_plugin_tools/src/common/repository_package.dart'; +import 'package:pubspec_parse/pubspec_parse.dart'; import 'package:test/test.dart'; import '../util.dart'; @@ -155,4 +156,23 @@ void main() { expect(RepositoryPackage(plugin).isPlatformImplementation, true); }); }); + + group('pubspec', () { + test('file', () async { + final Directory plugin = createFakePlugin('a_plugin', packagesDir); + + final File pubspecFile = RepositoryPackage(plugin).pubspecFile; + + expect(pubspecFile.path, plugin.childFile('pubspec.yaml').path); + }); + + test('parsing', () async { + final Directory plugin = createFakePlugin('a_plugin', packagesDir, + examples: ['example1', 'example2']); + + final Pubspec pubspec = RepositoryPackage(plugin).parsePubspec(); + + expect(pubspec.name, 'a_plugin'); + }); + }); } From c868e2e2611df39004706c603a7568ee4e65b326 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Thu, 2 Dec 2021 16:40:07 -0500 Subject: [PATCH 02/11] Add tests, fix issues found by tests --- script/tool/lib/src/pathify_command.dart | 30 ++- script/tool/test/pathify_command_test.dart | 284 +++++++++++++++++++++ 2 files changed, 305 insertions(+), 9 deletions(-) create mode 100644 script/tool/test/pathify_command_test.dart diff --git a/script/tool/lib/src/pathify_command.dart b/script/tool/lib/src/pathify_command.dart index 7bd4eab1dba1..46cc5e0ff796 100644 --- a/script/tool/lib/src/pathify_command.dart +++ b/script/tool/lib/src/pathify_command.dart @@ -4,6 +4,7 @@ import 'package:file/file.dart'; import 'package:flutter_plugin_tools/src/common/repository_package.dart'; +import 'package:git/git.dart'; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:pubspec_parse/pubspec_parse.dart'; @@ -24,7 +25,10 @@ const int _exitCannotUpdatePubspec = 4; /// plugin. class PathifyCommand extends PluginCommand { /// Creates an instance of the pathify command. - PathifyCommand(Directory packagesDir) : super(packagesDir) { + PathifyCommand( + Directory packagesDir, { + GitDir? gitDir, + }) : super(packagesDir, gitDir: gitDir) { argParser.addMultiOption(_targetPackagesArg, help: 'The names of the packages to convert to dependencies.\n' 'Ignored if --$_targetNonBreakingUpdatePackagesArg is passed.', @@ -68,8 +72,8 @@ class PathifyCommand extends PluginCommand { if (await _addDependencyOverridesIfNecessary( pubspec, localPackageTargets)) { // Print the relative path of the changed pubspec. - final String displayPath = p.posix.joinAll( - path.split(path.relative(pubspec.path, from: repoRootPath))); + final String displayPath = p.posix.joinAll(path + .split(path.relative(pubspec.absolute.path, from: repoRootPath))); print(' Modified $displayPath'); } } @@ -137,7 +141,8 @@ class PathifyCommand extends PluginCommand { final String commonBasePath = packagesDir.path; // Find the relative path to the common base. final int packageDepth = path - .split(path.relative(pubspecFile.parent.path, from: commonBasePath)) + .split(path.relative(pubspecFile.parent.absolute.path, + from: commonBasePath)) .length; final List relativeBasePathComponents = List.filled(packageDepth, '..'); @@ -158,8 +163,7 @@ dependency_overrides: from: commonBasePath)); newPubspecContents += ''' $packageName: - path: - ${p.posix.joinAll([ + path: ${p.posix.joinAll([ ...relativeBasePathComponents, ...repoRelativePathComponents, ])} @@ -218,8 +222,9 @@ dependency_overrides: return false; } - final String pubspecGitPath = p.posix.joinAll(path.split( - path.relative(package.pubspecFile.path, from: (await gitDir).path))); + final String pubspecGitPath = p.posix.joinAll(path.split(path.relative( + package.pubspecFile.absolute.path, + from: (await gitDir).path))); final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); final Version? previousVersion = await gitVersionFinder.getPackageVersion(pubspecGitPath); @@ -227,6 +232,13 @@ dependency_overrides: // The plugin is new, so nothing can be depending on it yet. return false; } - return pubspec.version != previousVersion; + final Version newVersion = pubspec.version!; + if ((newVersion.major > 0 && newVersion.major != previousVersion.major) || + (newVersion.major == 0 && newVersion.minor != previousVersion.minor)) { + // Breaking changes aren't targetted since they won't be picked up + // automatically. + return false; + } + return newVersion != previousVersion; } } diff --git a/script/tool/test/pathify_command_test.dart b/script/tool/test/pathify_command_test.dart new file mode 100644 index 000000000000..7c6e74168a16 --- /dev/null +++ b/script/tool/test/pathify_command_test.dart @@ -0,0 +1,284 @@ +// 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/pathify_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 Directory packagesDir; + late CommandRunner runner; + late RecordingProcessRunner processRunner; + + setUp(() { + fileSystem = MemoryFileSystem(); + 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 PathifyCommand command = PathifyCommand(packagesDir, gitDir: gitDir); + + runner = CommandRunner('pathify_command', 'Test for $PathifyCommand'); + runner.addCommand(command); + }); + + /// Adds dummy 'dependencies:' entries for each package in [dependencies] + /// to [package]. + void _addDependencies( + RepositoryPackage package, Iterable dependencies) { + final List lines = package.pubspecFile.readAsLinesSync(); + final int dependenciesStartIndex = lines.indexOf('dependencies:'); + assert(dependenciesStartIndex != -1); + lines.insertAll(dependenciesStartIndex + 1, [ + for (final String dependency in dependencies) ' $dependency: ^1.0.0', + ]); + package.pubspecFile.writeAsStringSync(lines.join('\n')); + } + + test('no-ops for no plugins', () async { + createFakePackage('foo', packagesDir); + + final List output = + await runCapturingPrint(runner, ['pathify']); + + expect( + output, + containsAllInOrder([ + contains('No target packages'), + ]), + ); + }); + + test('rewrites references', () async { + final RepositoryPackage simplePackage = RepositoryPackage( + createFakePackage('foo', packagesDir, isFlutter: true)); + final Directory pluginGroup = packagesDir.childDirectory('bar'); + + RepositoryPackage(createFakePackage('bar_platform_interface', pluginGroup, + isFlutter: true)); + final RepositoryPackage pluginImplementation = + RepositoryPackage(createFakePlugin('bar_android', pluginGroup)); + final RepositoryPackage pluginAppFacing = + RepositoryPackage(createFakePlugin('bar', pluginGroup)); + + _addDependencies(simplePackage, [ + 'bar', + 'bar_android', + 'bar_platform_interface', + ]); + _addDependencies(pluginAppFacing, [ + 'bar_platform_interface', + 'bar_android', + ]); + _addDependencies(pluginImplementation, [ + 'bar_platform_interface', + ]); + + final List output = await runCapturingPrint(runner, + ['pathify', '--target-packages=bar,bar_platform_interface']); + + expect( + output, + containsAll([ + 'Rewriting references to: bar, bar_platform_interface...', + ' Modified packages/bar/bar/pubspec.yaml', + ' Modified packages/bar/bar_android/pubspec.yaml', + ' Modified packages/foo/pubspec.yaml', + ])); + expect( + output, + isNot(contains( + ' Modified packages/bar/bar_platform_interface/pubspec.yaml'))); + + expect( + simplePackage.pubspecFile.readAsLinesSync(), + containsAllInOrder([ + '# FOR TESTING ONLY. DO NOT MERGE.', + 'dependency_overrides:', + ' bar:', + ' path: ../bar/bar', + ' bar_platform_interface:', + ' path: ../bar/bar_platform_interface', + ])); + expect( + pluginAppFacing.pubspecFile.readAsLinesSync(), + containsAllInOrder([ + 'dependency_overrides:', + ' bar_platform_interface:', + ' path: ../../bar/bar_platform_interface', + ])); + }); + + group('target-non-breaking-update-packages', () { + test('no-ops for no published changes', () async { + final Directory package = createFakePackage('foo', packagesDir); + + final String changedFileOutput = [ + package.childFile('pubspec.yaml'), + ].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(package).pubspecFile.readAsStringSync()), + ]; + + final List output = await runCapturingPrint( + runner, ['pathify', '--target-non-breaking-update-packages']); + + expect( + output, + containsAllInOrder([ + contains('No target packages'), + ]), + ); + }); + + test('includes bugfix version changes as targets', () async { + const String newVersion = '1.0.1'; + final Directory package = + createFakePackage('foo', packagesDir, version: newVersion); + + final File pubspecFile = RepositoryPackage(package).pubspecFile; + final String changedFileOutput = [ + pubspecFile, + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + final String gitPubspecContents = + pubspecFile.readAsStringSync().replaceAll(newVersion, '1.0.0'); + // Simulate no change to the version in the interface's pubspec.yaml. + processRunner.mockProcessesForExecutable['git-show'] = [ + MockProcess(stdout: gitPubspecContents), + ]; + + final List output = await runCapturingPrint( + runner, ['pathify', '--target-non-breaking-update-packages']); + + expect( + output, + containsAllInOrder([ + contains('Rewriting references to: foo...'), + ]), + ); + }); + + test('includes minor version changes to 1.0+ as targets', () async { + const String newVersion = '1.1.0'; + final Directory package = + createFakePackage('foo', packagesDir, version: newVersion); + + final File pubspecFile = RepositoryPackage(package).pubspecFile; + final String changedFileOutput = [ + pubspecFile, + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + final String gitPubspecContents = + pubspecFile.readAsStringSync().replaceAll(newVersion, '1.0.0'); + // Simulate no change to the version in the interface's pubspec.yaml. + processRunner.mockProcessesForExecutable['git-show'] = [ + MockProcess(stdout: gitPubspecContents), + ]; + + final List output = await runCapturingPrint( + runner, ['pathify', '--target-non-breaking-update-packages']); + + expect( + output, + containsAllInOrder([ + contains('Rewriting references to: foo...'), + ]), + ); + }); + + test('does not include major version changes as targets', () async { + const String newVersion = '2.0.0'; + final Directory package = + createFakePackage('foo', packagesDir, version: newVersion); + + final File pubspecFile = RepositoryPackage(package).pubspecFile; + final String changedFileOutput = [ + pubspecFile, + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + final String gitPubspecContents = + pubspecFile.readAsStringSync().replaceAll(newVersion, '1.0.0'); + // Simulate no change to the version in the interface's pubspec.yaml. + processRunner.mockProcessesForExecutable['git-show'] = [ + MockProcess(stdout: gitPubspecContents), + ]; + + final List output = await runCapturingPrint( + runner, ['pathify', '--target-non-breaking-update-packages']); + + expect( + output, + containsAllInOrder([ + contains('No target packages'), + ]), + ); + }); + + test('does not include minor version changes to 0.x as targets', () async { + const String newVersion = '0.8.0'; + final Directory package = + createFakePackage('foo', packagesDir, version: newVersion); + + final File pubspecFile = RepositoryPackage(package).pubspecFile; + final String changedFileOutput = [ + pubspecFile, + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: changedFileOutput), + ]; + final String gitPubspecContents = + pubspecFile.readAsStringSync().replaceAll(newVersion, '0.7.0'); + // Simulate no change to the version in the interface's pubspec.yaml. + processRunner.mockProcessesForExecutable['git-show'] = [ + MockProcess(stdout: gitPubspecContents), + ]; + + final List output = await runCapturingPrint( + runner, ['pathify', '--target-non-breaking-update-packages']); + + expect( + output, + containsAllInOrder([ + contains('No target packages'), + ]), + ); + }); + }); +} From 539e3d3662b7bac308d308d04940ebeab6483aa0 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 11:08:34 -0500 Subject: [PATCH 03/11] Changelog --- script/tool/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index a2a2ed824295..bebc3bdbd891 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,6 +1,8 @@ ## NEXT - Ensures that `firebase-test-lab` runs include an `integration_test` runner. +- Adds a `pathify` command to convert inter-repo package dependencies to + path-based dependencies. ## 0.7.3 From 2cd61020966693c9001e4d33db469352d9517b04 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 11:54:58 -0500 Subject: [PATCH 04/11] Add --run-on-dirty-packages --- script/tool/CHANGELOG.md | 1 + .../tool/lib/src/common/plugin_command.dart | 34 +++++-- .../tool/test/common/plugin_command_test.dart | 98 +++++++++++++++++++ 3 files changed, 126 insertions(+), 7 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index bebc3bdbd891..47cf27e8d7f8 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -3,6 +3,7 @@ - Ensures that `firebase-test-lab` runs include an `integration_test` runner. - Adds a `pathify` command to convert inter-repo package dependencies to path-based dependencies. +- Adds a (hidden) `--run-on-dirty-packages` flag for use with `pathify` in CI. ## 0.7.3 diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart index f40a102dfbc0..799cd4291d41 100644 --- a/script/tool/lib/src/common/plugin_command.dart +++ b/script/tool/lib/src/common/plugin_command.dart @@ -75,9 +75,15 @@ abstract class PluginCommand extends Command { help: 'Run the command on changed packages/plugins.\n' 'If no packages have changed, or if there have been changes that may\n' 'affect all packages, the command runs on all packages.\n' - 'The packages excluded with $_excludeArg is also excluded even if changed.\n' + 'Packages excluded with $_excludeArg are excluded even if changed.\n' 'See $_baseShaArg if a custom base is needed to determine the diff.\n\n' 'Cannot be combined with $_packagesArg.\n'); + argParser.addFlag(_runOnDirtyPackagesArg, + help: + 'Run the command on packages with changes that have not been committed.\n' + 'Packages excluded with $_excludeArg are excluded even if changed.\n' + 'Cannot be combined with $_packagesArg.\n', + hide: true); argParser.addFlag(_packagesForBranchArg, help: 'This runs on all packages (equivalent to no package selection flag)\n' @@ -103,6 +109,7 @@ abstract class PluginCommand extends Command { static const String _packagesForBranchArg = 'packages-for-branch'; static const String _pluginsArg = 'plugins'; static const String _runOnChangedPackagesArg = 'run-on-changed-packages'; + static const String _runOnDirtyPackagesArg = 'run-on-dirty-packages'; static const String _shardCountArg = 'shardCount'; static const String _shardIndexArg = 'shardIndex'; @@ -289,6 +296,7 @@ abstract class PluginCommand extends Command { final Set packageSelectionFlags = { _packagesArg, _runOnChangedPackagesArg, + _runOnDirtyPackagesArg, _packagesForBranchArg, }; if (packageSelectionFlags @@ -300,7 +308,7 @@ abstract class PluginCommand extends Command { throw ToolExit(exitInvalidArguments); } - Set plugins = Set.from(getStringListArg(_packagesArg)); + Set packages = Set.from(getStringListArg(_packagesArg)); final bool runOnChangedPackages; if (getBoolArg(_runOnChangedPackagesArg)) { @@ -331,7 +339,19 @@ abstract class PluginCommand extends Command { final List changedFiles = await gitVersionFinder.getChangedFiles(); if (!_changesRequireFullTest(changedFiles)) { - plugins = _getChangedPackages(changedFiles); + packages = _getChangedPackages(changedFiles); + } + } else if (getBoolArg(_runOnDirtyPackagesArg)) { + final GitVersionFinder gitVersionFinder = + GitVersionFinder(await gitDir, 'HEAD'); + print('Running for all packages that have uncommitted changes\n'); + // _changesRequireFullTest is deliberately not used here, as this flag is + // intended for use in CI to re-test packages changed by 'pathify'. + packages = _getChangedPackages(await gitVersionFinder.getChangedFiles()); + // For the same reason, empty is not treated as "all packages" as it is + // for other flags. + if (packages.isEmpty) { + return; } } @@ -347,7 +367,7 @@ abstract class PluginCommand extends Command { in dir.list(followLinks: false)) { // A top-level Dart package is a plugin package. if (_isDartPackage(entity)) { - if (plugins.isEmpty || plugins.contains(p.basename(entity.path))) { + if (packages.isEmpty || packages.contains(p.basename(entity.path))) { yield PackageEnumerationEntry( RepositoryPackage(entity as Directory), excluded: excludedPluginNames.contains(entity.basename)); @@ -364,9 +384,9 @@ abstract class PluginCommand extends Command { path.relative(subdir.path, from: dir.path); final String packageName = path.basename(subdir.path); final String basenamePath = path.basename(entity.path); - if (plugins.isEmpty || - plugins.contains(relativePath) || - plugins.contains(basenamePath)) { + if (packages.isEmpty || + packages.contains(relativePath) || + packages.contains(basenamePath)) { yield PackageEnumerationEntry( RepositoryPackage(subdir as Directory), excluded: excludedPluginNames.contains(basenamePath) || diff --git a/script/tool/test/common/plugin_command_test.dart b/script/tool/test/common/plugin_command_test.dart index 6d586e416b7d..222df544f344 100644 --- a/script/tool/test/common/plugin_command_test.dart +++ b/script/tool/test/common/plugin_command_test.dart @@ -486,6 +486,104 @@ packages/plugin3/plugin3.dart expect(command.plugins, unorderedEquals([plugin1.path])); }); }); + + group('test run-on-dirty-packages', () { + test('no packages should be tested if there are no changes.', () async { + createFakePackage('a_package', packagesDir); + await runCapturingPrint( + runner, ['sample', '--run-on-dirty-packages']); + + expect(command.plugins, unorderedEquals([])); + }); + + test( + 'no packages should be tested if there are no plugin related changes.', + () async { + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: 'AUTHORS'), + ]; + createFakePackage('a_package', packagesDir); + await runCapturingPrint( + runner, ['sample', '--run-on-dirty-packages']); + + expect(command.plugins, unorderedEquals([])); + }); + + test('no packages should be tested even if special repo files change.', + () async { + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: ''' +.cirrus.yml +.ci.yaml +.ci/Dockerfile +.clang-format +analysis_options.yaml +script/tool_runner.sh +'''), + ]; + createFakePackage('a_package', packagesDir); + await runCapturingPrint( + runner, ['sample', '--run-on-dirty-packages']); + + expect(command.plugins, unorderedEquals([])); + }); + + test('Only changed packages should be tested.', () async { + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: 'packages/a_package/lib/a_package.dart'), + ]; + final Directory packageA = createFakePackage('a_package', packagesDir); + createFakePlugin('b_package', packagesDir); + final List output = await runCapturingPrint( + runner, ['sample', '--run-on-dirty-packages']); + + expect( + output, + containsAllInOrder([ + contains( + 'Running for all packages that have uncommitted changes'), + ])); + + expect(command.plugins, unorderedEquals([packageA.path])); + }); + + test('multiple packages changed should test all the changed packages', + () async { + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: ''' +packages/a_package/lib/a_package.dart +packages/b_package/lib/src/foo.dart +'''), + ]; + final Directory packageA = createFakePackage('a_package', packagesDir); + final Directory packageB = createFakePackage('b_package', packagesDir); + createFakePackage('c_package', packagesDir); + await runCapturingPrint( + runner, ['sample', '--run-on-dirty-packages']); + + expect(command.plugins, + unorderedEquals([packageA.path, packageB.path])); + }); + + test('honors --exclude flag', () async { + processRunner.mockProcessesForExecutable['git-diff'] = [ + MockProcess(stdout: ''' +packages/a_package/lib/a_package.dart +packages/b_package/lib/src/foo.dart +'''), + ]; + final Directory packageA = createFakePackage('a_package', packagesDir); + createFakePackage('b_package', packagesDir); + createFakePackage('c_package', packagesDir); + await runCapturingPrint(runner, [ + 'sample', + '--exclude=b_package', + '--run-on-dirty-packages' + ]); + + expect(command.plugins, unorderedEquals([packageA.path])); + }); + }); }); group('--packages-for-branch', () { From 4766d851b779372e2ee30251d61d55216d9e1c8d Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 11:56:14 -0500 Subject: [PATCH 05/11] Enable the new check in CI --- .cirrus.yml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 59f686dbf5d6..9f5bf0c8067e 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -109,13 +109,24 @@ task: matrix: CHANNEL: "master" CHANNEL: "stable" - tool_script: + analyze_tool_script: - cd script/tool - dart analyze --fatal-infos - script: + analyze_script: # DO NOT change the custom-analysis argument here without changing the Dart repo. # See the comment in script/configs/custom_analysis.yaml for details. - ./script/tool_runner.sh analyze --custom-analysis=script/configs/custom_analysis.yaml + pathified_analyze_script: + # Re-run analysis with path-based dependencies to ensure that publishing + # the changes won't break analysis of other packages in the respository + # that depend on it. + - ./script/tool_runner.sh pathify --target-non-breaking-update-packages + # This uses --run-on-dirty-packages rather than --packages-for-branch + # since only the packages changed by 'pathify' need to be checked. + - dart $PLUGIN_TOOL analyze --run-on-dirty-packages --log-timing --custom-analysis=script/configs/custom_analysis.yaml + # Restore the tree to a clean state, to avoid accidental issues if + # other script steps are added to this task. + - git checkout . ### Android tasks ### - name: android-build_all_plugins env: From eadf0d836316111ec178c6d90f9916c970805429 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 11:56:54 -0500 Subject: [PATCH 06/11] Analyze warning fix --- script/tool/test/pathify_command_test.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/script/tool/test/pathify_command_test.dart b/script/tool/test/pathify_command_test.dart index 7c6e74168a16..5d8953ccab3c 100644 --- a/script/tool/test/pathify_command_test.dart +++ b/script/tool/test/pathify_command_test.dart @@ -7,7 +7,6 @@ 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/pathify_command.dart'; import 'package:mockito/mockito.dart'; From 37384eec8f909b5c9a3609dd03295cae723d72f3 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 12:09:09 -0500 Subject: [PATCH 07/11] Revert change accidentalyl included in branch --- packages/ios_platform_images/analysis_options.yaml | 1 + 1 file changed, 1 insertion(+) create mode 100644 packages/ios_platform_images/analysis_options.yaml diff --git a/packages/ios_platform_images/analysis_options.yaml b/packages/ios_platform_images/analysis_options.yaml new file mode 100644 index 000000000000..cda4f6e153e6 --- /dev/null +++ b/packages/ios_platform_images/analysis_options.yaml @@ -0,0 +1 @@ +include: ../../analysis_options_legacy.yaml From 5133e44d1b2051bd93345b0f085d62c4e53ee39e Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 12:36:18 -0500 Subject: [PATCH 08/11] Fix test expectation for incidental logging cleanup --- script/tool/test/publish_plugin_command_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/tool/test/publish_plugin_command_test.dart b/script/tool/test/publish_plugin_command_test.dart index 14e99a10f365..2cb3fc25af2e 100644 --- a/script/tool/test/publish_plugin_command_test.dart +++ b/script/tool/test/publish_plugin_command_test.dart @@ -675,7 +675,7 @@ void main() { contains('Running `pub publish ` in ${pluginDir1.path}...'), contains('Published plugin1 successfully!'), contains( - 'The pubspec file at ${pluginDir2.childFile('pubspec.yaml').path} does not exist. Publishing will not happen for plugin2.\nSafe to ignore if the package is deleted in this commit.\n'), + 'The pubspec file for plugin2/plugin2 does not exist, so no publishing will happen.\nSafe to ignore if the package is deleted in this commit.\n'), contains('SKIPPING: package deleted'), contains('skipped (with warning)'), ])); From 95e695231d9ed1326fe3d8cf9ae741dcf149cb73 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Fri, 3 Dec 2021 12:59:48 -0500 Subject: [PATCH 09/11] Fix diffing --- script/tool/lib/src/common/git_version_finder.dart | 10 ++++++++-- script/tool/lib/src/common/plugin_command.dart | 3 ++- script/tool/test/common/git_version_finder_test.dart | 11 +++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/script/tool/lib/src/common/git_version_finder.dart b/script/tool/lib/src/common/git_version_finder.dart index 331187335f51..32d30e60feb5 100644 --- a/script/tool/lib/src/common/git_version_finder.dart +++ b/script/tool/lib/src/common/git_version_finder.dart @@ -31,10 +31,16 @@ class GitVersionFinder { } /// Get a list of all the changed files. - Future> getChangedFiles() async { + Future> getChangedFiles( + {bool includeUncommitted = false}) async { final String baseSha = await getBaseSha(); final io.ProcessResult changedFilesCommand = await baseGitDir - .runCommand(['diff', '--name-only', baseSha, 'HEAD']); + .runCommand([ + 'diff', + '--name-only', + baseSha, + if (!includeUncommitted) 'HEAD' + ]); final String changedFilesStdout = changedFilesCommand.stdout.toString(); if (changedFilesStdout.isEmpty) { return []; diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart index 799cd4291d41..4184980aa7fc 100644 --- a/script/tool/lib/src/common/plugin_command.dart +++ b/script/tool/lib/src/common/plugin_command.dart @@ -347,7 +347,8 @@ abstract class PluginCommand extends Command { print('Running for all packages that have uncommitted changes\n'); // _changesRequireFullTest is deliberately not used here, as this flag is // intended for use in CI to re-test packages changed by 'pathify'. - packages = _getChangedPackages(await gitVersionFinder.getChangedFiles()); + packages = _getChangedPackages( + await gitVersionFinder.getChangedFiles(includeUncommitted: true)); // For the same reason, empty is not treated as "all packages" as it is // for other flags. if (packages.isEmpty) { diff --git a/script/tool/test/common/git_version_finder_test.dart b/script/tool/test/common/git_version_finder_test.dart index fa8b1c410dd5..ef56ec228aac 100644 --- a/script/tool/test/common/git_version_finder_test.dart +++ b/script/tool/test/common/git_version_finder_test.dart @@ -88,6 +88,17 @@ file2/file2.cc verify(gitDir .runCommand(['diff', '--name-only', customBaseSha, 'HEAD'])); }); + + test('include uncommitted files if requested', () async { + const String customBaseSha = 'aklsjdcaskf12312'; + gitDiffResponse = ''' +file1/pubspec.yaml +file2/file2.cc +'''; + final GitVersionFinder finder = GitVersionFinder(gitDir, customBaseSha); + await finder.getChangedFiles(includeUncommitted: true); + verify(gitDir.runCommand(['diff', '--name-only', customBaseSha])); + }); } class MockProcessResult extends Mock implements ProcessResult {} From 3390cdee61b047eda30c227252cf56ae60cb8f82 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Sat, 4 Dec 2021 10:47:55 -0500 Subject: [PATCH 10/11] Address review comments --- .cirrus.yml | 5 +- script/tool/CHANGELOG.md | 7 +- .../tool/lib/src/common/plugin_command.dart | 3 +- script/tool/lib/src/main.dart | 4 +- ...dart => make_deps_path_based_command.dart} | 65 ++++++++++--------- .../test/common/git_version_finder_test.dart | 1 + ...=> make_deps_path_based_command_test.dart} | 65 ++++++++++++------- 7 files changed, 90 insertions(+), 60 deletions(-) rename script/tool/lib/src/{pathify_command.dart => make_deps_path_based_command.dart} (81%) rename script/tool/test/{pathify_command_test.dart => make_deps_path_based_command_test.dart} (80%) diff --git a/.cirrus.yml b/.cirrus.yml index 9f5bf0c8067e..453d01b89f07 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -120,9 +120,10 @@ task: # Re-run analysis with path-based dependencies to ensure that publishing # the changes won't break analysis of other packages in the respository # that depend on it. - - ./script/tool_runner.sh pathify --target-non-breaking-update-packages + - ./script/tool_runner.sh make-deps-path-based --target-dependencies-with-non-breaking-updates # This uses --run-on-dirty-packages rather than --packages-for-branch - # since only the packages changed by 'pathify' need to be checked. + # since only the packages changed by 'make-deps-path-based' need to be + # checked. - dart $PLUGIN_TOOL analyze --run-on-dirty-packages --log-timing --custom-analysis=script/configs/custom_analysis.yaml # Restore the tree to a clean state, to avoid accidental issues if # other script steps are added to this task. diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 47cf27e8d7f8..234700ab5a7d 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,9 +1,10 @@ ## NEXT - Ensures that `firebase-test-lab` runs include an `integration_test` runner. -- Adds a `pathify` command to convert inter-repo package dependencies to - path-based dependencies. -- Adds a (hidden) `--run-on-dirty-packages` flag for use with `pathify` in CI. +- Adds a `make-deps-path-based` command to convert inter-repo package + dependencies to path-based dependencies. +- Adds a (hidden) `--run-on-dirty-packages` flag for use with + `make-deps-path-based` in CI. ## 0.7.3 diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart index 4184980aa7fc..7166c754e129 100644 --- a/script/tool/lib/src/common/plugin_command.dart +++ b/script/tool/lib/src/common/plugin_command.dart @@ -346,7 +346,8 @@ abstract class PluginCommand extends Command { GitVersionFinder(await gitDir, 'HEAD'); print('Running for all packages that have uncommitted changes\n'); // _changesRequireFullTest is deliberately not used here, as this flag is - // intended for use in CI to re-test packages changed by 'pathify'. + // intended for use in CI to re-test packages changed by + // 'make-deps-path-based'. packages = _getChangedPackages( await gitVersionFinder.getChangedFiles(includeUncommitted: true)); // For the same reason, empty is not treated as "all packages" as it is diff --git a/script/tool/lib/src/main.dart b/script/tool/lib/src/main.dart index b529e9ae6b04..cc25eb6886d3 100644 --- a/script/tool/lib/src/main.dart +++ b/script/tool/lib/src/main.dart @@ -21,7 +21,7 @@ import 'lint_android_command.dart'; import 'lint_podspecs_command.dart'; import 'list_command.dart'; import 'native_test_command.dart'; -import 'pathify_command.dart'; +import 'make_deps_path_based_command.dart'; import 'publish_check_command.dart'; import 'publish_plugin_command.dart'; import 'pubspec_check_command.dart'; @@ -59,7 +59,7 @@ void main(List args) { ..addCommand(LintPodspecsCommand(packagesDir)) ..addCommand(ListCommand(packagesDir)) ..addCommand(NativeTestCommand(packagesDir)) - ..addCommand(PathifyCommand(packagesDir)) + ..addCommand(MakeDepsPathBasedCommand(packagesDir)) ..addCommand(PublishCheckCommand(packagesDir)) ..addCommand(PublishPluginCommand(packagesDir)) ..addCommand(PubspecCheckCommand(packagesDir)) diff --git a/script/tool/lib/src/pathify_command.dart b/script/tool/lib/src/make_deps_path_based_command.dart similarity index 81% rename from script/tool/lib/src/pathify_command.dart rename to script/tool/lib/src/make_deps_path_based_command.dart index 46cc5e0ff796..04869639cf74 100644 --- a/script/tool/lib/src/pathify_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -23,29 +23,33 @@ const int _exitCannotUpdatePubspec = 4; /// where a non-breaking change to a platform interface package of a federated /// plugin would cause post-publish analyzer failures in another package of that /// plugin. -class PathifyCommand extends PluginCommand { - /// Creates an instance of the pathify command. - PathifyCommand( +class MakeDepsPathBasedCommand extends PluginCommand { + /// Creates an instance of the command to convert selected dependencies to + /// path-based. + MakeDepsPathBasedCommand( Directory packagesDir, { GitDir? gitDir, }) : super(packagesDir, gitDir: gitDir) { - argParser.addMultiOption(_targetPackagesArg, - help: 'The names of the packages to convert to dependencies.\n' - 'Ignored if --$_targetNonBreakingUpdatePackagesArg is passed.', + argParser.addMultiOption(_targetDependenciesArg, + help: + 'The names of the packages to convert to path-based dependencies.\n' + 'Ignored if --$_targetDependenciesWithNonBreakingUpdatesArg is ' + 'passed.', valueHelp: 'some_package'); argParser.addFlag( - _targetNonBreakingUpdatePackagesArg, - help: 'Causes all pacakges that have non-breaking version changes ' - 'relative to the git base to be treated as target packages.', + _targetDependenciesWithNonBreakingUpdatesArg, + help: 'Causes all packages that have non-breaking version changes ' + 'when compared against the git base to be treated as target ' + 'packages.', ); } - static const String _targetPackagesArg = 'target-packages'; - static const String _targetNonBreakingUpdatePackagesArg = - 'target-non-breaking-update-packages'; + static const String _targetDependenciesArg = 'target-dependencies'; + static const String _targetDependenciesWithNonBreakingUpdatesArg = + 'target-dependencies-with-non-breaking-updates'; @override - final String name = 'pathify'; + final String name = 'make-deps-path-based'; @override final String description = @@ -53,24 +57,24 @@ class PathifyCommand extends PluginCommand { @override Future run() async { - final Set targetPackages = - getBoolArg(_targetNonBreakingUpdatePackagesArg) + final Set targetDependencies = + getBoolArg(_targetDependenciesWithNonBreakingUpdatesArg) ? await _getNonBreakingUpdatePackages() - : getStringListArg(_targetPackagesArg).toSet(); + : getStringListArg(_targetDependenciesArg).toSet(); - if (targetPackages.isEmpty) { - print('No target packages; nothing to do.'); + if (targetDependencies.isEmpty) { + print('No target dependencies; nothing to do.'); return; } - print('Rewriting references to: ${targetPackages.join(', ')}...'); + print('Rewriting references to: ${targetDependencies.join(', ')}...'); - final Map localPackageTargets = - _findLocalPackages(targetPackages); + final Map localDependencyPackages = + _findLocalPackages(targetDependencies); final String repoRootPath = (await gitDir).path; for (final File pubspec in await _getAllPubspecs()) { if (await _addDependencyOverridesIfNecessary( - pubspec, localPackageTargets)) { + pubspec, localDependencyPackages)) { // Print the relative path of the changed pubspec. final String displayPath = p.posix.joinAll(path .split(path.relative(pubspec.absolute.path, from: repoRootPath))); @@ -118,12 +122,13 @@ class PathifyCommand extends PluginCommand { return targets; } - /// If [pubspecFile] has any dependencies on packages in [packageTargets], - /// adds dependency_overrides entries to redirect them to packageTargets. + /// If [pubspecFile] has any dependencies on packages in [localDependencies], + /// adds dependency_overrides entries to redirect them to the local version + /// using path-based dependencies. /// /// Returns true if any changes were made. - Future _addDependencyOverridesIfNecessary( - File pubspecFile, Map packageTargets) async { + Future _addDependencyOverridesIfNecessary(File pubspecFile, + Map localDependencies) async { final String pubspecContents = pubspecFile.readAsStringSync(); final Pubspec pubspec = Pubspec.parse(pubspecContents); // Fail if there are any dependency overrides already. If support for that @@ -135,8 +140,8 @@ class PathifyCommand extends PluginCommand { throw ToolExit(_exitCannotUpdatePubspec); } - final Iterable packagesToOverride = pubspec.dependencies.keys - .where((String packageName) => packageTargets.containsKey(packageName)); + final Iterable packagesToOverride = pubspec.dependencies.keys.where( + (String packageName) => localDependencies.containsKey(packageName)); if (packagesToOverride.isNotEmpty) { final String commonBasePath = packagesDir.path; // Find the relative path to the common base. @@ -157,9 +162,9 @@ class PathifyCommand extends PluginCommand { dependency_overrides: '''; for (final String packageName in packagesToOverride) { - // Find the relative path from the common base to the local pacakge. + // Find the relative path from the common base to the local package. final List repoRelativePathComponents = path.split( - path.relative(packageTargets[packageName]!.directory.path, + path.relative(localDependencies[packageName]!.directory.path, from: commonBasePath)); newPubspecContents += ''' $packageName: diff --git a/script/tool/test/common/git_version_finder_test.dart b/script/tool/test/common/git_version_finder_test.dart index ef56ec228aac..ad1a26ffc165 100644 --- a/script/tool/test/common/git_version_finder_test.dart +++ b/script/tool/test/common/git_version_finder_test.dart @@ -97,6 +97,7 @@ file2/file2.cc '''; final GitVersionFinder finder = GitVersionFinder(gitDir, customBaseSha); await finder.getChangedFiles(includeUncommitted: true); + // The call should not have HEAD as a final argument like the default diff. verify(gitDir.runCommand(['diff', '--name-only', customBaseSha])); }); } diff --git a/script/tool/test/pathify_command_test.dart b/script/tool/test/make_deps_path_based_command_test.dart similarity index 80% rename from script/tool/test/pathify_command_test.dart rename to script/tool/test/make_deps_path_based_command_test.dart index 5d8953ccab3c..29e4f724b338 100644 --- a/script/tool/test/pathify_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -8,7 +8,7 @@ import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:file/memory.dart'; import 'package:flutter_plugin_tools/src/common/repository_package.dart'; -import 'package:flutter_plugin_tools/src/pathify_command.dart'; +import 'package:flutter_plugin_tools/src/make_deps_path_based_command.dart'; import 'package:mockito/mockito.dart'; import 'package:test/test.dart'; @@ -40,9 +40,11 @@ void main() { }); processRunner = RecordingProcessRunner(); - final PathifyCommand command = PathifyCommand(packagesDir, gitDir: gitDir); + final MakeDepsPathBasedCommand command = + MakeDepsPathBasedCommand(packagesDir, gitDir: gitDir); - runner = CommandRunner('pathify_command', 'Test for $PathifyCommand'); + runner = CommandRunner( + 'make-deps-path-based_command', 'Test for $MakeDepsPathBasedCommand'); runner.addCommand(command); }); @@ -60,17 +62,24 @@ void main() { } test('no-ops for no plugins', () async { - createFakePackage('foo', packagesDir); + RepositoryPackage(createFakePackage('foo', packagesDir, isFlutter: true)); + final RepositoryPackage packageBar = RepositoryPackage( + createFakePackage('bar', packagesDir, isFlutter: true)); + _addDependencies(packageBar, ['foo']); + final String originalPubspecContents = + packageBar.pubspecFile.readAsStringSync(); final List output = - await runCapturingPrint(runner, ['pathify']); + await runCapturingPrint(runner, ['make-deps-path-based']); expect( output, containsAllInOrder([ - contains('No target packages'), + contains('No target dependencies'), ]), ); + // The 'foo' reference should not have been modified. + expect(packageBar.pubspecFile.readAsStringSync(), originalPubspecContents); }); test('rewrites references', () async { @@ -98,8 +107,10 @@ void main() { 'bar_platform_interface', ]); - final List output = await runCapturingPrint(runner, - ['pathify', '--target-packages=bar,bar_platform_interface']); + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies=bar,bar_platform_interface' + ]); expect( output, @@ -133,7 +144,7 @@ void main() { ])); }); - group('target-non-breaking-update-packages', () { + group('target-dependencies-with-non-breaking-updates', () { test('no-ops for no published changes', () async { final Directory package = createFakePackage('foo', packagesDir); @@ -149,13 +160,15 @@ void main() { stdout: RepositoryPackage(package).pubspecFile.readAsStringSync()), ]; - final List output = await runCapturingPrint( - runner, ['pathify', '--target-non-breaking-update-packages']); + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); expect( output, containsAllInOrder([ - contains('No target packages'), + contains('No target dependencies'), ]), ); }); @@ -179,8 +192,10 @@ void main() { MockProcess(stdout: gitPubspecContents), ]; - final List output = await runCapturingPrint( - runner, ['pathify', '--target-non-breaking-update-packages']); + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); expect( output, @@ -209,8 +224,10 @@ void main() { MockProcess(stdout: gitPubspecContents), ]; - final List output = await runCapturingPrint( - runner, ['pathify', '--target-non-breaking-update-packages']); + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); expect( output, @@ -239,13 +256,15 @@ void main() { MockProcess(stdout: gitPubspecContents), ]; - final List output = await runCapturingPrint( - runner, ['pathify', '--target-non-breaking-update-packages']); + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); expect( output, containsAllInOrder([ - contains('No target packages'), + contains('No target dependencies'), ]), ); }); @@ -269,13 +288,15 @@ void main() { MockProcess(stdout: gitPubspecContents), ]; - final List output = await runCapturingPrint( - runner, ['pathify', '--target-non-breaking-update-packages']); + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); expect( output, containsAllInOrder([ - contains('No target packages'), + contains('No target dependencies'), ]), ); }); From 78ed7bd34a9dd3b261884d37792c83514140f4ac Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Sat, 4 Dec 2021 10:50:34 -0500 Subject: [PATCH 11/11] Analyzer fix --- script/tool/lib/src/main.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/tool/lib/src/main.dart b/script/tool/lib/src/main.dart index cc25eb6886d3..3e8f19b044dd 100644 --- a/script/tool/lib/src/main.dart +++ b/script/tool/lib/src/main.dart @@ -20,8 +20,8 @@ import 'license_check_command.dart'; import 'lint_android_command.dart'; import 'lint_podspecs_command.dart'; import 'list_command.dart'; -import 'native_test_command.dart'; import 'make_deps_path_based_command.dart'; +import 'native_test_command.dart'; import 'publish_check_command.dart'; import 'publish_plugin_command.dart'; import 'pubspec_check_command.dart';