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

Update node-gyp to the latest version 🚀 #642

Merged
merged 1 commit into from
Jun 16, 2019

Conversation

greenkeeper[bot]
Copy link
Contributor

@greenkeeper greenkeeper bot commented Jun 13, 2019

The devDependency node-gyp was updated from 4.0.0 to 5.0.0.

This version is not covered by your current version range.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.


Find out more about this release.

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.


Your Greenkeeper bot 🌴

@vweevers
Copy link
Member

I want to test this on Windows because of:

@vweevers
Copy link
Member

Oh, this doesn't matter, because prebuildify uses the node-gyp that's available in PATH and when npm runs scripts, it adds the global node_modules/npm-lifecycle/node-gyp-bin folder to PATH. Effectively, we're always using the npm-bundled version of node-gyp (also for npm run rebuild).

@ralphtheninja Is that desirable? We're making prebuilds on node current which should ship a recent-ish npm + node-gyp. OTOH, we can't guarantee that Travis and AppVeyor use the same version of node-gyp. And testing things locally - depending on what you've got installed - is not representative of either.

As I see it we have two options:

  1. Fix prebuildify to prefer a locally-installed node-gyp
  2. Remove the unused devDependency here.

Anyway, I tested prebuildify paired with node-gyp@5 by setting npm_config_node_gyp. The resulting binary works fine in node and electron 4 and 5. Electron@3 still fails, as half-expected.

@vweevers vweevers merged commit 5a1c044 into master Jun 16, 2019
@vweevers vweevers deleted the greenkeeper/node-gyp-5.0.0 branch June 16, 2019 12:54
@ralphtheninja
Copy link
Member

Hmm, I think we should fix prebuildify to prefer a locally installed version, otherwise the user have no control over node-gyp whatsoever.

@vweevers
Copy link
Member

the user

Do you mean the consumer (if so, we also have to move node-gyp from devDeps to deps) or us?

@ralphtheninja
Copy link
Member

Sorry, us :)

@vweevers
Copy link
Member

=> prebuild/prebuildify#30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants