Skip to content

build(TreeDiffer): don't assume that rootPaths to dirty check are imm… #2053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
IgorMinar wants to merge 1 commit into from
Closed

build(TreeDiffer): don't assume that rootPaths to dirty check are imm… #2053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tools/broccoli/diffing-broccoli-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class DiffingPluginWrapper implements BroccoliTree {
let firstRun = !this.initialized;
this.init();

let diffResult = this.treeDiffer.diffTree();
let diffResult = this.treeDiffer.diffTree(this.inputPath);

@caitp caitp May 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if I understand this, broccoli sometimes changes the inputPath for us, and we need to be able to propagate that change to the TreeDiffer, and we do that here by passing it in here, right?

But, (some) plugins still think they need to use the old inputPath, they don't have any idea that it changed

@IgorMinar IgorMinar May 22, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it to me. my patch is failing in some cases so I need to investigate it further. initial explorations showed that the output path for certain plugins like replace and mergeTrees was not immutable.

@caitp caitp May 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought about something like this?

class DiffingPluginWrapper {
  get inputPath() { return this._inputPath; }
  set inputPath(path) {
    this._inputPath = path;
    // propagate the change to wrapped plugin
    if (this.initialized) this.wrappedPlugin.inputPath = path;
  }
  // etc
}

https://github.com/IgorMinar/angular/pull/1/files working example (tested with test.unit.tools and test.unit.cjs)

@IgorMinar IgorMinar May 25, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think about that, but it is simply wrong that the input path changes. The plugin should be the owner of the inputPath state and nobody else should change it, otherwise the whole system is unstable and it's easy to make assumptions that the input path doesn't change and cache absolute paths somewhere.

I have a different PR in works that stabilizes the input path.

@caitp caitp May 25, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contracts for these properties are pseudo-documented in the broccolijs codebase, but it's not clear how strong those contracts are.

diffResult.log(!firstRun);

var rebuildPromise = this.wrappedPlugin.rebuild(diffResult);
Expand Down Expand Up @@ -94,7 +94,7 @@ class DiffingPluginWrapper implements BroccoliTree {
let includeExtensions = this.pluginClass.includeExtensions || [];
let excludeExtensions = this.pluginClass.excludeExtensions || [];
this.initialized = true;
this.treeDiffer = new TreeDiffer(this.inputPath, includeExtensions, excludeExtensions);
this.treeDiffer = new TreeDiffer(includeExtensions, excludeExtensions);
this.wrappedPlugin =
new this.pluginClass(this.inputPath, this.cachePath, this.wrappedPluginArguments[1]);
}
Expand Down
72 changes: 36 additions & 36 deletions tools/broccoli/tree-differ.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1');
let differ = new TreeDiffer();

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths)
.toEqual(['file-1.txt', 'file-2.txt', 'subdir-1/file-1.1.txt']);
Expand All @@ -49,14 +49,14 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1');
let differ = new TreeDiffer();

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths).not.toEqual([]);
expect(diffResult.removedPaths).toEqual([]);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths).toEqual([]);
expect(diffResult.removedPaths).toEqual([]);
Expand All @@ -76,9 +76,9 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1');
let differ = new TreeDiffer();

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths)
.toEqual(['file-1.txt', 'file-2.txt', 'subdir-1/file-1.1.txt']);
Expand All @@ -89,7 +89,7 @@ describe('TreeDiffer', () => {
mockfs.file({content: 'file-1.1.txt content', mtime: new Date(9999)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths).toEqual(['file-1.txt', 'subdir-1/file-1.1.txt']);

Expand All @@ -99,7 +99,7 @@ describe('TreeDiffer', () => {
testDir['dir1']['file-1.txt'] = mockfs.file({content: 'super new', mtime: new Date(1000)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual(['file-1.txt']);
});

Expand All @@ -123,9 +123,9 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('symlinks');
let differ = new TreeDiffer();

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('symlinks');

expect(diffResult.changedPaths)
.toEqual(['file-1.txt', 'file-2.txt', 'subdir-1/file-1.1.txt']);
Expand All @@ -137,7 +137,7 @@ describe('TreeDiffer', () => {
mockfs.file({content: 'file-1.1.txt content', mtime: new Date(9999)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('symlinks');

expect(diffResult.changedPaths).toEqual(['file-1.txt', 'subdir-1/file-1.1.txt']);

Expand All @@ -148,21 +148,21 @@ describe('TreeDiffer', () => {
mockfs.file({content: 'super new', mtime: new Date(1000)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('symlinks');
expect(diffResult.changedPaths).toEqual(['file-1.txt']);

// remove a link
delete testDir['orig_path']['file-1.txt'];
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('symlinks');
expect(diffResult.changedPaths).toEqual([]);
expect(diffResult.removedPaths).toEqual(['file-1.txt']);

// don't report it as a removal twice
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('symlinks');
expect(diffResult.changedPaths).toEqual([]);
expect(diffResult.removedPaths).toEqual([]);

Expand All @@ -171,7 +171,7 @@ describe('TreeDiffer', () => {
mockfs.file({content: 'super new', mtime: new Date(1000)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('symlinks');
expect(diffResult.changedPaths).toEqual(['file-1.txt']);
expect(diffResult.removedPaths).toEqual([]);
});
Expand All @@ -190,9 +190,9 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1', ['.js', '.coffee']);
let differ = new TreeDiffer(['.js', '.coffee']);

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths).toEqual(['file-1.js', 'file-3.coffee']);

Expand All @@ -204,7 +204,7 @@ describe('TreeDiffer', () => {
mockfs.file({content: 'file-1.1.cc content', mtime: new Date(9999)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths).toEqual(['file-1.js', 'file-3.coffee']);

Expand All @@ -214,7 +214,7 @@ describe('TreeDiffer', () => {
testDir['dir1']['file-1.js'] = mockfs.file({content: 'super new', mtime: new Date(1000)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual(['file-1.js']);
});

Expand All @@ -236,9 +236,9 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1', ['.ts', '.cs'], ['.d.ts', '.d.cs']);
let differ = new TreeDiffer(['.ts', '.cs'], ['.d.ts', '.d.cs']);

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths)
.toEqual(['file-1.cs', 'file-1.ts', 'file-1d.cs', 'file-3.ts']);
Expand All @@ -253,7 +253,7 @@ describe('TreeDiffer', () => {
mockfs.file({content: 'file-1.1.cc content', mtime: new Date(9999)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');

expect(diffResult.changedPaths).toEqual(['file-1.cs', 'file-1.ts', 'file-3.ts']);

Expand All @@ -263,7 +263,7 @@ describe('TreeDiffer', () => {
testDir['dir1']['file-4.d.ts'] = mockfs.file({content: 'super new', mtime: new Date(1000)});
mockfs(testDir);

diffResult = differ.diffTree();
diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual([]);
});
});
Expand All @@ -277,13 +277,13 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1');
differ.diffTree();
let differ = new TreeDiffer();
differ.diffTree('dir1');

testDir['dir1']['file-2.txt'] = 'new file';
mockfs(testDir);

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual(['file-2.txt']);
});
});
Expand All @@ -295,14 +295,14 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1');
differ.diffTree();
let differ = new TreeDiffer();
differ.diffTree('dir1');

testDir['dir1']['file-1.txt'] = 'new content';
testDir['dir1']['file-2.txt'] = 'new file';
mockfs(testDir);

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual(['file-1.txt', 'file-2.txt']);
});

Expand All @@ -316,13 +316,13 @@ describe('TreeDiffer', () => {
};
mockfs(testDir);

let differ = new TreeDiffer('dir1');
differ.diffTree();
let differ = new TreeDiffer();
differ.diffTree('dir1');

delete testDir['dir1']['file-1.txt'];
mockfs(testDir);

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual([]);
expect(diffResult.removedPaths).toEqual(['file-1.txt']);
});
Expand All @@ -339,15 +339,15 @@ describe('TreeDiffer', () => {

mockfs(testDir);

let differ = new TreeDiffer('dir1');
differ.diffTree();
let differ = new TreeDiffer();
differ.diffTree('dir1');

testDir['dir1']['file-1.txt'] = 'changed content';
delete testDir['dir1']['file-2.txt'];
testDir['dir1']['file-3.txt'] = 'new content';
mockfs(testDir);

let diffResult = differ.diffTree();
let diffResult = differ.diffTree('dir1');
expect(diffResult.changedPaths).toEqual(['file-1.txt', 'file-3.txt']);
expect(diffResult.removedPaths).toEqual(['file-2.txt']);
});
Expand Down
9 changes: 5 additions & 4 deletions tools/broccoli/tree-differ.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,10 @@ export class TreeDiffer {
private rootDirName: string;
private include: RegExp = null;
private exclude: RegExp = null;
private rootPath: string = null;

constructor(private rootPath: string, includeExtensions?: string[],
constructor(includeExtensions?: string[],
excludeExtensions?: string[]) {
this.rootDirName = path.basename(rootPath);

let buildRegexp = (arr) => new RegExp(`(${arr.reduce(combine, "")})$`, "i");

this.include = (includeExtensions || []).length ? buildRegexp(includeExtensions) : null;
Expand All @@ -39,7 +38,9 @@ export class TreeDiffer {
}


public diffTree(): DiffResult {
public diffTree(rootPath): DiffResult {
this.rootPath = rootPath;
this.rootDirName = path.basename(rootPath);

@caitp caitp May 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a problem here: fingerprints still contain old absolute paths, so fingerprints basically need a reset (or alternatively, the fingerprints object keys need to be the relative path rather than absolute)

@IgorMinar IgorMinar May 22, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be a relative path

@caitp caitp May 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posteriority: I must have had a subtle error when trying the relativePath stuff before, it seems to work correctly now (so virtualPath is not needed) --- updating the pr

let result = new DirtyCheckingDiffResult(this.rootDirName);
this.dirtyCheckPath(this.rootPath, result);
this.detectDeletionsAndUpdateFingerprints(result);
Expand Down
10 changes: 0 additions & 10 deletions tools/broccoli/trees/browser_tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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);
};
5 changes: 0 additions & 5 deletions tools/broccoli/trees/dart_tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
10 changes: 0 additions & 10 deletions tools/broccoli/trees/node_tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,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,
Expand All @@ -137,10 +132,5 @@ module.exports = function makeNodeTree(destinationPath) {

nodeTree = mergeTrees([nodeTree, typescriptTree, docs, packageJsons]);

// 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);
};