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

Heads-up: npm may be looking to adopt node-gyp version 8.x soon #2392

Closed
DeeDeeG opened this issue May 7, 2021 · 17 comments
Closed

Heads-up: npm may be looking to adopt node-gyp version 8.x soon #2392

DeeDeeG opened this issue May 7, 2021 · 17 comments

Comments

@DeeDeeG
Copy link
Contributor

DeeDeeG commented May 7, 2021

See this comment from the engineering manager of npm: npm/cli#2839 (comment)

[ . . . ] when we upgrade to node-gyp@8.x in the very near future.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 7, 2021

I am aware there are some open bugfix PRs here at the node-gyp repo, so I think it would be nice to get some of those reviewed and/or merged before then. But I am biased, as I am the author of some of those PRs! In any case, I thought it would be good to pass this comment, from an important npm maintainer, along to the folks at this repo.

I don't know how soon he means by "the very near future," and I wouldn't hold him to it based on a comment on a tangentially related issue. But at the same time it's a pretty good hint.

For additional context, there was also this comment about a month ago, by another npm core contributor (and probably maintainer?): npm/cli#3030 (comment)

@rvagg
Copy link
Member

rvagg commented May 8, 2021

@DeeDeeG perhaps you can help progress #2286? I'd really like to get to a point where it's much easier for others to merge. Right now, because we use the nodejs/node style of metadata but don't have the nice tooling, there's big friction to get merges happening. If we can sort that out then we can make more steady progress and have a larger number of people performing merges and then even be on a path toward a more regular, perhaps automatic, release process.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 8, 2021

I have been looking at/becoming familiar with git-node https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md and I have a sneaking suspicion it might "just work" here too? (I haven't tried it yet (or honestly looked that closely), but maybe?)

(If not, perhaps it can be patched or forked to work here)

Acknowledging #2286, and I'm going to say that I haven't looked at that closely enough before to understand what's going wrong, but sure if that helps un-block things here it sounds like a really good idea.

[Update: #2286 is resolved via #2395. So various maintainers are un-blocked for easily merging PRs without worrying about commit metadata so much.]

@wraithgar
Copy link
Contributor

Trip report: we will not be updating to node-gyp@8 in the current major version of npm because our support contract is for all of node@10, and node-gyp@8 only works in node@10.12.0 or newer because of a recursive flag used in mkdir.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Jul 2, 2021

I appreciate the communication! (Although I am just a contributor to this repo, not a maintainer, I try to make sure my contributions will not disrupt npm, but it is easier to visualize possible impact if I can picture when changes here might land in npm.)

I believe the fs.mkdir with recursive option was added in node-gyp 7, which is already in use in npm 7. It shouldn't affect use of node-gyp rebuild at all, only other node-gyp subcommands run manually in certain circumstances (#2152), so by extension I think npm users are not affected. As far as I know, the only node-gyp subcommand npm tries to run is node-gyp rebuild. In any case, npm 7 series is already affected if there is some impact I've failed to think of.

(Potential impact: Custom package lifecycle scripts that invoke node-gyp configure or node-gyp install subcommands directly, for some reason, could trigger the bug (#2152). Does anyone do this? (node-gyp rebuild is more sensible for packages' lifecycle scripts, in my opinion.) npm's copy of node-gyp appears to be invoked in this case, and can be run with arbitrary subcommands, including the affected install and configure.

Seems pretty niche to me, though. node-gyp rebuild is an all-in-one that runs node-gyp install only if it has to fetch development headers for the given version of Node (that is, if the user doesn't already have them), and node-gyp clean to delete the existing build dir, then node-gyp configure to prepare the build, and node-gyp build to actually build the native module. It's simpler and IMO more appropriate for package authors to just use node-gyp rebuild than directly run any of the component parts of it as subcommands.)

@wraithgar
Copy link
Contributor

Yep there it is, bigger than life. https://github.com/nodejs/node-gyp/blob/v7.1.2/lib/configure.js#L75

So moving to 8 won't break any more than is already broken.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Jul 2, 2021

If anything, I would potentially worry about Debian 9 "Stretch" users being upset, as that distro ships Python 3.5 (https://packages.debian.org/stretch/python3), and node-gyp 8 requires Python 3.6 or newer.

So they'd be looking at manually installing a newer Python version some way or other, not from the distro repositories, which is not something you normally have to do as a Debian user.

Edit: I also forgot that Python 2 is totally dropped as well. Python 2.x and 3.5.x are both unsupported series of Python now, but it could still be disruptive or considered breaking to not support those versions of Python anymore.

@wraithgar
Copy link
Contributor

The Python support is a tricky question because I don't know that we've ever had any "contract" for how we support something that tangential to our setup.

If we do end up fixing the "user's can't set their own node-gyp" then I think it's something we can consider doing. We'll discuss this more later this month.

@cclauss
Copy link
Contributor

cclauss commented Jul 2, 2021

Python support is a tricky question because I don't know that we've ever had any "contract" for how we support something that tangential to our setup.

This repo is 88.8% Python and is 9.6% JavaScript so tangential might be the wrong word. README.md and setup.py are both quite clear about the version contract we keep.

@wraithgar
Copy link
Contributor

Python support is a tricky question because I don't know that we've ever had any "contract" for how we support something that tangential to our setup.

This repo is 88.8% Python and is 9.6% JavaScript so tangential might be the wrong word. README.md and setup.py are both quite clear about the version contract we keep.

That's a good point, I meant tangential in the sense of what we "support" from npm. Typically it's been about the node version. Our testing matrix tests osx, windows, and linux but not across a big range of distributions.

We do our best to support what we can, but from an npm cli standpoint this is definitely a grey area. For instance there are forms of openssh/openssl we don't support, and dropping support for older versions did not constitute a semver major in npm in the past.

We will discuss this further in the near future, but it is looking like this isn't going to be a blocker for us updating to node-gyp@8 in npm@7, especially considering we are going to be fixing the bug that prevents users from providing their own version if they need.

@wzrdtales
Copy link

wzrdtales commented Jul 27, 2021

I stumbled upon this b/c this completely broke the functionality of the official node docker containers (pnpm seems to have adopted a newer node-gyp release before it was really ready for the ecosystem).

The node:14 (or other version without special flag like alpine) container is broken already since it delivers the wrong python version, that is a major bummer. While I don't care at all about python, so I wouldn't care if the version would be updated, that has to be done, for the main docker image that most consume.

@wraithgar
Copy link
Contributor

wraithgar commented Jul 27, 2021

@wzrdtales thanks for adding to this conversation. Could you clarify what you mean here so I'm sure I understand? The current node:14 docker image is broken already, and requires an update in python to work? What is broken currently, that is what is broken about it/what doesn't work?

@cclauss
Copy link
Contributor

cclauss commented Jul 27, 2021

Node-gyp no longer supports end-of-life versions of Python that the Python core team no longer supports...
Supported: https://devguide.python.org/#branchstatus
Not supported: https://devguide.python.org/devcycle/#end-of-life-branches

@wzrdtales
Copy link

wzrdtales commented Jul 27, 2021 via email

@wraithgar
Copy link
Contributor

Thanks @cclauss, I get that's what node-gyp's contract is. We're just trying to weight the impact of updating in the current semver major version of npm. @wzrdtales feedback only further supports a decision to move forward with this once we fix the underlying bug wrt specifying an alternate node-gyp.

We really appreciate all the careful thought and work that went into this discussion.

@wzrdtales
Copy link

@wraithgar That might be an issue of the team managing the docker node container as well, since they essentially build them on outdated versions.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Oct 10, 2021

node-gyp 8.2.0 is in npm 8.0.0, which is bundled with Node 16.11.0.

See the PR that bumped node-gyp in npm:

And the node-gyp dependency's semver range '^8.2.0' in npm 8.0.0's package.json:

The PR landed to npm's release-next branch on October 7th, and npm v8.0.0 was released about four hours later.

(I will let the maintainers decide whether to leave this issue open, or close it, etc. Consider this comment a public service announcement: node-gyp 8.2.0 is now live on a lot of people's machines; Whoever is using the latest Node, 16.11.0 or greater, has it bundled by default.

Node 16.11.0 shipped on October 8th, and was bundled with npm 8, and thus node-gyp 8.)

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

No branches or pull requests

5 participants