From 57f7e0993465451e2b3540715c6bfa46593d0da4 Mon Sep 17 00:00:00 2001 From: Dillon Nys Date: Wed, 16 Aug 2023 11:11:41 -0700 Subject: [PATCH] chore(aft): Better handling of pre-release Dart SDK usage Some packages in the repo (for example, the `actions` pkg) set a pre-release Dart SDK constraint so that preview features of Dart can be leveraged. This updates `aft` so that: 1) `bootstrap` skips packages which set this constraint if the current Dart SDK doesn't support it; and 2) Ignores preview constraints in the constraints checker, since these packages do not need to conform to the global setting. --- .../lib/src/commands/bootstrap_command.dart | 9 ++ packages/aft/lib/src/config/config.dart | 17 +- packages/aft/lib/src/constraints_checker.dart | 16 +- packages/aft/test/common.dart | 67 ++++++++ .../aft/test/constraints_checker_test.dart | 148 ++++++++++-------- packages/aft/test/model_test.dart | 27 ++++ 6 files changed, 217 insertions(+), 67 deletions(-) create mode 100644 packages/aft/test/common.dart diff --git a/packages/aft/lib/src/commands/bootstrap_command.dart b/packages/aft/lib/src/commands/bootstrap_command.dart index d94a89af35..25a647a14b 100644 --- a/packages/aft/lib/src/commands/bootstrap_command.dart +++ b/packages/aft/lib/src/commands/bootstrap_command.dart @@ -79,6 +79,15 @@ const amplifyEnvironments = {}; // command is significantly newer/older than the embedded one. (pkg) => pkg.name != 'aft', ) + .where( + // Skip bootstrapping packages which set incompatible Dart SDK constraints, + // e.g. packages which are leveraging preview features. + // + // The problem of packages setting incorrect constraints, for example setting + // `^3.0.5` when the current repo constraint is `^3.0.0` and we're running + // `aft` with `3.0.1` is a different issue handled by the constraints commands. + (pkg) => pkg.compatibleWithCurrentSdk, + ) .expand((pkg) => [pkg, pkg.example, pkg.docs]) .nonNulls; for (final package in bootstrapPackages) { diff --git a/packages/aft/lib/src/config/config.dart b/packages/aft/lib/src/config/config.dart index 43dea9fecb..d0b99feb61 100644 --- a/packages/aft/lib/src/config/config.dart +++ b/packages/aft/lib/src/config/config.dart @@ -317,9 +317,6 @@ class PackageInfo pubspecInfo.pubspec.version ?? (throw StateError('No version for package: $name')); - @override - String get runtimeTypeName => 'PackageInfo'; - /// Skip package checks for Flutter packages when running in CI without /// Flutter, which may happen when testing Dart-only packages or specific /// Dart versions. @@ -330,9 +327,23 @@ class PackageInfo flavor == PackageFlavor.flutter; } + /// Whether this package is compatible with the current Dart version + /// running `aft`. + bool get compatibleWithCurrentSdk { + final dartSdkConstraint = pubspecInfo.pubspec.environment!['sdk']!; + return dartSdkConstraint.allows( + Version.parse(Platform.version.split(_whitespace).first), + ); + } + + static final _whitespace = RegExp(r'\s+'); + @override List get props => [name]; + @override + String get runtimeTypeName => 'PackageInfo'; + @override int compareTo(PackageInfo other) { return path.compareTo(other.path); diff --git a/packages/aft/lib/src/constraints_checker.dart b/packages/aft/lib/src/constraints_checker.dart index a7b354b7b3..862f2fdfa9 100644 --- a/packages/aft/lib/src/constraints_checker.dart +++ b/packages/aft/lib/src/constraints_checker.dart @@ -48,14 +48,15 @@ sealed class ConstraintsChecker { message: errorMessage, ), ); + return false; case ConstraintsAction.apply: case ConstraintsAction.update: package.pubspecInfo.pubspecYamlEditor.update( dependencyPath, expectedConstraint.toString(), ); + return true; } - return false; } } @@ -111,7 +112,12 @@ final class GlobalConstraintChecker extends ConstraintsChecker { // Check Dart SDK contraint final globalSdkConstraint = globalEnvironment.sdk; final localSdkConstraint = environment['sdk']; - final satisfiesSdkConstraint = globalSdkConstraint == localSdkConstraint; + final satisfiesSdkConstraint = switch (localSdkConstraint) { + // Ignore constraints where a pre-release version of Dart is specified + // and the package is not publishable. + VersionRange(:final min?) when min.isPreRelease => !package.isPublishable, + _ => globalSdkConstraint == localSdkConstraint, + }; if (!satisfiesSdkConstraint) { _processConstraint( package: package, @@ -140,7 +146,11 @@ final class GlobalConstraintChecker extends ConstraintsChecker { } } - return satisfiesSdkConstraint && satisfiesFlutterConstraint; + return switch (action) { + ConstraintsAction.check => + satisfiesSdkConstraint && satisfiesFlutterConstraint, + _ => true, + }; } /// Runs [action] for each dependency in [package]. diff --git a/packages/aft/test/common.dart b/packages/aft/test/common.dart new file mode 100644 index 0000000000..9c77fbb063 --- /dev/null +++ b/packages/aft/test/common.dart @@ -0,0 +1,67 @@ +import 'package:aft/aft.dart'; +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'; + +/// Creates a dummy package for testing repo operations. +MapEntry> dummyPackage( + String name, { + Version? version, + bool publishable = true, + VersionConstraint? sdkConstraint, + Map deps = const {}, + Map devDeps = const {}, +}) { + final path = 'packages/$name'; + sdkConstraint ??= VersionConstraint.compatibleWith(Version(3, 0, 0)); + + final pubspecEditor = YamlEditor(''' +name: $name + +environment: + sdk: $sdkConstraint + +dependencies: {} + +dev_dependencies: {} +'''); + + if (version != null) { + pubspecEditor.update(['version'], version.toString()); + } + + void addConstraints( + Map constraints, + DependencyType type, + ) { + for (final MapEntry(key: dep, value: constraint) in constraints.entries) { + final path = [type.key, dep.name]; + pubspecEditor.update(path, constraint.toString()); + } + } + + addConstraints(deps, DependencyType.dependency); + addConstraints(devDeps, DependencyType.devDependency); + + if (!publishable) { + pubspecEditor.update(['publish_to'], 'none'); + } + + final pubspecYaml = pubspecEditor.toString(); + final pubspec = Pubspec.parse(pubspecYaml); + final pubspecMap = loadYamlNode(pubspecYaml) as YamlMap; + + final package = PackageInfo( + name: name, + path: path, + pubspecInfo: PubspecInfo( + pubspec: pubspec, + pubspecYaml: pubspecYaml, + pubspecMap: pubspecMap, + uri: Uri.base.resolve(path), + ), + flavor: PackageFlavor.dart, + ); + return MapEntry(package, [...deps.keys, ...devDeps.keys]); +} diff --git a/packages/aft/test/constraints_checker_test.dart b/packages/aft/test/constraints_checker_test.dart index 9b8f612e9f..066a0ce37b 100644 --- a/packages/aft/test/constraints_checker_test.dart +++ b/packages/aft/test/constraints_checker_test.dart @@ -4,12 +4,97 @@ import 'package:aft/aft.dart'; import 'package:aft/src/constraints_checker.dart'; import 'package:pub_semver/pub_semver.dart'; -import 'package:pubspec_parse/pubspec_parse.dart'; import 'package:test/test.dart'; -import 'package:yaml/yaml.dart'; import 'package:yaml_edit/yaml_edit.dart'; +import 'common.dart'; + void main() { + group('GlobalConstraintsChecker', () { + for (final action in ConstraintsAction.values) { + test( + 'handles SDK constraints for preview Dart versions (${action.name})', + () { + final preReleaseConstraint = VersionConstraint.compatibleWith( + Version(3, 2, 0, pre: '0'), + ); + final actions = dummyPackage( + 'actions', + publishable: false, + sdkConstraint: preReleaseConstraint, + ); + final amplifyFlutter = dummyPackage( + 'amplify_flutter', + sdkConstraint: preReleaseConstraint, + ); + final checker = GlobalConstraintChecker( + action, + const {}, + Environment( + sdk: VersionConstraint.compatibleWith(Version(3, 0, 0)), + ), + ); + + { + expect( + checker.checkConstraints(actions.key), + isTrue, + reason: + 'Package is not publishable and can take a prerelease constraint ' + 'to leverage new Dart features', + ); + expect(checker.mismatchedDependencies, isEmpty); + expect(actions.key.pubspecInfo.pubspecYamlEditor.edits, isEmpty); + } + + { + switch (action) { + case ConstraintsAction.apply || ConstraintsAction.update: + expect( + checker.checkConstraints(amplifyFlutter.key), + isTrue, + ); + expect( + amplifyFlutter.key.pubspecInfo.pubspecYamlEditor.edits.single, + isA().having( + (edit) => edit.replacement.trim(), + 'replacement', + '^3.0.0', + ), + ); + expect(checker.mismatchedDependencies, isEmpty); + case ConstraintsAction.check: + expect( + checker.checkConstraints(amplifyFlutter.key), + isFalse, + reason: + 'Package is publishable and must match the global SDK constraint', + ); + expect( + checker.mismatchedDependencies.single, + isA() + .having( + (err) => err.package.name, + 'packageName', + 'amplify_flutter', + ) + .having( + (err) => err.dependencyName, + 'dependencyName', + 'sdk', + ), + ); + expect( + amplifyFlutter.key.pubspecInfo.pubspecYamlEditor.edits, + isEmpty, + ); + } + } + }, + ); + } + }); + group('PublishConstraintsChecker', () { for (final action in ConstraintsAction.values) { final result = switch (action) { @@ -119,62 +204,3 @@ void main() { } }); } - -MapEntry> dummyPackage( - String name, { - Version? version, - bool publishable = true, - Map deps = const {}, - Map devDeps = const {}, -}) { - final path = 'packages/$name'; - - final pubspecEditor = YamlEditor(''' -name: $name - -environment: - sdk: ^3.0.0 - -dependencies: {} - -dev_dependencies: {} -'''); - - if (version != null) { - pubspecEditor.update(['version'], version.toString()); - } - - void addConstraints( - Map constraints, - DependencyType type, - ) { - for (final MapEntry(key: dep, value: constraint) in constraints.entries) { - final path = [type.key, dep.name]; - pubspecEditor.update(path, constraint.toString()); - } - } - - addConstraints(deps, DependencyType.dependency); - addConstraints(devDeps, DependencyType.devDependency); - - if (!publishable) { - pubspecEditor.update(['publish_to'], 'none'); - } - - final pubspecYaml = pubspecEditor.toString(); - final pubspec = Pubspec.parse(pubspecYaml); - final pubspecMap = loadYamlNode(pubspecYaml) as YamlMap; - - final package = PackageInfo( - name: name, - path: path, - pubspecInfo: PubspecInfo( - pubspec: pubspec, - pubspecYaml: pubspecYaml, - pubspecMap: pubspecMap, - uri: Uri.base.resolve(path), - ), - flavor: PackageFlavor.dart, - ); - return MapEntry(package, [...deps.keys, ...devDeps.keys]); -} diff --git a/packages/aft/test/model_test.dart b/packages/aft/test/model_test.dart index 01c039fa04..f2a9b5fe2c 100644 --- a/packages/aft/test/model_test.dart +++ b/packages/aft/test/model_test.dart @@ -12,10 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. +import 'dart:io'; + import 'package:aft/src/models.dart'; import 'package:pub_semver/pub_semver.dart'; import 'package:test/test.dart'; +import 'common.dart'; + void main() { group('AmplifyVersion', () { const proagation = VersionPropagation.minor; @@ -110,4 +114,27 @@ void main() { expect(proagation.propagateToComponent(version, breaking), true); }); }); + + group('PackageInfo', () { + test('compatibleWithCurrentSdk', () { + final currentDartVersion = Version.parse( + Platform.version.split(RegExp(r'\s+')).first, + ); + final stablePackage = dummyPackage('stable_pkg'); + final previewPackage = dummyPackage( + 'preview_pkg', + sdkConstraint: VersionConstraint.compatibleWith( + Version(3, 2, 0, pre: '0'), + ), + ); + expect( + stablePackage.key.compatibleWithCurrentSdk, + isTrue, + ); + expect( + previewPackage.key.compatibleWithCurrentSdk, + currentDartVersion.isPreRelease, + ); + }); + }); }