Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: chromium_process.js

Issue 29435631: Issue 5230 - Properly download Chromium on macOS (Closed) Base URL: https://hg.adblockplus.org/adblockpluscore/
Patch Set: Created May 10, 2017, 2:13 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | package.json » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chromium_process.js
===================================================================
--- a/chromium_process.js
+++ b/chromium_process.js
@@ -22,17 +22,17 @@
const childProcess = require("child_process");
const fs = require("fs");
const https = require("https");
const os = require("os");
const path = require("path");
const remoteInterface = require("chrome-remote-interface");
-const unzip = require("unzip");
+const DecompressZip = require('decompress-zip');
hub 2017/05/10 14:15:27 I had to use a different module that actually work
Wladimir Palant 2017/05/16 14:21:15 If we don't need streaming support then we should
hub 2017/05/16 15:49:30 yauzl doesn't have an easy "extract archive API".
Wladimir Palant 2017/05/17 10:37:02 You are right. extract-zip sounds good.
hub 2017/05/17 17:25:02 Done.
const CHROMIUM_REVISION = 467222;
function rmdir(dirPath)
{
for (let file of fs.readdirSync(dirPath))
{
let filePath = path.join(dirPath, file);
@@ -88,44 +88,75 @@
};
if (!buildTypes.hasOwnProperty(platform))
{
let err = new Error(`Cannot run browser tests, ${platform} is unsupported`);
return Promise.reject(err);
}
+ let chromiumDownloadCache = path.join(__dirname, "chromium-snapshots",
+ "download-cache");
let chromiumDir = path.join(__dirname, "chromium-snapshots",
`chromium-${platform}-${CHROMIUM_REVISION}`);
if (fs.existsSync(chromiumDir))
return Promise.resolve(getChromiumExecutable(chromiumDir));
if (!fs.existsSync(path.dirname(chromiumDir)))
fs.mkdirSync(path.dirname(chromiumDir));
+
+ let [dir, fileName] = buildTypes[platform];
+ const archive = path.join(chromiumDownloadCache,
Wladimir Palant 2017/05/16 14:21:15 I'm not a big fan of using `const` for things that
hub 2017/05/16 15:49:30 Done.
+ `${CHROMIUM_REVISION}-${fileName}`);
+ if (fs.existsSync(archive))
+ {
+ console.info(`Reusing cached archive ${archive}`);
+ return unzipChromium(archive, chromiumDir);
+ }
return new Promise((resolve, reject) =>
{
console.info("Downloading Chromium...");
- let [dir, fileName] = buildTypes[platform];
let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;
+
+ if (!fs.existsSync(chromiumDownloadCache))
+ fs.mkdirSync(chromiumDownloadCache);
+ const writable = fs.createWriteStream(archive);
Wladimir Palant 2017/05/16 14:21:15 Same as above, I'd rather not have this marked as
hub 2017/05/16 15:49:30 Done.
+
https.get(url, response =>
{
if (response.statusCode != 200)
{
reject(new Error(`Unexpected server response: ${response.statusCode}`));
response.resume();
return;
}
- response.pipe(unzip.Extract({path: chromiumDir}))
- .on("error", reject)
- .on("close", () => resolve(getChromiumExecutable(chromiumDir)));
+ response.pipe(writable)
+ .on("error", reject )
Wladimir Palant 2017/05/16 14:21:15 What happens with the write stream if an error occ
hub 2017/05/16 15:49:30 Good idea let's unlink the file.
hub 2017/05/16 15:49:30 If I run eslint on that file I get: 229:46 erro
Wladimir Palant 2017/05/17 10:37:02 You should update eslint-config-eyeo package, supp
hub 2017/05/17 17:25:02 Acknowledged.
+ .on("close", () => {
+ writable.close();
+ unzipChromium(archive, chromiumDir).catch(reject);
Wladimir Palant 2017/05/16 14:21:15 Where is resolve being called here?
hub 2017/05/16 15:49:30 Not sure what happens here because it works, but I
+ });
}).on("error", reject);
Wladimir Palant 2017/05/16 14:21:15 Given how complicated the download logic gets, I t
});
}
+function unzipChromium(archive, chromiumDir)
Wladimir Palant 2017/05/16 14:21:15 Nit: this is a generic function, so name and param
hub 2017/05/16 15:49:30 Done.
+{
+ return new Promise((resolve, reject) =>
+ {
+ let unzipper = new DecompressZip(archive);
+ unzipper.on('extract', () => {
+ resolve(getChromiumExecutable(chromiumDir));
Wladimir Palant 2017/05/16 14:21:15 The getChromiumExecutable() call should stay in en
hub 2017/05/16 15:49:30 I'll resolve the promise with the destination path
Wladimir Palant 2017/05/17 10:37:02 This doesn't make much sense either - the destinat
hub 2017/05/17 17:25:02 Done.
+ });
+ unzipper.on('error', reject);
+ unzipper.extract({path: chromiumDir});
+ });
+}
+
function startChromium(chromiumPath)
{
fs.chmodSync(chromiumPath, fs.constants.S_IRWXU);
let dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "chromium-data"));
let child = null;
return {
kill: () => child && child.kill(),
« no previous file with comments | « no previous file | package.json » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld