From 79fbb7ade9d861dbafeeb8eef3c7000a2f5f7d12 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Fri, 10 Mar 2023 12:33:47 -0800 Subject: [PATCH] [tool] Include examples when pathifying deps (#3393) Improves the `make-deps-path-based` command: - When adding an override to a package, also adds it to that package's examples. This is needed for integration tests of app-facing packages of federated plugins when adding features to implementation packages, for instance. - Switches from string-based rewrites to `yaml_edit` to make it more robust (necessary to do the above without major restructuring) - Improves the auto-added comment since reviewers new to the repository are often confused by the overrides the first time they encounter them and think their inclusion in the PR is a mistake. Fixes https://github.com/flutter/flutter/issues/111091 --- .../lib/src/make_deps_path_based_command.dart | 149 +++++++++------- .../make_deps_path_based_command_test.dart | 160 +++++++++++++----- 2 files changed, 205 insertions(+), 104 deletions(-) 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 c6032249be..83ef037ae6 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -6,6 +6,8 @@ import 'package:file/file.dart'; import 'package:git/git.dart'; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; +import 'package:yaml/yaml.dart'; +import 'package:yaml_edit/yaml_edit.dart'; import 'common/core.dart'; import 'common/git_version_finder.dart'; @@ -13,9 +15,6 @@ import 'common/package_command.dart'; import 'common/repository_package.dart'; const int _exitPackageNotFound = 3; -const int _exitCannotUpdatePubspec = 4; - -enum _RewriteOutcome { changed, noChangesNeeded, alreadyChanged } /// Converts all dependencies on target packages to path-based dependencies. /// @@ -50,8 +49,12 @@ class MakeDepsPathBasedCommand extends PackageCommand { 'target-dependencies-with-non-breaking-updates'; // The comment to add to temporary dependency overrides. + // + // 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 ONLY. DO NOT MERGE.'; + '# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.\n' + '# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins'; @override final String name = 'make-deps-path-based'; @@ -80,17 +83,10 @@ class MakeDepsPathBasedCommand extends PackageCommand { for (final File pubspec in await _getAllPubspecs()) { final String displayPath = p.posix.joinAll( path.split(path.relative(pubspec.absolute.path, from: repoRootPath))); - final _RewriteOutcome outcome = await _addDependencyOverridesIfNecessary( - pubspec, localDependencyPackages); - switch (outcome) { - case _RewriteOutcome.changed: - print(' Modified $displayPath'); - break; - case _RewriteOutcome.alreadyChanged: - print(' Skipped $displayPath - Already rewritten'); - break; - case _RewriteOutcome.noChangesNeeded: - break; + final bool changed = await _addDependencyOverridesIfNecessary( + RepositoryPackage(pubspec.parent), localDependencyPackages); + if (changed) { + print(' Modified $displayPath'); } } } @@ -137,26 +133,25 @@ class MakeDepsPathBasedCommand extends PackageCommand { /// If [pubspecFile] has any dependencies on packages in [localDependencies], /// adds dependency_overrides entries to redirect them to the local version /// using path-based dependencies. - Future<_RewriteOutcome> _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, other than ones - // created by this script. 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) { - if (pubspecContents.contains(_dependencyOverrideWarningComment)) { - return _RewriteOutcome.alreadyChanged; - } - printError( - 'Packages with dependency overrides are not currently supported.'); - throw ToolExit(_exitCannotUpdatePubspec); - } + /// + /// Returns true if any overrides were added. + /// + /// If [additionalPackagesToOverride] are provided, they will get + /// dependency_overrides even if there is no direct dependency. This is + /// useful for overriding transitive dependencies. + Future _addDependencyOverridesIfNecessary( + RepositoryPackage package, + Map localDependencies, { + Iterable additionalPackagesToOverride = const {}, + }) async { + final String pubspecContents = package.pubspecFile.readAsStringSync(); + // Determine the dependencies to be overridden. + final Pubspec pubspec = Pubspec.parse(pubspecContents); final Iterable combinedDependencies = [ ...pubspec.dependencies.keys, ...pubspec.devDependencies.keys, + ...additionalPackagesToOverride, ]; final List packagesToOverride = combinedDependencies .where( @@ -164,42 +159,68 @@ class MakeDepsPathBasedCommand extends PackageCommand { .toList(); // Sort the combined list to avoid sort_pub_dependencies lint violations. packagesToOverride.sort(); - 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.absolute.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 + + if (packagesToOverride.isEmpty) { + return false; + } + + // Find the relative path to the common base. + final String commonBasePath = packagesDir.path; + final int packageDepth = path + .split(path.relative(package.directory.absolute.path, + from: commonBasePath)) + .length; + final List relativeBasePathComponents = + List.filled(packageDepth, '..'); + + // Add the overrides. + final YamlEditor editablePubspec = YamlEditor(pubspecContents); + final YamlNode root = editablePubspec.parseAt([]); + const String dependencyOverridesKey = 'dependency_overrides'; + // Ensure that there's a `dependencyOverridesKey` entry to update. + if ((root as YamlMap)[dependencyOverridesKey] == null) { + editablePubspec.update([dependencyOverridesKey], YamlMap()); + } + for (final String packageName in packagesToOverride) { + // Find the relative path from the common base to the local package. + final List repoRelativePathComponents = path.split(path.relative( + localDependencies[packageName]!.path, + from: commonBasePath)); + editablePubspec.update([ + dependencyOverridesKey, + packageName + ], { + 'path': p.posix.joinAll([ + ...relativeBasePathComponents, + ...repoRelativePathComponents, + ]) + }); + } + + // Add the warning if it's not already there. + String newContent = editablePubspec.toString(); + if (!newContent.contains(_dependencyOverrideWarningComment)) { + newContent = newContent.replaceFirst('$dependencyOverridesKey:', ''' $_dependencyOverrideWarningComment -dependency_overrides: -'''; - for (final String packageName in packagesToOverride) { - // Find the relative path from the common base to the local package. - final List repoRelativePathComponents = path.split( - path.relative(localDependencies[packageName]!.path, - from: commonBasePath)); - newPubspecContents += ''' - $packageName: - path: ${p.posix.joinAll([ - ...relativeBasePathComponents, - ...repoRelativePathComponents, - ])} -'''; - } - pubspecFile.writeAsStringSync(newPubspecContents); - return _RewriteOutcome.changed; +$dependencyOverridesKey: +'''); } - return _RewriteOutcome.noChangesNeeded; + + // Write the new pubspec. + package.pubspecFile.writeAsStringSync(newContent); + + // Update any examples. This is important for cases like integration tests + // of app-facing packages in federated plugins, where the app-facing + // package depends directly on the implementation packages, but the + // example app doesn't. Since integration tests are run in the example app, + // it needs the overrides in order for tests to pass. + for (final RepositoryPackage example in package.getExamples()) { + _addDependencyOverridesIfNecessary(example, localDependencies, + additionalPackagesToOverride: packagesToOverride); + } + + return true; } /// Returns all pubspecs anywhere under the packages directory. diff --git a/script/tool/test/make_deps_path_based_command_test.dart b/script/tool/test/make_deps_path_based_command_test.dart index e846a63fc6..dd248cd9c9 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -9,6 +9,7 @@ import 'package:file/file.dart'; import 'package:file/memory.dart'; import 'package:flutter_plugin_tools/src/make_deps_path_based_command.dart'; import 'package:mockito/mockito.dart'; +import 'package:pubspec_parse/pubspec_parse.dart'; import 'package:test/test.dart'; import 'common/package_command_test.mocks.dart'; @@ -73,6 +74,13 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} '''); } + Map getDependencyOverrides(RepositoryPackage package) { + final Pubspec pubspec = package.parsePubspec(); + return pubspec.dependencyOverrides.map((String name, Dependency dep) => + MapEntry( + name, (dep is PathDependency) ? dep.path : dep.toString())); + } + test('no-ops for no plugins', () async { createFakePackage('foo', packagesDir, isFlutter: true); final RepositoryPackage packageBar = @@ -94,6 +102,27 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} expect(packageBar.pubspecFile.readAsStringSync(), originalPubspecContents); }); + test('includes explanatory comment', () async { + final RepositoryPackage packageA = + createFakePackage('package_a', packagesDir, isFlutter: true); + createFakePackage('package_b', packagesDir, isFlutter: true); + + addDependencies(packageA, [ + 'package_b', + ]); + + await runCapturingPrint(runner, + ['make-deps-path-based', '--target-dependencies=package_b']); + + expect( + packageA.pubspecFile.readAsLinesSync(), + containsAllInOrder([ + '# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.', + '# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins', + 'dependency_overrides:', + ])); + }); + test('rewrites "dependencies" references', () async { final RepositoryPackage simplePackage = createFakePackage('foo', packagesDir, isFlutter: true); @@ -136,23 +165,18 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} 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', - ])); + final Map simplePackageOverrides = + getDependencyOverrides(simplePackage); + expect(simplePackageOverrides.length, 2); + expect(simplePackageOverrides['bar'], '../bar/bar'); + expect(simplePackageOverrides['bar_platform_interface'], + '../bar/bar_platform_interface'); + + final Map appFacingPackageOverrides = + getDependencyOverrides(pluginAppFacing); + expect(appFacingPackageOverrides.length, 1); + expect(appFacingPackageOverrides['bar_platform_interface'], + '../../bar/bar_platform_interface'); }); test('rewrites "dev_dependencies" references', () async { @@ -174,18 +198,68 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} ' Modified packages/foo_builder/pubspec.yaml', ])); - expect( - builderPackage.pubspecFile.readAsLinesSync(), - containsAllInOrder([ - '# FOR TESTING ONLY. DO NOT MERGE.', - 'dependency_overrides:', - ' foo:', - ' path: ../foo', - ])); + final Map overrides = + getDependencyOverrides(builderPackage); + expect(overrides.length, 1); + expect(overrides['foo'], '../foo'); + }); + + test('rewrites examples when rewriting the main package', () async { + final Directory pluginGroup = packagesDir.childDirectory('bar'); + createFakePackage('bar_platform_interface', pluginGroup, isFlutter: true); + final RepositoryPackage pluginImplementation = + createFakePlugin('bar_android', pluginGroup); + final RepositoryPackage pluginAppFacing = + createFakePlugin('bar', pluginGroup); + + addDependencies(pluginAppFacing, [ + 'bar_platform_interface', + 'bar_android', + ]); + addDependencies(pluginImplementation, [ + 'bar_platform_interface', + ]); + + await runCapturingPrint(runner, + ['make-deps-path-based', '--target-dependencies=bar_android']); + + final Map exampleOverrides = + getDependencyOverrides(pluginAppFacing.getExamples().first); + expect(exampleOverrides.length, 1); + expect(exampleOverrides['bar_android'], '../../../bar/bar_android'); + }); + + test('example overrides include both local and main-package dependencies', + () async { + final Directory pluginGroup = packagesDir.childDirectory('bar'); + createFakePackage('bar_platform_interface', pluginGroup, isFlutter: true); + createFakePlugin('bar_android', pluginGroup); + final RepositoryPackage pluginAppFacing = + createFakePlugin('bar', pluginGroup); + createFakePackage('another_package', packagesDir); + + addDependencies(pluginAppFacing, [ + 'bar_platform_interface', + 'bar_android', + ]); + addDependencies(pluginAppFacing.getExamples().first, [ + 'another_package', + ]); + + await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies=bar_android,another_package' + ]); + + final Map exampleOverrides = + getDependencyOverrides(pluginAppFacing.getExamples().first); + expect(exampleOverrides.length, 2); + expect(exampleOverrides['another_package'], '../../../another_package'); + expect(exampleOverrides['bar_android'], '../../../bar/bar_android'); }); test( - 'alphabetizes overrides from different sectinos to avoid lint warnings in analysis', + 'alphabetizes overrides from different sections to avoid lint warnings in analysis', () async { createFakePackage('a', packagesDir); createFakePackage('b', packagesDir); @@ -206,18 +280,12 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} ' Modified packages/target/pubspec.yaml', ])); + // This matches with a regex in order to all for either flow style or + // expanded style output. expect( - targetPackage.pubspecFile.readAsLinesSync(), - containsAllInOrder([ - '# FOR TESTING ONLY. DO NOT MERGE.', - 'dependency_overrides:', - ' a:', - ' path: ../a', - ' b:', - ' path: ../b', - ' c:', - ' path: ../c', - ])); + targetPackage.pubspecFile.readAsStringSync(), + matches(RegExp(r'dependency_overrides:.*a:.*b:.*c:.*', + multiLine: true, dotAll: true))); }); // This test case ensures that running CI using this command on an interim @@ -250,6 +318,12 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} 'make-deps-path-based', '--target-dependencies=bar,bar_platform_interface' ]); + final String simplePackageUpdatedContent = + simplePackage.pubspecFile.readAsStringSync(); + final String appFacingPackageUpdatedContent = + pluginAppFacing.pubspecFile.readAsStringSync(); + final String implementationPackageUpdatedContent = + pluginImplementation.pubspecFile.readAsStringSync(); final List output = await runCapturingPrint(runner, [ 'make-deps-path-based', '--target-dependencies=bar,bar_platform_interface' @@ -259,10 +333,16 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} output, containsAll([ 'Rewriting references to: bar, bar_platform_interface...', - ' Skipped packages/bar/bar/pubspec.yaml - Already rewritten', - ' Skipped packages/bar/bar_android/pubspec.yaml - Already rewritten', - ' Skipped packages/foo/pubspec.yaml - Already rewritten', + ' Modified packages/bar/bar/pubspec.yaml', + ' Modified packages/bar/bar_android/pubspec.yaml', + ' Modified packages/foo/pubspec.yaml', ])); + expect(simplePackageUpdatedContent, + simplePackage.pubspecFile.readAsStringSync()); + expect(appFacingPackageUpdatedContent, + pluginAppFacing.pubspecFile.readAsStringSync()); + expect(implementationPackageUpdatedContent, + pluginImplementation.pubspecFile.readAsStringSync()); }); group('target-dependencies-with-non-breaking-updates', () {