From bf02d2352d0b0884ba8fbf8cd9bd668ea41a729c Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 4 Sep 2024 12:20:45 -0400 Subject: [PATCH 1/2] fix(shorebird_cli): fix an issue where ios releases could not properly parse a flutter hash --- .../src/commands/release/ios_releaser.dart | 6 +- .../src/commands/release/release_command.dart | 10 +- .../lib/src/shorebird_flutter.dart | 36 ++++++ .../commands/release/ios_releaser_test.dart | 5 + .../release/release_command_test.dart | 28 +---- .../test/src/shorebird_flutter_test.dart | 105 ++++++++++++++++++ 6 files changed, 153 insertions(+), 37 deletions(-) diff --git a/packages/shorebird_cli/lib/src/commands/release/ios_releaser.dart b/packages/shorebird_cli/lib/src/commands/release/ios_releaser.dart index 712bca6e3..2d97ebd7e 100644 --- a/packages/shorebird_cli/lib/src/commands/release/ios_releaser.dart +++ b/packages/shorebird_cli/lib/src/commands/release/ios_releaser.dart @@ -4,7 +4,6 @@ import 'package:crypto/crypto.dart'; import 'package:mason_logger/mason_logger.dart'; import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; -import 'package:pub_semver/pub_semver.dart'; import 'package:shorebird_cli/src/archive_analysis/plist.dart'; import 'package:shorebird_cli/src/artifact_builder.dart'; import 'package:shorebird_cli/src/artifact_manager.dart'; @@ -82,8 +81,9 @@ To change the version of this release, change your app's version in your pubspec final flutterVersionArg = argResults['flutter-version'] as String?; if (flutterVersionArg != null) { - if (Version.parse(flutterVersionArg) < - minimumSupportedIosFlutterVersion) { + final version = + await shorebirdFlutter.resolveFlutterVersion(flutterVersionArg); + if (version != null && version < minimumSupportedIosFlutterVersion) { logger.err( ''' iOS releases are not supported with Flutter versions older than $minimumSupportedIosFlutterVersion. diff --git a/packages/shorebird_cli/lib/src/commands/release/release_command.dart b/packages/shorebird_cli/lib/src/commands/release/release_command.dart index d1c8a273d..2fa8c6a90 100644 --- a/packages/shorebird_cli/lib/src/commands/release/release_command.dart +++ b/packages/shorebird_cli/lib/src/commands/release/release_command.dart @@ -9,7 +9,6 @@ import 'package:shorebird_cli/src/commands/release/release.dart'; import 'package:shorebird_cli/src/common_arguments.dart'; import 'package:shorebird_cli/src/config/config.dart'; import 'package:shorebird_cli/src/extensions/arg_results.dart'; -import 'package:shorebird_cli/src/extensions/version.dart'; import 'package:shorebird_cli/src/logger.dart'; import 'package:shorebird_cli/src/metadata/metadata.dart'; import 'package:shorebird_cli/src/platform.dart'; @@ -306,16 +305,9 @@ of the iOS app that is using this module. (aar and ios-framework only)''', return shorebirdEnv.flutterRevision; } - final parsedVersion = tryParseVersion(flutterVersionArg!); - if (parsedVersion == null) { - // If we fail to parse flutterVersionArg as a semver version, attempt to - // use the provided value as a revision. - return flutterVersionArg!; - } - final String? revision; try { - revision = await shorebirdFlutter.getRevisionForVersion( + revision = await shorebirdFlutter.resolveFlutterRevision( flutterVersionArg!, ); } catch (error) { diff --git a/packages/shorebird_cli/lib/src/shorebird_flutter.dart b/packages/shorebird_cli/lib/src/shorebird_flutter.dart index 4063e750c..b4ebb69f9 100644 --- a/packages/shorebird_cli/lib/src/shorebird_flutter.dart +++ b/packages/shorebird_cli/lib/src/shorebird_flutter.dart @@ -7,6 +7,7 @@ import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:scoped_deps/scoped_deps.dart'; import 'package:shorebird_cli/src/executables/executables.dart'; +import 'package:shorebird_cli/src/extensions/version.dart'; import 'package:shorebird_cli/src/logger.dart'; import 'package:shorebird_cli/src/platform.dart'; import 'package:shorebird_cli/src/shorebird_env.dart'; @@ -205,6 +206,41 @@ class ShorebirdFlutter { .firstOrNull; } + /// Translates [versionOrHash] into a Flutter revision. If this is a semver + /// version, it will simply parse that into a [Version]. If not, it will + /// attempt to look up the Flutter version for the provided revision hash and + /// return the hash if a version is found, or null if not. + Future resolveFlutterRevision(String versionOrHash) async { + final parsedVersion = tryParseVersion(versionOrHash); + if (parsedVersion != null) { + return getRevisionForVersion(versionOrHash); + } + + // If we were unable to parse the version, assume it's a revision hash. + final version = await getVersionForRevision(flutterRevision: versionOrHash); + if (version != null) { + return versionOrHash; + } + + return null; + } + + /// Translates [versionOrHash] into a Flutter [Version]. If [versionOrHash] + /// is semver version string, it will simply parse that into a [Version]. If + /// not, it will assume that the input is a git commit hash and attempt to + /// map it to a Flutter version. + Future resolveFlutterVersion(String versionOrHash) async { + final parsedVersion = tryParseVersion(versionOrHash); + if (parsedVersion != null) { + return parsedVersion; + } + + // If we were unable to parse the version, assume it's a revision hash. + final versionString = + await getVersionForRevision(flutterRevision: versionOrHash); + return versionString != null ? tryParseVersion(versionString) : null; + } + /// Returns the git revision for the provided [version]. /// e.g. 3.16.3 -> b9b23902966504a9778f4c07e3a3487fa84dcb2a Future getRevisionForVersion(String version) async { diff --git a/packages/shorebird_cli/test/src/commands/release/ios_releaser_test.dart b/packages/shorebird_cli/test/src/commands/release/ios_releaser_test.dart index 16898ef11..409a99568 100644 --- a/packages/shorebird_cli/test/src/commands/release/ios_releaser_test.dart +++ b/packages/shorebird_cli/test/src/commands/release/ios_releaser_test.dart @@ -7,6 +7,7 @@ import 'package:mason_logger/mason_logger.dart'; import 'package:mocktail/mocktail.dart'; import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; +import 'package:pub_semver/pub_semver.dart'; import 'package:scoped_deps/scoped_deps.dart'; import 'package:shorebird_cli/src/artifact_builder.dart'; import 'package:shorebird_cli/src/artifact_manager.dart'; @@ -123,9 +124,13 @@ void main() { }); group('assertPreconditions', () { + final flutterVersion = Version(3, 0, 0); + setUp(() { when(() => doctor.iosCommandValidators) .thenReturn([flutterValidator]); + when(() => shorebirdFlutter.resolveFlutterVersion(any())) + .thenAnswer((_) async => flutterVersion); when(flutterValidator.validate).thenAnswer((_) async => []); }); diff --git a/packages/shorebird_cli/test/src/commands/release/release_command_test.dart b/packages/shorebird_cli/test/src/commands/release/release_command_test.dart index abbc9f275..376beb451 100644 --- a/packages/shorebird_cli/test/src/commands/release/release_command_test.dart +++ b/packages/shorebird_cli/test/src/commands/release/release_command_test.dart @@ -441,7 +441,7 @@ Note: ${lightCyan.wrap('shorebird patch --platforms=android --flavor=$flavor --t final exception = Exception('oops'); setUp(() { when( - () => shorebirdFlutter.getRevisionForVersion(any()), + () => shorebirdFlutter.resolveFlutterRevision(any()), ).thenThrow(exception); }); @@ -463,7 +463,7 @@ $exception''', group('when flutter version is not supported', () { setUp(() { when( - () => shorebirdFlutter.getRevisionForVersion(any()), + () => shorebirdFlutter.resolveFlutterRevision(any()), ).thenAnswer((_) async => null); }); @@ -484,7 +484,7 @@ $exception''', const revision = '771d07b2cf'; setUp(() { when( - () => shorebirdFlutter.getRevisionForVersion(any()), + () => shorebirdFlutter.resolveFlutterRevision(any()), ).thenAnswer((_) async => revision); }); @@ -522,28 +522,6 @@ $exception''', ).called(1); }); }); - - group('when flutter-version is a git hash', () { - const revision = 'deadbeef'; - setUp(() { - when(() => argResults['flutter-version']).thenReturn(revision); - }); - - test( - '''installs the flutter version with the provided revision and completes''', - () async { - await runWithOverrides(command.run); - - verify(() => shorebirdFlutter.installRevision(revision: revision)) - .called(1); - // We should never attempt to treat this as a semver version. - verifyNever( - () => shorebirdFlutter.getVersionForRevision( - flutterRevision: any(named: 'flutterRevision'), - ), - ); - }); - }); }); }); diff --git a/packages/shorebird_cli/test/src/shorebird_flutter_test.dart b/packages/shorebird_cli/test/src/shorebird_flutter_test.dart index 0b67f102b..c5af1a698 100644 --- a/packages/shorebird_cli/test/src/shorebird_flutter_test.dart +++ b/packages/shorebird_cli/test/src/shorebird_flutter_test.dart @@ -243,6 +243,111 @@ Tools • Dart 3.0.6 • DevTools 2.23.1'''); }); }); + group('resolveFlutterRevision', () { + group('when input is a semver version', () { + test('returns the revision associated with the version if it exists', + () async { + final revision = await runWithOverrides( + () => shorebirdFlutter.resolveFlutterRevision('3.10.6'), + ); + expect(revision, equals(flutterRevision)); + }); + }); + + group('when input is a commit hash that maps to a Flutter version', () { + setUp(() { + when( + () => git.forEachRef( + directory: any(named: 'directory'), + contains: any(named: 'contains'), + format: any(named: 'format'), + pattern: any(named: 'pattern'), + ), + ).thenAnswer((_) async => 'origin/flutter_release/1.2.3'); + }); + + test('returns the input string', () async { + final revision = await runWithOverrides( + () => shorebirdFlutter.resolveFlutterRevision('deadbeef'), + ); + expect(revision, equals('deadbeef')); + }); + }); + + group('when input is not a commit hash that maps to a Flutter version', + () { + setUp(() { + when( + () => git.forEachRef( + directory: any(named: 'directory'), + contains: any(named: 'contains'), + format: any(named: 'format'), + pattern: any(named: 'pattern'), + ), + ).thenAnswer((_) async => ''); + }); + + test('returns the input string', () async { + final revision = await runWithOverrides( + () => shorebirdFlutter.resolveFlutterRevision('not-a-version'), + ); + expect(revision, isNull); + }); + }); + }); + + group('resolveFlutterVersion', () { + group('when input is a semver version', () { + test('returns the revision associated with the version if it exists', + () async { + final revision = await runWithOverrides( + () => shorebirdFlutter.resolveFlutterVersion('3.10.6'), + ); + expect(revision, equals(Version(3, 10, 6))); + }); + }); + + group('when input is not a recognized commit hash', () { + setUp(() { + when( + () => git.forEachRef( + directory: any(named: 'directory'), + contains: any(named: 'contains'), + format: any(named: 'format'), + pattern: any(named: 'pattern'), + ), + ).thenAnswer((_) async => ''); + }); + + test('returns null', () async { + final revision = await runWithOverrides( + () => shorebirdFlutter.resolveFlutterVersion('not-a-version'), + ); + expect(revision, isNull); + }); + }); + + group('when input is a recognized commit hash', () { + setUp(() { + when( + () => git.forEachRef( + directory: any(named: 'directory'), + contains: any(named: 'contains'), + format: any(named: 'format'), + pattern: any(named: 'pattern'), + ), + ).thenAnswer((_) async => 'origin/flutter_release/1.2.3'); + }); + + test('returns a parsed version', () async { + final revision = await runWithOverrides( + () => shorebirdFlutter.resolveFlutterVersion('deadbeef'), + ); + expect(revision, equals(Version(1, 2, 3))); + }); + }); + }); + group('getRevisionForVersion', () { const version = '3.16.3'; const exception = ProcessException('git', ['rev-parse']); From 2d418c17b9f2abc9c5c463b9731af7d1cef93b2e Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Wed, 4 Sep 2024 12:30:01 -0400 Subject: [PATCH 2/2] handle exceptions thrown by getVersionForRevision --- .../lib/src/shorebird_flutter.dart | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/packages/shorebird_cli/lib/src/shorebird_flutter.dart b/packages/shorebird_cli/lib/src/shorebird_flutter.dart index b4ebb69f9..d68deb41c 100644 --- a/packages/shorebird_cli/lib/src/shorebird_flutter.dart +++ b/packages/shorebird_cli/lib/src/shorebird_flutter.dart @@ -217,9 +217,14 @@ class ShorebirdFlutter { } // If we were unable to parse the version, assume it's a revision hash. - final version = await getVersionForRevision(flutterRevision: versionOrHash); - if (version != null) { - return versionOrHash; + try { + final version = + await getVersionForRevision(flutterRevision: versionOrHash); + if (version != null) { + return versionOrHash; + } + } catch (_) { + return null; } return null; @@ -235,10 +240,14 @@ class ShorebirdFlutter { return parsedVersion; } - // If we were unable to parse the version, assume it's a revision hash. - final versionString = - await getVersionForRevision(flutterRevision: versionOrHash); - return versionString != null ? tryParseVersion(versionString) : null; + try { + // If we were unable to parse the version, assume it's a revision hash. + final versionString = + await getVersionForRevision(flutterRevision: versionOrHash); + return versionString != null ? tryParseVersion(versionString) : null; + } catch (_) { + return null; + } } /// Returns the git revision for the provided [version].