instead of logging, ToolExit when a transformation fails

Author: andrewkolos

packages/flutter_tools/lib/src/build_system/targets/assets.dart

@@ -6,6 +6,7 @@ import 'package:pool/pool.dart';
 
 import '../../artifacts.dart';
 import '../../asset.dart';
+import '../../base/common.dart';
 import '../../base/file_system.dart';
 import '../../base/logger.dart';
 import '../../build_info.dart';
@@ -98,7 +99,6 @@ Future<Depfile> copyAssets(
   );
   final AssetTransformer assetTransformer = AssetTransformer(
     processManager: environment.processManager,
-    logger: environment.logger,
     fileSystem: environment.fileSystem,
     dartBinaryPath: environment.artifacts.getArtifactPath(Artifact.engineDartBinary),
   );
@@ -143,12 +143,15 @@ Future<Depfile> copyAssets(
           switch (entry.value.kind) {
             case AssetKind.regular:
               if (entry.value.transformers.isNotEmpty) {
-                doCopy = !await assetTransformer.transformAsset(
+                final AssetTransformationFailure? failure = await assetTransformer.transformAsset(
                   asset: content.file as File,
                   outputPath: file.path,
                   workingDirectory: environment.projectDir.path,
                   transformerEntries: entry.value.transformers,
                 );
+                if (failure != null) {
+                  throwToolExit(failure.message);
+                }
               }
             case AssetKind.font:
               doCopy = !await iconTreeShaker.subsetFont(

packages/flutter_tools/lib/src/build_system/tools/asset_transformer.dart

@@ -8,24 +8,20 @@ import 'package:process/process.dart';
 import '../../base/error_handling_io.dart';
 import '../../base/file_system.dart';
 import '../../base/io.dart';
-import '../../base/logger.dart';
 import '../../flutter_manifest.dart';
 import '../build_system.dart';
 
 /// Applies a series of user-specified asset-transforming packages to an asset file.
 final class AssetTransformer {
   AssetTransformer({
     required ProcessManager processManager,
-    required Logger logger,
     required FileSystem fileSystem,
     required String dartBinaryPath,
   })  : _processManager = processManager,
-        _logger = logger,
         _fileSystem = fileSystem,
         _dartBinaryPath = dartBinaryPath;
 
   final ProcessManager _processManager;
-  final Logger _logger;
   final FileSystem _fileSystem;
   final String _dartBinaryPath;
 
@@ -40,9 +36,7 @@ final class AssetTransformer {
 
   /// Applies, in sequence, a list of transformers to an [asset] and then copies
   /// the output to [outputPath].
-  ///
-  /// Returns `true` if successful and `false` otherwise.
-  Future<bool> transformAsset({
+  Future<AssetTransformationFailure?> transformAsset({
     required File asset,
     required String outputPath,
     required String workingDirectory,
@@ -61,19 +55,18 @@ final class AssetTransformer {
 
     try {
       for (final (int i, AssetTransformerEntry transformer) in transformerEntries.indexed) {
-        final _AssetTransformerFailure? transformerFailure = await _applyTransformer(
+        final AssetTransformationFailure? transformerFailure = await _applyTransformer(
           asset: tempInputFile,
           output: tempOutputFile,
           transformer: transformer,
           workingDirectory: workingDirectory,
         );
 
         if (transformerFailure != null) {
-          _logger.printError(
+          return AssetTransformationFailure(
             'User-defined transformation of asset "${asset.path}" failed.\n'
             '${transformerFailure.message}',
           );
-          return false;
         }
 
         ErrorHandlingFileSystem.deleteIfExists(tempInputFile);
@@ -89,10 +82,10 @@ final class AssetTransformer {
       ErrorHandlingFileSystem.deleteIfExists(tempOutputFile);
     }
 
-    return true;
+    return null;
   }
 
-  Future<_AssetTransformerFailure?> _applyTransformer({
+  Future<AssetTransformationFailure?> _applyTransformer({
     required File asset,
     required File output,
     required AssetTransformerEntry transformer,
@@ -119,7 +112,7 @@ final class AssetTransformer {
     final String stderr = result.stderr as String;
 
     if (result.exitCode != 0) {
-      return _AssetTransformerFailure(
+      return AssetTransformationFailure(
         'Transformer process terminated with non-zero exit code: ${result.exitCode}\n'
         'Transformer package: ${transformer.package}\n'
         'Full command: ${command.join(' ')}\n'
@@ -129,7 +122,7 @@ final class AssetTransformer {
     }
 
     if (!_fileSystem.file(output).existsSync()) {
-      return _AssetTransformerFailure(
+      return AssetTransformationFailure(
         'Asset transformer ${transformer.package} did not produce an output file.\n'
         'Input file provided to transformer: "${asset.path}"\n'
         'Expected output file at: "${output.absolute.path}"\n'
@@ -143,8 +136,8 @@ final class AssetTransformer {
   }
 }
 
-final class _AssetTransformerFailure {
-  const _AssetTransformerFailure(this.message);
+final class AssetTransformationFailure {
+  const AssetTransformationFailure(this.message);
 
   final String message;
 }

packages/flutter_tools/test/general.shard/build_system/targets/asset_transformer_test.dart

@@ -51,12 +51,11 @@ void main() {
 
     final AssetTransformer transformer = AssetTransformer(
       processManager: processManager,
-      logger: logger,
       fileSystem: fileSystem,
       dartBinaryPath: artifacts.getArtifactPath(Artifact.engineDartBinary),
     );
 
-    final bool transformationSuccessful = await transformer.transformAsset(
+    final AssetTransformationFailure? transformationFailure = await transformer.transformAsset(
       asset: asset,
       outputPath: outputPath,
       workingDirectory: fileSystem.currentDirectory.path,
@@ -72,13 +71,12 @@ void main() {
       ],
     );
 
-    expect(transformationSuccessful, isTrue, reason: logger.errorText);
+    expect(transformationFailure, isNull, reason: logger.errorText);
     expect(processManager, hasNoRemainingExpectations);
   });
 
   testWithoutContext('logs useful error information when transformation process returns a nonzero exit code', () async {
     final FileSystem fileSystem = MemoryFileSystem.test();
-    final BufferLogger logger = BufferLogger.test();
     final Artifacts artifacts = Artifacts.test();
 
     final File asset = fileSystem.file('asset.txt')..createSync();
@@ -110,12 +108,11 @@ void main() {
 
     final AssetTransformer transformer = AssetTransformer(
       processManager: processManager,
-      logger: logger,
       fileSystem: fileSystem,
       dartBinaryPath: dartBinaryPath,
     );
 
-    final bool transformationSuccessful = await transformer.transformAsset(
+    final AssetTransformationFailure? failure = await transformer.transformAsset(
       asset: asset,
       outputPath: outputPath,
       workingDirectory: fileSystem.currentDirectory.path,
@@ -127,10 +124,10 @@ void main() {
       ],
     );
 
-    expect(transformationSuccessful, false, reason: logger.errorText);
     expect(asset, exists);
     expect(processManager, hasNoRemainingExpectations);
-    expect(logger.errorText, contains(
+    expect(failure, isNotNull);
+    expect(failure!.message,
 '''
 User-defined transformation of asset "asset.txt" failed.
 Transformer process terminated with non-zero exit code: 1
@@ -139,13 +136,11 @@ Full command: $dartBinaryPath run my_transformer --input=/.tmp_rand0/asset.txt-t
 stdout:
 Beginning transformation
 stderr:
-Something went wrong
-'''));
+Something went wrong''');
   });
 
   testWithoutContext('prints error message when the transformer does not produce an output file', () async {
     final FileSystem fileSystem = MemoryFileSystem.test();
-    final BufferLogger logger = BufferLogger.test();
     final Artifacts artifacts = Artifacts.test();
 
     final File asset = fileSystem.file('asset.txt')..createSync();
@@ -171,12 +166,11 @@ Something went wrong
 
     final AssetTransformer transformer = AssetTransformer(
       processManager: processManager,
-      logger: logger,
       fileSystem: fileSystem,
       dartBinaryPath: dartBinaryPath,
     );
 
-    final bool transformationSuccessful = await transformer.transformAsset(
+    final AssetTransformationFailure? failure = await transformer.transformAsset(
       asset: asset,
       outputPath: outputPath,
       workingDirectory: fileSystem.currentDirectory.path,
@@ -189,7 +183,8 @@ Something went wrong
     );
 
     expect(processManager, hasNoRemainingExpectations);
-    expect(logger.errorText,
+    expect(failure, isNotNull);
+    expect(failure!.message,
 '''
 User-defined transformation of asset "asset.txt" failed.
 Asset transformer my_transformer did not produce an output file.
@@ -199,15 +194,12 @@ Full command: $dartBinaryPath run my_transformer --input=/.tmp_rand0/asset.txt-t
 stdout:
 
 stderr:
-Transformation failed, but I forgot to exit with a non-zero code.
-'''
+Transformation failed, but I forgot to exit with a non-zero code.'''
     );
-    expect(transformationSuccessful, false);
   });
 
   testWithoutContext('correctly chains transformations when there are multiple of them', () async {
     final FileSystem fileSystem = MemoryFileSystem.test();
-    final BufferLogger logger = BufferLogger.test();
     final Artifacts artifacts = Artifacts.test();
 
     final File asset = fileSystem.file('asset.txt')
@@ -267,12 +259,11 @@ Transformation failed, but I forgot to exit with a non-zero code.
 
     final AssetTransformer transformer = AssetTransformer(
       processManager: processManager,
-      logger: logger,
       fileSystem: fileSystem,
       dartBinaryPath: dartBinaryPath,
     );
 
-    await transformer.transformAsset(
+    final AssetTransformationFailure? failure = await transformer.transformAsset(
       asset: asset,
       outputPath: outputPath,
       workingDirectory: fileSystem.currentDirectory.path,
@@ -289,12 +280,12 @@ Transformation failed, but I forgot to exit with a non-zero code.
     );
 
     expect(processManager, hasNoRemainingExpectations);
+    expect(failure, isNull);
     expect(fileSystem.file(outputPath).readAsStringSync(), '012');
   });
 
   testWithoutContext('prints an error when a transformer in a chain (thats not the first) does not produce an output', () async {
     final FileSystem fileSystem = MemoryFileSystem();
-    final BufferLogger logger = BufferLogger.test();
     final Artifacts artifacts = Artifacts.test();
 
     final File asset = fileSystem.file('asset.txt')
@@ -341,12 +332,11 @@ Transformation failed, but I forgot to exit with a non-zero code.
 
     final AssetTransformer transformer = AssetTransformer(
       processManager: processManager,
-      logger: logger,
       fileSystem: fileSystem,
       dartBinaryPath: dartBinaryPath,
     );
 
-    final bool transformationSuccessful = await transformer.transformAsset(
+    final AssetTransformationFailure? failure = await transformer.transformAsset(
       asset: asset,
       outputPath: outputPath,
       workingDirectory: fileSystem.currentDirectory.path,
@@ -362,10 +352,8 @@ Transformation failed, but I forgot to exit with a non-zero code.
       ],
     );
 
-    expect(transformationSuccessful, isFalse);
-    expect(processManager, hasNoRemainingExpectations);
-    expect(fileSystem.file(outputPath), isNot(exists));
-    expect(logger.errorText,
+    expect(failure, isNotNull);
+    expect(failure!.message,
 '''
 User-defined transformation of asset "asset.txt" failed.
 Asset transformer my_distance_from_ascii_a_transformer did not produce an output file.
@@ -375,9 +363,10 @@ Full command: Artifact.engineDartBinary run my_distance_from_ascii_a_transformer
 stdout:
 
 stderr:
-Transformation failed, but I forgot to exit with a non-zero code.
-'''
+Transformation failed, but I forgot to exit with a non-zero code.'''
     );
+    expect(processManager, hasNoRemainingExpectations);
+    expect(fileSystem.file(outputPath), isNot(exists));
     expect(fileSystem.systemTempDirectory.listSync(), isEmpty);
   });
 }

packages/flutter_tools/test/general.shard/build_system/targets/assets_test.dart

@@ -240,6 +240,69 @@ flutter:
     ),
   });
 
+  testUsingContext('exits tool if an asset transformation fails', () async {
+    Cache.flutterRoot = Cache.defaultFlutterRoot(
+      platform: globals.platform,
+      fileSystem: fileSystem,
+      userMessages: UserMessages(),
+    );
+
+    final Environment environment = Environment.test(
+      fileSystem.currentDirectory,
+      processManager: globals.processManager,
+      artifacts: Artifacts.test(),
+      fileSystem: fileSystem,
+      logger: logger,
+      platform: globals.platform,
+      defines: <String, String>{},
+    );
+
+    await fileSystem.file('.packages').create();
+
+    fileSystem.file('pubspec.yaml')
+      ..createSync()
+      ..writeAsStringSync('''
+name: example
+flutter:
+  assets:
+    - path: input.txt
+      transformers:
+        - package: my_transformer
+          args: ["-a", "-b", "--color", "green"]
+''');
+
+    await fileSystem.file('input.txt').create(recursive: true);
+
+    await expectToolExitLater(
+      const CopyAssets().build(environment),
+      startsWith('User-defined transformation of asset "/input.txt" failed.\n'),
+    );
+    expect(globals.processManager, hasNoRemainingExpectations);
+  }, overrides: <Type, Generator> {
+    Logger: () => logger,
+    FileSystem: () => fileSystem,
+    Platform: () => FakePlatform(),
+    ProcessManager: () => FakeProcessManager.list(
+      <FakeCommand>[
+        FakeCommand(
+          command: <Pattern>[
+            Artifacts.test().getArtifactPath(Artifact.engineDartBinary),
+            'run',
+            'my_transformer',
+            RegExp('--input=.*'),
+            RegExp('--output=.*'),
+            '-a',
+            '-b',
+            '--color',
+            'green',
+          ],
+          exitCode: 1,
+        ),
+      ],
+    ),
+  });
+
+
   testUsingContext('Throws exception if pubspec contains missing files', () async {
     fileSystem.file('pubspec.yaml')
       ..createSync()