From 6ccd054c013c66c3f7c5010003fdfe435e1f04f7 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Mon, 8 May 2023 11:11:24 -0400 Subject: [PATCH] [tools] Check version ranges when pathifying deps When making dependencies path-based using `--target-dependencies-with-non-breaking-updates`, there was an edge case where a non-breaking change would cause breaking updates in packages that weren't on the latest version of the target. E.g., this happened when updating Pigeon from 9.0.0 to 9.0.1 and there were still packages using Pigeon 4.x-8.x. Since the purpose of that flag is to find cases (particularly where we use it in CI) where publishing package B would break package A, we don't want to apply it in cases where the new version of B wouldn't be picked up by A anyway. This adds filtering on a per-package level when in this mode, which validates that the on-disk package version is within the referencing package's constraint range before adding an override. Fixes https://github.com/flutter/flutter/issues/121246 --- .../lib/src/make_deps_path_based_command.dart | 64 +++++++++---- .../make_deps_path_based_command_test.dart | 93 ++++++++++++++++++- 2 files changed, 132 insertions(+), 25 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 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';