Issue 5230 - Properly download Chromium on macOS

chromium_process.js

Index: chromium_process.js
===================================================================
--- a/chromium_process.js
+++ b/chromium_process.js
@@ -20,19 +20,20 @@
 
 "use strict";
 
 const childProcess = require("child_process");
 const fs = require("fs");
 const https = require("https");
 const os = require("os");
 const path = require("path");
+const process = require("process");

[Wladimir Palant, 2017/05/18 11:18:27]

You don't need this require, process is a global variable for all Node modules
(see https://nodejs.org/api/globals.html#globals_process).

[hub, 2017/05/18 13:22:37]

Acknowledged.

 
 const remoteInterface = require("chrome-remote-interface");
-const unzip = require("unzip");
+const extractZip = require("extract-zip");
 
 const CHROMIUM_REVISION = 467222;
 
 function rmdir(dirPath)
 {
   for (let file of fs.readdirSync(dirPath))
   {
     let filePath = path.join(dirPath, file);
@@ -88,41 +89,96 @@
   };
 
   if (!buildTypes.hasOwnProperty(platform))
   {
     let err = new Error(`Cannot run browser tests, ${platform} is unsupported`);
     return Promise.reject(err);
   }
 
-  let chromiumDir = path.join(__dirname, "chromium-snapshots",
-                              `chromium-${platform}-${CHROMIUM_REVISION}`);
-  if (fs.existsSync(chromiumDir))
-    return Promise.resolve(getChromiumExecutable(chromiumDir));
+
+  return new Promise((resolve, reject) =>

[Wladimir Palant, 2017/05/18 11:18:28]

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));

[hub, 2017/05/18 13:22:37]

Done.

+  {
+    const snapshotsDir = path.join(__dirname, "chromium-snapshots");
+    const chromiumDir = path.join(snapshotsDir,
+                                `chromium-${platform}-${CHROMIUM_REVISION}`);
+    if (fs.existsSync(chromiumDir))
+      resolve(chromiumDir);
+    else
+    {
+      if (!fs.existsSync(path.dirname(chromiumDir)))
+        fs.mkdirSync(path.dirname(chromiumDir));
 
-  if (!fs.existsSync(path.dirname(chromiumDir)))
-    fs.mkdirSync(path.dirname(chromiumDir));
+      let [dir, fileName] = buildTypes[platform];
+      const archive = path.join(snapshotsDir, "download-cache",
+                              `${CHROMIUM_REVISION}-${fileName}`);
+      const url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;

[Wladimir Palant, 2017/05/18 11:18:28]

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

[hub, 2017/05/18 13:22:37]

I ended up using const like in C++ which is not exactly what it means in ES6.
Will change back. Sorry about that.

+
+      downloadCache(url, archive)
+        .then(() => unzipArchive(archive, chromiumDir))
+        .then(() => resolve(chromiumDir));
+    }
+  }).then(dir => getChromiumExecutable(dir));
+}
+
+function downloadCache(url, destArchive)

[Wladimir Palant, 2017/05/18 11:18:27]

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));

[hub, 2017/05/18 13:22:37]

fixing it.

+{
   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`;
-    https.get(url, response =>
+    if (fs.existsSync(destArchive))
+    {
+      console.info(`Reusing cached archive ${destArchive}`);
+      resolve();
+    }
+    else
     {
-      if (response.statusCode != 200)
+      console.info("Downloading Chromium...");
+      let cacheDir = path.dirname(destArchive);
+      if (!fs.existsSync(cacheDir))
+        fs.mkdirSync(cacheDir);
+      let tempDest = destArchive + "-" + process.pid;
+      let writable = fs.createWriteStream(tempDest);
+
+      https.get(url, response =>
       {
-        reject(new Error(`Unexpected server response: ${response.statusCode}`));
-        response.resume();
-        return;
-      }
+        if (response.statusCode != 200)
+        {
+          reject(new Error(`Unexpected server response: ${response.statusCode}`));

[Wladimir Palant, 2017/05/18 11:18:28]

I'm pretty sure that ESLint dislikes the length of that line.

[hub, 2017/05/18 13:22:37]

it does. Fixing.

+          response.resume();
+          return;
+        }
 
-      response.pipe(unzip.Extract({path: chromiumDir}))
-              .on("error", reject)
-              .on("close", () => resolve(getChromiumExecutable(chromiumDir)));
-    }).on("error", reject);
+        response.pipe(writable)
+                .on("error", error =>
+                {
+                  fs.unlinkSync(tempDest);
+                  reject(error);
+                })
+                .on("close", () =>
+                {
+                  writable.close();
+                  fs.renameSync(tempDest, destArchive);
+                  resolve();
+                });
+      }).on("error", reject);
+    }
+  });
+}
+
+function unzipArchive(archive, destDir)
+{
+  return new Promise((resolve, reject) =>
+  {
+    extractZip(archive, {dir: destDir}, err =>
+    {
+      if (err)
+        reject(err);
+      else
+        resolve();
+    });
   });
 }
 
 function startChromium(chromiumPath)
 {
   fs.chmodSync(chromiumPath, fs.constants.S_IRWXU);
 
   let dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "chromium-data"));