Support directories other than "web" in pub build.

sdk/lib/_internal/pub/lib/src/command/build.dart

Index: sdk/lib/_internal/pub/lib/src/command/build.dart
diff --git a/sdk/lib/_internal/pub/lib/src/command/build.dart b/sdk/lib/_internal/pub/lib/src/command/build.dart
index b13e4d8bdb103e845d4df4542bb50b833ee19473..3c0abbde80070f26b02f74c658425bc833ab8073 100644
--- a/sdk/lib/_internal/pub/lib/src/command/build.dart
+++ b/sdk/lib/_internal/pub/lib/src/command/build.dart
@@ -9,8 +9,7 @@ import 'dart:async';
 import 'package:barback/barback.dart';
 import 'package:path/path.dart' as path;
 
-import '../barback/dart2js_transformer.dart';
-import '../barback/dart_forwarding_transformer.dart';
+import '../barback/build_environment.dart';
 import '../barback.dart' as barback;
 import '../command.dart';
 import '../exit_codes.dart' as exit_codes;
@@ -20,87 +19,62 @@ import '../utils.dart';
 
 final _arrow = getSpecial('\u2192', '=>');
 
+/// The set of top level directories in the entrypoint package that can be
+/// built.
+final _allowedBuildDirectories = [
+  "benchmark", "bin", "example", "test", "web"
+].toSet();

[nweiz, 2014/01/28 03:21:51]

Style nit: I slightly prefer [new Set.from] because it puts the fact that this
is going to be a set right up front.

[Bob Nystrom, 2014/01/28 23:02:04]

Done.

+
 /// Handles the `build` pub command.
 class BuildCommand extends PubCommand {
   String get description =>
       "Copy and compile all Dart entrypoints in the 'web' directory.";
   String get usage => "pub build [options]";
   List<String> get aliases => const ["deploy", "settle-up"];
+  bool get takesArguments => true;
 
-  // TODO(nweiz): make these configurable.
-  /// The path to the source directory of the application.
-  String get source => path.join(entrypoint.root.dir, 'web');
-
+  // TODO(nweiz): make this configurable.
   /// The path to the application's build output directory.
   String get target => path.join(entrypoint.root.dir, 'build');
 
   /// The build mode.
   BarbackMode get mode => new BarbackMode(commandOptions['mode']);
 
+  /// The number of files that have been built and written to disc so far.
+  int builtFiles = 0;

[nweiz, 2014/01/28 03:21:51]

It's weird that this is public but [_buildDirectories] is private.

[Bob Nystrom, 2014/01/28 23:02:04]

Made _buildDirectories public. In the command classes, we tend to be lax about
making stuff private.

+
+  /// The names of the top-level build directories that will be built.
+  final _buildDirectories = new Set<String>();
+
   BuildCommand() {
     commandParser.addOption('mode', defaultsTo: BarbackMode.RELEASE.toString(),
         help: 'Mode to run transformers in.');
   }
 
   Future onRun() {
-    if (!dirExists(source)) {
-      throw new ApplicationException('There is no "$source" directory.');
-    }
-
+    _parseBuildDirectories();
     cleanDir(target);
 
-    var dart2jsTransformer;
-    var builtFiles = 0;
-
-    return entrypoint.loadPackageGraph().then((graph) {
-      dart2jsTransformer = new Dart2JSTransformer(graph, mode);
-      var builtInTransformers = [
-        dart2jsTransformer,
-        new DartForwardingTransformer(mode)
-      ];
-
-      // Since this server will only be hit by the transformer loader and isn't
-      // user-facing, just use an IPv4 address to avoid a weird bug on the
-      // OS X buildbots.
-      // TODO(rnystrom): Allow specifying mode.
-      return barback.createServer("127.0.0.1", 0, graph, mode,
-          builtInTransformers: builtInTransformers,
-          watcher: barback.WatcherType.NONE);
-    }).then((server) {
+    // Since this server will only be hit by the transformer loader and isn't
+    // user-facing, just use an IPv4 address to avoid a weird bug on the
+    // OS X buildbots.
+    return BuildEnvironment.create(entrypoint, "127.0.0.1", 0, mode,
+        WatcherType.NONE, _buildDirectories, useDart2JS: true)
+          .then((environment) {
+
       // Show in-progress errors, but not results. Those get handled implicitly
       // by getAllAssets().
-      server.barback.errors.listen((error) {
+      environment.server.barback.errors.listen((error) {
         log.error(log.red("Build error:\n$error"));
       });
 
       return log.progress("Building ${entrypoint.root.name}",
-          () => server.barback.getAllAssets());
-    }).then((assets) {
-      return Future.wait(assets.map((asset) {
-        // In release mode, strip out .dart files since all relevant ones have
-        // been compiled to JavaScript already.
-        if (mode == BarbackMode.RELEASE && asset.id.extension == ".dart") {
-          return new Future.value();
-        }
-
-        builtFiles++;
-
-        // Figure out the output directory for the asset, which is the same
-        // as the path pub serve would use to serve it.
-        var relativeUrl = barback.idtoUrlPath(entrypoint.root.name, asset.id);
-
-        // Remove the leading "/".
-        relativeUrl = relativeUrl.substring(1);
-
-        var relativePath = path.fromUri(new Uri(path: relativeUrl));
-        var destPath = path.join(target, relativePath);
-
-        ensureDir(path.dirname(destPath));
-        // TODO(rnystrom): Should we display this to the user?
-        return createFileFromStream(asset.read(), destPath);
-      })).then((_) {
-        builtFiles += _copyBrowserJsFiles(dart2jsTransformer.entrypoints);
-        log.message("Built $builtFiles ${pluralize('file', builtFiles)}!");
+          () => environment.server.barback.getAllAssets()).then((assets) {
+        return Future.wait(assets.map(_writeAsset)).then((_) {
+          builtFiles += _copyBrowserJsFiles(
+              environment.dart2JSTransformer.entrypoints);

[nweiz, 2014/01/28 03:21:51]

Accessing dart2JSTransformer here is gross. It also doesn't seem safe to assume
that the browser JS files only need to go next to the ".dart.js" files. What if
an HTML file imports a dart2js-compiled file that's not in the same directory?

If we do make this assumption, it seems cleaner to look for ".dart.js" files and
to copy the JS bootstrap files next to those, or to have the dart2js transformer
emit them itself.

[Bob Nystrom, 2014/01/28 23:02:04]

Done.

+          log.message("Built $builtFiles ${pluralize('file', builtFiles)}!");
+        });
       });
     }).catchError((error) {
       // If [getAllAssets()] throws a BarbackException, the error has already
@@ -112,6 +86,113 @@ class BuildCommand extends PubCommand {
     });
   }
 
+  /// Parses the command-line arguments to determine the set of top-level
+  /// directories to build.
+  ///
+  /// If there are no arguments to `pub build`, this will just be "web".
+  ///
+  /// If the argument is `*`, then it will be all buildable directories that
+  /// exist.

[nweiz, 2014/01/28 03:21:51]

Passing * on the command line is very confusing. It makes users think about the
difference between "pub build *" and "pub build \*", which is asking for
trouble. I think we should either do "pub build all" or "pub build --all",
depending on whether or not we think people will want to build a directory named
"all/" at some point.

[Bob Nystrom, 2014/01/28 23:02:04]

Done. --all.

+  ///
+  /// Otherwise, all arguments should be the names of directories to include.
+  ///
+  /// Throws if any of the specified directories cannot be found.
+  void _parseBuildDirectories() {
+    if (commandOptions.rest.length == 1 && commandOptions.rest.single == "*") {
+      // Include every build directory that exists in the package.
+      var allowed = _allowedBuildDirectories.where(
+          (d) => dirExists(path.join(entrypoint.root.dir, d)));
+
+      if (allowed.isEmpty) {
+        throw new ApplicationException("There are no buildable directories.");

[nweiz, 2014/01/28 03:21:51]

List which directories are considered buildable in this error message.

[Bob Nystrom, 2014/01/28 23:02:04]

Done.

+      }
+
+      _buildDirectories.addAll(allowed);
+      return;
+    }
+
+    _buildDirectories.addAll(commandOptions.rest);
+
+    // If no directory was specified, default to "web".
+    if (commandOptions.rest.isEmpty) {
+      _buildDirectories.add("web");
+    }
+
+    // Make sure the arguments are known directories.
+    var disallowed = commandOptions.rest.where(
+        (dir) => !_allowedBuildDirectories.contains(dir));
+    if (disallowed.isNotEmpty) {
+      var dirs = pluralize("directory", disallowed.length,
+          plural: "directories");
+      var names = toSentence(ordered(disallowed).map((name) => '"$name"'));
+      var allowed = toSentence(ordered(_allowedBuildDirectories.map(
+          (name) => '"$name"')));
+      throw new ApplicationException(
+          'Unsupported build $dirs $names.\n'
+          'The allowed directories are: $allowed.');

[nweiz, 2014/01/28 03:21:51]

Nit: since [allowed] is sentenceized, I think this would read better without the
colon.

[Bob Nystrom, 2014/01/28 23:02:04]

Done.

+    }
+
+    // Make sure all of the build directories exist.
+    var missing = _buildDirectories.where(
+        (dir) => !dirExists(path.join(entrypoint.root.dir, dir)));
+
+    if (missing.isNotEmpty) {
+      var dirs = pluralize("directory", missing.length, plural: "directories");
+      var names = toSentence(ordered(missing).map((name) => '"$name"'));
+      throw new ApplicationException('Could not find $dirs $names.');

[nweiz, 2014/01/28 03:21:51]

I feel like "Could not find" is more ambiguous than "$dirs $names do(es) not
exist."

[Bob Nystrom, 2014/01/28 23:02:04]

Done.

+    }
+  }
+
+  /// Writes [asset] to the appropriate build directory.
+  ///
+  /// If [asset] is in the special "assets" directory, writes it to every
+  /// build directory.
+  Future _writeAsset(Asset asset) {
+    // In release mode, strip out .dart files since all relevant ones have been
+    // compiled to JavaScript already.
+    if (mode == BarbackMode.RELEASE && asset.id.extension == ".dart") {
+      return new Future.value();
+    }
+
+    // If the asset is from a package's "lib" directory, we make it available
+    // as an input for transformers, but don't want it in the final output.
+    // (Any Dart code in there should be imported and compiled to JS, anything
+    // else we want to omit.)
+    if (asset.id.path.startsWith("lib/")) {
+      return new Future.value();
+    }
+
+    // Figure out the output directory for the asset, which is the same as the
+    // path pub serve would use to serve it.
+    var relativeUrl = barback.idtoUrlPath(entrypoint.root.name, asset.id,
+        useWebAsRoot: false);
+
+    // Remove the leading "/".
+    relativeUrl = relativeUrl.substring(1);
+
+    // If the asset is from a special "shared" directory package, copy it into

[nweiz, 2014/01/28 03:21:51]

"'shared' directory package" -> "'assets/' directory"

[Bob Nystrom, 2014/01/28 23:02:04]

Done.

+    // all of the top-level build directories.
+    if (relativeUrl.startsWith("assets/")) {
+      builtFiles += _buildDirectories.length;
+      return Future.wait(_buildDirectories.map(
+          (buildDir) => _writeOutputFile(asset,
+              path.url.join(buildDir, relativeUrl))));
+    }
+
+    builtFiles++;
+    return _writeOutputFile(asset, relativeUrl);
+  }
+
+  /// Writes the contents of [asset] to [relativeUrl] within the build
+  /// directory.
+  Future _writeOutputFile(Asset asset, String relativeUrl) {
+    var relativePath = path.fromUri(new Uri(path: relativeUrl));
+    var destPath = path.join(target, relativePath);
+
+    ensureDir(path.dirname(destPath));
+    return createFileFromStream(asset.read(), destPath);
+  }
+
   /// If this package depends directly on the `browser` package, this ensures
   /// that the JavaScript bootstrap files are copied into `packages/browser/`
   /// directories next to each entrypoint in [entrypoints].
@@ -127,7 +208,6 @@ class BuildCommand extends PubCommand {
     // Get all of the directories that contain Dart entrypoints.
     var entrypointDirs = entrypoints
         .map((id) => path.url.split(id.path))
-        .map((parts) => parts.skip(1)) // Remove "web/".
         .map((relative) => path.dirname(path.joinAll(relative)))
         .toSet();
 
@@ -149,6 +229,10 @@ class BuildCommand extends PubCommand {
     var jsPath = path.join(
         target, directory, 'packages', 'browser', '$name.js');
     ensureDir(path.dirname(jsPath));
+
+    // TODO(rnystrom): This won't work if we get rid of symlinks and the top
+    // level "packages" directory. Will need to copy from the browser
+    // directory.
     copyFile(path.join(entrypoint.packagesDir, 'browser', '$name.js'), jsPath);
   }
 }