flutter/packages
1
0
Fork 0
mirror of https://github.com/flutter/packages.git synced 2025-05-31 05:30:36 +08:00
Code Issues Packages Projects Releases Wiki Activity

[tools] Check version ranges when pathifying deps (#3943)

Browse Source 
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
This commit is contained in:
stuartmorgan
2023-05-08 14:04:04 -07:00
committed by GitHub
parent 98c7062867
commit 090a3afad6
2 changed files with 132 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

64
script/tool/lib/src/make_deps_path_based_command.dart
View File

@ -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<void> run() async {
final Set<String> targetDependencies =
getBoolArg(_targetDependenciesWithNonBreakingUpdatesArg)
? await _getNonBreakingUpdatePackages()
: getStringListArg(_targetDependenciesArg).toSet();
final bool targetByVersion =
getBoolArg(_targetDependenciesWithNonBreakingUpdatesArg);
final Set<String> 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<String, RepositoryPackage> localDependencyPackages =
_findLocalPackages(targetDependencies);
// For targeting by version change, find the versions of the target
// dependencies.
final Map<String, Version?> localPackageVersions = targetByVersion
? <String, Version?>{
for (final RepositoryPackage package
in localDependencyPackages.values)
package.directory.basename: package.parsePubspec().version
}
: <String, 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<bool> _addDependencyOverridesIfNecessary(
RepositoryPackage package,
Map<String, RepositoryPackage> localDependencies, {
Map<String, RepositoryPackage> localDependencies,
Map<String, Version?> versions, {
Iterable<String> additionalPackagesToOverride = const <String>{},
}) 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<String> combinedDependencies = <String>[
...pubspec.dependencies.keys,
...pubspec.devDependencies.keys,
// Filter out any dependencies with version constraint that wouldn't allow
// the target if published.
...<MapEntry<String, Dependency>>[
...pubspec.dependencies.entries,
...pubspec.devDependencies.entries,
]
.where((MapEntry<String, Dependency> element) =>
allowsVersion(element.value, versions[element.key]))
.map((MapEntry<String, Dependency> entry) => entry.key),
...additionalPackagesToOverride,
];
final List<String> 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;
}

93
script/tool/test/make_deps_path_based_command_test.dart
View File

@ -48,13 +48,14 @@ void main() {
/// Adds dummy 'dependencies:' entries for each package in [dependencies]
/// to [package].
void addDependencies(
RepositoryPackage package, Iterable<String> dependencies) {
void addDependencies(RepositoryPackage package, Iterable<String> dependencies,
{String constraint = '<2.0.0'}) {
final List<String> lines = package.pubspecFile.readAsLinesSync();
final int dependenciesStartIndex = lines.indexOf('dependencies:');
assert(dependenciesStartIndex != -1);
lines.insertAll(dependenciesStartIndex + 1, <String>[
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<String> devDependencies) {
RepositoryPackage package, Iterable<String> 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, <String>['foo'],
constraint: '^1.0.0');
final File pubspecFile = targetPackage.pubspecFile;
final String changedFileOutput = <File>[
pubspecFile,
].map((File file) => file.path).join('\n');
processRunner.mockProcessesForExecutable['git-diff'] = <FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: changedFileOutput)),
];
final String gitPubspecContents =
pubspecFile.readAsStringSync().replaceAll(newVersion, '2.0.0');
processRunner.mockProcessesForExecutable['git-show'] = <FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: gitPubspecContents)),
];
final List<String> output = await runCapturingPrint(runner, <String>[
'make-deps-path-based',
'--target-dependencies-with-non-breaking-updates'
]);
final Pubspec referencingPubspec = referencingPackage.parsePubspec();
expect(
output,
containsAllInOrder(<Matcher>[
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, <String>['foo'],
constraint: '^2.0.0');
final File pubspecFile = targetPackage.pubspecFile;
final String changedFileOutput = <File>[
pubspecFile,
].map((File file) => file.path).join('\n');
processRunner.mockProcessesForExecutable['git-diff'] = <FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: changedFileOutput)),
];
final String gitPubspecContents =
pubspecFile.readAsStringSync().replaceAll(newVersion, '2.0.0');
processRunner.mockProcessesForExecutable['git-show'] = <FakeProcessInfo>[
FakeProcessInfo(MockProcess(stdout: gitPubspecContents)),
];
final List<String> output = await runCapturingPrint(runner, <String>[
'make-deps-path-based',
'--target-dependencies-with-non-breaking-updates'
]);
final Pubspec referencingPubspec = referencingPackage.parsePubspec();
expect(
output,
containsAllInOrder(<Matcher>[
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';