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.

Sign up for GitHub

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

Jump to bottom

fix: _ in npm_config_ env variables #2547

Merged
merged 1 commit into from
Feb 15, 2022
Merged

fix: _ in npm_config_ env variables #2547

merged 1 commit into from
Feb 15, 2022

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Nov 8, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

This PR fixes 2 things on npm_config_ env variables:

  1. Make npm_config_force_process_config=true env work by automatically converting _ to - when parsing npmConfigPrefix.
  2. Recognize --disturl as option in create-config-gypi.js, it is not documented anywhere but users have been relying on npm_config_disturl to pass --dist-url in env.

@rvagg
Copy link
Member

rvagg commented Nov 8, 2021

users have been relying on npm_config_disturl

ugh, searching the web confirms lots of advice on this, and we do this in process-release.js already

Re "converting _ to -" - I think the reason we've not had to deal with this previously is because we don't touch any environment variables that have _ in them (and maybe why we're accepting disturl). So this seems fine, it's just surprising that we've not had to deal with this before.

@rvagg rvagg merged commit eef4eef into nodejs:master Feb 15, 2022
@jgcook935 jgcook935 mentioned this pull request Apr 13, 2022
Merged
4 tasks
@richardlau richardlau mentioned this pull request Oct 21, 2022
Closed
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 5, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error
34448db 
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen mentioned this pull request Apr 5, 2023
Merged
2 tasks
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 10, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error
120cfad 
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 11, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error (#37829)
c326b00 
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
trop bot added a commit to electron/electron that referenced this pull request Apr 11, 2023
@trop @RaisinTen
fix: recommended node-gyp version in node.h error
52d62ee 
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Co-authored-by: Darshan Sen <raisinten@gmail.com>
trop bot added a commit to electron/electron that referenced this pull request Apr 11, 2023
@trop @RaisinTen
fix: recommended node-gyp version in node.h error
e87fd3b 
In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Co-authored-by: Darshan Sen <raisinten@gmail.com>
VerteDinde pushed a commit to electron/electron that referenced this pull request Apr 11, 2023
@trop @RaisinTen
fix: recommended node-gyp version in node.h error (#37927)
2e7bfb4 
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 11, 2023
@trop @RaisinTen
fix: recommended node-gyp version in node.h error (#37926)
95b727c 
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 12, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error (electron#37829)
fa315d7 
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/electron that referenced this pull request Apr 12, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error (electron#37829)
c421eda 
fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 12, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error (#37941)
3e9f1e6 
fix: recommended `node-gyp` version in `node.h` error (#37829)

fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
codebytere pushed a commit to electron/electron that referenced this pull request Apr 12, 2023
@RaisinTen
fix: recommended node-gyp version in node.h error (#37942)
ee668f1 
fix: recommended `node-gyp` version in `node.h` error (#37829)

fix: recommended node-gyp version in node.h error

In
https://github.com/electron/electron/blob/main/docs/tutorial/using-native-node-modules.md#using-npm,
we recommend setting the `npm_config_disturl` variable but doing that
does not work on node-gyp v8.4.0 because after
nodejs/node-gyp#2497
landed, the dist URL was read only from `gyp.opts['dist-url']`. The fix
for reading the value from `npm_config_disturl` by parsing
`gyp.opts.disturl` was landed in
nodejs/node-gyp#2547 and that change was
released in node-gyp v9.0.0, so this change updates the error macro to
recommend node-gyp v9.0.0 as the minimum required version.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvagg rvagg rvagg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zcbenz @rvagg