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

Side by Side 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.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View unified diff | Download patch
« no previous file with comments | « no previous file | package.json » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2017 eyeo GmbH 3 * Copyright (C) 2006-2017 eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12 * GNU General Public License for more details. 12 * GNU General Public License for more details.
13 * 13 *
14 * You should have received a copy of the GNU General Public License 14 * You should have received a copy of the GNU General Public License
15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. 15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
16 */ 16 */
17 17
18 /* eslint-env node */ 18 /* eslint-env node */
19 /* eslint no-console: "off" */ 19 /* eslint no-console: "off" */
20 20
21 "use strict"; 21 "use strict";
22 22
23 const childProcess = require("child_process"); 23 const childProcess = require("child_process");
24 const fs = require("fs"); 24 const fs = require("fs");
25 const https = require("https"); 25 const https = require("https");
26 const os = require("os"); 26 const os = require("os");
27 const path = require("path"); 27 const path = require("path");
28 28
29 const remoteInterface = require("chrome-remote-interface"); 29 const remoteInterface = require("chrome-remote-interface");
30 const unzip = require("unzip"); 30 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.
31 31
32 const CHROMIUM_REVISION = 467222; 32 const CHROMIUM_REVISION = 467222;
33 33
34 function rmdir(dirPath) 34 function rmdir(dirPath)
35 { 35 {
36 for (let file of fs.readdirSync(dirPath)) 36 for (let file of fs.readdirSync(dirPath))
37 { 37 {
38 let filePath = path.join(dirPath, file); 38 let filePath = path.join(dirPath, file);
39 try 39 try
40 { 40 {
(...skipping 45 matching lines...) Expand 10 before | Expand all | Expand 10 after
86 "linux": ["Linux_x64", "chrome-linux.zip"], 86 "linux": ["Linux_x64", "chrome-linux.zip"],
87 "darwin": ["Mac", "chrome-mac.zip"] 87 "darwin": ["Mac", "chrome-mac.zip"]
88 }; 88 };
89 89
90 if (!buildTypes.hasOwnProperty(platform)) 90 if (!buildTypes.hasOwnProperty(platform))
91 { 91 {
92 let err = new Error(`Cannot run browser tests, ${platform} is unsupported`); 92 let err = new Error(`Cannot run browser tests, ${platform} is unsupported`);
93 return Promise.reject(err); 93 return Promise.reject(err);
94 } 94 }
95 95
96 let chromiumDownloadCache = path.join(__dirname, "chromium-snapshots",
97 "download-cache");
96 let chromiumDir = path.join(__dirname, "chromium-snapshots", 98 let chromiumDir = path.join(__dirname, "chromium-snapshots",
97 `chromium-${platform}-${CHROMIUM_REVISION}`); 99 `chromium-${platform}-${CHROMIUM_REVISION}`);
98 if (fs.existsSync(chromiumDir)) 100 if (fs.existsSync(chromiumDir))
99 return Promise.resolve(getChromiumExecutable(chromiumDir)); 101 return Promise.resolve(getChromiumExecutable(chromiumDir));
100 102
101 if (!fs.existsSync(path.dirname(chromiumDir))) 103 if (!fs.existsSync(path.dirname(chromiumDir)))
102 fs.mkdirSync(path.dirname(chromiumDir)); 104 fs.mkdirSync(path.dirname(chromiumDir));
105
106 let [dir, fileName] = buildTypes[platform];
107 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.
108 `${CHROMIUM_REVISION}-${fileName}`);
109 if (fs.existsSync(archive))
110 {
111 console.info(`Reusing cached archive ${archive}`);
112 return unzipChromium(archive, chromiumDir);
113 }
103 return new Promise((resolve, reject) => 114 return new Promise((resolve, reject) =>
104 { 115 {
105 console.info("Downloading Chromium..."); 116 console.info("Downloading Chromium...");
106 let [dir, fileName] = buildTypes[platform];
107 let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser -snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`; 117 let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser -snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;
118
119 if (!fs.existsSync(chromiumDownloadCache))
120 fs.mkdirSync(chromiumDownloadCache);
121 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.
122
108 https.get(url, response => 123 https.get(url, response =>
109 { 124 {
110 if (response.statusCode != 200) 125 if (response.statusCode != 200)
111 { 126 {
112 reject(new Error(`Unexpected server response: ${response.statusCode}`)); 127 reject(new Error(`Unexpected server response: ${response.statusCode}`));
113 response.resume(); 128 response.resume();
114 return; 129 return;
115 } 130 }
116 131
117 response.pipe(unzip.Extract({path: chromiumDir})) 132 response.pipe(writable)
118 .on("error", reject) 133 .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.
119 .on("close", () => resolve(getChromiumExecutable(chromiumDir))); 134 .on("close", () => {
135 writable.close();
136 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
137 });
120 }).on("error", reject); 138 }).on("error", reject);
Wladimir Palant 2017/05/16 14:21:15 Given how complicated the download logic gets, I t
121 }); 139 });
122 } 140 }
123 141
142 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.
143 {
144 return new Promise((resolve, reject) =>
145 {
146 let unzipper = new DecompressZip(archive);
147 unzipper.on('extract', () => {
148 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.
149 });
150 unzipper.on('error', reject);
151 unzipper.extract({path: chromiumDir});
152 });
153 }
154
124 function startChromium(chromiumPath) 155 function startChromium(chromiumPath)
125 { 156 {
126 fs.chmodSync(chromiumPath, fs.constants.S_IRWXU); 157 fs.chmodSync(chromiumPath, fs.constants.S_IRWXU);
127 158
128 let dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "chromium-data")); 159 let dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "chromium-data"));
129 let child = null; 160 let child = null;
130 return { 161 return {
131 kill: () => child && child.kill(), 162 kill: () => child && child.kill(),
132 done: new Promise((resolve, reject) => 163 done: new Promise((resolve, reject) =>
133 { 164 {
(...skipping 129 matching lines...) Expand 10 before | Expand all | Expand 10 after
263 { 294 {
264 child.kill(); 295 child.kill();
265 return result; 296 return result;
266 }).catch(error => 297 }).catch(error =>
267 { 298 {
268 child.kill(); 299 child.kill();
269 throw error; 300 throw error;
270 }); 301 });
271 }); 302 });
272 }; 303 };
OLDNEW
« no previous file with comments | « no previous file | package.json » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld