From da12995451a353a93131c544cb4107cd170dcda3 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Mon, 24 Jun 2024 17:19:49 -0400 Subject: [PATCH] fix(shorebird_cli): infer `--build-name` and `--build-number` from `--release-version` if necessary (#2280) --- .../lib/src/commands/patch/aar_patcher.dart | 2 +- .../src/commands/patch/android_patcher.dart | 5 +- .../commands/patch/ios_framework_patcher.dart | 2 +- .../lib/src/commands/patch/ios_patcher.dart | 5 +- .../lib/src/commands/patch/patch_command.dart | 4 +- .../lib/src/commands/patch/patcher.dart | 40 +++++++++- .../commands/patch/android_patcher_test.dart | 20 +++++ .../src/commands/patch/ios_patcher_test.dart | 23 ++++++ .../commands/patch/patch_command_test.dart | 8 +- .../test/src/commands/patch/patcher_test.dart | 74 ++++++++++++++++++- 10 files changed, 171 insertions(+), 12 deletions(-) diff --git a/packages/shorebird_cli/lib/src/commands/patch/aar_patcher.dart b/packages/shorebird_cli/lib/src/commands/patch/aar_patcher.dart index f63f9f86..b716c7cf 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/aar_patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/aar_patcher.dart @@ -77,7 +77,7 @@ class AarPatcher extends Patcher { ); @override - Future buildPatchArtifact() async { + Future buildPatchArtifact({String? releaseVersion}) async { final flutterVersionString = await shorebirdFlutter.getVersionAndRevision(); final buildProgress = logger.progress('Building patch with Flutter $flutterVersionString'); diff --git a/packages/shorebird_cli/lib/src/commands/patch/android_patcher.dart b/packages/shorebird_cli/lib/src/commands/patch/android_patcher.dart index 16d235b2..08464683 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/android_patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/android_patcher.dart @@ -69,7 +69,7 @@ class AndroidPatcher extends Patcher { } @override - Future buildPatchArtifact() async { + Future buildPatchArtifact({String? releaseVersion}) async { final File aabFile; final flutterVersionString = await shorebirdFlutter.getVersionAndRevision(); final buildProgress = @@ -79,7 +79,8 @@ class AndroidPatcher extends Patcher { aabFile = await artifactBuilder.buildAppBundle( flavor: flavor, target: target, - args: argResults.forwardedArgs, + args: argResults.forwardedArgs + + buildNameAndNumberArgsFromReleaseVersion(releaseVersion), base64PublicKey: argResults.encodedPublicKey, ); buildProgress.complete(); diff --git a/packages/shorebird_cli/lib/src/commands/patch/ios_framework_patcher.dart b/packages/shorebird_cli/lib/src/commands/patch/ios_framework_patcher.dart index 4ed1ea8d..dc1146ed 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/ios_framework_patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/ios_framework_patcher.dart @@ -93,7 +93,7 @@ class IosFrameworkPatcher extends Patcher { ); @override - Future buildPatchArtifact() async { + Future buildPatchArtifact({String? releaseVersion}) async { final flutterVersionString = await shorebirdFlutter.getVersionAndRevision(); final buildProgress = logger.progress( 'Building patch with Flutter $flutterVersionString', diff --git a/packages/shorebird_cli/lib/src/commands/patch/ios_patcher.dart b/packages/shorebird_cli/lib/src/commands/patch/ios_patcher.dart index 0837d4f4..e0cc9b7c 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/ios_patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/ios_patcher.dart @@ -134,7 +134,7 @@ This may indicate that the patch contains native changes, which cannot be applie } @override - Future buildPatchArtifact() async { + Future buildPatchArtifact({String? releaseVersion}) async { final File exportOptionsPlist; try { exportOptionsPlist = ios.exportOptionsPlistFromArgs(argResults); @@ -172,7 +172,8 @@ For more information see: $supportedVersionsLink''', exportOptionsPlist: exportOptionsPlist, flavor: flavor, target: target, - args: argResults.forwardedArgs, + args: argResults.forwardedArgs + + buildNameAndNumberArgsFromReleaseVersion(releaseVersion), base64PublicKey: argResults.encodedPublicKey, ); } on ProcessException catch (error) { diff --git a/packages/shorebird_cli/lib/src/commands/patch/patch_command.dart b/packages/shorebird_cli/lib/src/commands/patch/patch_command.dart index 4f604181..a61267f8 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/patch_command.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/patch_command.dart @@ -264,7 +264,9 @@ NOTE: this is ${styleBold.wrap('not')} recommended. Asset changes cannot be incl // Don't built the patch artifact twice with the same Flutter revision. if (lastBuiltFlutterRevision != release.flutterRevision) { - patchArtifactFile = await patcher.buildPatchArtifact(); + patchArtifactFile = await patcher.buildPatchArtifact( + releaseVersion: release.version, + ); } final diffStatus = await assertUnpatchableDiffs( diff --git a/packages/shorebird_cli/lib/src/commands/patch/patcher.dart b/packages/shorebird_cli/lib/src/commands/patch/patcher.dart index 429a15eb..11c5463c 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/patcher.dart @@ -77,7 +77,7 @@ https://docs.shorebird.dev/status#link-percentage-ios /// Builds the release artifacts for the given platform. Returns the "primary" /// artifact for the platform (e.g. the AAB for Android, the IPA for iOS). - Future buildPatchArtifact(); + Future buildPatchArtifact({String? releaseVersion}); /// Determines the release version from the provided app artifact. Future extractReleaseVersionFromArtifact(File artifact); @@ -115,4 +115,42 @@ https://docs.shorebird.dev/status#link-percentage-ios File get debugInfoFile { return File(p.join(buildDirectory.path, 'patch-debug.zip')); } + + /// Extracts the --build-name and --build-number from the --release-version + /// argument if it's provided. Given `--release-version=1.2.3+4`, this will + /// return `['--build-name=1.2.3', '--build-number=4']`, with the intent that + /// these values will be forwarded to the `flutter build` command. + /// + /// Because not all platform types support both --build-name and + /// --build-number, this needs to be handled in the platform-specific + /// patchers instead of at the patch command level. + /// + /// We do this because some platforms encode the build version in their + /// binaries (Android does this with .dex files). If a release and a patch + /// have different version numbers, our [PatchDiffChecker] to warn the user of + /// native changes, even though the user may not have actually changed any + /// code or dependencies. + /// + /// Context: https://github.com/shorebirdtech/shorebird/issues/2270 + List buildNameAndNumberArgsFromReleaseVersion( + String? releaseVersion, + ) { + if (releaseVersion == null || !releaseVersion.contains('+')) { + return []; + } + + // If the user already provided --build-name or --build-number, we don't + // want to override them. + if (argResults.rest.any( + (a) => a.startsWith('--build-name') || a.startsWith('--build-number'), + )) { + return []; + } + + final parts = releaseVersion.split('+'); + return [ + '--build-name=${parts[0]}', + '--build-number=${parts[1]}', + ]; + } } diff --git a/packages/shorebird_cli/test/src/commands/patch/android_patcher_test.dart b/packages/shorebird_cli/test/src/commands/patch/android_patcher_test.dart index 55b52001..40779178 100644 --- a/packages/shorebird_cli/test/src/commands/patch/android_patcher_test.dart +++ b/packages/shorebird_cli/test/src/commands/patch/android_patcher_test.dart @@ -334,6 +334,26 @@ Looked in: }); }); + group('when releaseVersion is provided', () { + setUp(setUpProjectRootArtifacts); + + test('forwards --build-name and --build-number to builder', () async { + await runWithOverrides( + () => patcher.buildPatchArtifact(releaseVersion: '1.2.3+4'), + ); + verify( + () => artifactBuilder.buildAppBundle( + flavor: any(named: 'flavor'), + target: any(named: 'target'), + args: any( + named: 'args', + that: containsAll(['--build-name=1.2.3', '--build-number=4']), + ), + ), + ).called(1); + }); + }); + group('when build succeeds', () { setUp(setUpProjectRootArtifacts); diff --git a/packages/shorebird_cli/test/src/commands/patch/ios_patcher_test.dart b/packages/shorebird_cli/test/src/commands/patch/ios_patcher_test.dart index 690895c8..8148a6df 100644 --- a/packages/shorebird_cli/test/src/commands/patch/ios_patcher_test.dart +++ b/packages/shorebird_cli/test/src/commands/patch/ios_patcher_test.dart @@ -674,6 +674,29 @@ For more information see: $supportedVersionsLink''', ); }); + group('when releaseVersion is provided', () { + test('forwards --build-name and --build-number to builder', + () async { + await runWithOverrides( + () => patcher.buildPatchArtifact(releaseVersion: '1.2.3+4'), + ); + verify( + () => artifactBuilder.buildIpa( + flavor: any(named: 'flavor'), + exportOptionsPlist: any(named: 'exportOptionsPlist'), + codesign: any(named: 'codesign'), + target: any(named: 'target'), + args: any( + named: 'args', + that: containsAll( + ['--build-name=1.2.3', '--build-number=4'], + ), + ), + ), + ).called(1); + }); + }); + group('when platform was specified via arg results rest', () { setUp(() { when(() => argResults.rest).thenReturn(['ios', '--verbose']); diff --git a/packages/shorebird_cli/test/src/commands/patch/patch_command_test.dart b/packages/shorebird_cli/test/src/commands/patch/patch_command_test.dart index c4bde68c..2660be5d 100644 --- a/packages/shorebird_cli/test/src/commands/patch/patch_command_test.dart +++ b/packages/shorebird_cli/test/src/commands/patch/patch_command_test.dart @@ -225,7 +225,9 @@ void main() { () => patcher.extractReleaseVersionFromArtifact(any()), ).thenAnswer((_) async => releaseVersion); when( - () => patcher.buildPatchArtifact(), + () => patcher.buildPatchArtifact( + releaseVersion: any(named: 'releaseVersion'), + ), ).thenAnswer((_) async => File('')); when(() => patcher.releaseType).thenReturn(ReleaseType.android); when(() => patcher.primaryReleaseArtifactArch).thenReturn('aab'); @@ -580,7 +582,7 @@ void main() { arch: patcher.primaryReleaseArtifactArch, platform: releasePlatform, ), - () => patcher.buildPatchArtifact(), + () => patcher.buildPatchArtifact(releaseVersion: releaseVersion), () => patcher.assertUnpatchableDiffs( releaseArtifact: any(named: 'releaseArtifact'), releaseArchive: any(named: 'releaseArchive'), @@ -730,7 +732,7 @@ void main() { () => shorebirdEnv.copyWith( flutterRevisionOverride: releaseFlutterRevision, ), - () => patcher.buildPatchArtifact(), + () => patcher.buildPatchArtifact(releaseVersion: releaseVersion), ]); }); diff --git a/packages/shorebird_cli/test/src/commands/patch/patcher_test.dart b/packages/shorebird_cli/test/src/commands/patch/patcher_test.dart index 6ad7868d..263df22b 100644 --- a/packages/shorebird_cli/test/src/commands/patch/patcher_test.dart +++ b/packages/shorebird_cli/test/src/commands/patch/patcher_test.dart @@ -1,5 +1,7 @@ import 'dart:io'; +import 'package:args/args.dart'; +import 'package:mocktail/mocktail.dart'; import 'package:shorebird_cli/src/code_push_client_wrapper.dart'; import 'package:shorebird_cli/src/commands/commands.dart'; import 'package:shorebird_cli/src/patch_diff_checker.dart'; @@ -38,6 +40,76 @@ void main() { ); }); }); + + group('buildNameAndNumberArgsFromReleaseVersion', () { + late ArgResults argResults; + setUp(() { + argResults = MockArgResults(); + }); + + group('when releaseVersion is not specified', () { + test('returns an empty list', () { + expect( + _TestPatcher( + argResults: MockArgResults(), + flavor: null, + target: null, + ).buildNameAndNumberArgsFromReleaseVersion(null), + isEmpty, + ); + }); + }); + + group('when an invalid --release-version is specified', () { + test('returns an empty list', () { + expect( + _TestPatcher( + argResults: argResults, + flavor: null, + target: null, + ).buildNameAndNumberArgsFromReleaseVersion('invalid'), + isEmpty, + ); + }); + }); + + group('when a valid --release-version is specified', () { + group('when --build-name and --build-number are specified', () { + setUp(() { + when(() => argResults.rest).thenReturn([ + '--build-name=foo', + '--build-number=42', + ]); + }); + + test('returns an empty list', () { + expect( + _TestPatcher( + argResults: argResults, + flavor: null, + target: null, + ).buildNameAndNumberArgsFromReleaseVersion('1.2.3+4'), + isEmpty, + ); + }); + }); + + group('when neither --build-name nor --build-number are specified', () { + test('returns --build-name and --build-number', () { + when(() => argResults.rest).thenReturn([]); + + expect( + _TestPatcher( + argResults: argResults, + flavor: null, + target: null, + ).buildNameAndNumberArgsFromReleaseVersion('1.2.3+4'), + equals(['--build-name=1.2.3', '--build-number=4']), + ); + }); + }); + }); + }); }); } @@ -63,7 +135,7 @@ class _TestPatcher extends Patcher { } @override - Future buildPatchArtifact() { + Future buildPatchArtifact({String? releaseVersion}) { throw UnimplementedError(); }