From 6f90faff405ee971876d2b8d20795813979e976a Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Sun, 13 Sep 2020 12:01:02 -0400
Subject: [PATCH 1/8] feat: replace install.js internals with make-fetch

Migrate the use of deprecated `requests` to `make-fetch-happen`. Replaces a lot
of callbacks with async/await or promises.
---
 lib/install.js | 386 ++++++++++++++++++++-----------------------------
 package.json   |   2 +-
 2 files changed, 157 insertions(+), 231 deletions(-)

diff --git a/lib/install.js b/lib/install.js
index f9fa2b34bd..ad98e0471f 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -7,13 +7,18 @@ const path = require('path')
 const crypto = require('crypto')
 const log = require('npmlog')
 const semver = require('semver')
-const request = require('request')
+const fetch = require('make-fetch-happen')
 const processRelease = require('./process-release')
 const win = process.platform === 'win32'
 const getProxyFromURI = require('./proxy')
+const streamPipeline = require('util').promisify(require('stream').pipeline)
+
+/**
+ * @param {typeof import('graceful-fs')} fs
+ */
 
 function install (fs, gyp, argv, callback) {
-  var release = processRelease(argv, gyp, process.version, process.release)
+  const release = processRelease(argv, gyp, process.version, process.release)
 
   // ensure no double-callbacks happen
   function cb (err) {
@@ -60,7 +65,7 @@ function install (fs, gyp, argv, callback) {
   log.verbose('install', 'installing version: %s', release.versionDir)
 
   // the directory where the dev files will be installed
-  var devDir = path.resolve(gyp.devDir, release.versionDir)
+  const devDir = path.resolve(gyp.devDir, release.versionDir)
 
   // If '--ensure' was passed, then don't *always* install the version;
   // check if it is already installed, and only install when needed
@@ -70,7 +75,7 @@ function install (fs, gyp, argv, callback) {
       if (err) {
         if (err.code === 'ENOENT') {
           log.verbose('install', 'version not already installed, continuing with install', release.version)
-          go()
+          go().then(cb, cb)
         } else if (err.code === 'EACCES') {
           eaccesFallback(err)
         } else {
@@ -79,17 +84,17 @@ function install (fs, gyp, argv, callback) {
         return
       }
       log.verbose('install', 'version is already installed, need to check "installVersion"')
-      var installVersionFile = path.resolve(devDir, 'installVersion')
-      fs.readFile(installVersionFile, 'ascii', function (err, ver) {
+      const installVersionFile = path.resolve(devDir, 'installVersion')
+      fs.readFile(installVersionFile, 'ascii', (err, ver) => {
         if (err && err.code !== 'ENOENT') {
           return cb(err)
         }
-        var installVersion = parseInt(ver, 10) || 0
+        const installVersion = parseInt(ver, 10) || 0
         log.verbose('got "installVersion"', installVersion)
         log.verbose('needs "installVersion"', gyp.package.installVersion)
         if (installVersion < gyp.package.installVersion) {
           log.verbose('install', 'version is no good; reinstalling')
-          go()
+          go().then(cb, cb)
         } else {
           log.verbose('install', 'version is good')
           cb()
@@ -97,11 +102,11 @@ function install (fs, gyp, argv, callback) {
       })
     })
   } else {
-    go()
+    go().then(cb, cb)
   }
 
   function getContentSha (res, callback) {
-    var shasum = crypto.createHash('sha256')
+    const shasum = crypto.createHash('sha256')
     res.on('data', function (chunk) {
       shasum.update(chunk)
     }).on('end', function () {
@@ -109,247 +114,171 @@ function install (fs, gyp, argv, callback) {
     })
   }
 
-  function go () {
+  async function go () {
     log.verbose('ensuring nodedir is created', devDir)
 
     // first create the dir for the node dev files
-    fs.mkdir(devDir, { recursive: true }, function (err, created) {
-      if (err) {
-        if (err.code === 'EACCES') {
-          eaccesFallback(err)
-        } else {
-          cb(err)
-        }
-        return
-      }
+    try {
+      const created = await fs.promises.mkdir(devDir, { recursive: true })
 
       if (created) {
         log.verbose('created nodedir', created)
       }
-
-      // now download the node tarball
-      var tarPath = gyp.opts.tarball
-      var badDownload = false
-      var extractCount = 0
-      var contentShasums = {}
-      var expectShasums = {}
-
-      // checks if a file to be extracted from the tarball is valid.
-      // only .h header files and the gyp files get extracted
-      function isValid (path) {
-        var isValid = valid(path)
-        if (isValid) {
-          log.verbose('extracted file from tarball', path)
-          extractCount++
-        } else {
-          // invalid
-          log.silly('ignoring from tarball', path)
-        }
-        return isValid
+    } catch (err) {
+      if (err.code === 'EACCES') {
+        eaccesFallback(err)
+        return
       }
 
-      // download the tarball and extract!
-      if (tarPath) {
-        return tar.extract({
-          file: tarPath,
-          strip: 1,
-          filter: isValid,
-          cwd: devDir
-        }).then(afterTarball, cb)
-      }
+      throw err
+    }
 
-      try {
-        var req = download(gyp, process.env, release.tarballUrl)
-      } catch (e) {
-        return cb(e)
+    // now download the node tarball
+    const tarPath = gyp.opts.tarball
+    let extractCount = 0
+    const contentShasums = {}
+    const expectShasums = {}
+
+    // checks if a file to be extracted from the tarball is valid.
+    // only .h header files and the gyp files get extracted
+    function isValid (path) {
+      const isValid = valid(path)
+      if (isValid) {
+        log.verbose('extracted file from tarball', path)
+        extractCount++
+      } else {
+        // invalid
+        log.silly('ignoring from tarball', path)
       }
+      return isValid
+    }
 
-      // something went wrong downloading the tarball?
-      req.on('error', function (err) {
-        if (err.code === 'ENOTFOUND') {
-          return cb(new Error('This is most likely not a problem with node-gyp or the package itself and\n' +
-            'is related to network connectivity. In most cases you are behind a proxy or have bad \n' +
-            'network settings.'))
-        }
-        badDownload = true
-        cb(err)
-      })
+    // download the tarball and extract!
 
-      req.on('close', function () {
-        if (extractCount === 0) {
-          cb(new Error('Connection closed while downloading tarball file'))
-        }
+    if (tarPath) {
+      await tar.extract({
+        file: tarPath,
+        strip: 1,
+        filter: isValid,
+        cwd: devDir
       })
+    } else {
+      try {
+        const res = await download(gyp, process.env, release.tarballUrl)
 
-      req.on('response', function (res) {
-        if (res.statusCode !== 200) {
-          badDownload = true
-          cb(new Error(res.statusCode + ' response downloading ' + release.tarballUrl))
-          return
+        if (res.status !== 200) {
+          throw new Error(`${res.status} response downloading ${release.tarballUrl}`)
         }
+
         // content checksum
-        getContentSha(res, function (_, checksum) {
-          var filename = path.basename(release.tarballUrl).trim()
+        getContentSha(res.body, (_, checksum) => {
+          const filename = path.basename(release.tarballUrl).trim()
           contentShasums[filename] = checksum
           log.verbose('content checksum', filename, checksum)
         })
 
-        // start unzipping and untaring
-        res.pipe(tar.extract({
+        await streamPipeline(res.body, tar.extract({
           strip: 1,
           cwd: devDir,
           filter: isValid
-        }).on('close', afterTarball).on('error', cb))
-      })
-
-      // invoked after the tarball has finished being extracted
-      function afterTarball () {
-        if (badDownload) {
-          return
-        }
-        if (extractCount === 0) {
-          return cb(new Error('There was a fatal problem while downloading/extracting the tarball'))
+        }))
+      } catch (err) {
+        // something went wrong downloading the tarball?
+        if (err.code === 'ENOTFOUND') {
+          throw new Error('This is most likely not a problem with node-gyp or the package itself and\n' +
+            'is related to network connectivity. In most cases you are behind a proxy or have bad \n' +
+            'network settings.')
         }
-        log.verbose('tarball', 'done parsing tarball')
-        var async = 0
+        throw err
+      }
+    }
 
-        if (win) {
-          // need to download node.lib
-          async++
-          downloadNodeLib(deref)
-        }
+    // invoked after the tarball has finished being extracted
+    if (extractCount === 0) {
+      throw new Error('There was a fatal problem while downloading/extracting the tarball')
+    }
 
-        // write the "installVersion" file
-        async++
-        var installVersionPath = path.resolve(devDir, 'installVersion')
-        fs.writeFile(installVersionPath, gyp.package.installVersion + '\n', deref)
+    log.verbose('tarball', 'done parsing tarball')
+
+    const installVersionPath = path.resolve(devDir, 'installVersion')
+    await Promise.all([
+      // need to download node.lib
+      ...(win ? [downloadNodeLib()] : []),
+      // write the "installVersion" file
+      fs.promises.writeFile(installVersionPath, gyp.package.installVersion + '\n'),
+      // Only download SHASUMS.txt if we downloaded something in need of SHA verification
+      ...(!tarPath || win ? [downloadShasums()] : [])
+    ])
+
+    log.verbose('download contents checksum', JSON.stringify(contentShasums))
+    // check content shasums
+    for (const k in contentShasums) {
+      log.verbose('validating download checksum for ' + k, '(%s == %s)', contentShasums[k], expectShasums[k])
+      if (contentShasums[k] !== expectShasums[k]) {
+        throw new Error(k + ' local checksum ' + contentShasums[k] + ' not match remote ' + expectShasums[k])
+      }
+    }
 
-        // Only download SHASUMS.txt if we downloaded something in need of SHA verification
-        if (!tarPath || win) {
-          // download SHASUMS.txt
-          async++
-          downloadShasums(deref)
-        }
+    async function downloadShasums () {
+      log.verbose('check download content checksum, need to download `SHASUMS256.txt`...')
+      log.verbose('checksum url', release.shasumsUrl)
 
-        if (async === 0) {
-          // no async tasks required
-          cb()
-        }
+      const res = await download(gyp, process.env, release.shasumsUrl)
 
-        function deref (err) {
-          if (err) {
-            return cb(err)
-          }
+      if (res.status !== 200) {
+        throw new Error(`${res.status}  status code downloading checksum`)
+      }
 
-          async--
-          if (!async) {
-            log.verbose('download contents checksum', JSON.stringify(contentShasums))
-            // check content shasums
-            for (var k in contentShasums) {
-              log.verbose('validating download checksum for ' + k, '(%s == %s)', contentShasums[k], expectShasums[k])
-              if (contentShasums[k] !== expectShasums[k]) {
-                cb(new Error(k + ' local checksum ' + contentShasums[k] + ' not match remote ' + expectShasums[k]))
-                return
-              }
-            }
-            cb()
-          }
+      for (const line of (await res.text()).trim().split('\n')) {
+        const items = line.trim().split(/\s+/)
+        if (items.length !== 2) {
+          return
         }
+
+        // 0035d18e2dcf9aad669b1c7c07319e17abfe3762  ./node-v0.11.4.tar.gz
+        const name = items[1].replace(/^\.\//, '')
+        expectShasums[name] = items[0]
       }
 
-      function downloadShasums (done) {
-        log.verbose('check download content checksum, need to download `SHASUMS256.txt`...')
-        log.verbose('checksum url', release.shasumsUrl)
-        try {
-          var req = download(gyp, process.env, release.shasumsUrl)
-        } catch (e) {
-          return cb(e)
-        }
+      log.verbose('checksum data', JSON.stringify(expectShasums))
+    }
 
-        req.on('error', done)
-        req.on('response', function (res) {
-          if (res.statusCode !== 200) {
-            done(new Error(res.statusCode + ' status code downloading checksum'))
-            return
+    async function downloadNodeLib () {
+      log.verbose('on Windows; need to download `' + release.name + '.lib`...')
+      const archs = ['ia32', 'x64', 'arm64']
+      return Promise.all(archs.map(async (arch) => {
+        const dir = path.resolve(devDir, arch)
+        const targetLibPath = path.resolve(dir, release.name + '.lib')
+        const {libUrl, libPath} = release[arch]
+        const name = `${arch} ${release.name}.lib`
+        log.verbose(name, 'dir', dir)
+        log.verbose(name, 'url', libUrl)
+
+        await fs.promises.mkdir(dir, { recursive: true })
+        log.verbose('streaming', name, 'to:', targetLibPath)
+
+        const res = await download(gyp, process.env, libUrl)
+
+        if (res.status === 403 || res.status === 404) {
+          if (arch === 'arm64') {
+            // Arm64 is a newer platform on Windows and not all node distributions provide it.
+            log.verbose(`${name} was not found in ${libUrl}`)
+          } else {
+            log.warn(`${name} was not found in ${libUrl}`)
           }
+          return
+        } else if (res.status !== 200) {
+          throw new Error(`${res.status} status code downloading ${name}`)
+        }
 
-          var chunks = []
-          res.on('data', function (chunk) {
-            chunks.push(chunk)
-          })
-          res.on('end', function () {
-            var lines = Buffer.concat(chunks).toString().trim().split('\n')
-            lines.forEach(function (line) {
-              var items = line.trim().split(/\s+/)
-              if (items.length !== 2) {
-                return
-              }
-
-              // 0035d18e2dcf9aad669b1c7c07319e17abfe3762  ./node-v0.11.4.tar.gz
-              var name = items[1].replace(/^\.\//, '')
-              expectShasums[name] = items[0]
-            })
-
-            log.verbose('checksum data', JSON.stringify(expectShasums))
-            done()
-          })
+        getContentSha(res.body, (_, checksum) => {
+          contentShasums[libPath] = checksum
+          log.verbose('content checksum', libPath, checksum)
         })
-      }
 
-      function downloadNodeLib (done) {
-        log.verbose('on Windows; need to download `' + release.name + '.lib`...')
-        var archs = ['ia32', 'x64', 'arm64']
-        var async = archs.length
-        archs.forEach(function (arch) {
-          var dir = path.resolve(devDir, arch)
-          var targetLibPath = path.resolve(dir, release.name + '.lib')
-          var libUrl = release[arch].libUrl
-          var libPath = release[arch].libPath
-          var name = arch + ' ' + release.name + '.lib'
-          log.verbose(name, 'dir', dir)
-          log.verbose(name, 'url', libUrl)
-
-          fs.mkdir(dir, { recursive: true }, function (err) {
-            if (err) {
-              return done(err)
-            }
-            log.verbose('streaming', name, 'to:', targetLibPath)
-
-            try {
-              var req = download(gyp, process.env, libUrl, cb)
-            } catch (e) {
-              return cb(e)
-            }
-
-            req.on('error', done)
-            req.on('response', function (res) {
-              if (res.statusCode === 403 || res.statusCode === 404) {
-                if (arch === 'arm64') {
-                  // Arm64 is a newer platform on Windows and not all node distributions provide it.
-                  log.verbose(`${name} was not found in ${libUrl}`)
-                } else {
-                  log.warn(`${name} was not found in ${libUrl}`)
-                }
-                return
-              } else if (res.statusCode !== 200) {
-                done(new Error(res.statusCode + ' status code downloading ' + name))
-                return
-              }
-
-              getContentSha(res, function (_, checksum) {
-                contentShasums[libPath] = checksum
-                log.verbose('content checksum', libPath, checksum)
-              })
-
-              var ws = fs.createWriteStream(targetLibPath)
-              ws.on('error', cb)
-              req.pipe(ws)
-            })
-            req.on('end', function () { --async || done() })
-          })
-        })
-      } // downloadNodeLib()
-    }) // mkdir()
+        return streamPipeline(res.body, fs.createWriteStream(targetLibPath))
+      }))
+    } // downloadNodeLib()
   } // go()
 
   /**
@@ -358,7 +287,7 @@ function install (fs, gyp, argv, callback) {
 
   function valid (file) {
     // header files
-    var extname = path.extname(file)
+    const extname = path.extname(file)
     return extname === '.h' || extname === '.gypi'
   }
 
@@ -372,13 +301,13 @@ function install (fs, gyp, argv, callback) {
    */
 
   function eaccesFallback (err) {
-    var noretry = '--node_gyp_internal_noretry'
+    const noretry = '--node_gyp_internal_noretry'
     if (argv.indexOf(noretry) !== -1) {
       return cb(err)
     }
-    var tmpdir = os.tmpdir()
+    const tmpdir = os.tmpdir()
     gyp.devDir = path.resolve(tmpdir, '.node-gyp')
-    var userString = ''
+    let userString = ''
     try {
       // os.userInfo can fail on some systems, it's not critical here
       userString = ` ("${os.userInfo().username}")`
@@ -393,24 +322,23 @@ function install (fs, gyp, argv, callback) {
   }
 }
 
-function download (gyp, env, url) {
+async function download (gyp, env, url) {
   log.http('GET', url)
 
-  var requestOpts = {
-    uri: url,
+  const requestOpts = {
     headers: {
       'User-Agent': 'node-gyp v' + gyp.version + ' (node ' + process.version + ')',
       Connection: 'keep-alive'
     }
   }
 
-  var cafile = gyp.opts.cafile
+  const cafile = gyp.opts.cafile
   if (cafile) {
-    requestOpts.ca = readCAFile(cafile)
+    requestOpts.ca = await readCAFile(cafile)
   }
 
   // basic support for a proxy server
-  var proxyUrl = getProxyFromURI(gyp, env, url)
+  const proxyUrl = getProxyFromURI(gyp, env, url)
   if (proxyUrl) {
     if (/^https?:\/\//i.test(proxyUrl)) {
       log.verbose('download', 'using proxy url: "%s"', proxyUrl)
@@ -420,19 +348,17 @@ function download (gyp, env, url) {
     }
   }
 
-  var req = request(requestOpts)
-  req.on('response', function (res) {
-    log.http(res.statusCode, url)
-  })
+  const res = await fetch(url, requestOpts)
+  log.http(res.status, res.url)
 
-  return req
+  return res
 }
 
-function readCAFile (filename) {
+async function readCAFile (filename) {
   // The CA file can contain multiple certificates so split on certificate
   // boundaries.  [\S\s]*? is used to match everything including newlines.
-  var ca = fs.readFileSync(filename, 'utf8')
-  var re = /(-----BEGIN CERTIFICATE-----[\S\s]*?-----END CERTIFICATE-----)/g
+  const ca = await fs.promises.readFile(filename, 'utf8')
+  const re = /(-----BEGIN CERTIFICATE-----[\S\s]*?-----END CERTIFICATE-----)/g
   return ca.match(re)
 }
 
diff --git a/package.json b/package.json
index 8e256f0170..1a4fdf1845 100644
--- a/package.json
+++ b/package.json
@@ -25,9 +25,9 @@
     "env-paths": "^2.2.0",
     "glob": "^7.1.4",
     "graceful-fs": "^4.2.3",
+    "make-fetch-happen": "^8.0.9",
     "nopt": "^5.0.0",
     "npmlog": "^4.1.2",
-    "request": "^2.88.2",
     "rimraf": "^3.0.2",
     "semver": "^7.3.2",
     "tar": "^6.0.2",

From bd0334c129e7e716fcaf534c0a6ddd1daa59c758 Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Sun, 13 Sep 2020 12:36:46 -0400
Subject: [PATCH 2/8] test: fix tests in download.js that need promises

Some tests are calling the recently converted promise apis, this
converts them to use the new format.
---
 test/test-download.js | 196 ++++++++++++++++--------------------------
 1 file changed, 74 insertions(+), 122 deletions(-)

diff --git a/test/test-download.js b/test/test-download.js
index fe373e3280..8535f9f098 100644
--- a/test/test-download.js
+++ b/test/test-download.js
@@ -14,191 +14,143 @@ const log = require('npmlog')
 
 log.level = 'warn'
 
-test('download over http', function (t) {
+test('download over http', (t) => {
   t.plan(2)
 
-  var server = http.createServer(function (req, res) {
-    t.strictEqual(req.headers['user-agent'],
-      'node-gyp v42 (node ' + process.version + ')')
+  const server = http.createServer((req, res) => {
+    t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('ok')
     server.close()
   })
 
-  var host = 'localhost'
-  server.listen(0, host, function () {
-    var port = this.address().port
-    var gyp = {
+  const host = 'localhost'
+  return new Promise(resolve => server.listen(0, host, async () => {
+    const { port } = server.address()
+    const gyp = {
       opts: {},
       version: '42'
     }
-    var url = 'http://' + host + ':' + port
-    var req = install.test.download(gyp, {}, url)
-    req.on('response', function (res) {
-      var body = ''
-      res.setEncoding('utf8')
-      res.on('data', function (data) {
-        body += data
-      })
-      res.on('end', function () {
-        t.strictEqual(body, 'ok')
-      })
-    })
-  })
+    const url = `http://${host}:${port}`
+    const res = await install.test.download(gyp, {}, url)
+    t.strictEqual(await res.text(), 'ok')
+    resolve()
+  }))
 })
 
-test('download over https with custom ca', function (t) {
+test('download over https with custom ca', async (t) => {
   t.plan(3)
 
-  var cert = fs.readFileSync(path.join(__dirname, 'fixtures/server.crt'), 'utf8')
-  var key = fs.readFileSync(path.join(__dirname, 'fixtures/server.key'), 'utf8')
+  const [cert, key] = await Promise.all([
+    fs.promises.readFile(path.join(__dirname, 'fixtures/server.crt'), 'utf8'),
+    fs.promises.readFile(path.join(__dirname, 'fixtures/server.key'), 'utf8')
+  ])
 
-  var cafile = path.join(__dirname, '/fixtures/ca.crt')
-  var ca = install.test.readCAFile(cafile)
+  const cafile = path.join(__dirname, '/fixtures/ca.crt')
+  const ca = await install.test.readCAFile(cafile)
   t.strictEqual(ca.length, 1)
 
-  var options = { ca: ca, cert: cert, key: key }
-  var server = https.createServer(options, function (req, res) {
-    t.strictEqual(req.headers['user-agent'],
-      'node-gyp v42 (node ' + process.version + ')')
+  const options = { ca: ca, cert: cert, key: key }
+  const server = https.createServer(options, (req, res) => {
+    t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('ok')
     server.close()
   })
 
-  server.on('clientError', function (err) {
-    throw err
-  })
+  server.on('clientError', (err) => { throw err })
 
-  var host = 'localhost'
-  server.listen(8000, host, function () {
-    var port = this.address().port
-    var gyp = {
-      opts: { cafile: cafile },
+  const host = 'localhost'
+  return new Promise(resolve => server.listen(0, host, async () => {
+    const { port } = server.address()
+    const gyp = {
+      opts: { cafile },
       version: '42'
     }
-    var url = 'https://' + host + ':' + port
-    var req = install.test.download(gyp, {}, url)
-    req.on('response', function (res) {
-      var body = ''
-      res.setEncoding('utf8')
-      res.on('data', function (data) {
-        body += data
-      })
-      res.on('end', function () {
-        t.strictEqual(body, 'ok')
-      })
-    })
-  })
+    const url = `https://${host}:${port}`
+    const res = await install.test.download(gyp, {}, url)
+    t.strictEqual(await res.text(), 'ok')
+    resolve()
+  }))
 })
 
-test('download over http with proxy', function (t) {
+test('download over http with proxy', (t) => {
   t.plan(2)
 
-  var server = http.createServer(function (req, res) {
-    t.strictEqual(req.headers['user-agent'],
-      'node-gyp v42 (node ' + process.version + ')')
+  const server = http.createServer((_, res) => {
     res.end('ok')
-    pserver.close(function () {
-      server.close()
-    })
+    pserver.close(() => { server.close() })
   })
 
-  var pserver = http.createServer(function (req, res) {
-    t.strictEqual(req.headers['user-agent'],
-      'node-gyp v42 (node ' + process.version + ')')
+  const pserver = http.createServer((req, res) => {
+    t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('proxy ok')
-    server.close(function () {
-      pserver.close()
-    })
+    server.close(() => { pserver.close() })
   })
 
-  var host = 'localhost'
-  server.listen(0, host, function () {
-    var port = this.address().port
-    pserver.listen(port + 1, host, function () {
-      var gyp = {
+  const host = 'localhost'
+  return new Promise(resolve => server.listen(0, host, () => {
+    const { port } = server.address()
+    pserver.listen(port + 1, host, async () => {
+      const gyp = {
         opts: {
-          proxy: 'http://' + host + ':' + (port + 1)
+          proxy: `http://${host}:${port + 1}`
         },
         version: '42'
       }
-      var url = 'http://' + host + ':' + port
-      var req = install.test.download(gyp, {}, url)
-      req.on('response', function (res) {
-        var body = ''
-        res.setEncoding('utf8')
-        res.on('data', function (data) {
-          body += data
-        })
-        res.on('end', function () {
-          t.strictEqual(body, 'proxy ok')
-        })
-      })
+      const url = `http://${host}:${port}`
+      const res = await install.test.download(gyp, {}, url)
+      t.strictEqual(await res.text(), 'proxy ok')
+      resolve()
     })
-  })
+  }))
 })
 
-test('download over http with noproxy', function (t) {
+test('download over http with noproxy', (t) => {
   t.plan(2)
 
-  var server = http.createServer(function (req, res) {
-    t.strictEqual(req.headers['user-agent'],
-      'node-gyp v42 (node ' + process.version + ')')
+  const server = http.createServer((req, res) => {
+    t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('ok')
-    pserver.close(function () {
-      server.close()
-    })
+    pserver.close(() => { server.close() })
   })
 
-  var pserver = http.createServer(function (req, res) {
-    t.strictEqual(req.headers['user-agent'],
-      'node-gyp v42 (node ' + process.version + ')')
+  const pserver = http.createServer((_, res) => {
     res.end('proxy ok')
-    server.close(function () {
-      pserver.close()
-    })
+    server.close(() => { pserver.close() })
   })
 
-  var host = 'localhost'
-  server.listen(0, host, function () {
-    var port = this.address().port
-    pserver.listen(port + 1, host, function () {
-      var gyp = {
+  const host = 'localhost'
+  return new Promise(resolve => server.listen(0, host, () => {
+    const { port } = server.address()
+    pserver.listen(port + 1, host, async () => {
+      const gyp = {
         opts: {
-          proxy: 'http://' + host + ':' + (port + 1),
-          noproxy: 'localhost'
+          proxy: `http://${host}:${port + 1}`,
+          noproxy: host
         },
         version: '42'
       }
-      var url = 'http://' + host + ':' + port
-      var req = install.test.download(gyp, {}, url)
-      req.on('response', function (res) {
-        var body = ''
-        res.setEncoding('utf8')
-        res.on('data', function (data) {
-          body += data
-        })
-        res.on('end', function () {
-          t.strictEqual(body, 'ok')
-        })
-      })
+      const url = `http://${host}:${port}`
+      const res = await install.test.download(gyp, {}, url)
+      t.strictEqual(await res.text(), 'ok')
+      resolve()
     })
-  })
+  }))
 })
 
-test('download with missing cafile', function (t) {
+test('download with missing cafile', async (t) => {
   t.plan(1)
-  var gyp = {
+  const gyp = {
     opts: { cafile: 'no.such.file' }
   }
   try {
-    install.test.download(gyp, {}, 'http://bad/')
+    await install.test.download(gyp, {}, 'http://bad/')
   } catch (e) {
     t.ok(/no.such.file/.test(e.message))
   }
 })
 
-test('check certificate splitting', function (t) {
-  var cas = install.test.readCAFile(path.join(__dirname, 'fixtures/ca-bundle.crt'))
+test('check certificate splitting', async (t) => {
+  const cas = await install.test.readCAFile(path.join(__dirname, 'fixtures/ca-bundle.crt'))
   t.plan(2)
   t.strictEqual(cas.length, 2)
   t.notStrictEqual(cas[0], cas[1])
@@ -206,7 +158,7 @@ test('check certificate splitting', function (t) {
 
 // only run this test if we are running a version of Node with predictable version path behavior
 
-test('download headers (actual)', function (t) {
+test('download headers (actual)', async (t) => {
   if (process.env.FAST_TEST ||
       process.release.name !== 'node' ||
       semver.prerelease(process.version) !== null ||

From c73eb8c910c0070a9b222e6ece5168691d69dff1 Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Sun, 13 Sep 2020 15:43:31 -0400
Subject: [PATCH 3/8] fix: replace pipe listeners with transform stream

Because listeners were placed on the response body, it caused the data
to be consumed prematurely, hence using a stream Transform, no data is
consumed.
---
 lib/install.js        | 68 ++++++++++++++++++++++++++-----------------
 test/test-download.js |  2 +-
 2 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/lib/install.js b/lib/install.js
index ad98e0471f..2865039f85 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -4,6 +4,7 @@ const fs = require('graceful-fs')
 const os = require('os')
 const tar = require('tar')
 const path = require('path')
+const stream = require('stream')
 const crypto = require('crypto')
 const log = require('npmlog')
 const semver = require('semver')
@@ -11,7 +12,7 @@ const fetch = require('make-fetch-happen')
 const processRelease = require('./process-release')
 const win = process.platform === 'win32'
 const getProxyFromURI = require('./proxy')
-const streamPipeline = require('util').promisify(require('stream').pipeline)
+const streamPipeline = require('util').promisify(stream.pipeline)
 
 /**
  * @param {typeof import('graceful-fs')} fs
@@ -105,13 +106,22 @@ function install (fs, gyp, argv, callback) {
     go().then(cb, cb)
   }
 
-  function getContentSha (res, callback) {
-    const shasum = crypto.createHash('sha256')
-    res.on('data', function (chunk) {
-      shasum.update(chunk)
-    }).on('end', function () {
-      callback(null, shasum.digest('hex'))
-    })
+  class ShaSum extends stream.Transform {
+    constructor (callback) {
+      super()
+      this._callback = callback
+      this._digester = crypto.createHash('sha256')
+    }
+
+    _transform (chunk, _, callback) {
+      this._digester.update(chunk)
+      callback(null, chunk)
+    }
+
+    _flush (callback) {
+      this._callback(null, this._digester.digest('hex'))
+      callback()
+    }
   }
 
   async function go () {
@@ -170,18 +180,20 @@ function install (fs, gyp, argv, callback) {
           throw new Error(`${res.status} response downloading ${release.tarballUrl}`)
         }
 
-        // content checksum
-        getContentSha(res.body, (_, checksum) => {
-          const filename = path.basename(release.tarballUrl).trim()
-          contentShasums[filename] = checksum
-          log.verbose('content checksum', filename, checksum)
-        })
-
-        await streamPipeline(res.body, tar.extract({
-          strip: 1,
-          cwd: devDir,
-          filter: isValid
-        }))
+        await streamPipeline(
+          res.body,
+          // content checksum
+          new ShaSum((_, checksum) => {
+            const filename = path.basename(release.tarballUrl).trim()
+            contentShasums[filename] = checksum
+            log.verbose('content checksum', filename, checksum)
+          }),
+          tar.extract({
+            strip: 1,
+            cwd: devDir,
+            filter: isValid
+          })
+        )
       } catch (err) {
         // something went wrong downloading the tarball?
         if (err.code === 'ENOTFOUND') {
@@ -249,7 +261,7 @@ function install (fs, gyp, argv, callback) {
       return Promise.all(archs.map(async (arch) => {
         const dir = path.resolve(devDir, arch)
         const targetLibPath = path.resolve(dir, release.name + '.lib')
-        const {libUrl, libPath} = release[arch]
+        const { libUrl, libPath } = release[arch]
         const name = `${arch} ${release.name}.lib`
         log.verbose(name, 'dir', dir)
         log.verbose(name, 'url', libUrl)
@@ -271,12 +283,14 @@ function install (fs, gyp, argv, callback) {
           throw new Error(`${res.status} status code downloading ${name}`)
         }
 
-        getContentSha(res.body, (_, checksum) => {
-          contentShasums[libPath] = checksum
-          log.verbose('content checksum', libPath, checksum)
-        })
-
-        return streamPipeline(res.body, fs.createWriteStream(targetLibPath))
+        return streamPipeline(
+          res.body,
+          new ShaSum((_, checksum) => {
+            contentShasums[libPath] = checksum
+            log.verbose('content checksum', libPath, checksum)
+          }),
+          fs.createWriteStream(targetLibPath)
+        )
       }))
     } // downloadNodeLib()
   } // go()
diff --git a/test/test-download.js b/test/test-download.js
index 8535f9f098..966aefc445 100644
--- a/test/test-download.js
+++ b/test/test-download.js
@@ -158,7 +158,7 @@ test('check certificate splitting', async (t) => {
 
 // only run this test if we are running a version of Node with predictable version path behavior
 
-test('download headers (actual)', async (t) => {
+test('download headers (actual)', (t) => {
   if (process.env.FAST_TEST ||
       process.release.name !== 'node' ||
       semver.prerelease(process.version) !== null ||

From 36467d2d41d1f626aebc911089d454cf466cdb4e Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Sun, 13 Sep 2020 16:25:15 -0400
Subject: [PATCH 4/8] feat: remove proxy support code

make-fetch-happen already has proxy support
---
 lib/install.js | 18 +++-------
 lib/proxy.js   | 92 --------------------------------------------------
 2 files changed, 4 insertions(+), 106 deletions(-)
 delete mode 100644 lib/proxy.js

diff --git a/lib/install.js b/lib/install.js
index 2865039f85..1fe044ba40 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -11,7 +11,6 @@ const semver = require('semver')
 const fetch = require('make-fetch-happen')
 const processRelease = require('./process-release')
 const win = process.platform === 'win32'
-const getProxyFromURI = require('./proxy')
 const streamPipeline = require('util').promisify(stream.pipeline)
 
 /**
@@ -341,9 +340,11 @@ async function download (gyp, env, url) {
 
   const requestOpts = {
     headers: {
-      'User-Agent': 'node-gyp v' + gyp.version + ' (node ' + process.version + ')',
+      'User-Agent': `node-gyp v${gyp.version} (node ${process.version})`,
       Connection: 'keep-alive'
-    }
+    },
+    proxy: gyp.opts.proxy,
+    noProxy: gyp.opts.noproxy
   }
 
   const cafile = gyp.opts.cafile
@@ -351,17 +352,6 @@ async function download (gyp, env, url) {
     requestOpts.ca = await readCAFile(cafile)
   }
 
-  // basic support for a proxy server
-  const proxyUrl = getProxyFromURI(gyp, env, url)
-  if (proxyUrl) {
-    if (/^https?:\/\//i.test(proxyUrl)) {
-      log.verbose('download', 'using proxy url: "%s"', proxyUrl)
-      requestOpts.proxy = proxyUrl
-    } else {
-      log.warn('download', 'ignoring invalid "proxy" config setting: "%s"', proxyUrl)
-    }
-  }
-
   const res = await fetch(url, requestOpts)
   log.http(res.status, res.url)
 
diff --git a/lib/proxy.js b/lib/proxy.js
deleted file mode 100644
index 92d9ed2f7f..0000000000
--- a/lib/proxy.js
+++ /dev/null
@@ -1,92 +0,0 @@
-'use strict'
-// Taken from https://github.com/request/request/blob/212570b/lib/getProxyFromURI.js
-
-const url = require('url')
-
-function formatHostname (hostname) {
-  // canonicalize the hostname, so that 'oogle.com' won't match 'google.com'
-  return hostname.replace(/^\.*/, '.').toLowerCase()
-}
-
-function parseNoProxyZone (zone) {
-  zone = zone.trim().toLowerCase()
-
-  var zoneParts = zone.split(':', 2)
-  var zoneHost = formatHostname(zoneParts[0])
-  var zonePort = zoneParts[1]
-  var hasPort = zone.indexOf(':') > -1
-
-  return { hostname: zoneHost, port: zonePort, hasPort: hasPort }
-}
-
-function uriInNoProxy (uri, noProxy) {
-  var port = uri.port || (uri.protocol === 'https:' ? '443' : '80')
-  var hostname = formatHostname(uri.hostname)
-  var noProxyList = noProxy.split(',')
-
-  // iterate through the noProxyList until it finds a match.
-  return noProxyList.map(parseNoProxyZone).some(function (noProxyZone) {
-    var isMatchedAt = hostname.indexOf(noProxyZone.hostname)
-    var hostnameMatched = (
-      isMatchedAt > -1 &&
-        (isMatchedAt === hostname.length - noProxyZone.hostname.length)
-    )
-
-    if (noProxyZone.hasPort) {
-      return (port === noProxyZone.port) && hostnameMatched
-    }
-
-    return hostnameMatched
-  })
-}
-
-function getProxyFromURI (gyp, env, uri) {
-  // If a string URI/URL was given, parse it into a URL object
-  if (typeof uri === 'string') {
-    // eslint-disable-next-line
-    uri = url.parse(uri)
-  }
-
-  // Decide the proper request proxy to use based on the request URI object and the
-  // environmental variables (NO_PROXY, HTTP_PROXY, etc.)
-  // respect NO_PROXY environment variables (see: https://lynx.invisible-island.net/lynx2.8.7/breakout/lynx_help/keystrokes/environments.html)
-
-  var noProxy = gyp.opts.noproxy || env.NO_PROXY || env.no_proxy || env.npm_config_noproxy || ''
-
-  // if the noProxy is a wildcard then return null
-
-  if (noProxy === '*') {
-    return null
-  }
-
-  // if the noProxy is not empty and the uri is found return null
-
-  if (noProxy !== '' && uriInNoProxy(uri, noProxy)) {
-    return null
-  }
-
-  // Check for HTTP or HTTPS Proxy in environment Else default to null
-
-  if (uri.protocol === 'http:') {
-    return gyp.opts.proxy ||
-      env.HTTP_PROXY ||
-      env.http_proxy ||
-      env.npm_config_proxy || null
-  }
-
-  if (uri.protocol === 'https:') {
-    return gyp.opts.proxy ||
-      env.HTTPS_PROXY ||
-      env.https_proxy ||
-      env.HTTP_PROXY ||
-      env.http_proxy ||
-      env.npm_config_proxy || null
-  }
-
-  // if none of that works, return null
-  // (What uri protocol are you using then?)
-
-  return null
-}
-
-module.exports = getProxyFromURI

From 4357d61f7f0ad8eb1b8bb1bd0b77f5e4895e9405 Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Mon, 14 Sep 2020 07:55:56 -0400
Subject: [PATCH 5/8] feat: install is completely promisified

Due to a posibility that callbacks are called twice on error, the
install function is now internally completely promisified.
---
 lib/install.js        | 167 +++++++++++++++++++++---------------------
 test/test-download.js | 102 ++++++++++++--------------
 test/test-install.js  |  54 ++++++++------
 3 files changed, 161 insertions(+), 162 deletions(-)

diff --git a/lib/install.js b/lib/install.js
index 1fe044ba40..15a58cf74b 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -4,6 +4,7 @@ const fs = require('graceful-fs')
 const os = require('os')
 const tar = require('tar')
 const path = require('path')
+const util = require('util')
 const stream = require('stream')
 const crypto = require('crypto')
 const log = require('npmlog')
@@ -11,53 +12,34 @@ const semver = require('semver')
 const fetch = require('make-fetch-happen')
 const processRelease = require('./process-release')
 const win = process.platform === 'win32'
-const streamPipeline = require('util').promisify(stream.pipeline)
+const streamPipeline = util.promisify(stream.pipeline)
 
 /**
  * @param {typeof import('graceful-fs')} fs
  */
 
-function install (fs, gyp, argv, callback) {
+async function install (fs, gyp, argv) {
   const release = processRelease(argv, gyp, process.version, process.release)
 
-  // ensure no double-callbacks happen
-  function cb (err) {
-    if (cb.done) {
-      return
-    }
-    cb.done = true
-    if (err) {
-      log.warn('install', 'got an error, rolling back install')
-      // roll-back the install if anything went wrong
-      gyp.commands.remove([release.versionDir], function () {
-        callback(err)
-      })
-    } else {
-      callback(null, release.version)
-    }
-  }
-
   // Determine which node dev files version we are installing
   log.verbose('install', 'input version string %j', release.version)
 
   if (!release.semver) {
     // could not parse the version string with semver
-    return callback(new Error('Invalid version number: ' + release.version))
+    throw new Error('Invalid version number: ' + release.version)
   }
 
   if (semver.lt(release.version, '0.8.0')) {
-    return callback(new Error('Minimum target version is `0.8.0` or greater. Got: ' + release.version))
+    throw new Error('Minimum target version is `0.8.0` or greater. Got: ' + release.version)
   }
 
   // 0.x.y-pre versions are not published yet and cannot be installed. Bail.
   if (release.semver.prerelease[0] === 'pre') {
     log.verbose('detected "pre" node version', release.version)
-    if (gyp.opts.nodedir) {
-      log.verbose('--nodedir flag was passed; skipping install', gyp.opts.nodedir)
-      callback()
-    } else {
-      callback(new Error('"pre" versions of node cannot be installed, use the --nodedir flag instead'))
+    if (!gyp.opts.nodedir) {
+      throw new Error('"pre" versions of node cannot be installed, use the --nodedir flag instead')
     }
+    log.verbose('--nodedir flag was passed; skipping install', gyp.opts.nodedir)
     return
   }
 
@@ -71,55 +53,48 @@ function install (fs, gyp, argv, callback) {
   // check if it is already installed, and only install when needed
   if (gyp.opts.ensure) {
     log.verbose('install', '--ensure was passed, so won\'t reinstall if already installed')
-    fs.stat(devDir, function (err) {
-      if (err) {
-        if (err.code === 'ENOENT') {
-          log.verbose('install', 'version not already installed, continuing with install', release.version)
-          go().then(cb, cb)
-        } else if (err.code === 'EACCES') {
-          eaccesFallback(err)
-        } else {
-          cb(err)
+    try {
+      await fs.promises.stat(devDir)
+    } catch (err) {
+      if (err.code === 'ENOENT') {
+        log.verbose('install', 'version not already installed, continuing with install', release.version)
+        try {
+          return await go()
+        } catch (err) {
+          return rollback(err)
         }
-        return
+      } else if (err.code === 'EACCES') {
+        return eaccesFallback(err)
       }
-      log.verbose('install', 'version is already installed, need to check "installVersion"')
-      const installVersionFile = path.resolve(devDir, 'installVersion')
-      fs.readFile(installVersionFile, 'ascii', (err, ver) => {
-        if (err && err.code !== 'ENOENT') {
-          return cb(err)
-        }
-        const installVersion = parseInt(ver, 10) || 0
-        log.verbose('got "installVersion"', installVersion)
-        log.verbose('needs "installVersion"', gyp.package.installVersion)
-        if (installVersion < gyp.package.installVersion) {
-          log.verbose('install', 'version is no good; reinstalling')
-          go().then(cb, cb)
-        } else {
-          log.verbose('install', 'version is good')
-          cb()
-        }
-      })
-    })
-  } else {
-    go().then(cb, cb)
-  }
-
-  class ShaSum extends stream.Transform {
-    constructor (callback) {
-      super()
-      this._callback = callback
-      this._digester = crypto.createHash('sha256')
+      throw err
     }
-
-    _transform (chunk, _, callback) {
-      this._digester.update(chunk)
-      callback(null, chunk)
+    log.verbose('install', 'version is already installed, need to check "installVersion"')
+    const installVersionFile = path.resolve(devDir, 'installVersion')
+    let installVersion = 0
+    try {
+      const ver = await fs.promises.readFile(installVersionFile, 'ascii')
+      installVersion = parseInt(ver, 10) || 0
+    } catch (err) {
+      if (err.code !== 'ENOENT') {
+        throw err
+      }
     }
-
-    _flush (callback) {
-      this._callback(null, this._digester.digest('hex'))
-      callback()
+    log.verbose('got "installVersion"', installVersion)
+    log.verbose('needs "installVersion"', gyp.package.installVersion)
+    if (installVersion < gyp.package.installVersion) {
+      log.verbose('install', 'version is no good; reinstalling')
+      try {
+        return await go()
+      } catch (err) {
+        return rollback(err)
+      }
+    }
+    log.verbose('install', 'version is good')
+  } else {
+    try {
+      return await go()
+    } catch (err) {
+      return rollback(err)
     }
   }
 
@@ -135,8 +110,7 @@ function install (fs, gyp, argv, callback) {
       }
     } catch (err) {
       if (err.code === 'EACCES') {
-        eaccesFallback(err)
-        return
+        return eaccesFallback(err)
       }
 
       throw err
@@ -173,7 +147,7 @@ function install (fs, gyp, argv, callback) {
       })
     } else {
       try {
-        const res = await download(gyp, process.env, release.tarballUrl)
+        const res = await download(gyp, release.tarballUrl)
 
         if (res.status !== 200) {
           throw new Error(`${res.status} response downloading ${release.tarballUrl}`)
@@ -234,7 +208,7 @@ function install (fs, gyp, argv, callback) {
       log.verbose('check download content checksum, need to download `SHASUMS256.txt`...')
       log.verbose('checksum url', release.shasumsUrl)
 
-      const res = await download(gyp, process.env, release.shasumsUrl)
+      const res = await download(gyp, release.shasumsUrl)
 
       if (res.status !== 200) {
         throw new Error(`${res.status}  status code downloading checksum`)
@@ -268,7 +242,7 @@ function install (fs, gyp, argv, callback) {
         await fs.promises.mkdir(dir, { recursive: true })
         log.verbose('streaming', name, 'to:', targetLibPath)
 
-        const res = await download(gyp, process.env, libUrl)
+        const res = await download(gyp, libUrl)
 
         if (res.status === 403 || res.status === 404) {
           if (arch === 'arm64') {
@@ -304,6 +278,13 @@ function install (fs, gyp, argv, callback) {
     return extname === '.h' || extname === '.gypi'
   }
 
+  async function rollback (err) {
+    log.warn('install', 'got an error, rolling back install')
+    // roll-back the install if anything went wrong
+    await util.promisify(gyp.commands.remove)([release.versionDir])
+    throw err
+  }
+
   /**
    * The EACCES fallback is a workaround for npm's `sudo` behavior, where
    * it drops the permissions before invoking any child processes (like
@@ -313,10 +294,10 @@ function install (fs, gyp, argv, callback) {
    * the compilation will succeed...
    */
 
-  function eaccesFallback (err) {
+  async function eaccesFallback (err) {
     const noretry = '--node_gyp_internal_noretry'
     if (argv.indexOf(noretry) !== -1) {
-      return cb(err)
+      throw err
     }
     const tmpdir = os.tmpdir()
     gyp.devDir = path.resolve(tmpdir, '.node-gyp')
@@ -331,11 +312,29 @@ function install (fs, gyp, argv, callback) {
       log.verbose('tmpdir == cwd', 'automatically will remove dev files after to save disk space')
       gyp.todo.push({ name: 'remove', args: argv })
     }
-    gyp.commands.install([noretry].concat(argv), cb)
+    return util.promisify(gyp.commands.install)([noretry].concat(argv))
+  }
+}
+
+class ShaSum extends stream.Transform {
+  constructor (callback) {
+    super()
+    this._callback = callback
+    this._digester = crypto.createHash('sha256')
+  }
+
+  _transform (chunk, _, callback) {
+    this._digester.update(chunk)
+    callback(null, chunk)
+  }
+
+  _flush (callback) {
+    this._callback(null, this._digester.digest('hex'))
+    callback()
   }
 }
 
-async function download (gyp, env, url) {
+async function download (gyp, url) {
   log.http('GET', url)
 
   const requestOpts = {
@@ -367,11 +366,11 @@ async function readCAFile (filename) {
 }
 
 module.exports = function (gyp, argv, callback) {
-  return install(fs, gyp, argv, callback)
+  install(fs, gyp, argv).then(callback, callback)
 }
 module.exports.test = {
-  download: download,
-  install: install,
-  readCAFile: readCAFile
+  download,
+  install,
+  readCAFile
 }
 module.exports.usage = 'Install node development files for the specified node version.'
diff --git a/test/test-download.js b/test/test-download.js
index 966aefc445..c85b2ac166 100644
--- a/test/test-download.js
+++ b/test/test-download.js
@@ -1,8 +1,9 @@
 'use strict'
 
-const test = require('tap').test
+const { test } = require('tap')
 const fs = require('fs')
 const path = require('path')
+const util = require('util')
 const http = require('http')
 const https = require('https')
 const install = require('../lib/install')
@@ -31,7 +32,7 @@ test('download over http', (t) => {
       version: '42'
     }
     const url = `http://${host}:${port}`
-    const res = await install.test.download(gyp, {}, url)
+    const res = await install.test.download(gyp, url)
     t.strictEqual(await res.text(), 'ok')
     resolve()
   }))
@@ -66,7 +67,7 @@ test('download over https with custom ca', async (t) => {
       version: '42'
     }
     const url = `https://${host}:${port}`
-    const res = await install.test.download(gyp, {}, url)
+    const res = await install.test.download(gyp, url)
     t.strictEqual(await res.text(), 'ok')
     resolve()
   }))
@@ -97,7 +98,7 @@ test('download over http with proxy', (t) => {
         version: '42'
       }
       const url = `http://${host}:${port}`
-      const res = await install.test.download(gyp, {}, url)
+      const res = await install.test.download(gyp, url)
       t.strictEqual(await res.text(), 'proxy ok')
       resolve()
     })
@@ -130,7 +131,7 @@ test('download over http with noproxy', (t) => {
         version: '42'
       }
       const url = `http://${host}:${port}`
-      const res = await install.test.download(gyp, {}, url)
+      const res = await install.test.download(gyp, url)
       t.strictEqual(await res.text(), 'ok')
       resolve()
     })
@@ -158,7 +159,7 @@ test('check certificate splitting', async (t) => {
 
 // only run this test if we are running a version of Node with predictable version path behavior
 
-test('download headers (actual)', (t) => {
+test('download headers (actual)', async (t) => {
   if (process.env.FAST_TEST ||
       process.release.name !== 'node' ||
       semver.prerelease(process.version) !== null ||
@@ -166,55 +167,46 @@ test('download headers (actual)', (t) => {
     return t.skip('Skipping actual download of headers due to test environment configuration')
   }
 
-  t.plan(17)
+  t.plan(12)
 
   const expectedDir = path.join(devDir, process.version.replace(/^v/, ''))
-  rimraf(expectedDir, (err) => {
-    t.ifError(err)
-
-    const prog = gyp()
-    prog.parseArgv([])
-    prog.devDir = devDir
-    log.level = 'warn'
-    install(prog, [], (err) => {
-      t.ifError(err)
-
-      fs.readFile(path.join(expectedDir, 'installVersion'), 'utf8', (err, data) => {
-        t.ifError(err)
-        t.strictEqual(data, '9\n', 'correct installVersion')
-      })
-
-      fs.readdir(path.join(expectedDir, 'include/node'), (err, list) => {
-        t.ifError(err)
-
-        t.ok(list.includes('common.gypi'))
-        t.ok(list.includes('config.gypi'))
-        t.ok(list.includes('node.h'))
-        t.ok(list.includes('node_version.h'))
-        t.ok(list.includes('openssl'))
-        t.ok(list.includes('uv'))
-        t.ok(list.includes('uv.h'))
-        t.ok(list.includes('v8-platform.h'))
-        t.ok(list.includes('v8.h'))
-        t.ok(list.includes('zlib.h'))
-      })
-
-      fs.readFile(path.join(expectedDir, 'include/node/node_version.h'), 'utf8', (err, contents) => {
-        t.ifError(err)
-
-        const lines = contents.split('\n')
-
-        // extract the 3 version parts from the defines to build a valid version string and
-        // and check them against our current env version
-        const version = ['major', 'minor', 'patch'].reduce((version, type) => {
-          const re = new RegExp(`^#define\\sNODE_${type.toUpperCase()}_VERSION`)
-          const line = lines.find((l) => re.test(l))
-          const i = line ? parseInt(line.replace(/^[^0-9]+([0-9]+).*$/, '$1'), 10) : 'ERROR'
-          return `${version}${type !== 'major' ? '.' : 'v'}${i}`
-        }, '')
-
-        t.strictEqual(version, process.version)
-      })
-    })
-  })
+  await util.promisify(rimraf)(expectedDir)
+
+  const prog = gyp()
+  prog.parseArgv([])
+  prog.devDir = devDir
+  log.level = 'warn'
+  await util.promisify(install)(prog, [])
+
+  const [data, list, contents] = await Promise.all([
+    fs.promises.readFile(path.join(expectedDir, 'installVersion'), 'utf8'),
+    fs.promises.readdir(path.join(expectedDir, 'include/node')),
+    fs.promises.readFile(path.join(expectedDir, 'include/node/node_version.h'), 'utf8')
+  ])
+
+  t.strictEqual(data, '9\n', 'correct installVersion')
+
+  t.ok(list.includes('common.gypi'))
+  t.ok(list.includes('config.gypi'))
+  t.ok(list.includes('node.h'))
+  t.ok(list.includes('node_version.h'))
+  t.ok(list.includes('openssl'))
+  t.ok(list.includes('uv'))
+  t.ok(list.includes('uv.h'))
+  t.ok(list.includes('v8-platform.h'))
+  t.ok(list.includes('v8.h'))
+  t.ok(list.includes('zlib.h'))
+
+  const lines = contents.split('\n')
+
+  // extract the 3 version parts from the defines to build a valid version string and
+  // and check them against our current env version
+  const version = ['major', 'minor', 'patch'].reduce((version, type) => {
+    const re = new RegExp(`^#define\\sNODE_${type.toUpperCase()}_VERSION`)
+    const line = lines.find((l) => re.test(l))
+    const i = line ? parseInt(line.replace(/^[^0-9]+([0-9]+).*$/, '$1'), 10) : 'ERROR'
+    return `${version}${type !== 'major' ? '.' : 'v'}${i}`
+  }, '')
+
+  t.strictEqual(version, process.version)
 })
diff --git a/test/test-install.js b/test/test-install.js
index c3317155e0..5039dc992e 100644
--- a/test/test-install.js
+++ b/test/test-install.js
@@ -1,38 +1,46 @@
 'use strict'
 
-const test = require('tap').test
-const install = require('../lib/install').test.install
+const { test } = require('tap')
+const { test: { install } } = require('../lib/install')
+const log = require('npmlog')
 
-require('npmlog').level = 'error' // we expect a warning
+log.level = 'error' // we expect a warning
 
-test('EACCES retry once', function (t) {
+test('EACCES retry once', async (t) => {
   t.plan(3)
 
-  var fs = {}
-  fs.stat = function (path, cb) {
-    var err = new Error()
-    err.code = 'EACCES'
-    cb(err)
-    t.ok(true)
+  const fs = {
+    promises: {
+      stat (_) {
+        const err = new Error()
+        err.code = 'EACCES'
+        t.ok(true)
+        throw err
+      }
+    }
   }
 
-  var gyp = {}
-  gyp.devDir = __dirname
-  gyp.opts = {}
-  gyp.opts.ensure = true
-  gyp.commands = {}
-  gyp.commands.install = function (argv, cb) {
-    install(fs, gyp, argv, cb)
-  }
-  gyp.commands.remove = function (argv, cb) {
-    cb()
+  const Gyp = {
+    devDir: __dirname,
+    opts: {
+      ensure: true
+    },
+    commands: {
+      install (argv, cb) {
+        install(fs, Gyp, argv).then(cb, cb)
+      },
+      remove (_, cb) {
+        cb()
+      }
+    }
   }
 
-  gyp.commands.install([], function (err) {
+  try {
+    await install(fs, Gyp, [])
+  } catch (err) {
     t.ok(true)
     if (/"pre" versions of node cannot be installed/.test(err.message)) {
       t.ok(true)
-      t.ok(true)
     }
-  })
+  }
 })

From b2bfa2296ebfdf7272dbb3a281132cc761457129 Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Mon, 14 Sep 2020 11:12:34 -0400
Subject: [PATCH 6/8] fix: reduce one level promise

Windows seems to be hanging, hopefully this reduces the number of
promises in the chain.
---
 lib/install.js        | 8 ++++----
 test/test-download.js | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/install.js b/lib/install.js
index 15a58cf74b..4140d660a4 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -188,7 +188,7 @@ async function install (fs, gyp, argv) {
     const installVersionPath = path.resolve(devDir, 'installVersion')
     await Promise.all([
       // need to download node.lib
-      ...(win ? [downloadNodeLib()] : []),
+      ...(win ? downloadNodeLib() : []),
       // write the "installVersion" file
       fs.promises.writeFile(installVersionPath, gyp.package.installVersion + '\n'),
       // Only download SHASUMS.txt if we downloaded something in need of SHA verification
@@ -228,10 +228,10 @@ async function install (fs, gyp, argv) {
       log.verbose('checksum data', JSON.stringify(expectShasums))
     }
 
-    async function downloadNodeLib () {
+    function downloadNodeLib () {
       log.verbose('on Windows; need to download `' + release.name + '.lib`...')
       const archs = ['ia32', 'x64', 'arm64']
-      return Promise.all(archs.map(async (arch) => {
+      return archs.map(async (arch) => {
         const dir = path.resolve(devDir, arch)
         const targetLibPath = path.resolve(dir, release.name + '.lib')
         const { libUrl, libPath } = release[arch]
@@ -264,7 +264,7 @@ async function install (fs, gyp, argv) {
           }),
           fs.createWriteStream(targetLibPath)
         )
-      }))
+      })
     } // downloadNodeLib()
   } // go()
 
diff --git a/test/test-download.js b/test/test-download.js
index c85b2ac166..31ae5d311c 100644
--- a/test/test-download.js
+++ b/test/test-download.js
@@ -93,7 +93,8 @@ test('download over http with proxy', (t) => {
     pserver.listen(port + 1, host, async () => {
       const gyp = {
         opts: {
-          proxy: `http://${host}:${port + 1}`
+          proxy: `http://${host}:${port + 1}`,
+          noproxy: 'bad'
         },
         version: '42'
       }

From 95a730518b587d80efc12a8864be2cba2b1db72f Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Mon, 14 Sep 2020 19:21:17 -0400
Subject: [PATCH 7/8] test: use proper teardown

Tests are were not waiting for their servers to close.
---
 test/test-download.js | 142 ++++++++++++++++++++----------------------
 1 file changed, 68 insertions(+), 74 deletions(-)

diff --git a/test/test-download.js b/test/test-download.js
index 31ae5d311c..71a3c0d092 100644
--- a/test/test-download.js
+++ b/test/test-download.js
@@ -15,128 +15,126 @@ const log = require('npmlog')
 
 log.level = 'warn'
 
-test('download over http', (t) => {
+test('download over http', async (t) => {
   t.plan(2)
 
   const server = http.createServer((req, res) => {
     t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('ok')
-    server.close()
   })
 
+  t.tearDown(() => new Promise((resolve) => server.close(resolve)))
+
   const host = 'localhost'
-  return new Promise(resolve => server.listen(0, host, async () => {
-    const { port } = server.address()
-    const gyp = {
-      opts: {},
-      version: '42'
-    }
-    const url = `http://${host}:${port}`
-    const res = await install.test.download(gyp, url)
-    t.strictEqual(await res.text(), 'ok')
-    resolve()
-  }))
+  await new Promise((resolve) => server.listen(0, host, resolve))
+  const { port } = server.address()
+  const gyp = {
+    opts: {},
+    version: '42'
+  }
+  const url = `http://${host}:${port}`
+  const res = await install.test.download(gyp, url)
+  t.strictEqual(await res.text(), 'ok')
 })
 
 test('download over https with custom ca', async (t) => {
   t.plan(3)
 
-  const [cert, key] = await Promise.all([
+  const cafile = path.join(__dirname, '/fixtures/ca.crt')
+  const [cert, key, ca] = await Promise.all([
     fs.promises.readFile(path.join(__dirname, 'fixtures/server.crt'), 'utf8'),
-    fs.promises.readFile(path.join(__dirname, 'fixtures/server.key'), 'utf8')
+    fs.promises.readFile(path.join(__dirname, 'fixtures/server.key'), 'utf8'),
+    install.test.readCAFile(cafile)
   ])
 
-  const cafile = path.join(__dirname, '/fixtures/ca.crt')
-  const ca = await install.test.readCAFile(cafile)
   t.strictEqual(ca.length, 1)
 
   const options = { ca: ca, cert: cert, key: key }
   const server = https.createServer(options, (req, res) => {
     t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('ok')
-    server.close()
   })
 
+  t.tearDown(() => new Promise((resolve) => server.close(resolve)))
+
   server.on('clientError', (err) => { throw err })
 
   const host = 'localhost'
-  return new Promise(resolve => server.listen(0, host, async () => {
-    const { port } = server.address()
-    const gyp = {
-      opts: { cafile },
-      version: '42'
-    }
-    const url = `https://${host}:${port}`
-    const res = await install.test.download(gyp, url)
-    t.strictEqual(await res.text(), 'ok')
-    resolve()
-  }))
+  await new Promise((resolve) => server.listen(0, host, resolve))
+  const { port } = server.address()
+  const gyp = {
+    opts: { cafile },
+    version: '42'
+  }
+  const url = `https://${host}:${port}`
+  const res = await install.test.download(gyp, url)
+  t.strictEqual(await res.text(), 'ok')
 })
 
-test('download over http with proxy', (t) => {
+test('download over http with proxy', async (t) => {
   t.plan(2)
 
   const server = http.createServer((_, res) => {
     res.end('ok')
-    pserver.close(() => { server.close() })
   })
 
   const pserver = http.createServer((req, res) => {
     t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('proxy ok')
-    server.close(() => { pserver.close() })
   })
 
+  t.tearDown(() => Promise.all([
+    new Promise((resolve) => server.close(resolve)),
+    new Promise((resolve) => pserver.close(resolve))
+  ]))
+
   const host = 'localhost'
-  return new Promise(resolve => server.listen(0, host, () => {
-    const { port } = server.address()
-    pserver.listen(port + 1, host, async () => {
-      const gyp = {
-        opts: {
-          proxy: `http://${host}:${port + 1}`,
-          noproxy: 'bad'
-        },
-        version: '42'
-      }
-      const url = `http://${host}:${port}`
-      const res = await install.test.download(gyp, url)
-      t.strictEqual(await res.text(), 'proxy ok')
-      resolve()
-    })
-  }))
+  await new Promise((resolve) => server.listen(0, host, resolve))
+  const { port } = server.address()
+  await new Promise((resolve) => pserver.listen(port + 1, host, resolve))
+  const gyp = {
+    opts: {
+      proxy: `http://${host}:${port + 1}`,
+      noproxy: 'bad'
+    },
+    version: '42'
+  }
+  const url = `http://${host}:${port}`
+  const res = await install.test.download(gyp, url)
+  t.strictEqual(await res.text(), 'proxy ok')
 })
 
-test('download over http with noproxy', (t) => {
+test('download over http with noproxy', async (t) => {
   t.plan(2)
 
   const server = http.createServer((req, res) => {
     t.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
     res.end('ok')
-    pserver.close(() => { server.close() })
   })
 
   const pserver = http.createServer((_, res) => {
     res.end('proxy ok')
-    server.close(() => { pserver.close() })
   })
 
+  t.tearDown(() => Promise.all([
+    new Promise((resolve) => server.close(resolve)),
+    new Promise((resolve) => pserver.close(resolve))
+  ]))
+
   const host = 'localhost'
-  return new Promise(resolve => server.listen(0, host, () => {
-    const { port } = server.address()
-    pserver.listen(port + 1, host, async () => {
-      const gyp = {
-        opts: {
-          proxy: `http://${host}:${port + 1}`,
-          noproxy: host
-        },
-        version: '42'
-      }
-      const url = `http://${host}:${port}`
-      const res = await install.test.download(gyp, url)
-      t.strictEqual(await res.text(), 'ok')
-      resolve()
-    })
-  }))
+  await new Promise((resolve) => server.listen(0, host, resolve))
+  const { port } = server.address()
+  await new Promise((resolve) => pserver.listen(port + 1, host, resolve))
+  const gyp = {
+    opts: {
+      proxy: `http://${host}:${port + 1}`,
+      noproxy: host
+    },
+    version: '42'
+  }
+  const url = `http://${host}:${port}`
+  const res = await install.test.download(gyp, url)
+  t.strictEqual(await res.text(), 'ok')
 })
 
 test('download with missing cafile', async (t) => {
@@ -179,14 +177,10 @@ test('download headers (actual)', async (t) => {
   log.level = 'warn'
   await util.promisify(install)(prog, [])
 
-  const [data, list, contents] = await Promise.all([
-    fs.promises.readFile(path.join(expectedDir, 'installVersion'), 'utf8'),
-    fs.promises.readdir(path.join(expectedDir, 'include/node')),
-    fs.promises.readFile(path.join(expectedDir, 'include/node/node_version.h'), 'utf8')
-  ])
-
+  const data = await fs.promises.readFile(path.join(expectedDir, 'installVersion'), 'utf8')
   t.strictEqual(data, '9\n', 'correct installVersion')
 
+  const list = await fs.promises.readdir(path.join(expectedDir, 'include/node'))
   t.ok(list.includes('common.gypi'))
   t.ok(list.includes('config.gypi'))
   t.ok(list.includes('node.h'))
@@ -198,7 +192,7 @@ test('download headers (actual)', async (t) => {
   t.ok(list.includes('v8.h'))
   t.ok(list.includes('zlib.h'))
 
-  const lines = contents.split('\n')
+  const lines = (await fs.promises.readFile(path.join(expectedDir, 'include/node/node_version.h'), 'utf8')).split('\n')
 
   // extract the 3 version parts from the defines to build a valid version string and
   // and check them against our current env version

From cd6d856ce2a16c935a8e180c70370bbd48bab2ba Mon Sep 17 00:00:00 2001
From: Matias Lopez <imatlopez@gmail.com>
Date: Sun, 24 Jan 2021 09:18:45 -0500
Subject: [PATCH 8/8] fix: skip error return on then callback

Otherwise any succesful return would make the program think it failed
---
 lib/install.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/install.js b/lib/install.js
index 4140d660a4..99f6d8592a 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -366,7 +366,7 @@ async function readCAFile (filename) {
 }
 
 module.exports = function (gyp, argv, callback) {
-  install(fs, gyp, argv).then(callback, callback)
+  install(fs, gyp, argv).then(callback.bind(undefined, null), callback)
 }
 module.exports.test = {
   download,