JBDS-3849 use promises to fetch requirements #158
Conversation
| } | ||
| } | ||
| promise.all(promises); |
There was a problem hiding this comment.
Nick it should be something like this snipped below and coounter would not be neded anymore
promise.all(promises).then((result)=> {
cb();
}).catch((error)=>{
cb(error);
});
nickboldt
force-pushed
the
JBDS-3849_2
branch
2 times, most recently
from
37d0e38
to
6663d27
Compare
nickboldt
changed the title
|
SHA fix for requirements moved to #159 at Denis' request. |
|
So, this PR works if you run But if you start from a clean checkout (no prefetch folder) or do So... the promises are not blocking. :( |
| gulp.task('prefetch', function(cb) { | ||
| let counter=0; | ||
| let promises = []; | ||
| if (!fs.existsSync(prefetchFolder)) { mkdirp(prefetchFolder, cb); } |
There was a problem hiding this comment.
This is the reason why, you call back here and the in promise.all().then().catch();
|
With this last suggestion, build works. Here's a console log (expires in 1 week): HOWEVER the generated .exe is missing the JBDS jar (because the 7zip archive embeds a 0-length file)... details here: |
|
Fix changes a lot and not easy to follow, instead is should be simple promise wrap around download we already have. Here is an example https://github.com/redhat-developer-tooling/developer-platform-install/compare/master...dgolovin:JBDS-3849?expand=1. |
| @@ -17,6 +17,7 @@ var gulp = require('gulp'), | |||
| minimatch = require('minimatch'), | |||
| copy = require('gulp-copy'), | |||
| concat = require('gulp-concat'), | |||
| promise = require('promise'), | |||
There was a problem hiding this comment.
Promises are natively supported by Node.js 4 and 5 so you don't need this dependency.
There was a problem hiding this comment.
I disagree. I get "ReferenceError: promise is not defined" if I don't declare it in gulpfile.js.
There was a problem hiding this comment.
you need to use Promise (with upper-case "P")
|
I added comments directly to Nick's code. I still need to test it
|
|
|
||
| // Create bundled installer | ||
| gulp.task('package-bundle', function(cb) { | ||
| runSequence(['check-requirements', 'clean'], 'create-dist-win-dir', ['generate', | ||
| runSequence(['check-requirements', 'clean'], 'create-dist-win-dir', ['generate', | ||
| 'prepare-tools'], 'prefetch', 'package', 'cleanup', cb); |
There was a problem hiding this comment.
we should be able to run prefetch in parallel with generate and prepare-tools, it will speed up the build a little bit but only if we split downloading and copying deps into separate tasks, i.e. downloading can run in parallel with generate and prepare-tools, copying has to run after generate
There was a problem hiding this comment.
that is doable and should work good, because download is going to be done often for jbds bits only, probably
There was a problem hiding this comment.
I would leave it as is for now and improve performance in another PR
nickboldt
force-pushed
the
JBDS-3849_2
branch
3 times, most recently
from
e4beb04
to
24a98e5
Compare
…y from cache to dist simpler approach from Denis; use requirements folder instead of prefetch-dependencies; also re-enable creation of sha256 file in requirements folder as we'll need it later add requirements/ folder to gitignore better but now prefetch task ENDS THE PROCESS instead of returning and running the package task
| Promise.all(promises).then((result) => { | ||
| installerExe = resolveInstallerExePath('-bundle'); | ||
| cb(); | ||
| }).catch((error)=>{ |
There was a problem hiding this comment.
should be just
}).catch(cb);
|
#161 reworked version of this PR |
|
See #161 for details. |
JBDS-3849 use promise to fetch requirements into cache folder and copy from cache to dist