Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop npm config set support #2879

Closed
lukekarrys opened this issue Jun 28, 2023 · 3 comments · Fixed by #2880
Closed

Drop npm config set support #2879

lukekarrys opened this issue Jun 28, 2023 · 3 comments · Fixed by #2880

Comments

@lukekarrys
Copy link
Member

lukekarrys commented Jun 28, 2023

npm 9 no longer supports setting arbitrary config items. This should be removed from the docs, with the other methods of setting items such as environs being the ideal solution.

This is a breaking change so it is good timing to make this change along with the breaking engine changes that already landed.

Ref #2798 #2800

@cclauss
Copy link
Contributor

cclauss commented Jun 28, 2023

Is removing things that no longer work from the docs a breaking change?

Perhaps doc edits could be separated from other methods of setting items.

@richardlau
Copy link
Member

FWIW the relevant code is:

node-gyp/lib/node-gyp.js

Lines 134 to 156 in 53c99ae

// support for inheriting config env variables from npm
const npmConfigPrefix = 'npm_config_'
Object.keys(process.env).forEach(function (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
}
}
}, this)

Removing that would be semver major for node-gyp. While npm 9 may no longer be setting npm_config_ prefixed environment variables for arbitrary config items, what about the other package managers? Or in other words, node-gyp doesn't care who sets them, but will process any npm_config_ prefixed environment variables.

We could, for example keep the above code and this documentation

node-gyp/README.md

Lines 224 to 241 in 53c99ae

### Environment variables
Use the form `npm_config_OPTION_NAME` for any of the command options listed
above (dashes in option names should be replaced by underscores).
For example, to set `devdir` equal to `/tmp/.gyp`, you would:
Run this on Unix:
```bash
export npm_config_devdir=/tmp/.gyp
```
Or this on Windows:
```console
set npm_config_devdir=c:\temp\.gyp
```
as manually setting such environment variables still works but drop

node-gyp/README.md

Lines 243 to 254 in 53c99ae

### `npm` configuration
Use the form `OPTION_NAME` for any of the command options listed above.
For example, to set `devdir` equal to `/tmp/.gyp`, you would run:
```bash
npm config set [--global] devdir /tmp/.gyp
```
**Note:** Configuration set via `npm` will only be used when `node-gyp`
is run via `npm`, not when `node-gyp` is run directly.
or amend it to say it no longer works for npm 9 onwards.

@lukekarrys
Copy link
Member Author

Good points @cclauss and @richardlau. Here's what I'm thinking now:

  • Change npm config set python X to npm_config_python=X in method 2
  • Possibly add a note to the bottom of the section saying in npm <= 8 you can run npm config set python as well

Then this is no longer a breaking change.

My main goal is to not have npm config set python as part of the docs that is easily scannable by users since the error message for this in npm is pretty generic and doesn't offer guidance to the user that is specific to node-gyp resulting in a lot of confusion.

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 a pull request may close this issue.

3 participants