Skip to content

fix: let --loglevel on the command line override npm_config_loglevel - #3384

Open
gengjiawen wants to merge 1 commit into
nodejs:mainfrom
gengjiawen:fix/cli-loglevel-precedence
Open

gengjiawen wants to merge 1 commit into
nodejs:mainfrom
gengjiawen:fix/cli-loglevel-precedence

Conversation

@gengjiawen

Copy link
Copy Markdown
Member
Checklist
  • npm install && npm run lint && npm test passes
  • tests are included
  • commit message follows commit guidelines
Description of change

Fixes the citgm node-gyp failure reported in nodejs/citgm#1153. It fails the same 3 tests on every OS and Node.js 22/24/26 (run):

  • addon › build simple addon
  • addon › addon works with renamed host executable
  • options › options in environment
Root cause

citgm always sets npm_config_loglevel (default error) in the environment of the module's npm test. Since #3196 (v11.4.0), parseArgv() copies npm_config_* into opts after parsing the command line, so the inherited npm_config_loglevel replaces an explicit --loglevel:

$ npm_config_loglevel=error node-gyp rebuild --loglevel=verbose
# v11.3.0: logger level = verbose
# v11.4.0 … main: logger level = error
  • test-addon runs node-gyp rebuild --loglevel=verbose and expects gyp info ok as the last line. At error level that line is never printed, so the last line is a compiler warning.
  • test-options asserts the shared logger starts at info, but earlier tests' parseArgv() calls have already set it to error from the environment.

Locally, npm_config_loglevel=error npm test on main reproduces exactly these 3 failures.

This isn't limited to CI: a non-default loglevel in .npmrc (npm exports non-default configs to scripts) silently overrides node-gyp rebuild --verbose in a package script.

Fix
  • lib/node-gyp.js: remember the command-line --loglevel before the environment is read and prefer it, restoring the pre-v11.4.0 behaviour for loglevel.
    Other options keep the existing environment-last precedence (as noted in test-options.js). Changing that for e.g. --target/--nodedir could break setups that rely on npm_config_* overriding flags hard-coded in install scripts, so it's out of scope here.
  • test/test-options.js: reset the shared logger before asserting its initial level, and add a test that --loglevel= and the --silly shorthand win over npm_config_loglevel, while no flag still falls back to it.

With this change, npm_config_loglevel=error npm test passes.

Since nodejs#3196 (v11.4.0), npm_config_* environment variables are written
into opts after the command line is parsed, so an inherited
npm_config_loglevel replaces an explicit --loglevel. Restore the
previous precedence for loglevel only; other options still read the
environment last.

Also reset the shared logger in the options test, which otherwise
inherits whatever level earlier tests' parseArgv() calls left behind.

Refs: nodejs/citgm#1153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant