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 83ef037ae6a..93953871fdb 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,7 @@ 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:pubspec_parse/pubspec_parse.dart'; import 'package:yaml/yaml.dart'; import 'package:yaml_edit/yaml_edit.dart'; @@ -40,7 +41,10 @@ class MakeDepsPathBasedCommand extends PackageCommand { _targetDependenciesWithNonBreakingUpdatesArg, help: 'Causes all packages that have non-breaking version changes ' 'when compared against the git base to be treated as target ' - 'packages.', + 'packages.\n\nOnly packages with dependency constraints that allow ' + 'the new version of a given target package will be updated. E.g., ' + 'if package A depends on B: ^1.0.0, and B is updated from 2.0.0 to ' + '2.0.1, the dependency on B in A will not become path based.', ); } @@ -65,10 +69,11 @@ class MakeDepsPathBasedCommand extends PackageCommand { @override Future run() async { - final Set targetDependencies = - getBoolArg(_targetDependenciesWithNonBreakingUpdatesArg) - ? await _getNonBreakingUpdatePackages() - : getStringListArg(_targetDependenciesArg).toSet(); + final bool targetByVersion = + getBoolArg(_targetDependenciesWithNonBreakingUpdatesArg); + final Set targetDependencies = targetByVersion + ? await _getNonBreakingUpdatePackages() + : getStringListArg(_targetDependenciesArg).toSet(); if (targetDependencies.isEmpty) { print('No target dependencies; nothing to do.'); @@ -78,13 +83,24 @@ class MakeDepsPathBasedCommand extends PackageCommand { final Map localDependencyPackages = _findLocalPackages(targetDependencies); + // For targeting by version change, find the versions of the target + // dependencies. + final Map localPackageVersions = targetByVersion + ? { + for (final RepositoryPackage package + in localDependencyPackages.values) + package.directory.basename: package.parsePubspec().version + } + : {}; final String repoRootPath = (await gitDir).path; for (final File pubspec in await _getAllPubspecs()) { final String displayPath = p.posix.joinAll( path.split(path.relative(pubspec.absolute.path, from: repoRootPath))); final bool changed = await _addDependencyOverridesIfNecessary( - RepositoryPackage(pubspec.parent), localDependencyPackages); + RepositoryPackage(pubspec.parent), + localDependencyPackages, + localPackageVersions); if (changed) { print(' Modified $displayPath'); } @@ -141,16 +157,33 @@ class MakeDepsPathBasedCommand extends PackageCommand { /// useful for overriding transitive dependencies. Future _addDependencyOverridesIfNecessary( RepositoryPackage package, - Map localDependencies, { + Map localDependencies, + Map versions, { Iterable additionalPackagesToOverride = const {}, }) async { final String pubspecContents = package.pubspecFile.readAsStringSync(); + // Returns true if [dependency] allows a dependency on [version]. Always + // returns true if [version] is null, to err on the side of assuming it + // will apply in cases where we don't have a target version. + bool allowsVersion(Dependency dependency, Version? version) { + return version == null || + dependency is! HostedDependency || + dependency.version.allows(version); + } + // Determine the dependencies to be overridden. final Pubspec pubspec = Pubspec.parse(pubspecContents); final Iterable combinedDependencies = [ - ...pubspec.dependencies.keys, - ...pubspec.devDependencies.keys, + // Filter out any dependencies with version constraint that wouldn't allow + // the target if published. + ...>[ + ...pubspec.dependencies.entries, + ...pubspec.devDependencies.entries, + ] + .where((MapEntry element) => + allowsVersion(element.value, versions[element.key])) + .map((MapEntry entry) => entry.key), ...additionalPackagesToOverride, ]; final List packagesToOverride = combinedDependencies @@ -216,7 +249,7 @@ $dependencyOverridesKey: // 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, + _addDependencyOverridesIfNecessary(example, localDependencies, versions, additionalPackagesToOverride: packagesToOverride); } @@ -271,15 +304,6 @@ $dependencyOverridesKey: print(' Skipping $packageName; no non-breaking version change.'); continue; } - // TODO(stuartmorgan): Remove this special-casing once this tool checks - // for major version differences relative to the dependencies being - // updated rather than the version change in the PR: - // https://github.com/flutter/flutter/issues/121246 - if (packageName == 'pigeon') { - print(' Skipping $packageName; see ' - 'https://github.com/flutter/flutter/issues/121246'); - continue; - } changedPackages.add(packageName); } return changedPackages; @@ -304,7 +328,7 @@ $dependencyOverridesKey: 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 + // Breaking changes aren't targeted since they won't be picked up // automatically. return false; } 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 05ca0ca6aab..1f812005aaa 100644 --- a/script/tool/test/make_deps_path_based_command_test.dart +++ b/script/tool/test/make_deps_path_based_command_test.dart @@ -48,13 +48,14 @@ void main() { /// Adds dummy 'dependencies:' entries for each package in [dependencies] /// to [package]. - void addDependencies( - RepositoryPackage package, Iterable dependencies) { + void addDependencies(RepositoryPackage package, Iterable dependencies, + {String constraint = '<2.0.0'}) { 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', + for (final String dependency in dependencies) + ' $dependency: $constraint', ]); package.pubspecFile.writeAsStringSync(lines.join('\n')); } @@ -62,13 +63,14 @@ void main() { /// Adds a 'dev_dependencies:' section with entries for each package in /// [dependencies] to [package]. void addDevDependenciesSection( - RepositoryPackage package, Iterable devDependencies) { + RepositoryPackage package, Iterable devDependencies, + {String constraint = '<2.0.0'}) { final String originalContent = package.pubspecFile.readAsStringSync(); package.pubspecFile.writeAsStringSync(''' $originalContent dev_dependencies: -${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} +${devDependencies.map((String dep) => ' $dep: $constraint').join('\n')} '''); } @@ -523,6 +525,87 @@ ${devDependencies.map((String dep) => ' $dep: ^1.0.0').join('\n')} ); }); + test('does not update references with an older major version', () async { + const String newVersion = '2.0.1'; + final RepositoryPackage targetPackage = + createFakePackage('foo', packagesDir, version: newVersion); + final RepositoryPackage referencingPackage = + createFakePackage('bar', packagesDir); + + // For a dependency on ^1.0.0, the 2.0.0->2.0.1 update should not apply. + addDependencies(referencingPackage, ['foo'], + constraint: '^1.0.0'); + + final File pubspecFile = targetPackage.pubspecFile; + final String changedFileOutput = [ + pubspecFile, + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo(MockProcess(stdout: changedFileOutput)), + ]; + final String gitPubspecContents = + pubspecFile.readAsStringSync().replaceAll(newVersion, '2.0.0'); + processRunner.mockProcessesForExecutable['git-show'] = [ + FakeProcessInfo(MockProcess(stdout: gitPubspecContents)), + ]; + + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); + + final Pubspec referencingPubspec = referencingPackage.parsePubspec(); + + expect( + output, + containsAllInOrder([ + contains('Rewriting references to: foo'), + ]), + ); + expect(referencingPubspec.dependencyOverrides.isEmpty, true); + }); + + test('does update references with a matching version range', () async { + const String newVersion = '2.0.1'; + final RepositoryPackage targetPackage = + createFakePackage('foo', packagesDir, version: newVersion); + final RepositoryPackage referencingPackage = + createFakePackage('bar', packagesDir); + + // For a dependency on ^1.0.0, the 2.0.0->2.0.1 update should not apply. + addDependencies(referencingPackage, ['foo'], + constraint: '^2.0.0'); + + final File pubspecFile = targetPackage.pubspecFile; + final String changedFileOutput = [ + pubspecFile, + ].map((File file) => file.path).join('\n'); + processRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo(MockProcess(stdout: changedFileOutput)), + ]; + final String gitPubspecContents = + pubspecFile.readAsStringSync().replaceAll(newVersion, '2.0.0'); + processRunner.mockProcessesForExecutable['git-show'] = [ + FakeProcessInfo(MockProcess(stdout: gitPubspecContents)), + ]; + + final List output = await runCapturingPrint(runner, [ + 'make-deps-path-based', + '--target-dependencies-with-non-breaking-updates' + ]); + + final Pubspec referencingPubspec = referencingPackage.parsePubspec(); + + expect( + output, + containsAllInOrder([ + contains('Rewriting references to: foo'), + ]), + ); + expect(referencingPubspec.dependencyOverrides['foo'] is PathDependency, + true); + }); + test('skips anything outside of the packages directory', () async { final Directory toolDir = packagesDir.parent.childDirectory('tool'); const String newVersion = '1.1.0';