Issue 29435631: Issue 5230 - Properly download Chromium on macOS (Closed)
|
Created:
May 10, 2017, 2:13 p.m. by hub Modified:
May 18, 2017, 5:54 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 5230 - Properly download Chromium on macOS
Patch Set 1 #
Total comments: 23
Patch Set 2 : Fix the issues from the review. #
Total comments: 10
Patch Set 3 : Rework the code to be clearer. Address the other review issues. #
Total comments: 10
Patch Set 4 : Addressed review comments. #MessagesTotal messages: 9
https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:30: const DecompressZip = require('decompress-zip'); I had to use a different module that actually work on macOS. Also I don't stream so one can cache the archive.
https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:30: const DecompressZip = require('decompress-zip'); On 2017/05/10 14:15:27, hub wrote: > I had to use a different module that actually work on macOS. Also I don't stream > so one can cache the archive. If we don't need streaming support then we should go with a mature and widely used module here. In my understanding, yauzl is the usual choice. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:107: const archive = path.join(chromiumDownloadCache, I'm not a big fan of using `const` for things that aren't inherently constant but merely aren't being changed by the current implementation. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:121: const writable = fs.createWriteStream(archive); Same as above, I'd rather not have this marked as const. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:133: .on("error", reject ) What happens with the write stream if an error occurs? Will we have some half-written archive on disk that will prevent successful installations for all eternity? We should definitely clean up here. However, we should also make sure that we don't leave an invalid archive in the cache. Usually this means downloading to a temporary file in the same directory and only renaming if the download succeeds. We need to check response code as well now - this wasn't important before but now the user might get stuck in an invalid state. Nit: I'm pretty sure that ESLint will object against the extra space added here. Please make sure to run it. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:136: unzipChromium(archive, chromiumDir).catch(reject); Where is resolve being called here? https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:138: }).on("error", reject); Given how complicated the download logic gets, I think it would help to wrap it in a generic helper returning a promise. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:142: function unzipChromium(archive, chromiumDir) Nit: this is a generic function, so name and parameters should be generic as well. How about unzipArchive(archive, targetDir)? https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:148: resolve(getChromiumExecutable(chromiumDir)); The getChromiumExecutable() call should stay in ensureChromium(), the function here should merely do what its name says - unzip, then resolve promise.
Updated patch. The only thing not done is using another zip module like yauzl (see comments). But if extract-zip (that uses yauzl) is ok for you, I can do it. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:30: const DecompressZip = require('decompress-zip'); On 2017/05/16 14:21:15, Wladimir Palant wrote: > On 2017/05/10 14:15:27, hub wrote: > > I had to use a different module that actually work on macOS. Also I don't > stream > > so one can cache the archive. > > If we don't need streaming support then we should go with a mature and widely > used module here. In my understanding, yauzl is the usual choice. yauzl doesn't have an easy "extract archive API". There is "extract-zip" for that https://www.npmjs.com/package/extract-zip. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:107: const archive = path.join(chromiumDownloadCache, On 2017/05/16 14:21:15, Wladimir Palant wrote: > I'm not a big fan of using `const` for things that aren't inherently constant > but merely aren't being changed by the current implementation. Done. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:121: const writable = fs.createWriteStream(archive); On 2017/05/16 14:21:15, Wladimir Palant wrote: > Same as above, I'd rather not have this marked as const. Done. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:133: .on("error", reject ) > Nit: I'm pretty sure that ESLint will object against the extra space added here. > Please make sure to run it. If I run eslint on that file I get: 229:46 error Parsing error: Unexpected token client it seems to not understand the async keyword. So it never catches the error above. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:133: .on("error", reject ) On 2017/05/16 14:21:15, Wladimir Palant wrote: > What happens with the write stream if an error occurs? Will we have some > half-written archive on disk that will prevent successful installations for all > eternity? We should definitely clean up here. Good idea let's unlink the file. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:136: unzipChromium(archive, chromiumDir).catch(reject); On 2017/05/16 14:21:15, Wladimir Palant wrote: > Where is resolve being called here? Not sure what happens here because it works, but I'll make sure it called. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:142: function unzipChromium(archive, chromiumDir) On 2017/05/16 14:21:15, Wladimir Palant wrote: > Nit: this is a generic function, so name and parameters should be generic as > well. How about unzipArchive(archive, targetDir)? Done. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:148: resolve(getChromiumExecutable(chromiumDir)); On 2017/05/16 14:21:15, Wladimir Palant wrote: > The getChromiumExecutable() call should stay in ensureChromium(), the function > here should merely do what its name says - unzip, then resolve promise. I'll resolve the promise with the destination path and call ensureChromium() above in the resolution of the unzipArchive
I didn't comment on the issues that ESLint will catch. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:30: const DecompressZip = require('decompress-zip'); On 2017/05/16 15:49:30, hub wrote: > yauzl doesn't have an easy "extract archive API". There is "extract-zip" for > that https://www.npmjs.com/package/extract-zip. You are right. extract-zip sounds good. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:133: .on("error", reject ) On 2017/05/16 15:49:30, hub wrote: > it seems to not understand the async keyword. So it never catches the error > above. You should update eslint-config-eyeo package, support for ECMAScript 2017 features has been enabled. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:148: resolve(getChromiumExecutable(chromiumDir)); On 2017/05/16 15:49:30, hub wrote: > I'll resolve the promise with the destination path This doesn't make much sense either - the destination path is simply the parameter that the caller passed in, it cannot change. I suggest resolving without any value. https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:115: }); This is equivalent to: .then(dir => getChromiumExecutable(dir)); https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:120: return downloadCacheAndExtract(url, chromiumDownloadCache, archive, chromiumDir); The point of using promises is making the logic obvious instead of splitting the flow into dozens of callbacks. While we have multiple functions here, this is merely compensating for the fact that Node doesn't provide promise-based APIs - we have promise-based wrappers for the core APIs. So we should really have a simple download helper here, without any assumptions about what is being downloaded: return download(url, archive) .then(() => unzipArchive(archive, chromiumDir)) .then(() => getChromiumExecutable(chromiumDir)); You can also avoid duplicating the logic for the cache hit and download scenarios: let retrieveArchive; if (fs.existsSync(archive)) retrieveArchive = Promise.resove(); else retrieveArchive = download(url, archive); return retrieveArchive .then(...); https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:130: fs.mkdirSync(downloadCache); You don't need the downloadCache parameter, just use path.dirname(targetPath). https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:144: fs.unlinkSync(destArchive); Will unlink work on Windows if the stream is still open? I don't think that unlinking is sufficient. Your process might crash and then the cleanup will never run - the result would be a broken installation that cannot be fixed any more without removing files manually. As I said, we should download to a file with a different name first and only rename on success - a rename is an atomic operation as long as the file stays in the same directory.
updated patch. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:30: const DecompressZip = require('decompress-zip'); On 2017/05/17 10:37:02, Wladimir Palant wrote: > On 2017/05/16 15:49:30, hub wrote: > > yauzl doesn't have an easy "extract archive API". There is "extract-zip" for > > that https://www.npmjs.com/package/extract-zip. > > You are right. extract-zip sounds good. Done. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:133: .on("error", reject ) On 2017/05/17 10:37:02, Wladimir Palant wrote: > On 2017/05/16 15:49:30, hub wrote: > > it seems to not understand the async keyword. So it never catches the error > > above. > > You should update eslint-config-eyeo package, support for ECMAScript 2017 > features has been enabled. Acknowledged. https://codereview.adblockplus.org/29435631/diff/29435632/chromium_process.js... chromium_process.js:148: resolve(getChromiumExecutable(chromiumDir)); On 2017/05/17 10:37:02, Wladimir Palant wrote: > On 2017/05/16 15:49:30, hub wrote: > > I'll resolve the promise with the destination path > > This doesn't make much sense either - the destination path is simply the > parameter that the caller passed in, it cannot change. I suggest resolving > without any value. Done. https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:115: }); On 2017/05/17 10:37:02, Wladimir Palant wrote: > This is equivalent to: > > .then(dir => getChromiumExecutable(dir)); Done. https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:120: return downloadCacheAndExtract(url, chromiumDownloadCache, archive, chromiumDir); On 2017/05/17 10:37:02, Wladimir Palant wrote: > The point of using promises is making the logic obvious instead of splitting the > flow into dozens of callbacks. While we have multiple functions here, this is > merely compensating for the fact that Node doesn't provide promise-based APIs - > we have promise-based wrappers for the core APIs. So we should really have a > simple download helper here, without any assumptions about what is being > downloaded: > > return download(url, archive) > .then(() => unzipArchive(archive, chromiumDir)) > .then(() => getChromiumExecutable(chromiumDir)); > > You can also avoid duplicating the logic for the cache hit and download > scenarios: > > let retrieveArchive; > if (fs.existsSync(archive)) > retrieveArchive = Promise.resove(); > else > retrieveArchive = download(url, archive); > return retrieveArchive > .then(...); Acknowledged. https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:130: fs.mkdirSync(downloadCache); On 2017/05/17 10:37:02, Wladimir Palant wrote: > You don't need the downloadCache parameter, just use path.dirname(targetPath). Acknowledged. https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:144: fs.unlinkSync(destArchive); On 2017/05/17 10:37:02, Wladimir Palant wrote: > Will unlink work on Windows if the stream is still open? From what I can read it will work and the file will really be deleted when it is closed (immediatly if it is not open anymore). Will fix including the above.
https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:144: fs.unlinkSync(destArchive); On 2017/05/17 17:25:02, hub wrote: > From what I can read it will work and the file will really be deleted when it is > closed (immediatly if it is not open anymore). Depends on the lock type I think. With an exclusive lock (which is what we have here) unlinking will actually fail IMHO. We should definitely close this stream on error, and be it merely for correctness. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:28: const process = require("process"); You don't need this require, process is a global variable for all Node modules (see https://nodejs.org/api/globals.html#globals_process). https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:98: return new Promise((resolve, reject) => There is no async action here that isn't based on promises already, no point using the Promise constructor. You can achieve the same effect like this: return Promise.resolve().then(() => { ... if (fs.existsSync(chromiumDir)) return chromiumDir; ... return download().then(unzip).then(() => chromiumDir); }).then(dir => getChromiumExecutable(dir)); https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:113: const url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`; You changed a bunch more variables from let to const here... Our coding style actually mentions this explicitly under https://adblockplus.org/en/coding-style#javascript-modern: > Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.). That's not what we have here so please change it back. Note that we discussed that rule on IRC today (particularly given that there are multiple opinions here) and decided that changing it isn't feasible/worthwhile right now. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:122: function downloadCache(url, destArchive) Nit: the function name sounds strange, downloadCached maybe? More substantial issue: for what should be a generic function, this one has too much application logic in it. It starts with the parameter name destArchive - there is no reason for this particular function to assume that the download is an archive. And it continues with the console.info() output which also mentions archives and Chromium. Maybe handling the cache in ensureChromium() and keeping this a completely generic download() helper like I suggested makes more sense after all? If you dislike the code I suggested, the following pattern would work as well: return Promise.resolve() .then(() => { if (!fs.existsSync(archive)) { console.info("Downloading Chromium..."); return download(url, achive); } console.info(`Reusing cached archive ${archive}`); }) .then(() => unzipArchive(archive, chromiumDir)) .then(() => resolve(chromiumDir)); https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:144: reject(new Error(`Unexpected server response: ${response.statusCode}`)); I'm pretty sure that ESLint dislikes the length of that line.
Updated patch. https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29439609/chromium_process.js... chromium_process.js:144: fs.unlinkSync(destArchive); On 2017/05/18 11:18:27, Wladimir Palant wrote: > On 2017/05/17 17:25:02, hub wrote: > > From what I can read it will work and the file will really be deleted when it > is > > closed (immediatly if it is not open anymore). > > Depends on the lock type I think. With an exclusive lock (which is what we have > here) unlinking will actually fail IMHO. We should definitely close this stream > on error, and be it merely for correctness. I'll add a "writeable.close()" first, just to make sure. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:28: const process = require("process"); On 2017/05/18 11:18:27, Wladimir Palant wrote: > You don't need this require, process is a global variable for all Node modules > (see https://nodejs.org/api/globals.html#globals_process). Acknowledged. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:98: return new Promise((resolve, reject) => On 2017/05/18 11:18:28, Wladimir Palant wrote: > There is no async action here that isn't based on promises already, no point > using the Promise constructor. You can achieve the same effect like this: > > return Promise.resolve().then(() => > { > ... > if (fs.existsSync(chromiumDir)) > return chromiumDir; > ... > return download().then(unzip).then(() => chromiumDir); > }).then(dir => getChromiumExecutable(dir)); Done. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:113: const url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`; On 2017/05/18 11:18:28, Wladimir Palant wrote: > You changed a bunch more variables from let to const here... Our coding style > actually mentions this explicitly under > https://adblockplus.org/en/coding-style#javascript-modern: > > > Use const for constant expressions that could as well have been inlined (e.g. > static parameters, imported modules, etc.). > > That's not what we have here so please change it back. > > Note that we discussed that rule on IRC today (particularly given that there are > multiple opinions here) and decided that changing it isn't feasible/worthwhile > right now. I ended up using const like in C++ which is not exactly what it means in ES6. Will change back. Sorry about that. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:122: function downloadCache(url, destArchive) On 2017/05/18 11:18:27, Wladimir Palant wrote: > Nit: the function name sounds strange, downloadCached maybe? > > More substantial issue: for what should be a generic function, this one has too > much application logic in it. It starts with the parameter name destArchive - > there is no reason for this particular function to assume that the download is > an archive. And it continues with the console.info() output which also mentions > archives and Chromium. Maybe handling the cache in ensureChromium() and keeping > this a completely generic download() helper like I suggested makes more sense > after all? > > If you dislike the code I suggested, the following pattern would work as well: > > return Promise.resolve() > .then(() => > { > if (!fs.existsSync(archive)) > { > console.info("Downloading Chromium..."); > return download(url, achive); > } > > console.info(`Reusing cached archive ${archive}`); > }) > .then(() => unzipArchive(archive, chromiumDir)) > .then(() => resolve(chromiumDir)); fixing it. https://codereview.adblockplus.org/29435631/diff/29440589/chromium_process.js... chromium_process.js:144: reject(new Error(`Unexpected server response: ${response.statusCode}`)); On 2017/05/18 11:18:28, Wladimir Palant wrote: > I'm pretty sure that ESLint dislikes the length of that line. it does. Fixing.
LGTM This needs to land on master and emscripten branch. 
|
