From cf617aef81bb9755b0168d46fdfaef8d6398f3ac Mon Sep 17 00:00:00 2001 From: Felix Angelov Date: Mon, 30 Sep 2024 13:59:51 -0400 Subject: [PATCH] refactor(shorebird_cli): `AotTools` process execution and error reporting (#2503) --- .../lib/src/executables/aot_tools.dart | 172 +++++++------ .../test/src/executables/aot_tools_test.dart | 228 +++++++++--------- 2 files changed, 220 insertions(+), 180 deletions(-) diff --git a/packages/shorebird_cli/lib/src/executables/aot_tools.dart b/packages/shorebird_cli/lib/src/executables/aot_tools.dart index 73ee799b..312d9cc3 100644 --- a/packages/shorebird_cli/lib/src/executables/aot_tools.dart +++ b/packages/shorebird_cli/lib/src/executables/aot_tools.dart @@ -67,6 +67,39 @@ const _preLinkerFlutterRevisions = { '1a6115bebe31e63508c312d14e69e973e1a59dbf', }; +/// {@template aot_tools_execution_failure} +/// Exception thrown when aot_tools execution exits with a non-zero exit code. +/// {@endtemplate} +class AotToolsExecutionFailure implements Exception { + /// {@macro aot_tools_execution_failure} + const AotToolsExecutionFailure({ + required this.exitCode, + required this.stdout, + required this.stderr, + required this.command, + }); + + /// The exit code of the failed aot_tools execution. + final int exitCode; + + /// The standard output of the failed aot_tools execution. + final String stdout; + + /// The standard error of the failed aot_tools execution. + final String stderr; + + /// The command that was executed. + final String command; + + @override + String toString() { + return ''' +$command failed with exit code $exitCode +stdout: $stdout +stderr: $stderr'''; + } +} + /// Wrapper around the shorebird `aot-tools` executable. class AotTools { /// Returns true if the linker should be used for the given Flutter revision. @@ -86,17 +119,13 @@ class AotTools { /// Runs the `aot-tools` executable with the given [command] and await for /// its completion. /// - /// If no [runCommand] is provided, [ShorebirdProcess.run] is used. + /// If the command exits with a non-zero exit code, an + /// [AotToolsExecutionFailure] is thrown. Future _exec( List command, { - Future Function( - String, - List, { - String? workingDirectory, - })? runCommand, String? workingDirectory, + bool throwOnError = true, }) async { - final runFn = runCommand ?? process.run; await cache.updateAll(); // This will be a path to either a kernel (.dill) file or a Dart script if @@ -105,68 +134,78 @@ class AotTools { artifact: ShorebirdArtifact.aotTools, ); + final ShorebirdProcessResult result; + + // Similar to [ShorebirdProcess.run] but uses [ShorebirdProcess.start] + // instead and captures the live stdout and stderr of the process. + Future execute( + String exe, + List args, { + String? workingDirectory, + }) async { + final subprocess = await process.start( + exe, + args, + workingDirectory: workingDirectory, + ); + + final stdout = StringBuffer(); + final stderr = StringBuffer(); + + final stdoutSubscription = subprocess.stdout.map(utf8.decode).listen( + (data) { + logger.detail(data); + stdout.write(data); + }, + ); + + final stderrSubscription = subprocess.stderr.map(utf8.decode).listen( + (data) { + logger.detail(data); + stderr.write(data); + }, + ); + + final exitCode = await subprocess.exitCode; + + stdoutSubscription.cancel().ignore(); + stderrSubscription.cancel().ignore(); + + return ShorebirdProcessResult( + exitCode: exitCode, + stdout: stdout.toString(), + stderr: stderr.toString(), + ); + } + // Fallback behavior for older versions of shorebird where aot-tools was // distributed as an executable. final extension = p.extension(artifactPath); if (extension != '.dill' && extension != '.dart') { - return runFn( + result = await execute( artifactPath, command, workingDirectory: workingDirectory, ); + } else { + // local engine versions use .dart and we distribute aot-tools as a .dill + result = await execute( + shorebirdEnv.dartBinaryFile.path, + ['run', artifactPath, ...command], + workingDirectory: workingDirectory, + ); } - // local engine versions use .dart and we distribute aot-tools as a .dill - return runFn( - shorebirdEnv.dartBinaryFile.path, - ['run', artifactPath, ...command], - workingDirectory: workingDirectory, - ); - } + if (throwOnError && result.exitCode != 0) { + throw AotToolsExecutionFailure( + exitCode: result.exitCode, + stdout: result.stdout.toString(), + stderr: result.stderr.toString(), + command: ['aot_tools', ...command].join(' '), + ); + } - /// Similar to [_exec], but logs the sub process stdout and stderr - /// as they are emitted. - Future _execWithLiveLogs( - List command, { - String? workingDirectory, - }) { - return _exec( - command, - runCommand: ( - String exe, - List args, { - String? workingDirectory, - }) async { - final spawnedProcess = await process.start( - exe, - args, - workingDirectory: workingDirectory, - ); - - final stdout = StringBuffer(); - final stderr = StringBuffer(); - - final stdoutSub = spawnedProcess.stdout.map(utf8.decode).listen((data) { - logger.detail(data); - stdout.write(data); - }); - - final stderrSub = - spawnedProcess.stderr.map(utf8.decode).listen(stderr.write); - - final exitCode = await spawnedProcess.exitCode; - - await stdoutSub.cancel(); - await stderrSub.cancel(); - - return ShorebirdProcessResult( - exitCode: exitCode, - stdout: stdout.toString(), - stderr: stderr.toString(), - ); - }, - workingDirectory: workingDirectory, - ); + return result; } Future _linkerUsesGenSnapshot() async { @@ -179,7 +218,7 @@ class AotTools { // If callers need to care about null, we can change this function to // return Version?. final noVersion = Version(0, 0, 0); - final result = await _exec(['--version']); + final result = await _exec(['--version'], throwOnError: false); if (result.exitCode != ExitCode.success.code) { return noVersion; } @@ -209,7 +248,7 @@ class AotTools { const linkJson = 'link.jsonl'; final outputDir = p.dirname(outputPath); final linkerUsesGenSnapshot = await _linkerUsesGenSnapshot(); - final result = await _execWithLiveLogs( + await _exec( [ 'link', '--base=$base', @@ -228,13 +267,6 @@ class AotTools { workingDirectory: workingDirectory, ); - if (result.exitCode != 0) { - throw Exception(''' -Failed to link: -stdout: ${result.stdout} -stderr: ${result.stderr}'''); - } - return linkerUsesGenSnapshot ? _extractLinkPercentage(File(p.join(workingDirectory!, linkJson))) : null; @@ -275,7 +307,7 @@ stderr: ${result.stderr}'''); }) async { final tmpDir = Directory.systemTemp.createTempSync(); final outFile = File(p.join(tmpDir.path, 'diff_base')); - final result = await _exec( + await _exec( [ 'dump_blobs', '--analyze-snapshot=$analyzeSnapshotPath', @@ -284,10 +316,6 @@ stderr: ${result.stderr}'''); ], ); - if (result.exitCode != ExitCode.success.code) { - throw Exception('Failed to generate patch diff base: ${result.stderr}'); - } - if (!outFile.existsSync()) { throw Exception( 'Failed to generate patch diff base: output file does not exist', diff --git a/packages/shorebird_cli/test/src/executables/aot_tools_test.dart b/packages/shorebird_cli/test/src/executables/aot_tools_test.dart index 910e2379..d49bfe0c 100644 --- a/packages/shorebird_cli/test/src/executables/aot_tools_test.dart +++ b/packages/shorebird_cli/test/src/executables/aot_tools_test.dart @@ -1,7 +1,6 @@ import 'dart:convert'; import 'dart:io'; -import 'package:mason_logger/mason_logger.dart'; import 'package:mocktail/mocktail.dart'; import 'package:path/path.dart' as p; import 'package:scoped_deps/scoped_deps.dart'; @@ -67,7 +66,7 @@ void main() { final outputPath = p.join('.', 'path', 'to', 'out.vmcode'); final linkJsonPath = p.join('.', 'path', 'to', 'link.jsonl'); - test('throws Exception when process exits with non-zero code', () async { + test('throws exception when process exits with non-zero code', () async { when( () => shorebirdArtifacts.getArtifactPath( artifact: ShorebirdArtifact.aotTools, @@ -118,13 +117,14 @@ void main() { ), ), throwsA( - isA().having( + isA().having( (e) => '$e', - 'exception', - ''' -Exception: Failed to link: + 'toString', + contains( + ''' stdout: info stderr: error''', + ), ), ), ); @@ -206,19 +206,6 @@ stderr: error''', }); test('forwards stdout from aot_tools link to the logger', () async { - when( - () => process.run( - aotToolsPath, - any(), - workingDirectory: any(named: 'workingDirectory'), - ), - ).thenAnswer((_) async { - return const ShorebirdProcessResult( - exitCode: 0, - stdout: '', - stderr: '', - ); - }); when( () => process.start( aotToolsPath, @@ -252,7 +239,8 @@ stderr: error''', ), ); - verify(() => logger.detail('stdout')).called(1); + // One for --version and one for the link command. + verify(() => logger.detail('stdout')).called(2); }); }); @@ -506,17 +494,23 @@ stderr: error''', test('passes gen_snapshot to aot_tools', () async { when( - () => process.run( + () => process.start( aotToolsPath, ['--version'], workingDirectory: any(named: 'workingDirectory'), ), ).thenAnswer( - (_) async => const ShorebirdProcessResult( - exitCode: 0, - stdout: '0.0.1', - stderr: '', - ), + (_) async { + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => 0); + when( + () => mockProcess.stdout, + ).thenAnswer((_) => Stream.value(utf8.encode('0.0.1'))); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => const Stream.empty()); + return mockProcess; + }, ); when( () => process.start( @@ -576,18 +570,24 @@ stderr: error''', test('returns link percentage', () async { workingDirectory = Directory.systemTemp.createTempSync(); when( - () => process.run( + () => process.start( aotToolsPath, ['--version'], workingDirectory: any(named: 'workingDirectory'), ), - ).thenAnswer((_) async { - return const ShorebirdProcessResult( - exitCode: 0, - stdout: '0.0.1', - stderr: '', - ); - }); + ).thenAnswer( + (_) async { + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => 0); + when( + () => mockProcess.stdout, + ).thenAnswer((_) => Stream.value(utf8.encode('0.0.1'))); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => const Stream.empty()); + return mockProcess; + }, + ); when( () => process.start( aotToolsPath, @@ -653,9 +653,18 @@ stderr: error''', group('isLinkDebugInfoSupported', () { test('returns true when the argument is present in the help', () async { - final result = MockShorebirdProcessResult(); - when(() => result.exitCode).thenReturn(ExitCode.success.code); - when(() => result.stdout).thenReturn(''' + when( + () => process.start( + any(), + any(), + workingDirectory: any(named: 'workingDirectory'), + ), + ).thenAnswer((_) async { + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => 0); + when(() => mockProcess.stdout).thenAnswer( + (_) => Stream.value( + utf8.encode(''' Link two aot snapshots. Usage: aot_tools link [arguments] @@ -677,15 +686,14 @@ Usage: aot_tools link [arguments] --redirect-to Redirect output to a file. Run "aot_tools help" to see global options. -'''); - - when( - () => process.run( - any(), - any(), - workingDirectory: any(named: 'workingDirectory'), - ), - ).thenAnswer((_) async => result); +'''), + ), + ); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => const Stream.empty()); + return mockProcess; + }); await expectLater( runWithOverrides(() => aotTools.isLinkDebugInfoSupported()), @@ -696,9 +704,18 @@ Run "aot_tools help" to see global options. test( 'returns false when the argument is not present in the help', () async { - final result = MockShorebirdProcessResult(); - when(() => result.exitCode).thenReturn(ExitCode.success.code); - when(() => result.stdout).thenReturn(''' + when( + () => process.start( + any(), + any(), + workingDirectory: any(named: 'workingDirectory'), + ), + ).thenAnswer((_) async { + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => 0); + when(() => mockProcess.stdout).thenAnswer( + (_) => Stream.value( + utf8.encode(''' Link two aot snapshots. Usage: aot_tools link [arguments] @@ -719,15 +736,14 @@ Usage: aot_tools link [arguments] --redirect-to Redirect output to a file. Run "aot_tools help" to see global options. -'''); - - when( - () => process.run( - any(), - any(), - workingDirectory: any(named: 'workingDirectory'), - ), - ).thenAnswer((_) async => result); +'''), + ), + ); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => const Stream.empty()); + return mockProcess; + }); await expectLater( runWithOverrides(() => aotTools.isLinkDebugInfoSupported()), @@ -742,17 +758,23 @@ Run "aot_tools help" to see global options. var stdout = ''; setUp(() { when( - () => process.run( + () => process.start( dartBinaryFile.path, any(), workingDirectory: any(named: 'workingDirectory'), ), ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: ExitCode.success.code, - stdout: stdout, - stderr: '', - ), + (_) async { + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => 0); + when( + () => mockProcess.stdout, + ).thenAnswer((_) => Stream.value(utf8.encode(stdout))); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => const Stream.empty()); + return mockProcess; + }, ); }); @@ -812,37 +834,39 @@ Run "aot_tools help " for more information about a command. }); group('generatePatchDiffBase', () { + late int exitCode; + late String stdout; + late String stderr; + setUp(() { + exitCode = 0; + stdout = ''; + stderr = ''; when( - () => process.run( + () => process.start( any(), any(), workingDirectory: any(named: 'workingDirectory'), ), ).thenAnswer( - (_) async => const ShorebirdProcessResult( - exitCode: 0, - stdout: '', - stderr: 'error', - ), + (_) async { + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => exitCode); + when( + () => mockProcess.stdout, + ).thenAnswer((_) => Stream.value(utf8.encode(stdout))); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => Stream.value(utf8.encode(stderr))); + return mockProcess; + }, ); }); group('when command returns non-zero exit code', () { setUp(() { - when( - () => process.run( - any(), - any(), - workingDirectory: any(named: 'workingDirectory'), - ), - ).thenAnswer( - (_) async => const ShorebirdProcessResult( - exitCode: 1, - stdout: '', - stderr: 'error', - ), - ); + exitCode = 1; + stderr = 'error'; }); test('throws exception', () async { @@ -854,10 +878,10 @@ Run "aot_tools help " for more information about a command. ), ), throwsA( - isA().having( + isA().having( (e) => '$e', - 'exception', - 'Exception: Failed to generate patch diff base: error', + 'toString', + contains('stderr: error'), ), ), ); @@ -865,22 +889,6 @@ Run "aot_tools help " for more information about a command. }); group('when out file does not exist', () { - setUp(() { - when( - () => process.run( - any(), - any(), - workingDirectory: any(named: 'workingDirectory'), - ), - ).thenAnswer( - (_) async => const ShorebirdProcessResult( - exitCode: 0, - stdout: '', - stderr: '', - ), - ); - }); - test('throws exception', () async { await expectLater( () => runWithOverrides( @@ -903,7 +911,7 @@ Run "aot_tools help " for more information about a command. group('when out file is created', () { setUp(() { when( - () => process.run( + () => process.start( any(), any(), workingDirectory: any(named: 'workingDirectory'), @@ -915,11 +923,15 @@ Run "aot_tools help " for more information about a command. .split('=') .last; File(outArgument).createSync(recursive: true); - return const ShorebirdProcessResult( - exitCode: 0, - stdout: '', - stderr: '', - ); + final mockProcess = MockProcess(); + when(() => mockProcess.exitCode).thenAnswer((_) async => exitCode); + when( + () => mockProcess.stdout, + ).thenAnswer((_) => Stream.value(utf8.encode(stdout))); + when( + () => mockProcess.stderr, + ).thenAnswer((_) => Stream.value(utf8.encode(stderr))); + return mockProcess; }); });