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

feat: support gyp with ninja

Open codebytere opened this issue 4 years ago • 1 comments

Description of change

This is a draft feedback request for adding baseline ninja support to node-gyp. gyp already supports ninja out of the box, so this installs ninja to a user's computer if it doesn't exist, and then allows for its use when the --ninja flag is passed.

This is motivated by work i did recently to support Swift compilation using node-gyp: https://github.com/codebytere/node-mac-swift-addon and this commit.

The whole patchset is likely untenable to upstream, so I'd like just to seek feedback on this portion at this point!

Tests and documentation would come after initial feedback on the potential path forward here.

cc @rvagg

Checklist
  • [ ] npm install && npm test passes
  • [ ] tests are included
  • [ ] documentation is changed or added
  • [x] commit message follows commit guidelines

codebytere avatar Jul 24 '20 20:07 codebytere

Does this work on Windows too? There's a few places where if windows is bypassed entirely for the ninja case, is it safe to assume that ninja will work consistently across the board?

I'm fine with this all except for the download-ninja bit which makes me hesitant. It's checking if it's installed, but only if it's installed in the exact place where it will place it after downloading. What I wonder is - if you're using --ninja or the npm config set variation of it, wouldn't you be more likely to have ninja on your system already and therefore not need to download and install it? We don't do this for any other piece of tooling surrounding node-gyp (aside from gyp itself which is bundled), we rely on the user to handle it and point them to documentation or npm packages (like the one to install the windows tooling) to handle it. Isn't it safe enough to just say "if you want to use --ninja then make sure you have ninja on your system, here's how you might do that ..."?

rvagg avatar Jul 27 '20 05:07 rvagg