fix: let --loglevel on the command line override npm_config_loglevel - #3384
Open
gengjiawen wants to merge 1 commit into
Open
gengjiawen wants to merge 1 commit into
gengjiawen wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
npm install && npm run lint && npm testpassesDescription of change
Fixes the
citgm node-gypfailure reported in nodejs/citgm#1153. It fails the same 3 tests on every OS and Node.js 22/24/26 (run):addon › build simple addonaddon › addon works with renamed host executableoptions › options in environmentRoot cause
citgm always sets
npm_config_loglevel(defaulterror) in the environment of the module'snpm test. Since #3196 (v11.4.0),parseArgv()copiesnpm_config_*intooptsafter parsing the command line, so the inheritednpm_config_loglevelreplaces an explicit--loglevel:test-addonrunsnode-gyp rebuild --loglevel=verboseand expectsgyp info okas the last line. Aterrorlevel that line is never printed, so the last line is a compiler warning.test-optionsasserts the shared logger starts atinfo, but earlier tests'parseArgv()calls have already set it toerrorfrom the environment.Locally,
npm_config_loglevel=error npm testonmainreproduces exactly these 3 failures.This isn't limited to CI: a non-default
loglevelin.npmrc(npm exports non-default configs to scripts) silently overridesnode-gyp rebuild --verbosein a package script.Fix
lib/node-gyp.js: remember the command-line--loglevelbefore the environment is read and prefer it, restoring the pre-v11.4.0 behaviour forloglevel.Other options keep the existing environment-last precedence (as noted in
test-options.js). Changing that for e.g.--target/--nodedircould break setups that rely onnpm_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--sillyshorthand win overnpm_config_loglevel, while no flag still falls back to it.With this change,
npm_config_loglevel=error npm testpasses.