refactor

Author: lukekarrys

lib/node-gyp.js

@@ -122,54 +122,41 @@ class Gyp extends EventEmitter {
     }
 
     // support for inheriting config env variables from npm
+    // npm will set environment variables in the following forms:
+    // - `npm_config_<key>` for values from npm's own config. Setting arbitrary
+    //   options on npm's config was deprecated in npm v11 but node-gyp still
+    //   supports it for backwards compatibility.
+    //   See https://github.com/nodejs/node-gyp/issues/3156
+    // - `npm_package_config_node_gyp_<key>` for values from the `config` object
+    //   in package.json. This is the preferred way to set options for node-gyp
+    //   since npm v11. The `node_gyp_` prefix is used to avoid conflicts with
+    //   other tools.
+    // The `npm_package_config_node_gyp_` prefix will take precedence over
+    // `npm_config_` keys.
     const npmConfigPrefix = 'npm_config_'
-    Object.keys(process.env).forEach((name) => {
-      if (name.indexOf(npmConfigPrefix) !== 0) {
-        return
-      }
-      const val = process.env[name]
-      if (name === npmConfigPrefix + 'loglevel') {
-        log.logger.level = val
-      } else {
-      // add the user-defined options to the config
-        name = name.substring(npmConfigPrefix.length)
-        // gyp@741b7f1 enters an infinite loop when it encounters
-        // zero-length options so ensure those don't get through.
-        if (name) {
-        // convert names like force_process_config to force-process-config
-          if (name.includes('_')) {
-            name = name.replace(/_/g, '-')
-          }
-          this.opts[name] = val
-        }
-      }
-    })
-
-    // Read values that npm sets based on the `config` object in package.json.
-    // These take precendence over the npm_config_ prefixed values which
-    // are deprecated in npm@11.
-    // These can also be set directly in the environment, if setting in
-    // package.json is not desired.
     const npmPackageConfigPrefix = 'npm_package_config_node_gyp_'
-    Object.keys(process.env).forEach((name) => {
-      if (name.indexOf(npmPackageConfigPrefix) !== 0) {
-        return
-      }
-      const val = process.env[name]
-      name = name.substring(npmPackageConfigPrefix.length)
+
+    const configEnvKeys = Object.keys(process.env)
+      .filter((k) => k.startsWith(npmConfigPrefix) || k.startsWith(npmPackageConfigPrefix))
+      // sort so that npm_package_config_node_gyp_ keys come last and will override
+      .sort((a) => a.startsWith(npmConfigPrefix) ? -1 : 1)
+
+    for (const key of configEnvKeys) {
+      // add the user-defined options to the config
+      const name = key.startsWith(npmConfigPrefix)
+        ? key.substring(npmConfigPrefix.length)
+        : key.substring(npmPackageConfigPrefix.length)
       // gyp@741b7f1 enters an infinite loop when it encounters
       // zero-length options so ensure those don't get through.
       if (name) {
         // convert names like force_process_config to force-process-config
-        if (name.includes('_')) {
-          name = name.replace(/_/g, '-')
-        }
-        this.opts[name] = val
+        this.opts[name.replaceAll('_', '-')] = process.env[key]
       }
-    })
+    }
 
     if (this.opts.loglevel) {
       log.logger.level = this.opts.loglevel
+      delete this.opts.loglevel
     }
     log.resume()
   }

test/test-options.js

@@ -3,19 +3,19 @@
 const { describe, it } = require('mocha')
 const assert = require('assert')
 const gyp = require('../lib/node-gyp')
+const log = require('../lib/log')
 
 describe('options', function () {
   it('options in environment', () => {
     // `npm test` dumps a ton of npm_config_* variables in the environment.
     Object.keys(process.env)
-      .filter((key) => /^npm_config_/.test(key) || /^npm_package_config_node_gyp_/.test(key))
+      .filter((key) => /^npm_config_/i.test(key) || /^npm_package_config_node_gyp_/i.test(key))
       .forEach((key) => { delete process.env[key] })
 
     // in some platforms, certain keys are stubborn and cannot be removed
     const keys = Object.keys(process.env)
-      .filter((key) => /^npm_config_/.test(key) || /^npm_package_config_node_gyp_/.test(key))
+      .filter((key) => /^npm_config_/i.test(key) || /^npm_package_config_node_gyp_/i.test(key))
       .map((key) => key.substring('npm_config_'.length))
-      .concat('argv', 'x', 'y', 'foo')
 
     // Environment variables with the following prefixes should be added to opts.
     // - `npm_config_` for npm versions before v11.
@@ -25,18 +25,23 @@ describe('options', function () {
     process.env.npm_config_ = '42'
     process.env.npm_package_config_node_gyp_ = '42'
     // Other keys should get added.
+    process.env.npm_package_config_node_gyp_foo = '42'
     process.env.npm_config_x = '42'
     process.env.npm_config_y = '41'
-    process.env.npm_package_config_node_gyp_foo = '42'
     // Package config should take precedence over npm_config_ keys.
     process.env.npm_package_config_node_gyp_y = '42'
-    // Except loglevel.
-    process.env.npm_config_loglevel = 'debug'
+    // loglevel does not get added to opts but will change the logger's level.
+    process.env.npm_config_loglevel = 'silly'
 
     const g = gyp()
+
+    assert.strictEqual(log.logger.level.id, 'info')
+
     g.parseArgv(['rebuild']) // Also sets opts.argv.
 
-    assert.deepStrictEqual(Object.keys(g.opts).sort(), keys.sort())
+    assert.strictEqual(log.logger.level.id, 'silly')
+
+    assert.deepStrictEqual(Object.keys(g.opts).sort(), [...keys, 'argv', 'x', 'y', 'foo'].sort())
     assert.strictEqual(g.opts['x'], '42')
     assert.strictEqual(g.opts['y'], '42')
     assert.strictEqual(g.opts['foo'], '42')