Skip to content

Commit

Permalink
fix(shorebird_cli): fix hangs in patch command output (#1647)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bryanoltman
bryanoltman authored Jan 18, 2024
1 parent f7b0e05 commit fcc6e63
Show file tree
Hide file tree
Showing 10 changed files with 132 additions and 99 deletions.
36 changes: 22 additions & 14 deletions packages/shorebird_cli/lib/src/archive_analysis/archive_differ.dart
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'dart:io';
import 'dart:isolate';

import 'package:archive/archive_io.dart';
import 'package:collection/collection.dart';
Expand Down Expand Up @@ -74,20 +75,27 @@ abstract class ArchiveDiffer {

/// Files that have been added, removed, or that have changed between the
/// archives at the two provided paths.
FileSetDiff changedFiles(String oldArchivePath, String newArchivePath) =>
FileSetDiff.fromPathHashes(
oldPathHashes: fileHashes(File(oldArchivePath)),
newPathHashes: fileHashes(File(newArchivePath)),
);
Future<FileSetDiff> changedFiles(
String oldArchivePath,
String newArchivePath,
) async {
return FileSetDiff.fromPathHashes(
oldPathHashes: await fileHashes(File(oldArchivePath)),
newPathHashes: await fileHashes(File(newArchivePath)),
);
}

Future<PathHashes> fileHashes(File archive) async {
return Isolate.run(() {
final zipDirectory = ZipDirectory.read(InputFileStream(archive.path));

PathHashes fileHashes(File archive) {
final zipDirectory = ZipDirectory.read(InputFileStream(archive.path));
return {
for (final file in zipDirectory.fileHeaders)
// Zip files contain an (optional) crc32 checksum for a file. IPAs and
// AARs 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(),
};
return {
for (final file in zipDirectory.fileHeaders)
// Zip files contain an (optional) crc32 checksum for a file. IPAs and
// AARs 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(),
};
});
}
}
46 changes: 27 additions & 19 deletions packages/shorebird_cli/lib/src/archive_analysis/ios_archive_differ.dart
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'dart:io';
import 'dart:isolate';

import 'package:archive/archive_io.dart';
import 'package:collection/collection.dart';
Expand Down Expand Up @@ -32,15 +33,18 @@ class IosArchiveDiffer extends ArchiveDiffer {
/// archives at the two provided paths. This method will also unisgn mach-o
/// binaries in the archives before computing the diff.
@override
FileSetDiff changedFiles(String oldArchivePath, String newArchivePath) {
final oldPathHashes = fileHashes(File(oldArchivePath));
final newPathHashes = fileHashes(File(newArchivePath));

_updateHashes(
Future<FileSetDiff> changedFiles(
String oldArchivePath,
String newArchivePath,
) async {
var oldPathHashes = await fileHashes(File(oldArchivePath));
var newPathHashes = await fileHashes(File(newArchivePath));

oldPathHashes = await _updateHashes(
archivePath: oldArchivePath,
pathHashes: oldPathHashes,
);
_updateHashes(
newPathHashes = await _updateHashes(
archivePath: newArchivePath,
pathHashes: newPathHashes,
);
Expand All @@ -55,17 +59,21 @@ class IosArchiveDiffer extends ArchiveDiffer {
/// includes:
/// - Signed files (those with a .app extension)
/// - Compiled asset catalogs (those with a .car extension)
void _updateHashes({
Future<PathHashes> _updateHashes({
required String archivePath,
required PathHashes pathHashes,
}) {
for (final file in _filesToUnsign(archivePath)) {
pathHashes[file.name] = _unsignedFileHash(file);
}

for (final file in _carFiles(archivePath)) {
pathHashes[file.name] = _carFileHash(file);
}
}) async {
return Isolate.run(() async {
for (final file in _filesToUnsign(archivePath)) {
pathHashes[file.name] = await _unsignedFileHash(file);
}

for (final file in _carFiles(archivePath)) {
pathHashes[file.name] = await _carFileHash(file);
}

return pathHashes;
});
}

List<ArchiveFile> _filesToUnsign(String archivePath) {
Expand All @@ -90,12 +98,12 @@ class IosArchiveDiffer extends ArchiveDiffer {
.toList();
}

String _unsignedFileHash(ArchiveFile file) {
Future<String> _unsignedFileHash(ArchiveFile file) async {
final tempDir = Directory.systemTemp.createTempSync();
final outPath = p.join(tempDir.path, file.name);
final outputStream = OutputFileStream(outPath);
file.writeContent(outputStream);
outputStream.close();
await outputStream.close();

if (Platform.isMacOS) {
// coverage:ignore-start
Expand All @@ -110,12 +118,12 @@ class IosArchiveDiffer extends ArchiveDiffer {
/// Uses assetutil to write a json description of a .car file to disk and
/// diffs the contents of that file, less a timestamp line that chnages based
/// on when the .car file was created.
String _carFileHash(ArchiveFile file) {
Future<String> _carFileHash(ArchiveFile file) async {
final tempDir = Directory.systemTemp.createTempSync();
final outPath = p.join(tempDir.path, file.name);
final outputStream = OutputFileStream(outPath);
file.writeContent(outputStream);
outputStream.close();
await outputStream.close();

final assetInfoPath = '$outPath.json';

Expand Down
11 changes: 7 additions & 4 deletions packages/shorebird_cli/lib/src/commands/patch/patch_android_command.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,17 @@ Current Flutter Revision: $originalFlutterRevision
}
}

final releaseAabArtifactFile = await artifactManager.downloadFile(
Uri.parse(releaseAabArtifact.url),
);

downloadReleaseArtifactProgress.complete();

try {
await patchDiffChecker.confirmUnpatchableDiffsIfNecessary(
final diffChecker = patchDiffChecker;
await diffChecker.confirmUnpatchableDiffsIfNecessary(
localArtifact: File(bundlePath),
releaseArtifact: await artifactManager.downloadFile(
Uri.parse(releaseAabArtifact.url),
),
releaseArtifact: releaseAabArtifactFile,
archiveDiffer: _archiveDiffer,
force: force,
);
Expand Down
2 changes: 1 addition & 1 deletion packages/shorebird_cli/lib/src/patch_diff_checker.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class PatchDiffChecker {
final progress =
logger.progress('Verifying patch can be applied to release');

final contentDiffs = archiveDiffer.changedFiles(
final contentDiffs = await archiveDiffer.changedFiles(
releaseArtifact.path,
localArtifact.path,
);
Expand Down
67 changes: 36 additions & 31 deletions packages/shorebird_cli/test/src/archive_analysis/android_archive_differ_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@ void main() {

group('aab', () {
group('changedFiles', () {
test('finds no differences between the same aab', () {
expect(differ.changedFiles(baseAabPath, baseAabPath), isEmpty);
test('finds no differences between the same aab', () async {
expect(await differ.changedFiles(baseAabPath, baseAabPath), isEmpty);
});

test('finds differences between two different aabs', () {
test('finds differences between two different aabs', () async {
final fileSetDiff =
await differ.changedFiles(baseAabPath, changedDartAabPath);
expect(
differ.changedFiles(baseAabPath, changedDartAabPath).changedPaths,
fileSetDiff.changedPaths,
{
'BUNDLE-METADATA/com.android.tools.build.libraries/dependencies.pb',
'base/lib/arm64-v8a/libapp.so',
Expand All @@ -52,37 +54,39 @@ void main() {
});

group('contentDifferences', () {
test('detects no differences between the same aab', () {
expect(differ.changedFiles(baseAabPath, baseAabPath), isEmpty);
test('detects no differences between the same aab', () async {
expect(await differ.changedFiles(baseAabPath, baseAabPath), isEmpty);
});

test('detects asset changes', () {
test('detects asset changes', () async {
final fileSetDiff =
differ.changedFiles(baseAabPath, changedAssetAabPath);
await differ.changedFiles(baseAabPath, changedAssetAabPath);
expect(differ.assetsFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isEmpty);
});

test('detects kotlin changes', () {
test('detects kotlin changes', () async {
final fileSetDiff =
differ.changedFiles(baseAabPath, changedKotlinAabPath);
await differ.changedFiles(baseAabPath, changedKotlinAabPath);
expect(differ.assetsFileSetDiff(fileSetDiff), isEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isNotEmpty);
});

test('detects dart changes', () {
test('detects dart changes', () async {
final fileSetDiff =
differ.changedFiles(baseAabPath, changedDartAabPath);
await differ.changedFiles(baseAabPath, changedDartAabPath);
expect(differ.assetsFileSetDiff(fileSetDiff), isEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isEmpty);
});

test('detects dart and asset changes', () {
final fileSetDiff =
differ.changedFiles(baseAabPath, changedDartAndAssetAabPath);
test('detects dart and asset changes', () async {
final fileSetDiff = await differ.changedFiles(
baseAabPath,
changedDartAndAssetAabPath,
);
expect(differ.assetsFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isEmpty);
Expand All @@ -92,42 +96,43 @@ void main() {

group('aar', () {
group('changedFiles', () {
test('finds no differences between the same aar', () {
expect(differ.changedFiles(baseAarPath, baseAarPath), isEmpty);
test('finds no differences between the same aar', () async {
expect(await differ.changedFiles(baseAarPath, baseAarPath), isEmpty);
});

test('finds differences between two different aars', () {
expect(
differ.changedFiles(baseAarPath, changedDartAarPath).changedPaths,
{'jni/arm64-v8a/libapp.so'},
);
test('finds differences between two different aars', () async {
final fileSetDiff =
await differ.changedFiles(baseAarPath, changedDartAarPath);
expect(fileSetDiff.changedPaths, {'jni/arm64-v8a/libapp.so'});
});
});

group('changedFiles', () {
test('detects no differences between the same aar', () {
expect(differ.changedFiles(baseAarPath, baseAarPath), isEmpty);
test('detects no differences between the same aar', () async {
expect(await differ.changedFiles(baseAarPath, baseAarPath), isEmpty);
});

test('detects asset changes', () {
test('detects asset changes', () async {
final fileSetDiff =
differ.changedFiles(baseAarPath, changedAssetAarPath);
await differ.changedFiles(baseAarPath, changedAssetAarPath);
expect(differ.assetsFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isEmpty);
});

test('detects dart changes', () {
test('detects dart changes', () async {
final fileSetDiff =
differ.changedFiles(baseAarPath, changedDartAarPath);
await differ.changedFiles(baseAarPath, changedDartAarPath);
expect(differ.assetsFileSetDiff(fileSetDiff), isEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isEmpty);
});

test('detects dart and asset changes', () {
final fileSetDiff =
differ.changedFiles(baseAarPath, changedDartAndAssetAarPath);
test('detects dart and asset changes', () async {
final fileSetDiff = await differ.changedFiles(
baseAarPath,
changedDartAndAssetAarPath,
);
expect(differ.assetsFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.dartFileSetDiff(fileSetDiff), isNotEmpty);
expect(differ.nativeFileSetDiff(fileSetDiff), isEmpty);
Expand Down
5 changes: 4 additions & 1 deletion packages/shorebird_cli/test/src/archive_analysis/archive_differ_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ class TestArchiveDiffer extends ArchiveDiffer {
FileSetDiff changedFileSetDiff = FileSetDiff.empty();

@override
FileSetDiff changedFiles(String oldArchivePath, String newArchivePath) =>
Future<FileSetDiff> changedFiles(
String oldArchivePath,
String newArchivePath,
) async =>
changedFileSetDiff;

@override
Expand Down
Loading

0 comments on commit fcc6e63

Please sign in to comment.