From 7b1e9286d8e07e30c90e3b56403bf111ac08a1db Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Sun, 24 May 2015 17:27:38 -0700 Subject: [PATCH] build(broccoli): add tree-stabilizer plugin to deal with unstable trees Previously we assumed that all input and ouput paths for broccoli trees are immutable, that turned out to be incorrect. By adding a tree stabilizer plugin in front of each diffing plugin, we ensure that the input trees are stable. The stabilization is done via symlinks which is super cheap on platforms that support symlinks. On Windows we currently copy the whole input directory, which is far from ideal. We should investagate if using move operation on Windows is ok in the future to improve performance. Closes #2051 --- tools/broccoli/broccoli-tree-stabilizer.ts | 45 ++++++++++++++++++++++ tools/broccoli/diffing-broccoli-plugin.ts | 26 ++++++++++--- tools/broccoli/trees/browser_tree.ts | 10 ----- tools/broccoli/trees/dart_tree.ts | 5 --- tools/broccoli/trees/node_tree.ts | 12 ------ 5 files changed, 65 insertions(+), 33 deletions(-) create mode 100644 tools/broccoli/broccoli-tree-stabilizer.ts diff --git a/tools/broccoli/broccoli-tree-stabilizer.ts b/tools/broccoli/broccoli-tree-stabilizer.ts new file mode 100644 index 0000000000..9a1bf97a4f --- /dev/null +++ b/tools/broccoli/broccoli-tree-stabilizer.ts @@ -0,0 +1,45 @@ +/// +/// + +import fs = require('fs'); +let symlinkOrCopy = require('symlink-or-copy'); + + +/** + * Stabilizes the inputPath for the following plugins in the build tree. + * + * All broccoli plugins that inherit from `broccoli-writer` or `broccoli-filter` change their + * outputPath during each rebuild. + * + * This means that all following plugins in the build tree can't rely on their inputPath being + * immutable. This results in breakage of any plugin that is not expecting such behavior. + * + * For example all `DiffingBroccoliPlugin`s expect their inputPath to be stable. + * + * By inserting this plugin into the tree after any misbehaving plugin, we can stabilize the + * inputPath for the following plugin in the tree and correct the surprising behavior. + */ +class TreeStabilizer implements BroccoliTree { + inputPath: string; + outputPath: string; + + + constructor(public inputTree: BroccoliTree) {} + + + rebuild() { + fs.rmdirSync(this.outputPath); + + // TODO: investigate if we can use rename the directory instead to improve performance on + // Windows + symlinkOrCopy.sync(this.inputPath, this.outputPath); + } + + + cleanup() {} +} + + +export default function stabilizeTree(inputTree) { + return new TreeStabilizer(inputTree); +} diff --git a/tools/broccoli/diffing-broccoli-plugin.ts b/tools/broccoli/diffing-broccoli-plugin.ts index ae133996a2..dc03289591 100644 --- a/tools/broccoli/diffing-broccoli-plugin.ts +++ b/tools/broccoli/diffing-broccoli-plugin.ts @@ -6,6 +6,7 @@ import fs = require('fs'); import fse = require('fs-extra'); import path = require('path'); import {TreeDiffer, DiffResult} from './tree-differ'; +import stabilizeTree from './broccoli-tree-stabilizer'; let symlinkOrCopy = require('symlink-or-copy'); @@ -51,9 +52,9 @@ class DiffingPluginWrapper implements BroccoliTree { constructor(private pluginClass, private wrappedPluginArguments) { if (Array.isArray(wrappedPluginArguments[0])) { - this.inputTrees = wrappedPluginArguments[0]; + this.inputTrees = this.stabilizeTrees(wrappedPluginArguments[0]); } else { - this.inputTree = wrappedPluginArguments[0]; + this.inputTree = this.stabilizeTree(wrappedPluginArguments[0]); } this.description = this.pluginClass.name; @@ -82,6 +83,13 @@ class DiffingPluginWrapper implements BroccoliTree { } + cleanup() { + if (this.wrappedPlugin.cleanup) { + this.wrappedPlugin.cleanup(); + } + } + + private relinkOutputAndCachePaths() { // just symlink the cache and output tree fs.rmdirSync(this.outputPath); @@ -101,9 +109,15 @@ class DiffingPluginWrapper implements BroccoliTree { } - cleanup() { - if (this.wrappedPlugin.cleanup) { - this.wrappedPlugin.cleanup(); - } + private stabilizeTrees(trees: BroccoliTree[]) { + return trees.map((tree) => this.stabilizeTree(tree)); + } + + + private stabilizeTree(tree: BroccoliTree) { + // Ignore all DiffingPlugins as they are already stable, for others we don't know for sure + // so we need to stabilize them. + // Since it's not safe to use instanceof operator in node, we are checking the constructor.name. + return (tree.constructor['name'] === 'DiffingPluginWrapper') ? tree : stabilizeTree(tree); } } diff --git a/tools/broccoli/trees/browser_tree.ts b/tools/broccoli/trees/browser_tree.ts index c16eccdf88..3df98400cf 100644 --- a/tools/broccoli/trees/browser_tree.ts +++ b/tools/broccoli/trees/browser_tree.ts @@ -56,11 +56,6 @@ module.exports = function makeBrowserTree(options, destinationPath) { var es6Tree = mergeTrees([traceurTree, typescriptTree]); - // TODO(iminar): tree differ seems to have issues with trees created by mergeTrees, investigate! - // ENOENT error is thrown while doing fs.readdirSync on inputRoot - // in the meantime, we just do noop mv to create a new tree - es6Tree = stew.mv(es6Tree, ''); - // Call Traceur again to lower the ES6 build tree to ES5 var es5Tree = transpileWithTraceur(es6Tree, { destExtension: '.js', @@ -152,10 +147,5 @@ module.exports = function makeBrowserTree(options, destinationPath) { var mergedTree = mergeTrees([stew.mv(es6Tree, '/es6'), stew.mv(es5Tree, '/es5')]); - // TODO(iminar): tree differ seems to have issues with trees created by mergeTrees, investigate! - // ENOENT error is thrown while doing fs.readdirSync on inputRoot - // in the meantime, we just do noop mv to create a new tree - mergedTree = stew.mv(mergedTree, ''); - return destCopy(mergedTree, destinationPath); }; diff --git a/tools/broccoli/trees/dart_tree.ts b/tools/broccoli/trees/dart_tree.ts index 82c965e278..d95f16d030 100644 --- a/tools/broccoli/trees/dart_tree.ts +++ b/tools/broccoli/trees/dart_tree.ts @@ -144,10 +144,5 @@ module.exports = function makeDartTree(destinationPath) { var dartTree = mergeTrees([sourceTree, getTemplatedPubspecsTree(), getDocsTree()]); - // TODO(iminar): tree differ seems to have issues with trees created by mergeTrees, investigate! - // ENOENT error is thrown while doing fs.readdirSync on inputRoot - // in the meantime, we just do noop mv to create a new tree - dartTree = stew.mv(dartTree, ''); - return destCopy(dartTree, destinationPath); }; diff --git a/tools/broccoli/trees/node_tree.ts b/tools/broccoli/trees/node_tree.ts index 539be352b7..8e59acfe18 100644 --- a/tools/broccoli/trees/node_tree.ts +++ b/tools/broccoli/trees/node_tree.ts @@ -96,11 +96,6 @@ module.exports = function makeNodeTree(destinationPath) { } }); - // TODO(iminar): tree differ seems to have issues with trees created by mergeTrees, investigate! - // ENOENT error is thrown while doing fs.readdirSync on inputRoot - // in the meantime, we just do noop mv to create a new tree - traceurCompatibleTsModulesTree = stew.mv(traceurCompatibleTsModulesTree, ''); - var typescriptTree = compileWithTypescript(traceurCompatibleTsModulesTree, { allowNonTsExtensions: false, emitDecoratorMetadata: true, @@ -131,12 +126,5 @@ module.exports = function makeNodeTree(destinationPath) { }] }); - - - // TODO(iminar): tree differ seems to have issues with trees created by mergeTrees, investigate! - // ENOENT error is thrown while doing fs.readdirSync on inputRoot - // in the meantime, we just do noop mv to create a new tree - nodeTree = stew.mv(nodeTree, ''); - return destCopy(nodeTree, destinationPath); };