-
Notifications
You must be signed in to change notification settings - Fork 73
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
chore: add npm support #146
Conversation
Actually |
After making it an npm package, node-gyp may simply depend on it in "dependencies" field. |
@ryzokuken Why are so many tests failing? Do we need to work on our tests? Rerunning failing tests... Still fail. |
What is the relationship between detect-python-interpreter and find-python.js ? Do they both have the same kind of test coverage because gyp gets run on a lot of esoteric hardware and OSes these daze. |
Maybe we can improve to pass the priority in |
What issue does it solve? |
|
I don't know if it something that node-gyp wants, but from the Node.js perspective, it is unlikely to happen, as node needs to copy so we can build it offline from the tarball. |
@cclauss Looks like changes were made to gyp in Node without properly being upstreamed here first: nodejs/node#40735. @targos I am sympathetic to the use-case of building Node.js from a tarball but how could we keep gyp-next in the tree and avoid this from happening over and over again? Perhaps include it as a git submodule instead? |
The same question applies to all our bundled dependencies. I don't have a perfect solution, but for now reviewers should pay attention to this. |
Edit: I opened nodejs/node-gyp#2645 |
@targos https://github.com/nodejs/node/tree/master/tools/node_modules/eslint ESLint stays in |
Maybe, but I don't see any advantage to doing so. And if we merge this, we need to manage npm releases, which is more burden for a project that has almost no maintainer. |
fair point.
I agree. This is the opposite of what we want, we should be instead reducing the maintenance cost of this project as much as possible. |
I don't think this is going to make things any better for Node.js core. For node-gyp though it might make maintenance of node-gyp easier (albeit slightly increasing the maintenance burden here) -- at the moment there is a lot of confusion over gyp-next vs node-gyp and we frequently get PR's opened in node-gyp that are actually gyp changes that should be made here. Doing this and then making gyp-next a dependency for node-gyp would allow us to remove the gyp copy in node-gyp so the only code left there would be the Node.js wrapper. |
@richardlau would we need npm for that? Couldn't that also be done with a git submodule for example? |
Other package managers are available 🙂. The point would be to make, from node-gyp's point of view, gyp-next the same as any other dependency node-gyp currently has. You already need to |
I won't object to this PR, but keep in mind that if we merge, someone's going to have to setup and maintain the npm release. |
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.
Sorry but I am going to object to this PR.
detect-python-interpreter
only tells us if some version of Python 2 or 3 is installed. It does not tell us what that version is and if that version meets our build requirements. We do not need additional support requests from users who are attempting to build with legacy Python.
Also, let's fix the tests before approving a bunch more PRs with failing tests.
Closing. This repo is all Python code and no JavaScript code. The code is released into PyPI which is the Python equivalent of npm. https://github.com/nodejs/gyp-next/blob/main/README.md |
Someday we may import gyp-next as an npm package. just like clang-format. Even, we can replace this in node repo.