-
Notifications
You must be signed in to change notification settings - Fork 29k
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
tools: remove gyp from tools directory #22343
Conversation
@maclover7 sadly an error occured when I tried to trigger a build :( |
This is on hold until #22342 lands, which contains |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do that. The node-gyp that is a part of npm is managed by npm not us. We should not add a dependency on that.
Rely on gyp via npm's node-gyp dependency. All changes to gyp must now be sent to https://github.com/nodejs/node-gyp.
f04bcd3
to
9adbddb
Compare
RE that; those two forks have been "intentionally" diverging. IMHO a better solution would be to take responsibility of CIing patches, and vendoring to those two locations. I'm working on that, but time... BTW: we have many opportunities for optimizations and streamlining of our build process via GYP. /CC @nodejs/gyp |
I had thought of node core's gyp as being a kind of testing ground for the community version distributed via node-gyp. In theory, any patches that fix bugs in gyp should be rolled out to all end users via node-gyp, and shouldn't exclusively live in node core's gyp. Also, it has been difficult keeping more than one copy of gyp in sync, and this commit creates a single place where gyp maintenance efforts can be concentrated. Since Node.js maintains this codebase now, this alone is a good enough reason, for me at least, to land this. |
Since Node.js maintains node-gyp as well, I think this is fine. Especially since each npm update that would change something would result in failures on our side and we would be able to solve them then. |
@nodejs/gyp |
IMHO it's hasn't been that difficult.
That is not officially true, the upstream project is not dead, and the node org did not official take responsibility for GYP Anyway, adding a dependency on npm IMHO is sub-optimal. Especially since: node/doc/guides/maintaining-npm.md Lines 7 to 9 in 2ce0380
i.e. if a bug or update is needed, we'll need to patch node-gyp , release a version, get it landed in npm , wait for npm release, then wait at least two weeks before it can land.This also essentially excludes fixing in backports. tl;dr I'm a hard -1, if needed this can go the TSC. |
So we have to accept that we may end up carrying patches on top of npm, in the same way we carry patches on OpenSSL and V8 etc.? |
Why do that, when the status quo is IMHO better? We have an easy to use and maintain version of the tool. The benefits of removing this copy are minimal, and outweighed by the cost. |
Also as I wrote above, our current dependency is on If we were to replace |
IMHO the question is about the tradeoffs of maintaining an internal version of a tool (node core's copy of gyp) while we already have to maintain/release/support essentially the same thing, but for the whole community (node-gyp's copy of gyp). The patches from node core's copy of gyp have already been applied to node-gyp, so we are now carrying two essentially identical gyp directories in nodejs/node (building addons doesn't even use node core's copy of gyp). It would seem to be better, to me at least, to centralize long-term maintenance efforts around the community version, so everyone can benefit from bugfixes, etc, and then to treat node core like any other downstream user, and float patches as necessary. |
@maclover7 Any chance you're still around and want to see this through? If so, let's get it on TSC agenda. If not, anyone else interested in this or should it be closed? @nodejs/build @BridgeAR |
Regarding Refael's concerns: on one hand npm have been very responsive to taking node-gyp updates, plus we have a precedent of patching the npm we release where it's served us (including node-gyp, e.g. 8b4af64, 0454725). On the other hand .. what about when this makes its way into LTS branches where we don't upgrade npm much but node-gyp becomes a problem for us for whatever reason and the fixes involve changes that might conflict with npm's use of node-gyp—or at least be undetermined and the risk of us breaking npm for downstream LTS users is large? That's going to put us in a bit of a corner with the easiest path to re-introduce our own fork into tools/ or deps/ again. |
@nodejs/tsc it was asked to have a look at this due to the -1. @maclover7 this needs a rebase. |
Maybe @nodejs/python or @nodejs/gyp would have thoughts. Based on @rvagg's comment above, I'd be inclined to go with whatever the most active Python/gyp folks on the project think is best, because they're the ones who are most likely to do the work to address any pain associated with whatever approach we take here. |
I share some of @refack concerns, but I’m happy to delegate this decision to the people that are going to do the work. |
Well this has stalled for a log time, and based on the resent feedback IMHO it could be closed, so I'm going to cast the die. |
Rely on gyp via npm's node-gyp dependency. All changes to gyp
must now be sent to https://github.com/nodejs/node-gyp.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes