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

[QUESTION] node-gyp@7 for npm@7 please #1281

Closed
rvagg opened this issue May 13, 2020 · 7 comments
Closed

[QUESTION] node-gyp@7 for npm@7 please #1281

rvagg opened this issue May 13, 2020 · 7 comments
Assignees
Labels
Awaiting Information further information is requested Needs Discussion is pending a discussion Release 7.x work is associated with a specific npm 7 release

Comments

@rvagg
Copy link
Contributor

rvagg commented May 13, 2020

nodejs/node-gyp#2124

I don't know what the npm@7 timeframe is, but I'll try and get node-gyp@7 out this week, there's just a couple of notable updates.

The current version in here is v5, which works, but still prefers Python 2 over 3. node-gyp@6 switches that around and will use Python 3 first before searching for Python 2. node-gyp@7 includes an updated GYP (we've now fully accepted responsibility for Google's abandonware, see https://github.com/nodejs/gyp-next) and will update dependencies to match the ones you have in the v7.0.0-beta branch here so you can do some heavy deduping over what you're shipping now, see nodejs/node-gyp#2125 (notably semver, tar and which, which you'll be pulling in two versions of to use node-gyp@5).

If you're working to a more agressive timeframe than this please let me know. It would be good to get this all synced and sorted out.

@rvagg
Copy link
Contributor Author

rvagg commented Jun 3, 2020

node-gyp@7 is out; most notably for npm is the synced dependencies which should lead to a big dedupe.

One thing we haven't dealt with yet is that request is still on our dependency list so there's a deprecation warning involved. That's high priority to get to and we may do it in within the 7.x stream if we can do it safely enough. Corporate proxy support and making sure we don't change anything for those users is the top priority and the thing that's making this difficult.

@DeeDeeG
Copy link

DeeDeeG commented Aug 18, 2020

Hi there folks,

I contributed a fix to node-gyp v7 that helps it find Python 3 on Windows, when installed from Python.org.

I think it would be great to have that available to more npm users. 👍 to this from me.

It can mean the difference between being able to install native code "automatically"/out-of-the-box, vs fiddling with config options to tell node-gyp where to find Python.

@darcyclarke darcyclarke added Needs Discussion is pending a discussion Awaiting Information further information is requested labels Aug 18, 2020
@darcyclarke darcyclarke added the Release 7.x work is associated with a specific npm 7 release label Aug 18, 2020
@isaacs
Copy link
Contributor

isaacs commented Aug 18, 2020

@rvagg This is awesome! We're allegedly doing another beta release today, but a few fixes are taking a bit longer than expected, so it might delay a few days. I expect at least a month or two before it's GA.

I'll try to pull in node-gyp 7, and let you know if there are more things that can be deduped out. We're also going to be bumping nopt and a few others, so there may be more opportunity for efficiency there. Happy to send a PR to bring things up to date, as some of them are a bit of a hassle, but in most cases, the biggest "breaking" change is just the node version support.

@isaacs
Copy link
Contributor

isaacs commented Aug 18, 2020

For the request thing, make-fetch-happen is the one that npm uses, and it supports all the corporate proxy stuff, since npm has that same restriction. It's a completely different API, but if you wanna point us to where request is being used, i wouldn't mind taking a look.

@rvagg
Copy link
Contributor Author

rvagg commented Aug 19, 2020

I often check our deps against npm's deps to make sure there's maximum opportunity to dedupe, so if you do bump things then I think we'd be happy to push out releases with just those things bumped so we can get in sync.

The only use of request that we have is in https://github.com/nodejs/node-gyp/blob/master/lib/install.js, but we also load in a proxy.js to handle some custom env var stuff for proxies. It probably shouldn't be too difficult to get something else in place of request, even with a different API. But we're dealing with heavily legacy code in node-gyp that doesn't lend itself to minor refactoring (it's the kind of thing that invites major refactoring), and the concern about getting the proxying situation right is always a big deal. But a major bump in npm version might be a good opportunity to introduce major changes like this that may break for users, they at least get a more clear choice about versions.

@isaacs
Copy link
Contributor

isaacs commented Aug 19, 2020

But we're dealing with heavily legacy code in node-gyp that doesn't lend itself to minor refactoring (it's the kind of thing that invites major refactoring)

im-in-this-photo-meme

I'll put this on my todo list to take a look at in the next few weeks, with an intention to get request out of there before npm v7 goes to GA. Hopefully we can keep the refactoring to a minimum 😆

isaacs added a commit that referenced this issue Sep 22, 2020
This updates node-gyp to v7, allowing us to deduplicate a lot of
significant dependencies.

Fix: #1281
Fix: npm/statusboard#67
ruyadorno pushed a commit that referenced this issue Sep 22, 2020
This updates node-gyp to v7, allowing us to deduplicate a lot of
significant dependencies.

Fix: #1281
Fix: npm/statusboard#67

PR-URL: #1846
Credit: @isaacs
Close: #1846
Reviewed-by: @ruyadorno
@ruyadorno
Copy link
Contributor

landed in v7.0.0-beta.12 ref: b3a50d2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Information further information is requested Needs Discussion is pending a discussion Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

5 participants