gyp-next icon indicating copy to clipboard operation
gyp-next copied to clipboard

chore: add npm support

Open XadillaX opened this issue 2 years ago • 18 comments

Someday we may import gyp-next as an npm package. just like clang-format. Even, we can replace this in node repo.

XadillaX avatar Apr 12 '22 11:04 XadillaX

Actually node-gyp got the full clone, maybe redundant for npm package. https://github.com/nodejs/node-gyp/tree/master/gyp

gengjiawen avatar Apr 13 '22 01:04 gengjiawen

Actually node-gyp got the full clone, maybe redundant for npm package. https://github.com/nodejs/node-gyp/tree/master/gyp

After making it an npm package, node-gyp may simply depend on it in "dependencies" field.

XadillaX avatar Apr 13 '22 02:04 XadillaX

@ryzokuken Why are so many tests failing? Do we need to work on our tests? Rerunning failing tests... Still fail.

cclauss avatar Apr 13 '22 10:04 cclauss

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.

cclauss avatar Apr 13 '22 11:04 cclauss

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.

detect-python-interpreter only simply tests python* one by one and returns the first succeeded one on python* -v.

Maybe we can improve to pass the priority in detect-python-interpreter.

XadillaX avatar Apr 13 '22 12:04 XadillaX

What issue does it solve?

targos avatar Apr 13 '22 12:04 targos

What issue does it solve?

  1. Projects depend on it can remove the hard copy, and use it in a package.json. E.g. Node.js (like https://github.com/nodejs/node/pull/42724)
  2. node-gyp may replace the hard copy with a dependency.

XadillaX avatar Apr 14 '22 05:04 XadillaX

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.

targos avatar Apr 14 '22 06:04 targos

@ryzokuken Why are so many tests failing? Do we need to work on our tests? Rerunning failing tests... Still fail.

@cclauss Looks like changes were made to gyp in Node without properly being upstreamed here first: https://github.com/nodejs/node/issues/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?

ryzokuken avatar Apr 14 '22 09:04 ryzokuken

how could we keep gyp-next in the tree and avoid this from happening over and over again?

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. BTW, I tried to do https://github.com/nodejs/node/issues/40735 myself, but the changes are really non-trivial and I'm afraid to introduce them here and break node-gyp users.

targos avatar Apr 14 '22 10:04 targos

node-gyp integration errors should be fixed in the node-gyp repo: not ok 2 - request to https://localhost:49373/ failed, reason: certificate has expired

Edit: I opened https://github.com/nodejs/node-gyp/issues/2645

targos avatar Apr 14 '22 10:04 targos

@targos https://github.com/nodejs/node/tree/master/tools/node_modules/eslint

ESLint stays in node_modules and be added to repository tree. Maybe we can do it for gyp-next either?

XadillaX avatar Apr 14 '22 11:04 XadillaX

@targos https://github.com/nodejs/node/tree/master/tools/node_modules/eslint

ESLint stays in node_modules and be added to repository tree. Maybe we can do it for gyp-next either?

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.

targos avatar Apr 14 '22 11:04 targos

the changes are really non-trivial and I'm afraid to introduce them here and break node-gyp users.

fair point.

And if we merge this, we need to manage npm releases, which is more burden for a project that has almost no maintainer.

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.

ryzokuken avatar Apr 14 '22 14:04 ryzokuken

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 avatar Apr 14 '22 16:04 richardlau

@richardlau would we need npm for that? Couldn't that also be done with a git submodule for example?

ryzokuken avatar Apr 15 '22 08:04 ryzokuken

@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 npm install (or your package manager's equivalent) node-gyp in order to use it or run its tests if maintaining it. We do not, for example, vendor in the tar module into node-gyp. The main reason gyp was vendored into node-gyp was that when gyp was maintained by Google it was not versioned or published to a package repository.

richardlau avatar Apr 15 '22 12:04 richardlau

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.

targos avatar Apr 15 '22 13:04 targos

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

cclauss avatar Jun 17 '24 22:06 cclauss