Issue 5230 - Properly download Chromium on macOS

chromium_process.js

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 on macOS. Also I don't stream
so one can cache the archive.

[Wladimir Palant, 2017/05/16 14:21:15]

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.

[hub, 2017/05/16 15:49:30]

yauzl doesn't have an easy "extract archive API". There is "extract-zip" for
that https://www.npmjs.com/package/extract-zip.

[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 aren't inherently constant
but merely aren't being changed by the current implementation.

[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 const.

[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 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.

[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  error  Parsing error: Unexpected token client

it seems to not understand the async keyword. So it never catches the error
above.

[Wladimir Palant, 2017/05/17 10:37:02]

You should update eslint-config-eyeo package, support for ECMAScript 2017
features has been enabled.

[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'll make sure it called.

+              });
     }).on("error", reject);

[Wladimir Palant, 2017/05/16 14:21:15]

Given how complicated the download logic gets, I think it would help to wrap it
in a generic helper returning a promise.

   });
 }
 
+function unzipChromium(archive, chromiumDir)

[Wladimir Palant, 2017/05/16 14:21:15]

Nit: this is a generic function, so name and parameters should be generic as
well. How about unzipArchive(archive, targetDir)?

[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 ensureChromium(), the function
here should merely do what its name says - unzip, then resolve promise.

[hub, 2017/05/16 15:49:30]

I'll resolve the promise with the destination path and call ensureChromium()
above in the resolution of the unzipArchive

[Wladimir Palant, 2017/05/17 10:37:02]

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.

[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(),