Skip to content

[tools] Check version ranges when pathifying deps #3943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 1 commit into from
May 8, 2023
Merged

[tools] Check version ranges when pathifying deps #3943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 44 additions & 20 deletions script/tool/lib/src/make_deps_path_based_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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.',
);
}

Expand All @@ -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.');
Expand All @@ -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');
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down
93 changes: 88 additions & 5 deletions script/tool/test/make_deps_path_based_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,29 @@ 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'));
}

/// 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')}
''');
}

Expand Down Expand Up @@ -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';
Expand Down