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

feat: support gyp with ninja #2171

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jul 24, 2020

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
  • commit message follows commit guidelines

@codebytere codebytere requested a review from rvagg July 24, 2020 20:00
@rvagg
Copy link
Member

rvagg commented Jul 27, 2020

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 ..."?

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

Successfully merging this pull request may close these issues.

2 participants