diff --git a/packages/shorebird_cli/lib/src/archive_analysis/ios_archive_differ.dart b/packages/shorebird_cli/lib/src/archive_analysis/apple_archive_differ.dart similarity index 85% rename from packages/shorebird_cli/lib/src/archive_analysis/ios_archive_differ.dart rename to packages/shorebird_cli/lib/src/archive_analysis/apple_archive_differ.dart index d20989a85..5756ca630 100644 --- a/packages/shorebird_cli/lib/src/archive_analysis/ios_archive_differ.dart +++ b/packages/shorebird_cli/lib/src/archive_analysis/apple_archive_differ.dart @@ -11,8 +11,11 @@ import 'package:shorebird_cli/src/archive_analysis/archive_differ.dart'; import 'package:shorebird_cli/src/archive_analysis/file_set_diff.dart'; import 'package:shorebird_cli/src/archive_analysis/macho.dart'; -/// {@template ios_archive_differ} -/// Finds differences between two IPAs or zipped Xcframeworks. +/// {@template apple_archive_differ} +/// Finds differences between two IPAs, zipped Xcframeworks, or zipped macOS +/// .apps. Note that, due to a quirk in how macOS .app zips are handled by +/// package:archive, they must *not* include the top-level .app directory (i.e., +/// they should unzip to a Contents directory). /// /// Asset changes will be in the `Assets.car` file (which is a combination of /// the `.xcasset` catalogs in the Xcode project) and the `flutter_assets` @@ -23,9 +26,9 @@ import 'package:shorebird_cli/src/archive_analysis/macho.dart'; /// /// Dart changes will appear in the App.framework/App executable. /// {@endtemplate} -class IosArchiveDiffer extends ArchiveDiffer { - /// {@macro ios_archive_differ} - const IosArchiveDiffer(); +class AppleArchiveDiffer extends ArchiveDiffer { + /// {@macro apple_archive_differ} + const AppleArchiveDiffer(); String _hash(List bytes) => sha256.convert(bytes).toString(); @@ -34,11 +37,16 @@ class IosArchiveDiffer extends ArchiveDiffer { RegExp(r'Flutter.framework/Flutter$'), }; - /// The regex pattern for identifying app files within an archive. - static final RegExp appRegex = RegExp( + /// The regex pattern for identifying app files within an xcarchive. + static final RegExp xcFrameworkAppRegex = RegExp( r'^Products/Applications/[\w\-. ]+.app/[\w\- ]+$', ); + /// The regex pattern for identifying executable files within a macOS .app. + static final RegExp macosAppRegex = RegExp( + r'^Contents/MacOS/.+$', + ); + /// Files that have been added, removed, or that have changed between the /// archives at the two provided paths. This method will also unsign mach-o /// binaries in the archives before computing the diff. @@ -95,7 +103,7 @@ class IosArchiveDiffer extends ArchiveDiffer { (file) => _binaryFilePatterns .any((pattern) => pattern.hasMatch(file.name)) || - appRegex.hasMatch(file.name), + xcFrameworkAppRegex.hasMatch(file.name), ) .toList(); } @@ -170,5 +178,7 @@ class IosArchiveDiffer extends ArchiveDiffer { filePath.endsWith('App.framework/App'); @override - bool isNativeFilePath(String filePath) => appRegex.hasMatch(filePath); + bool isNativeFilePath(String filePath) => + xcFrameworkAppRegex.hasMatch(filePath) || + macosAppRegex.hasMatch(filePath); } diff --git a/packages/shorebird_cli/lib/src/archive_analysis/archive_analysis.dart b/packages/shorebird_cli/lib/src/archive_analysis/archive_analysis.dart index b26b7599d..1d7ef655d 100644 --- a/packages/shorebird_cli/lib/src/archive_analysis/archive_analysis.dart +++ b/packages/shorebird_cli/lib/src/archive_analysis/archive_analysis.dart @@ -1,4 +1,4 @@ export 'android_archive_differ.dart'; +export 'apple_archive_differ.dart'; export 'file_set_diff.dart'; -export 'ios_archive_differ.dart'; export 'plist.dart'; 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 84922ab7c..036d2459c 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 @@ -6,7 +6,7 @@ import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; import 'package:shorebird_cli/src/archive/directory_archive.dart'; -import 'package:shorebird_cli/src/archive_analysis/ios_archive_differ.dart'; +import 'package:shorebird_cli/src/archive_analysis/apple_archive_differ.dart'; import 'package:shorebird_cli/src/artifact_builder.dart'; import 'package:shorebird_cli/src/artifact_manager.dart'; import 'package:shorebird_cli/src/code_push_client_wrapper.dart'; @@ -96,7 +96,7 @@ class IosFrameworkPatcher extends Patcher { patchDiffChecker.confirmUnpatchableDiffsIfNecessary( localArchive: patchArchive, releaseArchive: releaseArchive, - archiveDiffer: const IosArchiveDiffer(), + archiveDiffer: const AppleArchiveDiffer(), allowAssetChanges: allowAssetDiffs, allowNativeChanges: allowNativeDiffs, ); 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 d926b9431..4d3886348 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/ios_patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/ios_patcher.dart @@ -122,7 +122,7 @@ class IosPatcher extends Patcher { await patchDiffChecker.confirmUnpatchableDiffsIfNecessary( localArchive: patchArchive, releaseArchive: releaseArchive, - archiveDiffer: const IosArchiveDiffer(), + archiveDiffer: const AppleArchiveDiffer(), allowAssetChanges: allowAssetDiffs, allowNativeChanges: allowNativeDiffs, confirmNativeChanges: false, diff --git a/packages/shorebird_cli/lib/src/commands/patch/macos_patcher.dart b/packages/shorebird_cli/lib/src/commands/patch/macos_patcher.dart index 2f1f974e7..65146383b 100644 --- a/packages/shorebird_cli/lib/src/commands/patch/macos_patcher.dart +++ b/packages/shorebird_cli/lib/src/commands/patch/macos_patcher.dart @@ -6,6 +6,7 @@ import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; import 'package:shorebird_cli/src/archive/directory_archive.dart'; +import 'package:shorebird_cli/src/archive_analysis/apple_archive_differ.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'; @@ -24,6 +25,7 @@ import 'package:shorebird_cli/src/platform/platform.dart'; import 'package:shorebird_cli/src/release_type.dart'; import 'package:shorebird_cli/src/shorebird_artifacts.dart'; import 'package:shorebird_cli/src/shorebird_documentation.dart'; +import 'package:shorebird_cli/src/shorebird_env.dart'; import 'package:shorebird_cli/src/shorebird_flutter.dart'; import 'package:shorebird_cli/src/shorebird_validator.dart'; import 'package:shorebird_cli/src/third_party/flutter_tools/lib/flutter_tools.dart'; @@ -106,14 +108,61 @@ class MacosPatcher extends Patcher { } } + // FIXME: this is a direct copy of IosPatcher's implementation. We should + // consolidate this and other copied code. @override Future assertUnpatchableDiffs({ required ReleaseArtifact releaseArtifact, required File releaseArchive, required File patchArchive, }) async { - // TODO(bryanoltman): implement assertUnpatchableDiffs - return const DiffStatus(hasAssetChanges: false, hasNativeChanges: false); + // Check for diffs without warning about native changes, as Xcode builds + // can be nondeterministic. So we still have some hope of alerting users of + // unpatchable native changes, we compare the Podfile.lock hash between the + // patch and the release. + final diffStatus = + await patchDiffChecker.confirmUnpatchableDiffsIfNecessary( + localArchive: patchArchive, + releaseArchive: releaseArchive, + archiveDiffer: const AppleArchiveDiffer(), + allowAssetChanges: allowAssetDiffs, + allowNativeChanges: allowNativeDiffs, + confirmNativeChanges: false, + ); + + if (!diffStatus.hasNativeChanges) { + return diffStatus; + } + + final String? podfileLockHash; + if (shorebirdEnv.macosPodfileLockFile.existsSync()) { + podfileLockHash = sha256 + .convert(shorebirdEnv.macosPodfileLockFile.readAsBytesSync()) + .toString(); + } else { + podfileLockHash = null; + } + + if (releaseArtifact.podfileLockHash != null && + podfileLockHash != releaseArtifact.podfileLockHash) { + logger.warn( + ''' +Your macos/Podfile.lock is different from the one used to build the release. +This may indicate that the patch contains native changes, which cannot be applied with a patch. Proceeding may result in unexpected behavior or crashes.''', + ); + + if (!allowNativeDiffs) { + if (!shorebirdEnv.canAcceptUserInput) { + throw UnpatchableChangeException(); + } + + if (!logger.confirm('Continue anyways?')) { + throw UserCancelledException(); + } + } + } + + return diffStatus; } @override @@ -189,7 +238,9 @@ For more information see: ${supportedFlutterVersionsUrl.toLink()}''', await ditto.archive( source: appPath, destination: zippedApp.path, - keepParent: true, + // keepParent is false here in order to ensure the directory structure of + // this zip file matches what will be provided to the [AppleArchiveDiffer] + // (which uses package:archive to stream zip file contents). ); return zippedApp; } diff --git a/packages/shorebird_cli/test/src/archive_analysis/ios_archive_differ_test.dart b/packages/shorebird_cli/test/src/archive_analysis/apple_archive_differ_test.dart similarity index 97% rename from packages/shorebird_cli/test/src/archive_analysis/ios_archive_differ_test.dart rename to packages/shorebird_cli/test/src/archive_analysis/apple_archive_differ_test.dart index 918e8503c..ac0d8af9d 100644 --- a/packages/shorebird_cli/test/src/archive_analysis/ios_archive_differ_test.dart +++ b/packages/shorebird_cli/test/src/archive_analysis/apple_archive_differ_test.dart @@ -40,18 +40,18 @@ void main() { p.join(xcframeworkFixturesBasePath, 'changed_dart.xcframework.zip'); group( - IosArchiveDiffer, + AppleArchiveDiffer, () { - late IosArchiveDiffer differ; + late AppleArchiveDiffer differ; setUp(() { - differ = const IosArchiveDiffer(); + differ = const AppleArchiveDiffer(); }); group('appRegex', () { test('identifies Runner.app/Runner as an app file', () { expect( - IosArchiveDiffer.appRegex.hasMatch( + AppleArchiveDiffer.xcFrameworkAppRegex.hasMatch( 'Products/Applications/Runner.app/Runner', ), isTrue, @@ -60,7 +60,7 @@ void main() { test('does not identify Runner.app/Assets.car as an app file', () { expect( - IosArchiveDiffer.appRegex.hasMatch( + AppleArchiveDiffer.xcFrameworkAppRegex.hasMatch( 'Products/Applications/Runner.app/Assets.car', ), isFalse, diff --git a/packages/shorebird_cli/test/src/commands/patch/ios_framework_patcher_test.dart b/packages/shorebird_cli/test/src/commands/patch/ios_framework_patcher_test.dart index 1eece923c..337fca2c7 100644 --- a/packages/shorebird_cli/test/src/commands/patch/ios_framework_patcher_test.dart +++ b/packages/shorebird_cli/test/src/commands/patch/ios_framework_patcher_test.dart @@ -6,7 +6,7 @@ import 'package:mocktail/mocktail.dart'; import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; import 'package:scoped_deps/scoped_deps.dart'; -import 'package:shorebird_cli/src/archive_analysis/ios_archive_differ.dart'; +import 'package:shorebird_cli/src/archive_analysis/apple_archive_differ.dart'; import 'package:shorebird_cli/src/artifact_builder.dart'; import 'package:shorebird_cli/src/artifact_manager.dart'; import 'package:shorebird_cli/src/code_push_client_wrapper.dart'; @@ -90,7 +90,7 @@ void main() { setUpAll(() { registerFallbackValue(Directory('')); registerFallbackValue(File('')); - registerFallbackValue(const IosArchiveDiffer()); + registerFallbackValue(const AppleArchiveDiffer()); registerFallbackValue(ReleasePlatform.ios); registerFallbackValue(ShorebirdArtifact.genSnapshotIos); registerFallbackValue(Uri.parse('https://example.com')); 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 915035461..614e8fd41 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 @@ -9,7 +9,7 @@ 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/archive_analysis/ios_archive_differ.dart'; +import 'package:shorebird_cli/src/archive_analysis/apple_archive_differ.dart'; import 'package:shorebird_cli/src/artifact_builder.dart'; import 'package:shorebird_cli/src/artifact_manager.dart'; import 'package:shorebird_cli/src/code_push_client_wrapper.dart'; @@ -101,7 +101,7 @@ void main() { registerFallbackValue(FakeArgResults()); registerFallbackValue(Directory('')); registerFallbackValue(File('')); - registerFallbackValue(const IosArchiveDiffer()); + registerFallbackValue(const AppleArchiveDiffer()); registerFallbackValue(ReleasePlatform.ios); registerFallbackValue(ShorebirdArtifact.genSnapshotIos); registerFallbackValue(Uri.parse('https://example.com')); diff --git a/packages/shorebird_cli/test/src/commands/patch/macos_patcher_test.dart b/packages/shorebird_cli/test/src/commands/patch/macos_patcher_test.dart index 16b069280..575de8ae6 100644 --- a/packages/shorebird_cli/test/src/commands/patch/macos_patcher_test.dart +++ b/packages/shorebird_cli/test/src/commands/patch/macos_patcher_test.dart @@ -1,12 +1,15 @@ +import 'dart:convert'; import 'dart:io'; import 'package:args/args.dart'; +import 'package:crypto/crypto.dart'; 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/archive_analysis/archive_analysis.dart'; import 'package:shorebird_cli/src/artifact_builder.dart'; import 'package:shorebird_cli/src/artifact_manager.dart'; import 'package:shorebird_cli/src/code_push_client_wrapper.dart'; @@ -58,7 +61,7 @@ void main() { late Directory appDirectory; late ShorebirdLogger logger; late OperatingSystemInterface operatingSystemInterface; - // late PatchDiffChecker patchDiffChecker; + late PatchDiffChecker patchDiffChecker; late Progress progress; late ShorebirdArtifacts shorebirdArtifacts; late ShorebirdFlutterValidator flutterValidator; @@ -83,7 +86,7 @@ void main() { engineConfigRef.overrideWith(() => engineConfig), loggerRef.overrideWith(() => logger), osInterfaceRef.overrideWith(() => operatingSystemInterface), - // patchDiffCheckerRef.overrideWith(() => patchDiffChecker), + patchDiffCheckerRef.overrideWith(() => patchDiffChecker), processRef.overrideWith(() => shorebirdProcess), shorebirdArtifactsRef.overrideWith(() => shorebirdArtifacts), shorebirdEnvRef.overrideWith(() => shorebirdEnv), @@ -95,6 +98,7 @@ void main() { } setUpAll(() { + registerFallbackValue(const AppleArchiveDiffer()); registerFallbackValue(Directory('')); registerFallbackValue(File('')); registerFallbackValue(ReleasePlatform.macos); @@ -114,7 +118,7 @@ void main() { doctor = MockDoctor(); engineConfig = MockEngineConfig(); operatingSystemInterface = MockOperatingSystemInterface(); - // patchDiffChecker = MockPatchDiffChecker(); + patchDiffChecker = MockPatchDiffChecker(); progress = MockProgress(); projectRoot = Directory.systemTemp.createTempSync(); logger = MockShorebirdLogger(); @@ -136,7 +140,6 @@ void main() { () => ditto.archive( source: any(named: 'source'), destination: any(named: 'destination'), - keepParent: any(named: 'keepParent'), ), ).thenAnswer((_) async {}); when( @@ -299,23 +302,218 @@ void main() { }); group('assertUnpatchableDiffs', () { - test('returns no diffs (currently unimplemented)', () async { - final diffStatus = await runWithOverrides( - () => patcher.assertUnpatchableDiffs( - releaseArtifact: FakeReleaseArtifact(), - releaseArchive: File(''), - patchArchive: File(''), - ), + group('when no native changes are detected', () { + const noChangeDiffStatus = DiffStatus( + hasAssetChanges: false, + hasNativeChanges: false, ); - expect( - diffStatus, - equals( - const DiffStatus( - hasAssetChanges: false, - hasNativeChanges: false, + + setUp(() { + when( + () => patchDiffChecker.confirmUnpatchableDiffsIfNecessary( + localArchive: any(named: 'localArchive'), + releaseArchive: any(named: 'releaseArchive'), + archiveDiffer: any(named: 'archiveDiffer'), + allowAssetChanges: any(named: 'allowAssetChanges'), + allowNativeChanges: any(named: 'allowNativeChanges'), + confirmNativeChanges: false, ), - ), + ).thenAnswer((_) async => noChangeDiffStatus); + }); + + test('returns diff status from patchDiffChecker', () async { + final diffStatus = await runWithOverrides( + () => patcher.assertUnpatchableDiffs( + releaseArtifact: FakeReleaseArtifact(), + releaseArchive: File(''), + patchArchive: File(''), + ), + ); + expect(diffStatus, equals(noChangeDiffStatus)); + verifyNever( + () => logger.warn( + '''Your macos/Podfile.lock is different from the one used to build the release.''', + ), + ); + }); + }); + + group('when native changes are detected', () { + const nativeChangeDiffStatus = DiffStatus( + hasAssetChanges: false, + hasNativeChanges: true, ); + + late String podfileLockHash; + + setUp(() { + when( + () => patchDiffChecker.confirmUnpatchableDiffsIfNecessary( + localArchive: any(named: 'localArchive'), + releaseArchive: any(named: 'releaseArchive'), + archiveDiffer: any(named: 'archiveDiffer'), + allowAssetChanges: any(named: 'allowAssetChanges'), + allowNativeChanges: any(named: 'allowNativeChanges'), + confirmNativeChanges: false, + ), + ).thenAnswer((_) async => nativeChangeDiffStatus); + + const podfileLockContents = 'lock file'; + podfileLockHash = + sha256.convert(utf8.encode(podfileLockContents)).toString(); + final podfileLockFile = File( + p.join( + Directory.systemTemp.createTempSync().path, + 'Podfile.lock', + ), + ) + ..createSync(recursive: true) + ..writeAsStringSync(podfileLockContents); + + when(() => shorebirdEnv.macosPodfileLockFile) + .thenReturn(podfileLockFile); + }); + + group('when release has podspec lock hash', () { + group('when release podspec lock hash matches patch', () { + late final releaseArtifact = ReleaseArtifact( + id: 0, + releaseId: 0, + arch: 'aarch64', + platform: ReleasePlatform.macos, + hash: '#', + size: 42, + url: 'https://example.com', + podfileLockHash: podfileLockHash, + canSideload: true, + ); + + test('does not warn of native changes', () async { + final diffStatus = await runWithOverrides( + () => patcher.assertUnpatchableDiffs( + releaseArtifact: releaseArtifact, + releaseArchive: File(''), + patchArchive: File(''), + ), + ); + expect(diffStatus, equals(nativeChangeDiffStatus)); + verifyNever( + () => logger.warn( + '''Your macos/Podfile.lock is different from the one used to build the release.''', + ), + ); + }); + }); + + group('when release podspec lock hash does not match patch', () { + const releaseArtifact = ReleaseArtifact( + id: 0, + releaseId: 0, + arch: 'aarch64', + platform: ReleasePlatform.macos, + hash: '#', + size: 42, + url: 'https://example.com', + podfileLockHash: 'podfile-lock-hash', + canSideload: true, + ); + + group('when native diffs are allowed', () { + setUp(() { + when(() => argResults['allow-native-diffs']).thenReturn(true); + }); + + test( + 'logs warning, does not prompt for confirmation to proceed', + () async { + final diffStatus = await runWithOverrides( + () => patcher.assertUnpatchableDiffs( + releaseArtifact: releaseArtifact, + releaseArchive: File(''), + patchArchive: File(''), + ), + ); + expect(diffStatus, equals(nativeChangeDiffStatus)); + verify( + () => logger.warn( + ''' +Your macos/Podfile.lock is different from the one used to build the release. +This may indicate that the patch contains native changes, which cannot be applied with a patch. Proceeding may result in unexpected behavior or crashes.''', + ), + ).called(1); + verifyNever(() => logger.confirm(any())); + }); + }); + + group('when native diffs are not allowed', () { + group('when in an environment that accepts user input', () { + setUp(() { + when(() => shorebirdEnv.canAcceptUserInput) + .thenReturn(true); + }); + + group('when user opts to continue at prompt', () { + setUp(() { + when(() => logger.confirm(any())).thenReturn(true); + }); + + test('returns diff status from patchDiffChecker', () async { + final diffStatus = await runWithOverrides( + () => patcher.assertUnpatchableDiffs( + releaseArtifact: releaseArtifact, + releaseArchive: File(''), + patchArchive: File(''), + ), + ); + expect(diffStatus, equals(nativeChangeDiffStatus)); + }); + }); + + group('when user aborts at prompt', () { + setUp(() { + when(() => logger.confirm(any())).thenReturn(false); + }); + + test('throws UserCancelledException', () async { + await expectLater( + () => runWithOverrides( + () => patcher.assertUnpatchableDiffs( + releaseArtifact: releaseArtifact, + releaseArchive: File(''), + patchArchive: File(''), + ), + ), + throwsA(isA()), + ); + }); + }); + }); + + group('when in an environment that does not accept user input', + () { + setUp(() { + when(() => shorebirdEnv.canAcceptUserInput) + .thenReturn(false); + }); + + test('throws UnpatchableChangeException', () async { + await expectLater( + () => runWithOverrides( + () => patcher.assertUnpatchableDiffs( + releaseArtifact: releaseArtifact, + releaseArchive: File(''), + patchArchive: File(''), + ), + ), + throwsA(isA()), + ); + }); + }); + }); + }); + }); + + group('when release does not have podspec lock hash', () {}); }); }); @@ -327,7 +525,6 @@ void main() { () => ditto.archive( source: any(named: 'source'), destination: any(named: 'destination'), - keepParent: any(named: 'keepParent'), ), ).thenAnswer((invocation) async { File( diff --git a/packages/shorebird_cli/test/src/mocks.dart b/packages/shorebird_cli/test/src/mocks.dart index 3aafe74f0..76c967831 100644 --- a/packages/shorebird_cli/test/src/mocks.dart +++ b/packages/shorebird_cli/test/src/mocks.dart @@ -45,8 +45,6 @@ class MockAccessCredentials extends Mock implements AccessCredentials {} class MockAdb extends Mock implements Adb {} -class MockAndroidArchiveDiffer extends Mock implements AndroidArchiveDiffer {} - class MockAndroidSdk extends Mock implements AndroidSdk {} class MockAndroidStudio extends Mock implements AndroidStudio {} @@ -111,8 +109,6 @@ class MockIOSink extends Mock implements IOSink {} class MockIos extends Mock implements Ios {} -class MockIosArchiveDiffer extends Mock implements IosArchiveDiffer {} - class MockJava extends Mock implements Java {} class MockJwtHeader extends Mock implements JwtHeader {}