Skip to content
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.

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

feat(broccoli): add incremental MergeTrees plugin #2064

Closed
wants to merge 6 commits into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented May 21, 2015

Closes #1815

@caitp
Copy link
Contributor Author

caitp commented May 21, 2015

TODO: add tests, fix the dart build which seems to be messed up.

@caitp
Copy link
Contributor Author

caitp commented May 21, 2015

There's a problem with this --- if the inputTrees come from another incremental plugin, there's no way to know if the "real" source file was removed (in which case the output should be removed), or if an input was not included in the last build process.

So what this means is, right now, the mergeTrees plugin will delete any specs because it doesn't think they're around anymore. That's not very good

illustration:

EchoBeach:angular caitp$ gulp test.unit.cjs
Dart SDK detected
[13:10:10] Using gulpfile ~/git/angular/gulpfile.js
[13:10:10] Starting 'build/clean.js'...
[13:10:10] Starting 'build/clean.tools'...
[13:10:10] Finished 'build/clean.tools' after 1.38 ms
[13:10:10] Starting 'build.tools'...
[13:10:10] Starting '!build.tools'...
[13:10:11] Finished 'build/clean.js' after 433 ms
[13:10:15] Finished '!build.tools' after 4.1 s
[13:10:15] Finished 'build.tools' after 4.11 s
[13:10:15] Starting 'test.unit.cjs'...
[13:10:15] Starting '!build.js.cjs'...
Tree diff:          funnel-output_path-cxvFz9UF.tmp, duration:    56ms,   208 changes detected (files:   208, directories:  167)
Tree diff: replace_filter-tmp_dest_dir-ZJvM0X0C.tmp, duration:    21ms,   416 changes detected (files:   416, directories:   55)
Tree diff:          funnel-output_path-9hlvAMyv.tmp, duration:     0ms,     1 changes detected (files:     1, directories:    1)
Tree diff: diffing_plugin_wrapper-output_path-DHPT9XAs.tmp, duration:    23ms,   417 changes detected (files:   417, directories:   55)
Tree diff:          funnel-output_path-YUWFgTka.tmp, duration:     0ms,     1 changes detected (files:     1, directories:    1)
Tree diff: diffing_plugin_wrapper-output_path-VAUHrzcS.tmp, duration:    21ms,   418 changes detected (files:   418, directories:   55)
Tree diff:          funnel-output_path-2Rh5EuAW.tmp, duration:     1ms,     1 changes detected (files:     1, directories:    1)
Tree diff:                   funnel-dest_UULMeC.tmp, duration:    24ms,   128 changes detected (files:   128, directories:  167)
Tree diff: diffing_plugin_wrapper-output_path-A8wXyHUC.tmp, duration:    22ms,   419 changes detected (files:   419, directories:   55)
Tree diff: diffing_plugin_wrapper-output_path-QmXT90nW.tmp, duration:    14ms,   384 changes detected (files:   384, directories:   24)
Tree diff:                   funnel-dest_1liPKx.tmp, duration:     2ms,    27 changes detected (files:    27, directories:   15)
Tree diff:    lodash_renderer-dest_dir-TUPYyHtV.tmp, duration:     0ms,     3 changes detected (files:     3, directories:    3)
Tree diff:                   funnel-dest_GqlAaQ.tmp, duration:    39ms,   833 changes detected (files:   833, directories:   87)

Slowest Trees                                 | Total               
----------------------------------------------+---------------------
DiffingTraceurCompiler                        | 6604ms              
DiffingTSCompiler                             | 4859ms              

Slowest Trees (cumulative)                    | Total (avg)         
----------------------------------------------+---------------------
DiffingTraceurCompiler (1)                    | 6604ms              
DiffingTSCompiler (1)                         | 4859ms              
MergeTrees (4)                                | 875ms (218 ms)      

creating node_modules symlink hack
creating link dist/js/cjs/node_modules/angular2 ../angular2
creating link dist/js/cjs/node_modules/benchmarks ../benchmarks
creating link dist/js/cjs/node_modules/benchmarks_external ../benchmarks_external
creating link dist/js/cjs/node_modules/benchpress ../benchpress
creating link dist/js/cjs/node_modules/examples ../examples
creating link dist/js/cjs/node_modules/rtts_assert ../rtts_assert
[13:10:28] Finished '!build.js.cjs' after 14 s
[13:10:28] Starting 'test.unit.cjs/ci'...
Started
..............................*.....................................................*.........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................*.................................................................................................................................................................................................................................................................................................................................................................

Pending:

1) change detection dynamic change detection error handling should wrap exceptions into ChangeDetectionError
  No reason given


2) change detection JIT change detection error handling should wrap exceptions into ChangeDetectionError
  No reason given


3) Form Model ControlGroup optional components valueChanges should not fire an event when an excluded control is updated
  No reason given

1040 specs, 0 failures, 3 pending specs
Finished in 4.816 seconds
[13:10:35] Finished 'test.unit.cjs/ci' after 6.66 s

==================================================
 WATCH TRIGGERED BY FILE CHANGE #1
   (modules/angular2/src/facade/async.ts touched in another terminal)
 On: 5/21/2015 at 1:10:39 PM
==================================================
[13:10:39] Starting '!build.js.cjs'...
Tree diff:          funnel-output_path-cxvFz9UF.tmp, duration:    27ms,     0 changes detected (files:   208, directories:  167)
Tree diff: replace_filter-tmp_dest_dir-ZJvM0X0C.tmp, duration:     6ms,   416 changes detected (files:     0, directories:    0) /* long pointless array of changed files */
Tree diff:          funnel-output_path-9hlvAMyv.tmp, duration:     0ms,     0 changes detected (files:     1, directories:    1)
Tree diff: diffing_plugin_wrapper-output_path-DHPT9XAs.tmp, duration:     8ms,   416 changes detected (files:     1, directories:   55) /* long pointless array of changed files */
Tree diff:          funnel-output_path-YUWFgTka.tmp, duration:     0ms,     0 changes detected (files:     1, directories:    1)
Tree diff: diffing_plugin_wrapper-output_path-VAUHrzcS.tmp, duration:     8ms,   416 changes detected (files:     2, directories:   55) /* long pointless array of changed files */
Tree diff:          funnel-output_path-2Rh5EuAW.tmp, duration:     0ms,     0 changes detected (files:     1, directories:    1)
Tree diff:                   funnel-dest_UULMeC.tmp, duration:    25ms,     0 changes detected (files:   128, directories:  167)
Tree diff: diffing_plugin_wrapper-output_path-A8wXyHUC.tmp, duration:    10ms,   416 changes detected (files:     3, directories:   55) /* long pointless array of changed files */
Tree diff: diffing_plugin_wrapper-output_path-QmXT90nW.tmp, duration:    13ms,     0 changes detected (files:   384, directories:   24)
Tree diff:                   funnel-dest_1liPKx.tmp, duration:     2ms,     0 changes detected (files:    27, directories:   15)
Tree diff:    lodash_renderer-dest_dir-TUPYyHtV.tmp, duration:     0ms,     3 changes detected (files:     0, directories:    0) /* long pointless array of changed files */
Tree diff:                   funnel-dest_GqlAaQ.tmp, duration:    37ms,   419 changes detected (files:   414, directories:   87) /* long pointless array of changed files */

Slowest Trees                                 | Total               
----------------------------------------------+---------------------
ReplaceFilter                                 | 349ms               
Funnel                                        | 246ms               
MergeTrees                                    | 121ms               
ReplaceFilter                                 | 117ms               
MergeTrees                                    | 81ms                
DestCopy                                      | 74ms                
MergeTrees                                    | 72ms                
MergeTrees                                    | 71ms                

Slowest Trees (cumulative)                    | Total (avg)         
----------------------------------------------+---------------------
ReplaceFilter (2)                             | 466ms (233 ms)      
MergeTrees (4)                                | 346ms (86 ms)       
Funnel (10)                                   | 329ms (32 ms)       
DestCopy (1)                                  | 74ms                

[13:10:40] Finished '!build.js.cjs' after 1.28 s
[13:10:40] Starting 'test.unit.cjs/ci'...
Started


No specs found
Finished in 0.001 seconds
[13:10:40] Finished 'test.unit.cjs/ci' after 295 ms

@caitp
Copy link
Contributor Author

caitp commented May 21, 2015

interestingly, test.unit.dart works properly, and seems to rebuild very quickly

@caitp
Copy link
Contributor Author

caitp commented May 21, 2015

so um, any ideas there?

@caitp caitp added this to the Build System milestone May 21, 2015
let inputPath = this.inputPaths[index];
let existsLater = (relativePath) => {
for (let i = treeDiffs.length - 1;
i > index; --i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-format bug?

@IgorMinar
Copy link
Contributor

my theory is that the reason why this is not working is that you are hitting #2051

incremental plugins producing input trees should not be a problem because the input tree is always complete even if it was incrementally updated. so you will never get an input tree just with the incremental updates, it will always be a full tree that contains incremental updates.

@caitp
Copy link
Contributor Author

caitp commented May 22, 2015

my theory is that the reason why this is not working is that you are hitting #2051

incremental plugins producing input trees should not be a problem because the input tree is always complete even if it was incrementally updated. so you will never get an input tree just with the incremental updates, it will always be a full tree that contains incremental updates.

Yeah, when I was looking at your patch it occurred to me that that's probably what this was. I think we should get 2053 in today

@caitp
Copy link
Contributor Author

caitp commented May 28, 2015

PTAL

@caitp caitp force-pushed the multi-tree branch 2 times, most recently from 7b04472 to bc8bc48 Compare May 29, 2015 00:08
@mhevery mhevery added area: build & ci Related the build and CI infrastructure of the project and removed area: build & ci Related the build and CI infrastructure of the project #development_infrastructure labels May 29, 2015
@mhevery mhevery removed this from the Build System milestone May 29, 2015
@IgorMinar
Copy link
Contributor

the first commit looks good.

the second commit could use some tests. can you add them please?

@caitp
Copy link
Contributor Author

caitp commented May 29, 2015

okay, I added "some" test coverage --- I'm not sure why travis is complaining about "toBe" on the Matchers interface, since I can see it locally quite plainly. But it will probably complain about the thing fixed by https://github.com/angular/DefinitelyTyped/pull/3 too when it gets around to it

@caitp
Copy link
Contributor Author

caitp commented May 30, 2015

At the moment a bunch of integration tests fail, investigating

@caitp
Copy link
Contributor Author

caitp commented May 30, 2015

nah, same tests fail locally on master too. I don't think this PR is breaking em, but not sure why travis is green for the master branch

@IgorMinar
Copy link
Contributor

yeah, the original merger is conservative and throws when duplicates are detected. I think that this is a good default and avoids accidental over-writes.

I was also hacking on the funnel plugin and realized that having addedPaths in diff result is useful for plugins like funnel and mergeTrees. I implemented this in one of the commits under #2257

@IgorMinar
Copy link
Contributor

@caitp still no update? do you need help?

@caitp caitp force-pushed the multi-tree branch 2 times, most recently from 2a8e631 to c28a57e Compare June 4, 2015 21:42
@caitp
Copy link
Contributor Author

caitp commented Jun 4, 2015

https://www.diffchecker.com/fejjguqf here's a diff of the outputs from mergeTrees, with the master branch on the right

@caitp
Copy link
Contributor Author

caitp commented Jun 4, 2015

https://gist.github.com/caitp/b9251a63bba113cf4832 and here is a log of what the tree-differ is checking. It looks like merge-trees never gets notified of the existence of those other files. Are they symlinks perhaps? Would that matter?

@caitp
Copy link
Contributor Author

caitp commented Jun 5, 2015

As mentioned on slack:

The problem seems to be coming from the fact that we are mutating live inputTrees in the tree builder (

function writeScriptsForPath(relativePath, result) {
copyVendorScriptsTo(path.dirname(relativePath));
return result.replace('@@FILENAME_NO_EXT', relativePath.replace(/\.\w+$/, ''));
}
var htmlTree = new Funnel(modulesTree, {include: ['*/src/**/*.html'], destDir: '/'});
htmlTree = replace(htmlTree, {
files: ['examples*/**'],
patterns: [{match: /\$SCRIPTS\$/, replacement: htmlReplace('SCRIPTS')}],
replaceWithPath: writeScriptsForPath
});
htmlTree = replace(htmlTree, {
files: ['benchmarks/**'],
patterns: [{match: /\$SCRIPTS\$/, replacement: htmlReplace('SCRIPTS_benchmarks')}],
replaceWithPath: writeScriptsForPath
});
htmlTree = replace(htmlTree, {
files: ['benchmarks_external/**'],
patterns: [{match: /\$SCRIPTS\$/, replacement: htmlReplace('SCRIPTS_benchmarks_external')}],
replaceWithPath: writeScriptsForPath
});
,
function copyVendorScriptsTo(destDir) {
servingTrees.push(new Funnel(vendorScriptsTree, {srcDir: '/', destDir: destDir}));
if (destDir.indexOf('benchmarks') > -1) {
servingTrees.push(new Funnel(vendorScripts_benchmark, {srcDir: '/', destDir: destDir}));
}
if (destDir.indexOf('benchmarks_external') > -1) {
servingTrees.push(
new Funnel(vendorScripts_benchmarks_external, {srcDir: '/', destDir: destDir}));
}
}
).

This is a problem for the diffing plugin wrapper, because stabilizeTrees() will replace the live array with a copy of the inital array at the time stabilization occurred, so mergeTrees() never sees all the extra inputTrees that should be there.

The right thing to do here is to just not mutate the live inputTrees, and make sure there is a workable set of inputTrees to begin with. On top of that, in the existing code on master, some duplicate work is actually happening because trees are pushed into the live inputTrees array multiple times. So we should be able to see a slight perf boost from fixing that. (this is probably the reason why overwrite: true is needed at all)

@caitp
Copy link
Contributor Author

caitp commented Jun 5, 2015

The extra {overwrite: true} seems to be needed due to the polymer url_params_for_form script for some reason.

tmp/diffing_plugin_wrapper-output_path-qmFFK91M.tmp/benchmarks_external/src/tree/polymer/url_params_to_form.js is duplicate of tmp/tree_stabilizer-output_path-Z35ehLqR.tmp/benchmarks_external/src/tree/polymer/url_params_to_form.js

// TODO(caitp): Figure out how to make this fast, while still being able to
// determine the trees immediately. Manually maintaining a list of paths to
// manage seems cumbersome.
function getServedPaths() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per TODO comment above, Igor wants to just manually maintain the list of paths, and we probably want that list to happen later (or maybe be a constant kServedPaths or something.

@caitp caitp force-pushed the multi-tree branch 2 times, most recently from 2e8d36c to 7bae3f4 Compare June 5, 2015 19:59
@@ -145,7 +184,8 @@ module.exports = function makeBrowserTree(options, destinationPath) {

es5Tree = mergeTrees([es5Tree, htmlTree]);

var mergedTree = mergeTrees([stew.mv(es6Tree, '/es6'), stew.mv(es5Tree, '/es5')]);
var mergedTree =
stew.log(mergeTrees([stew.mv(es6Tree, '/es6'), stew.mv(es5Tree, '/es5')]), {output: 'tree'});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed locally, won't be checked in

@caitp
Copy link
Contributor Author

caitp commented Jun 5, 2015

CI failure doesn't seem to be fault of this PR, same error in https://travis-ci.org/angular/angular/jobs/65624606

@caitp caitp closed this in 41ae8e7 Jun 5, 2015
caitp added a commit that referenced this pull request Jun 5, 2015
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: build & ci Related the build and CI infrastructure of the project cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rebuild broccoli-merge-trees plugin to be incremental
5 participants