Take a single Builder in BuilderTransformer (#138)

Typical use case is to have a single Builder and this will make it
easier to write helper code around using BuilderTransformers for
codegen.

- Take a single Builder in the constructor of BuilderTransformer
- Remove looping around builders
- Add MultiplexingBuilder to make it easier to compose builders for the
  few use cases that may need it
- Update package version as a breaking change
- Loosen version constraint from build_test since it is not impacted
- Update tests to use the new interface and expect slightly different log
  messages

CHANGELOG.md

@@ -1,3 +1,11 @@
+## 0.5.0
+
+- **BREAKING** BuilderTransformer must be constructed with a single Builder. Use
+  the MultiplexingBuilder to cover cases with a list of builders
+- When using a MultiplexingBuilder if multiple Builders have overlapping outputs
+  the entire step will not run rather than running builders up to the point
+  where there is an overlap
+
 ## 0.4.1+3
 
 - With the default logger, print exceptions with a terse stack trace.

build_test/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.2.0+1
+
+- Forwards compatible version bump in package:build
+
 ## 0.2.0
 
 - Upgrade build package to 0.4.0

build_test/pubspec.yaml

@@ -1,11 +1,11 @@
 name: build_test
 description: Utilities for writing unit tests of Builders.
-version: 0.2.0
+version: 0.2.0+1
 author: Dart Team <misc@dartlang.org>
 homepage: https://github.com/dart-lang/build
 
 dependencies:
-  build: ^0.4.0
+  build: ">=0.4.0 <0.6.0"
   logging: ^0.11.2
   test: ^0.12.0
   watcher: ^0.9.7

e2e_example/lib/transformer.dart

@@ -7,5 +7,5 @@ import 'copy_builder.dart';
 
 /// A pub compatible Transformer built on top of [BuilderTransformer].
 class CopyTransformer extends BuilderTransformer {
-  CopyTransformer.asPlugin(_) : super([new CopyBuilder()]);
+  CopyTransformer.asPlugin(_) : super(new CopyBuilder());
 }

lib/build.dart

@@ -11,6 +11,7 @@ export 'src/asset/writer.dart';
 export 'src/builder/build_step.dart';
 export 'src/builder/builder.dart';
 export 'src/builder/exceptions.dart';
+export 'src/builder/multiplexing_builder.dart';
 export 'src/builder/transformer_builder.dart';
 export 'src/generate/build.dart';
 export 'src/generate/build_result.dart';

lib/src/builder/multiplexing_builder.dart

@@ -0,0 +1,40 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+import 'dart:async';
+
+import '../asset/id.dart';
+import 'build_step.dart';
+import 'builder.dart';
+
+/// A [Builder] that runs multiple delegate builders asynchronously.
+///
+/// **Note**: All builders are ran without ordering guarantees. Thus, none of
+/// the builders can use the outputs of other builders in this group. All
+/// builders must also have distinct outputs.
+class MultiplexingBuilder implements Builder {
+  final Iterable<Builder> _builders;
+
+  MultiplexingBuilder(this._builders);
+
+  @override
+  Future build(BuildStep buildStep) =>
+      Future.wait(_builders.map((builder) => builder.build(buildStep)));
+
+  /// Collects declared outputs from all of the builders.
+  ///
+  /// If multiple builders declare the same output it will appear in this List
+  /// more than once. This should be considered an error.
+  @override
+  List<AssetId> declareOutputs(AssetId inputId) =>
+      _collectOutputs(inputId).toList();
+
+  Iterable<AssetId> _collectOutputs(AssetId id) sync* {
+    for (var builder in _builders) {
+      yield* builder.declareOutputs(id);
+    }
+  }
+
+  @override
+  String toString() => '$_builders';
+}

lib/src/transformer/transformer.dart

@@ -17,84 +17,72 @@ import '../builder/build_step_impl.dart';
 import '../builder/builder.dart';
 import '../util/barback.dart';
 
-/// A [Transformer] which runs multiple [Builder]s.
+/// A [Transformer] which runs a single [Builder] with a new [BuildStep].
 ///
 /// By default all [BuilderTransformer]s are [DeclaringTransformer]s. If you
 /// wish to run as a [LazyTransformer], extend this class and mix in
 /// LazyTransformer.
 class BuilderTransformer implements Transformer, DeclaringTransformer {
-  /// The builders that will run during this Transformer step.
-  ///
-  /// **Note**: All [builders] are ran in the same phase, and there are no
-  /// ordering guarantees. Thus, none of the [builders] can use the outputs of
-  /// other [builders]. In order to do this you must create a [TransformerGroup]
-  /// with multiple [BuilderTransformer]s.
-  final List<Builder> builders;
-
+  final Builder _builder;
   final Resolvers _resolvers;
 
-  BuilderTransformer(this.builders, {Resolvers resolvers: const Resolvers()})
+  BuilderTransformer(this._builder, {Resolvers resolvers: const Resolvers()})
       : this._resolvers = resolvers;
 
   @override
   String get allowedExtensions => null;
 
   @override
   bool isPrimary(barback.AssetId id) =>
-      _expectedOutputs(id, builders).isNotEmpty;
+      _builder.declareOutputs(toBuildAssetId(id)).isNotEmpty;
 
   @override
   Future apply(Transform transform) async {
     var input = await toBuildAsset(transform.primaryInput);
     var reader = new _TransformAssetReader(transform);
     var writer = new _TransformAssetWriter(transform);
 
-    // Tracks all the expected outputs for each builder to make sure they don't
-    // overlap.
-    var allExpected = new Set<build.AssetId>();
-
-    // Run all builders at the same time.
-    await Future.wait(builders.map((builder) async {
-      var expected = _expectedOutputs(transform.primaryInput.id, [builder]);
-      if (expected.isEmpty) return;
-
-      // Check that no expected outputs already exist.
-      var preExistingFiles = [];
-      for (var output in expected) {
-        if (!allExpected.add(output) ||
-            await transform.hasInput(toBarbackAssetId(output))) {
-          preExistingFiles.add(toBarbackAssetId(output));
-        }
-      }
-      if (preExistingFiles.isNotEmpty) {
-        transform.logger.error(
-            'Builder `$builder` declared outputs `$preExistingFiles` but those '
-            'files already exist. That build step has been skipped.',
-            asset: transform.primaryInput.id);
-        return;
+    var expected =
+        _builder.declareOutputs(toBuildAssetId(transform.primaryInput.id));
+    if (expected.isEmpty) return;
+
+    // Check for overlapping outputs.
+    var uniqueExpected = new Set<build.AssetId>();
+    var preExistingFiles = [];
+    for (var output in expected) {
+      if (!uniqueExpected.add(output) ||
+          await transform.hasInput(toBarbackAssetId(output))) {
+        preExistingFiles.add(toBarbackAssetId(output));
       }
+    }
+    if (preExistingFiles.isNotEmpty) {
+      transform.logger.error(
+          'Builder `$_builder` declared outputs `$preExistingFiles` but those '
+          'files already exist. This build step has been skipped.',
+          asset: transform.primaryInput.id);
+      return;
+    }
 
-      // Run the build step.
-      var buildStep = new BuildStepImpl(
-          input, expected, reader, writer, input.id.package, _resolvers);
-      Logger.root.level = Level.ALL;
-      var logSubscription = buildStep.logger.onRecord.listen((LogRecord log) {
-        if (log.loggerName != buildStep.logger.fullName) return;
-
-        if (log.level <= Level.CONFIG) {
-          transform.logger.fine(_logRecordToMessage(log));
-        } else if (log.level <= Level.INFO) {
-          transform.logger.info(_logRecordToMessage(log));
-        } else if (log.level <= Level.WARNING) {
-          transform.logger.warning(_logRecordToMessage(log));
-        } else {
-          transform.logger.error(_logRecordToMessage(log));
-        }
-      });
-      await builder.build(buildStep);
-      await buildStep.complete();
-      await logSubscription.cancel();
-    }));
+    // Run the build step.
+    var buildStep = new BuildStepImpl(
+        input, expected, reader, writer, input.id.package, _resolvers);
+    Logger.root.level = Level.ALL;
+    var logSubscription = buildStep.logger.onRecord.listen((LogRecord log) {
+      if (log.loggerName != buildStep.logger.fullName) return;
+
+      if (log.level <= Level.CONFIG) {
+        transform.logger.fine(_logRecordToMessage(log));
+      } else if (log.level <= Level.INFO) {
+        transform.logger.info(_logRecordToMessage(log));
+      } else if (log.level <= Level.WARNING) {
+        transform.logger.warning(_logRecordToMessage(log));
+      } else {
+        transform.logger.error(_logRecordToMessage(log));
+      }
+    });
+    await _builder.build(buildStep);
+    await buildStep.complete();
+    await logSubscription.cancel();
   }
 
   String _logRecordToMessage(LogRecord log) {
@@ -111,13 +99,12 @@ class BuilderTransformer implements Transformer, DeclaringTransformer {
 
   @override
   void declareOutputs(DeclaringTransform transform) {
-    for (var outputId in _expectedOutputs(transform.primaryId, builders)) {
-      transform.declareOutput(toBarbackAssetId(outputId));
-    }
+    var outputs = _builder.declareOutputs(toBuildAssetId(transform.primaryId));
+    outputs.map(toBarbackAssetId).forEach(transform.declareOutput);
   }
 
   @override
-  String toString() => 'BuilderTransformer: [$builders]';
+  String toString() => 'BuilderTransformer: $_builder';
 }
 
 /// Very simple [AssetReader] which uses a [Transform].
@@ -159,11 +146,3 @@ class _TransformAssetWriter implements AssetWriter {
   Future delete(build.AssetId id) =>
       throw new UnsupportedError('_TransformAssetWriter can\'t delete files.');
 }
-
-/// All the expected outputs for [id] given [builders].
-Iterable<build.AssetId> _expectedOutputs(
-    barback.AssetId id, Iterable<Builder> builders) sync* {
-  for (var builder in builders) {
-    yield* builder.declareOutputs(toBuildAssetId(id));
-  }
-}

pubspec.yaml

@@ -1,5 +1,5 @@
 name: build
-version: 0.4.1+3
+version: 0.5.0
 description: A build system for Dart.
 author: Dart Team <misc@dartlang.org>
 homepage: https://github.com/dart-lang/build

test/transformer/transformer_test.dart

@@ -11,11 +11,12 @@ import 'package:transformer_test/utils.dart';
 import '../common/common.dart' hide testPhases;
 
 void main() {
-  var singleCopyTransformer = new BuilderTransformer([new CopyBuilder()]);
+  var singleCopyTransformer = new BuilderTransformer(new CopyBuilder());
   var multiCopyTransformer =
-      new BuilderTransformer([new CopyBuilder(numCopies: 2)]);
+      new BuilderTransformer(new CopyBuilder(numCopies: 2));
   var singleAndMultiCopyTransformer = new BuilderTransformer(
-      [new CopyBuilder(), new CopyBuilder(numCopies: 2)]);
+      new MultiplexingBuilder(
+          [new CopyBuilder(), new CopyBuilder(numCopies: 2)]));
 
   testPhases('single builder, single output', [
     [singleCopyTransformer],
@@ -46,12 +47,14 @@ void main() {
 
   testPhases('multiple builders, same outputs', [
     [
-      new BuilderTransformer([new CopyBuilder(), new CopyBuilder()])
+      new BuilderTransformer(
+          new MultiplexingBuilder([new CopyBuilder(), new CopyBuilder()]))
     ],
   ], {
     'a|web/a.txt': 'hello',
   }, {}, messages: [
-    _fileExistsError('CopyBuilder', ['a|web/a.txt.copy']),
+    _fileExistsError(
+        '${[new CopyBuilder(), new CopyBuilder()]}', ['a|web/a.txt.copy']),
   ]);
 
   testPhases('multiple phases', [
@@ -85,18 +88,15 @@ void main() {
       {'a|web/a.txt': 'hello', 'a|web/a.txt.copy': 'hello'},
       {},
       messages: [
-        _fileExistsError("CopyBuilder", ["a|web/a.txt.copy"]),
+        _fileExistsError('${new CopyBuilder()}', ['a|web/a.txt.copy']),
       ],
       expectBarbackErrors: true);
 
   // Gives a barback error only, we can't detect this situation.
   testPhases(
       'builders in the same phase can\'t output the same file',
       [
-        [
-          singleCopyTransformer,
-          new BuilderTransformer([new CopyBuilder()])
-        ]
+        [singleCopyTransformer, new BuilderTransformer(new CopyBuilder())]
       ],
       {
         'a|web/a.txt': 'hello',
@@ -113,14 +113,12 @@ void main() {
       {'a|web/a.txt': 'hello'},
       {'a|web/a.txt.copy': 'hello'},
       messages: [
-        _fileExistsError("CopyBuilder", ["a|web/a.txt.copy"]),
+        _fileExistsError('${new CopyBuilder()}', ['a|web/a.txt.copy']),
       ],
       expectBarbackErrors: true);
 
   testPhases('loggers log errors', [
-    [
-      new BuilderTransformer([new LoggingCopyBuilder()])
-    ],
+    [new BuilderTransformer(new LoggingCopyBuilder())],
   ], {
     'a|web/a.txt': 'a',
     'a|web/b.txt': 'b',
@@ -152,7 +150,7 @@ class LoggingCopyBuilder extends CopyBuilder {
 }
 
 String _fileExistsError(String builder, List<String> files) {
-  return "error: Builder `Instance of '$builder'` declared outputs "
-      "`$files` but those files already exist. That build step has been "
+  return "error: Builder `$builder` declared outputs "
+      "`$files` but those files already exist. This build step has been "
       "skipped.";
 }