diff --git a/packages/shorebird_cli/lib/src/archive_analysis/android_archive_differ.dart b/packages/shorebird_cli/lib/src/archive_analysis/android_archive_differ.dart index 48beecc4..9fea415c 100644 --- a/packages/shorebird_cli/lib/src/archive_analysis/android_archive_differ.dart +++ b/packages/shorebird_cli/lib/src/archive_analysis/android_archive_differ.dart @@ -6,14 +6,6 @@ import 'package:shorebird_cli/src/archive_analysis/archive_differ.dart'; abstract class AndroidArchiveDiffer extends ArchiveDiffer { @override bool containsPotentiallyBreakingAssetDiffs(FileSetDiff fileSetDiff) { - // If only files in this set have changed, we don't need to warn the user - // about asset differences. - const assetFileNamesToIgnore = { - 'AssetManifest.bin', - 'AssetManifest.json', - 'NOTICES.Z', - }; - final assetsDiff = assetsFileSetDiff(fileSetDiff); // If assets were added, we need to warn the user about asset differences. @@ -22,7 +14,10 @@ abstract class AndroidArchiveDiffer extends ArchiveDiffer { } return assetsDiff.changedPaths - .whereNot((path) => assetFileNamesToIgnore.contains(p.basename(path))) + .whereNot( + (path) => + ArchiveDiffer.assetFileNamesToIgnore.contains(p.basename(path)), + ) .isNotEmpty; } @@ -30,25 +25,8 @@ abstract class AndroidArchiveDiffer extends ArchiveDiffer { bool containsPotentiallyBreakingNativeDiffs(FileSetDiff fileSetDiff) => nativeFileSetDiff(fileSetDiff).isNotEmpty; - FileSetDiff assetsFileSetDiff(FileSetDiff fileSetDiff) => FileSetDiff( - addedPaths: fileSetDiff.addedPaths.where(_isAssetFilePath).toSet(), - removedPaths: fileSetDiff.removedPaths.where(_isAssetFilePath).toSet(), - changedPaths: fileSetDiff.changedPaths.where(_isAssetFilePath).toSet(), - ); - - FileSetDiff dartFileSetDiff(FileSetDiff fileSetDiff) => FileSetDiff( - addedPaths: fileSetDiff.addedPaths.where(_isDartFilePath).toSet(), - removedPaths: fileSetDiff.removedPaths.where(_isDartFilePath).toSet(), - changedPaths: fileSetDiff.changedPaths.where(_isDartFilePath).toSet(), - ); - - FileSetDiff nativeFileSetDiff(FileSetDiff fileSetDiff) => FileSetDiff( - addedPaths: fileSetDiff.addedPaths.where(_isNativeFilePath).toSet(), - removedPaths: fileSetDiff.removedPaths.where(_isNativeFilePath).toSet(), - changedPaths: fileSetDiff.changedPaths.where(_isNativeFilePath).toSet(), - ); - - static bool _isAssetFilePath(String filePath) { + @override + bool isAssetFilePath(String filePath) { const assetDirNames = ['assets', 'res']; const assetFileNames = ['AssetManifest.json']; @@ -58,11 +36,12 @@ abstract class AndroidArchiveDiffer extends ArchiveDiffer { assetFileNames.contains(p.basename(filePath)); } - static bool _isDartFilePath(String filePath) { + @override + bool isDartFilePath(String filePath) { const dartFileNames = ['libapp.so', 'libflutter.so']; return dartFileNames.contains(p.basename(filePath)); } - static bool _isNativeFilePath(String filePath) => - p.extension(filePath) == '.dex'; + @override + bool isNativeFilePath(String filePath) => p.extension(filePath) == '.dex'; } 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 de60b4f9..f3bf2704 100644 --- a/packages/shorebird_cli/lib/src/archive_analysis/archive_analysis.dart +++ b/packages/shorebird_cli/lib/src/archive_analysis/archive_analysis.dart @@ -2,3 +2,4 @@ export 'aab_differ.dart'; export 'aar_differ.dart'; export 'file_set_diff.dart'; export 'ipa.dart'; +export 'ipa_differ.dart'; diff --git a/packages/shorebird_cli/lib/src/archive_analysis/archive_differ.dart b/packages/shorebird_cli/lib/src/archive_analysis/archive_differ.dart index 760a15d8..198e2993 100644 --- a/packages/shorebird_cli/lib/src/archive_analysis/archive_differ.dart +++ b/packages/shorebird_cli/lib/src/archive_analysis/archive_differ.dart @@ -1,3 +1,5 @@ +import 'package:collection/collection.dart'; +import 'package:path/path.dart' as p; import 'package:shorebird_cli/src/archive_analysis/archive_analysis.dart'; /// Thrown when an [ArchiveDiffer] fails to generate a [FileSetDiff]. @@ -5,15 +7,69 @@ class DiffFailedException implements Exception {} /// Computes content differences between two archives. abstract class ArchiveDiffer { + /// Asset files that are not considered to be breaking changes. + static const assetFileNamesToIgnore = { + 'AssetManifest.bin', + 'AssetManifest.json', + 'NOTICES.Z', + }; + /// Files that have been added, removed, or that have changed between the /// archives at the two provided paths. FileSetDiff changedFiles(String oldArchivePath, String newArchivePath); /// Whether there are asset differences between the archives that may cause /// issues when patching a release. - bool containsPotentiallyBreakingAssetDiffs(FileSetDiff fileSetDiff); + bool containsPotentiallyBreakingAssetDiffs(FileSetDiff fileSetDiff) { + final assetsDiff = assetsFileSetDiff(fileSetDiff); + + // If assets were added, we need to warn the user about asset differences. + if (assetsDiff.addedPaths.isNotEmpty) { + return true; + } + + return assetsDiff.changedPaths + .whereNot( + (path) => + ArchiveDiffer.assetFileNamesToIgnore.contains(p.basename(path)), + ) + .isNotEmpty; + } /// Whether there are native code differences between the archives that may /// cause issues when patching a release. bool containsPotentiallyBreakingNativeDiffs(FileSetDiff fileSetDiff); + + /// Whether the provided file path represents a changed asset. + bool isAssetFilePath(String filePath); + + /// Whether the provided file path represents changed Dart code. + bool isDartFilePath(String filePath); + + /// Whether the provided file path represents changed Native code. + bool isNativeFilePath(String filePath); + + /// The subset of [fileSetDiff] that contains only changes that result from + /// edited assets. + FileSetDiff assetsFileSetDiff(FileSetDiff fileSetDiff) => FileSetDiff( + addedPaths: fileSetDiff.addedPaths.where(isAssetFilePath).toSet(), + removedPaths: fileSetDiff.removedPaths.where(isAssetFilePath).toSet(), + changedPaths: fileSetDiff.changedPaths.where(isAssetFilePath).toSet(), + ); + + /// The subset of [fileSetDiff] that contains only changes that result from + /// edited Dart code. + FileSetDiff dartFileSetDiff(FileSetDiff fileSetDiff) => FileSetDiff( + addedPaths: fileSetDiff.addedPaths.where(isDartFilePath).toSet(), + removedPaths: fileSetDiff.removedPaths.where(isDartFilePath).toSet(), + changedPaths: fileSetDiff.changedPaths.where(isDartFilePath).toSet(), + ); + + /// The subset of [fileSetDiff] that contains only changes that result from + /// edited native code. + FileSetDiff nativeFileSetDiff(FileSetDiff fileSetDiff) => FileSetDiff( + addedPaths: fileSetDiff.addedPaths.where(isNativeFilePath).toSet(), + removedPaths: fileSetDiff.removedPaths.where(isNativeFilePath).toSet(), + changedPaths: fileSetDiff.changedPaths.where(isNativeFilePath).toSet(), + ); } diff --git a/packages/shorebird_cli/lib/src/archive_analysis/ipa_differ.dart b/packages/shorebird_cli/lib/src/archive_analysis/ipa_differ.dart new file mode 100644 index 00000000..74d41184 --- /dev/null +++ b/packages/shorebird_cli/lib/src/archive_analysis/ipa_differ.dart @@ -0,0 +1,74 @@ +import 'dart:io'; + +import 'package:archive/archive_io.dart'; +import 'package:path/path.dart' as p; +import 'package:shorebird_cli/src/archive_analysis/archive_differ.dart'; +import 'package:shorebird_cli/src/archive_analysis/file_set_diff.dart'; + +/// Finds differences between two IPAs. +/// +/// 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` +/// directory. +/// +/// Native changes will appear in the Runner.app/Runner executable and non +/// Flutter.framework or App.framework files. +/// +/// Dart changes will appear in the App.framework/App executable. +class IpaDiffer extends ArchiveDiffer { + static const binaryFiles = { + 'App.framework/App', + 'Flutter.framework/Flutter', + }; + static RegExp appRegex = RegExp(r'^Payload/[\w\-. ]+.app/[\w\-. ]+$'); + + @override + FileSetDiff changedFiles(String oldArchivePath, String newArchivePath) => + FileSetDiff.fromPathHashes( + oldPathHashes: _fileHashes(File(oldArchivePath)), + newPathHashes: _fileHashes(File(newArchivePath)), + ); + + @override + bool containsPotentiallyBreakingAssetDiffs(FileSetDiff fileSetDiff) => + assetsFileSetDiff(fileSetDiff).isNotEmpty; + + @override + bool containsPotentiallyBreakingNativeDiffs(FileSetDiff fileSetDiff) { + // Because the mach-o binaries are signed (we believe with in expiration + // date), they will always have different hashes, even if the code used to + // generate them is identical. + // + // TODO(bryanoltman): support mach-o binary diffing. + // We can do this using the `codesign --remove-signature` command, but this + // is slow and requires a temporary directory to store the unsigned binary. + return false; + } + + @override + bool isAssetFilePath(String filePath) { + /// The flutter_assets directory contains the assets listed in the assets + /// section of the pubspec.yaml file. + /// Assets.car is the compiled asset catalog(s) (.xcassets files). + return p.basename(filePath) == 'Assets.car' || + p.split(filePath).contains('flutter_assets'); + } + + @override + bool isDartFilePath(String filePath) => + filePath.endsWith('App.framework/App'); + + @override + bool isNativeFilePath(String filePath) => appRegex.hasMatch(filePath); + + PathHashes _fileHashes(File ipa) { + final zipDirectory = ZipDirectory.read(InputFileStream(ipa.path)); + return { + for (final file in zipDirectory.fileHeaders) + // Zip files contain an (optional) crc32 checksum for a file. IPAs seem + // to always include this for files, so a quick way for us to tell if + // file contents differ is if their checksums differ. + file.filename: file.crc32!.toString() + }; + } +} diff --git a/packages/shorebird_cli/test/fixtures/ipas/README.md b/packages/shorebird_cli/test/fixtures/ipas/README.md index f098c35d..0055a8ef 100644 --- a/packages/shorebird_cli/test/fixtures/ipas/README.md +++ b/packages/shorebird_cli/test/fixtures/ipas/README.md @@ -2,7 +2,11 @@ The ipa files in this folder were generated by building the stock Flutter counte Some of their contents has been removed to reduce the size of the files. All .dylib files have been removed. -Files: -- base.ipa is meant to represent an ipa uploaded as part of a release. -- no_version.ipa is base.ipa with the version info removed from the Info.plist, along with several other files to save space. -- no_plist.ipa is base.ipa with the Info.plist (and many other parts) removed. +Files: + +- base.ipa is meant to represent an ipa uploaded as part of a release. App.framework/App and Flutter.framework/Flutter have been removed to save space. +- no_version.ipa an .ipa with the version info removed from the Info.plist, along with several other files to save space. +- no_plist.ipa an .ipa with the Info.plist (and many other parts) removed. +- app_file_space.ipa is an .ipa with a space in the .app file name. +- asset_changes.ipa is built from the same codebase as base.ipa with a change made to one of the assets. App.framework/App and Flutter.framework/Flutter have been removed to save space. +- dart_changes.ipa is built from the same codebase as base.ipa with a change made to the dart code. App.framework/App and Flutter.framework/Flutter have been removed to save space. diff --git a/packages/shorebird_cli/test/fixtures/ipas/asset_changes.ipa b/packages/shorebird_cli/test/fixtures/ipas/asset_changes.ipa new file mode 100644 index 00000000..b03a9ae7 Binary files /dev/null and b/packages/shorebird_cli/test/fixtures/ipas/asset_changes.ipa differ diff --git a/packages/shorebird_cli/test/fixtures/ipas/base.ipa b/packages/shorebird_cli/test/fixtures/ipas/base.ipa index d86f21a8..caede42f 100644 Binary files a/packages/shorebird_cli/test/fixtures/ipas/base.ipa and b/packages/shorebird_cli/test/fixtures/ipas/base.ipa differ diff --git a/packages/shorebird_cli/test/fixtures/ipas/dart_changes.ipa b/packages/shorebird_cli/test/fixtures/ipas/dart_changes.ipa new file mode 100644 index 00000000..27c63c95 Binary files /dev/null and b/packages/shorebird_cli/test/fixtures/ipas/dart_changes.ipa differ diff --git a/packages/shorebird_cli/test/fixtures/ipas/swift_changes.ipa b/packages/shorebird_cli/test/fixtures/ipas/swift_changes.ipa new file mode 100644 index 00000000..f559a114 Binary files /dev/null and b/packages/shorebird_cli/test/fixtures/ipas/swift_changes.ipa differ diff --git a/packages/shorebird_cli/test/src/archive_analysis/aab_differ_test.dart b/packages/shorebird_cli/test/src/archive_analysis/aab_differ_test.dart index 9ba8a09e..4548f4ed 100644 --- a/packages/shorebird_cli/test/src/archive_analysis/aab_differ_test.dart +++ b/packages/shorebird_cli/test/src/archive_analysis/aab_differ_test.dart @@ -22,7 +22,7 @@ void main() { differ = AabDiffer(); }); - group('changedPaths', () { + group('changedFiles', () { test('finds no differences between the same aab', () { expect(differ.changedFiles(baseAabPath, baseAabPath), isEmpty); }); diff --git a/packages/shorebird_cli/test/src/archive_analysis/aar_differ_test.dart b/packages/shorebird_cli/test/src/archive_analysis/aar_differ_test.dart index e59650ed..22a6b6ce 100644 --- a/packages/shorebird_cli/test/src/archive_analysis/aar_differ_test.dart +++ b/packages/shorebird_cli/test/src/archive_analysis/aar_differ_test.dart @@ -16,7 +16,7 @@ void main() { differ = AarDiffer(); }); - group('changedPaths', () { + group('changedFiles', () { test('finds no differences between the same aar', () { expect(differ.changedFiles(baseAarPath, baseAarPath), isEmpty); }); diff --git a/packages/shorebird_cli/test/src/archive_analysis/archive_differ_test.dart b/packages/shorebird_cli/test/src/archive_analysis/archive_differ_test.dart new file mode 100644 index 00000000..4cf25340 --- /dev/null +++ b/packages/shorebird_cli/test/src/archive_analysis/archive_differ_test.dart @@ -0,0 +1,79 @@ +import 'package:shorebird_cli/src/archive_analysis/archive_differ.dart'; +import 'package:shorebird_cli/src/archive_analysis/file_set_diff.dart'; +import 'package:test/test.dart'; + +const assetFilePath = 'some/assets/file'; + +void main() { + group(ArchiveDiffer, () { + late TestArchiveDiffer archiveDiffer; + + setUp(() { + archiveDiffer = TestArchiveDiffer(); + }); + + group('containsPotentiallyBreakingAssetDiffs', () { + test('returns true if any assets were added', () { + archiveDiffer.changedFileSetDiff = FileSetDiff( + addedPaths: {assetFilePath}, + removedPaths: {}, + changedPaths: {}, + ); + expect( + archiveDiffer.containsPotentiallyBreakingAssetDiffs( + archiveDiffer.changedFileSetDiff, + ), + isTrue, + ); + }); + + test('returns false if changed assets are all in the ignore list', () { + archiveDiffer.changedFileSetDiff = FileSetDiff( + addedPaths: {}, + removedPaths: {}, + changedPaths: {'AssetManifest.bin'}, + ); + expect( + archiveDiffer.containsPotentiallyBreakingAssetDiffs( + archiveDiffer.changedFileSetDiff, + ), + isFalse, + ); + }); + + test('returns true if changed assets are not all in the ignore list', () { + archiveDiffer.changedFileSetDiff = FileSetDiff( + addedPaths: {}, + removedPaths: {}, + changedPaths: {assetFilePath}, + ); + expect( + archiveDiffer.containsPotentiallyBreakingAssetDiffs( + archiveDiffer.changedFileSetDiff, + ), + isTrue, + ); + }); + }); + }); +} + +class TestArchiveDiffer extends ArchiveDiffer { + FileSetDiff changedFileSetDiff = FileSetDiff.empty(); + + @override + FileSetDiff changedFiles(String oldArchivePath, String newArchivePath) => + changedFileSetDiff; + + @override + bool containsPotentiallyBreakingNativeDiffs(FileSetDiff fileSetDiff) => false; + + @override + bool isAssetFilePath(String filePath) => filePath == assetFilePath; + + @override + bool isDartFilePath(String filePath) => true; + + @override + bool isNativeFilePath(String filePath) => true; +} diff --git a/packages/shorebird_cli/test/src/archive_analysis/ipa_differ_test.dart b/packages/shorebird_cli/test/src/archive_analysis/ipa_differ_test.dart new file mode 100644 index 00000000..4055a8bf --- /dev/null +++ b/packages/shorebird_cli/test/src/archive_analysis/ipa_differ_test.dart @@ -0,0 +1,105 @@ +import 'package:path/path.dart' as p; +import 'package:shorebird_cli/src/archive_analysis/archive_analysis.dart'; +import 'package:test/test.dart'; + +void main() { + final ipaFixturesBasePath = p.join('test', 'fixtures', 'ipas'); + final baseIpaPath = p.join(ipaFixturesBasePath, 'base.ipa'); + final changedAssetIpaPath = p.join(ipaFixturesBasePath, 'asset_changes.ipa'); + final changedDartIpaPath = p.join(ipaFixturesBasePath, 'dart_changes.ipa'); + final changedSwiftIpaPath = p.join(ipaFixturesBasePath, 'swift_changes.ipa'); + + late IpaDiffer differ; + + setUp(() { + differ = IpaDiffer(); + }); + + group(IpaDiffer, () { + group('changedPaths', () { + test('finds no differences between the same ipa', () { + expect(differ.changedFiles(baseIpaPath, baseIpaPath), isEmpty); + }); + + test('finds differences between two different ipas', () { + expect( + differ.changedFiles(baseIpaPath, changedAssetIpaPath).changedPaths, + { + 'Payload/Runner.app/_CodeSignature/CodeResources', + 'Payload/Runner.app/Runner', + 'Payload/Runner.app/Frameworks/Flutter.framework/Flutter', + 'Payload/Runner.app/Frameworks/App.framework/_CodeSignature/CodeResources', + 'Payload/Runner.app/Frameworks/App.framework/App', + 'Payload/Runner.app/Frameworks/App.framework/flutter_assets/assets/asset.json', + 'Symbols/4C4C4411-5555-3144-A13A-E47369D8ACD5.symbols', + 'Symbols/BC970605-0A53-3457-8736-D7A870AB6E71.symbols', + 'Symbols/0CBBC9EF-0745-3074-81B7-765F5B4515FD.symbols', + }, + ); + }); + }); + + group('changedFiles', () { + test('detects asset changes', () { + final fileSetDiff = + differ.changedFiles(baseIpaPath, changedAssetIpaPath); + expect(differ.assetsFileSetDiff(fileSetDiff), isNotEmpty); + expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty); + expect(differ.nativeFileSetDiff(fileSetDiff), isNotEmpty); + }); + + test('detects dart changes', () { + final fileSetDiff = + differ.changedFiles(baseIpaPath, changedDartIpaPath); + expect(differ.assetsFileSetDiff(fileSetDiff), isEmpty); + expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty); + expect(differ.nativeFileSetDiff(fileSetDiff), isNotEmpty); + }); + + test('detects swift changes', () { + final fileSetDiff = + differ.changedFiles(baseIpaPath, changedSwiftIpaPath); + expect(differ.assetsFileSetDiff(fileSetDiff), isEmpty); + expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty); + expect(differ.nativeFileSetDiff(fileSetDiff), isNotEmpty); + }); + }); + + group('containsPotentiallyBreakingAssetDiffs', () { + test('returns true if a file in flutter_assets has changed', () { + final fileSetDiff = differ.changedFiles( + baseIpaPath, + changedAssetIpaPath, + ); + expect( + differ.containsPotentiallyBreakingAssetDiffs(fileSetDiff), + isTrue, + ); + }); + + test('returns false if no files in flutter_assets has changed', () { + final fileSetDiff = differ.changedFiles( + baseIpaPath, + changedDartIpaPath, + ); + expect( + differ.containsPotentiallyBreakingAssetDiffs(fileSetDiff), + isFalse, + ); + }); + }); + + group('containsPotentiallyBreakingNativeDiffs', () { + test("always returns false, as we don't check for this yet", () { + final fileSetDiff = differ.changedFiles( + baseIpaPath, + changedSwiftIpaPath, + ); + expect( + differ.containsPotentiallyBreakingNativeDiffs(fileSetDiff), + isFalse, + ); + }); + }); + }); +}