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: recommended node-gyp version in node.h error #37829

Merged
merged 1 commit into from Apr 11, 2023
Merged

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

merged 1 commit into from Apr 11, 2023

Conversation

RaisinTen
Copy link
Contributor

Description of Change

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.

Checklist

Release Notes

Notes: Fix recommended node-gyp version in node.h error

cc @zcbenz

@RaisinTen RaisinTen requested a review from a team as a code owner April 5, 2023 08:51
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 5, 2023
@RaisinTen RaisinTen added semver/patch backwards-compatible bug fixes target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Apr 5, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 6, 2023
@jkleinsc jkleinsc added the target/25-x-y PR should also be added to the "25-x-y" branch. label Apr 6, 2023
@dsanders11
Copy link
Member

@RaisinTen, you'll need to re-export the patches so they're updated correctly - that's causing the current CI failure.

electron/docs/development/patches.md

Line 54 in e8c8785

Re-exporting patches will sometimes cause shasums in unrelated patches to change. This is generally harmless and can be ignored (but go ahead and add those changes to your PR, it'll stop them from showing up for other people).

@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>
@RaisinTen RaisinTen force-pushed the fix-recommeneded-node-gyp-version-in-node.h-error branch from aa1d1b8 to 120cfad Compare April 10, 2023 08:48
@RaisinTen
Copy link
Contributor Author

@dsanders11 thanks, done!

@codebytere codebytere merged commit c326b00 into electron:main Apr 11, 2023
10 checks passed
@release-clerk
Copy link

release-clerk bot commented Apr 11, 2023

Release Notes Persisted

Fix recommended node-gyp version in node.h error

@trop
Copy link
Contributor

trop bot commented Apr 11, 2023

I was unable to backport this PR to "22-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Apr 11, 2023

I was unable to backport this PR to "23-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. labels Apr 11, 2023
@trop trop bot mentioned this pull request Apr 11, 2023
Merged
@trop trop bot mentioned this pull request Apr 11, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Apr 11, 2023

I have automatically backported this PR to "25-x-y", please check out #37926

@trop
Copy link
Contributor

trop bot commented Apr 11, 2023

I have automatically backported this PR to "24-x-y", please check out #37927

@trop trop bot added in-flight/24-x-y and removed target/25-x-y PR should also be added to the "25-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Apr 11, 2023
@RaisinTen RaisinTen deleted the fix-recommeneded-node-gyp-version-in-node.h-error branch April 11, 2023 12:28
@trop trop bot added merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. and removed in-flight/24-x-y in-flight/25-x-y labels Apr 11, 2023
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 RaisinTen mentioned this pull request Apr 12, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Apr 12, 2023

@RaisinTen has manually backported this PR to "23-x-y", please check out #37941

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>
@RaisinTen RaisinTen mentioned this pull request Apr 12, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Apr 12, 2023

@RaisinTen has manually backported this PR to "22-x-y", please check out #37942

codebytere pushed a commit 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>
@trop trop bot added merged/23-x-y PR was merged to the "23-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. and removed in-flight/22-x-y labels Apr 12, 2023
codebytere pushed a commit 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>
@commit-lint commit-lint bot mentioned this pull request Jun 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

Assignees
No one assigned
Labels
merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@RaisinTen @dsanders11 @zcbenz @deepak1556 @jkleinsc @codebytere