From 2f3e3440a4f51c0a72cba43c4bf7b2f29c460a16 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 09:20:47 -0500 Subject: [PATCH 1/7] Add a test for the desired behavior --- .../make_deps_path_based_command_test.dart | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) 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 e846a63fc68..8453625335f 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -184,6 +184,51 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} ])); }); + 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', + ]); + + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies=bar,bar_platform_interface' + ]); + + expect( + output, + containsAll([ + 'Rewriting references to: bar, bar_platform_interface...', + ' Modified packages/bar/bar/pubspec.yaml', + ' Modified packages/bar/bar/example/pubspec.yaml', + ' Modified packages/bar/bar_android/pubspec.yaml', + ])); + expect( + output, + isNot(contains( + ' Modified packages/bar/bar_platform_interface/pubspec.yaml'))); + + expect( + pluginAppFacing.getExamples().first.pubspecFile.readAsLinesSync(), + containsAllInOrder([ + 'dependency_overrides:', + ' bar_android:', + ' path: ../../bar/bar_android', + ' bar_platform_interface:', + ' path: ../../bar/bar_platform_interface', + ])); + }); + test( 'alphabetizes overrides from different sectinos to avoid lint warnings in analysis', () async { From bce8d19e6bfaca90d7c2864727ac10be0ea4be78 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 10:02:23 -0500 Subject: [PATCH 2/7] Update tests to be more flexible, adds better comment --- .../lib/src/make_deps_path_based_command.dart | 6 +- .../make_deps_path_based_command_test.dart | 89 +++++++++++-------- 2 files changed, 57 insertions(+), 38 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 c6032249be2..65ed654837a 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -50,8 +50,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'; 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 8453625335f..45f376f562f 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,28 @@ ${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); + final RepositoryPackage packageB = + createFakePackage('package_b', packagesDir, isFlutter: true); + + addDependencies(packageA, [ + 'package_b', + ]); + + final List output = 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 +166,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,14 +199,10 @@ ${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 { @@ -230,7 +251,7 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} }); 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); @@ -251,18 +272,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 From fbb5b5b569339b096ada55030bb2e56fc6dc5fdb Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 10:20:57 -0500 Subject: [PATCH 3/7] Convert from string manipulation to yaml_edit --- .../lib/src/make_deps_path_based_command.dart | 107 +++++++++--------- .../make_deps_path_based_command_test.dart | 18 ++- 2 files changed, 71 insertions(+), 54 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 65ed654837a..1ee2fadf658 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,8 @@ import 'common/package_command.dart'; import 'common/repository_package.dart'; const int _exitPackageNotFound = 3; -const int _exitCannotUpdatePubspec = 4; -enum _RewriteOutcome { changed, noChangesNeeded, alreadyChanged } +enum _RewriteOutcome { changed, noChangesNeeded } /// Converts all dependencies on target packages to path-based dependencies. /// @@ -90,9 +91,6 @@ class MakeDepsPathBasedCommand extends PackageCommand { case _RewriteOutcome.changed: print(' Modified $displayPath'); break; - case _RewriteOutcome.alreadyChanged: - print(' Skipped $displayPath - Already rewritten'); - break; case _RewriteOutcome.noChangesNeeded: break; } @@ -144,20 +142,9 @@ class MakeDepsPathBasedCommand extends PackageCommand { 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); - } + // Determine the dependencies to be overridden. + final Pubspec pubspec = Pubspec.parse(pubspecContents); final Iterable combinedDependencies = [ ...pubspec.dependencies.keys, ...pubspec.devDependencies.keys, @@ -168,42 +155,60 @@ 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 _RewriteOutcome.noChangesNeeded; + } + + // Find the relative path to the common base. + // XXX + //final String relativeRootPath = + // getRelativePosixPath(repoRoot, from: package.directory); + final String commonBasePath = packagesDir.path; + final int packageDepth = path + .split(path.relative(pubspecFile.parent.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. + // XXX + 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. + pubspecFile.writeAsStringSync(newContent); + return _RewriteOutcome.changed; } /// 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 45f376f562f..4b4340fd5a5 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -310,6 +310,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' @@ -319,10 +325,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', () { From e9b88133455125caf8cff9bc91a99aadc1e68940 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 10:43:23 -0500 Subject: [PATCH 4/7] Add example rewriting, and expand/fix testing of that behavior --- .../lib/src/make_deps_path_based_command.dart | 35 ++++++++---- .../make_deps_path_based_command_test.dart | 57 +++++++++++-------- 2 files changed, 58 insertions(+), 34 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 1ee2fadf658..463e07fa5aa 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -86,7 +86,7 @@ class MakeDepsPathBasedCommand extends PackageCommand { final String displayPath = p.posix.joinAll( path.split(path.relative(pubspec.absolute.path, from: repoRootPath))); final _RewriteOutcome outcome = await _addDependencyOverridesIfNecessary( - pubspec, localDependencyPackages); + RepositoryPackage(pubspec.parent), localDependencyPackages); switch (outcome) { case _RewriteOutcome.changed: print(' Modified $displayPath'); @@ -139,15 +139,23 @@ 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(); + /// + /// If [additionalPackagesToOverride] are provided, they will get + /// dependency_overrides even if there is no direct dependency. This is + /// useful for overriding transitive dependencies. + Future<_RewriteOutcome> _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( @@ -161,12 +169,9 @@ class MakeDepsPathBasedCommand extends PackageCommand { } // Find the relative path to the common base. - // XXX - //final String relativeRootPath = - // getRelativePosixPath(repoRoot, from: package.directory); final String commonBasePath = packagesDir.path; final int packageDepth = path - .split(path.relative(pubspecFile.parent.absolute.path, + .split(path.relative(package.directory.absolute.path, from: commonBasePath)) .length; final List relativeBasePathComponents = @@ -182,7 +187,6 @@ class MakeDepsPathBasedCommand extends PackageCommand { } for (final String packageName in packagesToOverride) { // Find the relative path from the common base to the local package. - // XXX final List repoRelativePathComponents = path.split(path.relative( localDependencies[packageName]!.path, from: commonBasePath)); @@ -207,7 +211,18 @@ $dependencyOverridesKey: } // Write the new pubspec. - pubspecFile.writeAsStringSync(newContent); + 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 _RewriteOutcome.changed; } 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 4b4340fd5a5..65b38e2d9b5 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -221,33 +221,42 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} 'bar_platform_interface', ]); - final List output = await runCapturingPrint(runner, [ - 'make-deps-path-based', - '--target-dependencies=bar,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', ]); - expect( - output, - containsAll([ - 'Rewriting references to: bar, bar_platform_interface...', - ' Modified packages/bar/bar/pubspec.yaml', - ' Modified packages/bar/bar/example/pubspec.yaml', - ' Modified packages/bar/bar_android/pubspec.yaml', - ])); - expect( - output, - isNot(contains( - ' Modified packages/bar/bar_platform_interface/pubspec.yaml'))); + await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies=bar_android,another_package' + ]); - expect( - pluginAppFacing.getExamples().first.pubspecFile.readAsLinesSync(), - containsAllInOrder([ - 'dependency_overrides:', - ' bar_android:', - ' path: ../../bar/bar_android', - ' bar_platform_interface:', - ' path: ../../bar/bar_platform_interface', - ])); + 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( From fceefa1d583fc9d4e86af276406137f4797318c8 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 10:54:36 -0500 Subject: [PATCH 5/7] Better warning output --- script/tool/lib/src/make_deps_path_based_command.dart | 1 + 1 file changed, 1 insertion(+) 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 463e07fa5aa..0cc6faa4645 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -205,6 +205,7 @@ class MakeDepsPathBasedCommand extends PackageCommand { String newContent = editablePubspec.toString(); if (!newContent.contains(_dependencyOverrideWarningComment)) { newContent = newContent.replaceFirst('$dependencyOverridesKey:', ''' + $_dependencyOverrideWarningComment $dependencyOverridesKey: '''); From bd670fd80836721c1e87472878ee9285cec05a84 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 11:04:24 -0500 Subject: [PATCH 6/7] Remove the enum that is no longer needed --- .../lib/src/make_deps_path_based_command.dart | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 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 0cc6faa4645..83ef037ae6a 100644 --- a/script/tool/lib/src/make_deps_path_based_command.dart +++ b/script/tool/lib/src/make_deps_path_based_command.dart @@ -16,8 +16,6 @@ import 'common/repository_package.dart'; const int _exitPackageNotFound = 3; -enum _RewriteOutcome { changed, noChangesNeeded } - /// Converts all dependencies on target packages to path-based dependencies. /// /// This is to allow for pre-publish testing of changes that could affect other @@ -85,14 +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( + final bool changed = await _addDependencyOverridesIfNecessary( RepositoryPackage(pubspec.parent), localDependencyPackages); - switch (outcome) { - case _RewriteOutcome.changed: - print(' Modified $displayPath'); - break; - case _RewriteOutcome.noChangesNeeded: - break; + if (changed) { + print(' Modified $displayPath'); } } } @@ -140,10 +134,12 @@ class MakeDepsPathBasedCommand extends PackageCommand { /// adds dependency_overrides entries to redirect them to the local version /// using path-based dependencies. /// + /// 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<_RewriteOutcome> _addDependencyOverridesIfNecessary( + Future _addDependencyOverridesIfNecessary( RepositoryPackage package, Map localDependencies, { Iterable additionalPackagesToOverride = const {}, @@ -165,7 +161,7 @@ class MakeDepsPathBasedCommand extends PackageCommand { packagesToOverride.sort(); if (packagesToOverride.isEmpty) { - return _RewriteOutcome.noChangesNeeded; + return false; } // Find the relative path to the common base. @@ -224,7 +220,7 @@ $dependencyOverridesKey: additionalPackagesToOverride: packagesToOverride); } - return _RewriteOutcome.changed; + return true; } /// Returns all pubspecs anywhere under the packages directory. From 23a4502cf78390600dffa2eabc4eb80864d972ba Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 6 Mar 2023 11:04:51 -0500 Subject: [PATCH 7/7] Analyzer cleanup --- script/tool/test/make_deps_path_based_command_test.dart | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 65b38e2d9b5..dd248cd9c94 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -105,14 +105,13 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} test('includes explanatory comment', () async { final RepositoryPackage packageA = createFakePackage('package_a', packagesDir, isFlutter: true); - final RepositoryPackage packageB = - createFakePackage('package_b', packagesDir, isFlutter: true); + createFakePackage('package_b', packagesDir, isFlutter: true); addDependencies(packageA, [ 'package_b', ]); - final List output = await runCapturingPrint(runner, + await runCapturingPrint(runner, ['make-deps-path-based', '--target-dependencies=package_b']); expect(