From db6da7100c0f10d8e84cad7df14e8f7d5be9130c Mon Sep 17 00:00:00 2001 From: Dillon Nys <24740863+dnys1@users.noreply.github.com> Date: Fri, 25 Aug 2023 09:08:56 -0700 Subject: [PATCH] chore(aft): Better handling of pre-release Dart SDK usage (#3569) 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. --- .../aft/lib/src/commands/amplify_command.dart | 4 +- .../lib/src/commands/bootstrap_command.dart | 18 +++ packages/aft/lib/src/config/config.dart | 42 ++++- packages/aft/lib/src/constraints_checker.dart | 20 ++- packages/aft/test/common.dart | 70 +++++++++ .../aft/test/constraints_checker_test.dart | 148 ++++++++++-------- packages/aft/test/model_test.dart | 27 ++++ 7 files changed, 261 insertions(+), 68 deletions(-) create mode 100644 packages/aft/test/common.dart diff --git a/packages/aft/lib/src/commands/amplify_command.dart b/packages/aft/lib/src/commands/amplify_command.dart index 50a957470e..32d9633a88 100644 --- a/packages/aft/lib/src/commands/amplify_command.dart +++ b/packages/aft/lib/src/commands/amplify_command.dart @@ -190,7 +190,9 @@ abstract class AmplifyCommand extends Command if (globalResults?['verbose'] as bool? ?? false) { AWSLogger().logLevel = LogLevel.verbose; } - logger.verbose('Got configuration: $aftConfig'); + logger + ..info('Found Dart SDK: $activeDartSdkVersion') + ..verbose('Got configuration: $aftConfig'); repo = await Repo.open(aftConfig, logger: logger); } diff --git a/packages/aft/lib/src/commands/bootstrap_command.dart b/packages/aft/lib/src/commands/bootstrap_command.dart index d94a89af35..b051da9156 100644 --- a/packages/aft/lib/src/commands/bootstrap_command.dart +++ b/packages/aft/lib/src/commands/bootstrap_command.dart @@ -79,6 +79,24 @@ 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) { + final compatibleWithActiveSdk = pkg.compatibleWithActiveSdk; + if (!compatibleWithActiveSdk) { + logger.info( + 'Skipping package ${pkg.name} since it sets an incompatible Dart SDK constraint: ' + '${pkg.dartSdkConstraint}', + ); + } + return compatibleWithActiveSdk; + }, + ) .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..7b238abfda 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,21 @@ class PackageInfo flavor == PackageFlavor.flutter; } + /// Whether this package is compatible with the active Dart SDK + /// in the current environment. + bool get compatibleWithActiveSdk => + dartSdkConstraint.allows(activeDartSdkVersion); + + /// The Dart SDK constraint set by the package. + VersionConstraint get dartSdkConstraint => + pubspecInfo.pubspec.environment!['sdk']!; + @override List get props => [name]; + @override + String get runtimeTypeName => 'PackageInfo'; + @override int compareTo(PackageInfo other) { return path.compareTo(other.path); @@ -403,3 +412,30 @@ extension DirectoryX on Directory { } } } + +/// The version of the current environment's Dart SDK. +/// +/// This parses the version from calling `dart --version`. +final Version activeDartSdkVersion = () { + final ProcessResult( + :exitCode, + :stdout, + :stderr, + ) = Process.runSync('dart', ['--version']); + if (exitCode != 0) { + throw Exception( + 'Error running `dart --version` ($exitCode): $stderr', + ); + } + // Example output: + // Dart SDK version: 3.1.0 (stable) (Tue Aug 15 21:33:36 2023 +0000) on "macos_arm64" + final activeSdkVersionRaw = _versionRegex.firstMatch(stdout as String); + if (activeSdkVersionRaw == null) { + throw Exception( + 'Running `dart --version` produced unexpected output: $stdout', + ); + } + return Version.parse(activeSdkVersionRaw.group(0)!); +}(); + +final _versionRegex = RegExp(r'\d+\.\d+\.\d+(-[a-zA-Z\d]+)?'); diff --git a/packages/aft/lib/src/constraints_checker.dart b/packages/aft/lib/src/constraints_checker.dart index a7b354b7b3..6854f69e51 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,16 @@ 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) { + // In the special case where we've set a constraint like `^3.2.0-0` + // (i.e. a pre-release constraint), the package is allowed to differ + // from all other packages iff it's not a publishable package. + VersionRange(:final min?) when min.isPreRelease => !package.isPublishable, + + // Otherwise, in all other cases, the package's listed SDK constraint + // must exactly match all other packages. + _ => globalSdkConstraint == localSdkConstraint, + }; if (!satisfiesSdkConstraint) { _processConstraint( package: package, @@ -140,7 +150,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..d33b695faf --- /dev/null +++ b/packages/aft/test/common.dart @@ -0,0 +1,70 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +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..7df04612bf 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('compatibleWithActiveSdk', () { + 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.compatibleWithActiveSdk, + isTrue, + ); + expect( + previewPackage.key.compatibleWithActiveSdk, + currentDartVersion.isPreRelease, + ); + }); + }); }