Fix polymer build steps so we don't trust existing packages symlinks or folders.

pkg/polymer/lib/src/barback_runner.dart

Index: pkg/polymer/lib/src/barback_runner.dart
diff --git a/pkg/polymer/lib/src/barback_runner.dart b/pkg/polymer/lib/src/barback_runner.dart
index dde15a3074c645add4f51fe962985b002cc41cf7..2208e9ff3e1a3e6b6d4b3a824820b9087e1f462d 100644
--- a/pkg/polymer/lib/src/barback_runner.dart
+++ b/pkg/polymer/lib/src/barback_runner.dart
@@ -186,55 +186,137 @@ void _attachListeners(Barback barback) {
  * polymer's libraries).
  */
 Future _emitAllFiles(Barback barback, BarbackOptions options) {
-  return _emitFiles(barback, options, 'web').then((res) {
-    if (options.transformTests) return _emitFiles(barback, options, 'test');
-    return res;
+  return barback.getAllAssets().then((assets) {
+    return _emitPackagesDir(options)
+      .then((_) => _emitTransformedFiles(assets, options))
+      .then((_) => _addPackagesSymlinks(assets, options))
+      .then((_) => assets);
   });
 }
 
-Future _emitFiles(Barback barback, BarbackOptions options, String emitSubDir) {

[Siggi Cherem (dart-lang), 2013/09/11 23:54:58]

this code became _emitTransformedFiles

[Jennifer Messerly, 2013/09/12 02:45:03]

yeah, nice cleanup! I like how the main Future chain is isolated into
_emitAllFiles

-  return barback.getAllAssets().then((assets) {
-    // Copy all the assets we transformed
-    var futures = [];
-    for (var asset in assets) {
-      var id = asset.id;
-      var filepath;
-      if (id.package == options.currentPackage &&
-          id.path.startsWith('$emitSubDir/')) {
-        filepath = path.join(options.outDir, id.path);
-      } else if (id.path.startsWith('lib/')) {
-        filepath = path.join(options.outDir, emitSubDir, 'packages', id.package,
-            id.path.substring(4));
-      } else {
-        // TODO(sigmund): do something about other assets?
-        continue;
-      }
-
-      _ensureDir(path.dirname(filepath));
-      var writer = new File(filepath).openWrite();
-      futures.add(writer.addStream(asset.read()).then((_) => writer.close()));
+Future _emitTransformedFiles(AssetSet assets, BarbackOptions options) {
+  // Copy all the assets we transformed
+  var futures = [];
+  var currentPackage = options.currentPackage;
+  var transformTests = options.transformTests;
+  var outPackages = path.join(options.outDir, 'packages');
+  for (var asset in assets) {
+    var id = asset.id;
+    var dir = _firstDir(id.path);
+    if (dir == null) continue;
+
+    var filepath;
+    if (dir == 'lib') {
+      // Put lib files directly under the packages folder (e.g. 'lib/foo.dart'
+      // will be emitted at out/packages/package_name/foo.dart).
+      filepath = path.join(outPackages, id.package, id.path.substring(4));
+    } else if (id.package == currentPackage &&
+        (dir == 'web' || (transformTests && dir == 'test'))) {
+      filepath = path.join(options.outDir, id.path);
+    } else {
+      // TODO(sigmund): do something about other assets?
+      continue;
     }
-    return Future.wait(futures).then((_) {

[Siggi Cherem (dart-lang), 2013/09/11 23:54:58]

this code moved to _emitPackagesDir

-      // Copy also all the files we didn't process
-      var futures = [];
-      for (var package in _polymerPackageDependencies) {
-        for (var relpath in _listPackageDir(package, 'lib', options)) {
-          var inpath = path.join(options.packageDirs[package], relpath);
-          var outpath = path.join(options.outDir, emitSubDir,
-              'packages', package, relpath.substring(4));
-          _ensureDir(path.dirname(outpath));
-
-          var writer = new File(outpath).openWrite();
-          futures.add(writer.addStream(new File(inpath).openRead())
-            .then((_) => writer.close()));
-        }
-      }
-      return Future.wait(futures);
-    }).then((_) => assets);
-  });
+
+    futures.add(_writeAsset(filepath, asset));
+  }
+  return Future.wait(futures);
+}
+
+/**
+ * Adds a package symlink from each directory under `out/web/foo/` to
+ * `out/packages`.
+ */
+Future _addPackagesSymlinks(AssetSet assets, BarbackOptions options) {
+  var outPackages = path.join(options.outDir, 'packages');
+  var currentPackage = options.currentPackage;
+  for (var asset in assets) {
+    var id = asset.id;
+    if (id.package != currentPackage) continue;
+    var firstDir = _firstDir(id.path);
+    if (firstDir == null) continue;
+
+    if (firstDir == 'web' || (options.transformTests && firstDir == 'test')) {
+      var dir = path.join(options.outDir, path.dirname(id.path));
+      var linkPath = path.join(dir, 'packages');
+      var targetPath = path.relative(outPackages, from: dir);
+      _deleteIfPresent(linkPath);
+      new Link(linkPath).createSync(targetPath);
+    }
+  }
+}
+
+/**
+ * Emits a 'packages' directory directly under `out/packages` with the contents
+ * of every file that was not transformed by barback.
+ */
+Future _emitPackagesDir(BarbackOptions options) {
+  // Ensure we don't have a packages symlink in our output folder (could happen
+  // when people are using nested packages).
+  var outPackages = path.join(options.outDir, 'packages');
+  _deleteIfPresent(outPackages);

[Siggi Cherem (dart-lang), 2013/09/11 23:54:58]

alternatively, instead of deleting 'packages/' I can just delete the entire
outDir. Any preferences?

+
+  if (options.transformPolymerDependencies) return new Future.value(null);
+
+  // Copy all the files we didn't process
+  var futures = [];
+  var dirs = options.packageDirs;
+  for (var package in _polymerPackageDependencies) {
+    for (var relpath in _listPackageDir(package, 'lib', options)) {
+      var inpath = path.join(dirs[package], relpath);
+      var outpath = path.join(outPackages, package, relpath.substring(4));
+      futures.add(_copyFile(inpath, outpath));
+    }
+  }
+  return Future.wait(futures);
 }
 
 /** Ensure [dirpath] exists. */
 void _ensureDir(var dirpath) {
   new Directory(dirpath).createSync(recursive: true);
 }
+
+/** Deletes [packagesPath] if it's a packages symlink, file, or folder. */
+void _deleteIfPresent(var packagesPath) {

[Jennifer Messerly, 2013/09/12 02:45:03]

nit: "var" doesn't add anything.

you could type this to String if you wanted to?

[Siggi Cherem (dart-lang), 2013/09/12 16:53:14]

thanks, I think I copied it from the other method, which I copied from somewhere
else. Fixed.

+  var link = new Link(packagesPath);
+  if (link.existsSync()) {

[Jennifer Messerly, 2013/09/12 02:45:03]

I dunno if it's worth worrying about, but every time I see this pattern or use
it myself, it makes me think that technically it isn't safe. Another process
could (in theory) mess up the path right before we touch it again. Ideally we'd
have an API that would allow us to express the intent, which is more like a
transaction.

Anyway there's nothing to really fix here, since it's an issue with the API
you're building on. We could go all crazy with try+catch guards around these
APIs, but I dunno if it's worth the added ugliness.

[Siggi Cherem (dart-lang), 2013/09/12 16:53:14]

good point. I added the try+catch.

+    link.deleteSync();
+    return;
+  }
+
+  var dir = new Directory(packagesPath);
+  if (dir.existsSync()) {
+    dir.deleteSync(recursive: true);
+    return;
+  }
+
+  var file = new File(packagesPath);

[Jennifer Messerly, 2013/09/12 02:45:03]

do you actually need to check for all 3 kinds of things?

from a few quick tests:

* new File(path).deleteSync() works on symlinks
* new Directory(path).deleteSync(recursive: true) works on symlinks, and will
not delete contents.
* new Directory(path).deleteSync(recursive: true) works on files too

this makes me think that `new Directory(path).deleteSync(recursive: true)` is
all you need here.

... combining this with the earlier suggestion, perhaps just this?

void _deleteIfPresent(String path) {
  try {
    new Directory(path).deleteSync(recursive: true);
  } on DirectoryException catch (e) {}
}


For the record, I didn't try this on any OS except Linux, so no promises... :)

[Siggi Cherem (dart-lang), 2013/09/12 16:53:14]

I'm a little paranoid on whether this works in Windows. I just found  that pub
has a similar pattern, they basically do the 3 checks as well.

[Jennifer Messerly, 2013/09/12 17:53:47]

yeah, I wonder though. Pub can be a tricky place to copy code from; as the first
big thing using dart:io cross platform, they developed a lot of crazy
workarounds, and sometimes the original bug has been fixed for a long time :).


anyway, if the pattern is required, it seems like it should be in a
library/package somewhere. (it's a pet peeve of mine when something simple like
"delete this path if it exists" becomes a 15 line function ;) )

+  if (file.existsSync()) {
+    file.deleteSync();
+  }
+}
+
+/**
+ * Returns the first directory name on a url-style path, or null if there are no
+ * slashes.
+ */
+String _firstDir(String url) {
+  var firstSlash = url.indexOf('/');
+  if (firstSlash == -1) return null;
+  return url.substring(0, firstSlash);
+}
+
+/** Copy a file from [inpath] to [outpath]. */
+Future _copyFile(String inpath, String outpath) {

[Jennifer Messerly, 2013/09/12 02:45:03]

this feels like a helper that should exist somewhere...

[Siggi Cherem (dart-lang), 2013/09/12 16:53:14]

Yeah, I couldn't find it anywhere in the repo though

+  _ensureDir(path.dirname(outpath));
+  var writer = new File(outpath).openWrite();
+  return writer.addStream(new File(inpath).openRead())
+      .then((_) => writer.close());
+}
+
+/** Write contents of an [asset] into a file at [filepath]. */
+Future _writeAsset(String filepath, Asset asset) {
+  _ensureDir(path.dirname(filepath));
+  var writer = new File(filepath).openWrite();
+  return writer.addStream(asset.read()).then((_) => writer.close());
+}