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

node-pre-gyp as non bundled dependency #162

Closed
3y3 opened this issue May 5, 2015 · 39 comments
Closed

node-pre-gyp as non bundled dependency #162

3y3 opened this issue May 5, 2015 · 39 comments

Comments

@3y3
Copy link

3y3 commented May 5, 2015

How I understand the main reason to bundle node-pre-gyp is a usage it on installation phase.
And what happens if it will be used in postinstall step, is this resolves problem with bundling?

    "scripts": {
        "postinstall": "node-pre-gyp install --fallback-to-build",
    },

Unfortunately I can't test this now, but maybe the answer is on the surface?

@springmeyer
Copy link
Contributor

Moving to postinstall might work to avoid the chicken 'n egg problem of node-pre-gyp needing to be available at install. But install would still need to be overridden to prevent npm from detecting the binding.gyp and calling node-gyp to build. So, overall: maybe this would work but it would need a lot of testing and we'd need to figure out what would make sense to put into the install to make it do nothing.

@3y3
Copy link
Author

3y3 commented May 6, 2015

But install would still need to be overridden to prevent npm from detecting the binding.gyp and calling node-gyp to build

We need to redefine npm preinstall script. proof link

@3y3
Copy link
Author

3y3 commented May 6, 2015

@springmeyer , this works like a charm on my Windows machine:

  "scripts": {
    "preinstall": "rem",
    "postinstall": "node-pre-gyp install --fallback-to-build"
  }

Can you test something like this for Linux or Mac. (I'm not sure which dummy command is preferable in bash).

@rmg
Copy link

rmg commented May 12, 2015

The install script is run after dependencies are installed. It is essentially a pre-postinstall, but with a default that auto-runs node-gyp.

@3y3
Copy link
Author

3y3 commented May 12, 2015

@rmg , my current solution (it tested on v8-debug and v8-profiler) is:

  "scripts": {
    "preinstall": " ",
    "postinstall": "node-pre-gyp install --fallback-to-build"
  }

Can you propose something more elegant?

@rmg
Copy link

rmg commented May 12, 2015

@3y3 nothing more elegant, sorry.

My point was just that you can use install instead of postinstall:

 "scripts": {
    "preinstall": " ",
    "install": "node-pre-gyp install --fallback-to-build"
  }

@springmeyer
Copy link
Contributor

My point was just that you can use install instead of postinstall:

@rmg - yes, but I think @3y3's point is that approach (the current, recommended approach) also necessitates node-pre-gyp being a bundledDependency as per https://github.com/mapbox/node-pre-gyp#1-add-new-entries-to-your-packagejson. And this is what @3y3 would like to avoid. The need for being a bundled dep is because node-pre-gyp needs to be fully installed before npm tries to install the module that uses it. Otherwise you can see errors where node-pre-gyp is only partially installed and cannot run because it is missing dependencies it expects. If you already understand this, sorry maybe I am missing something. Definitely happy to have more eyes on this problem.

@3y3
Copy link
Author

3y3 commented Jun 23, 2015

@springmeyer , I switched to @rgm solution in production and it works fine. So there is no big difference between install and postinstall in this case.

@springmeyer
Copy link
Contributor

@3y3 - hmm, can you explain in more detail what your solution is? You've removed the bundling and added an empty preinstall only? Explain to me how that solves the problem exactly? I think I'm missing something.

@springmeyer
Copy link
Contributor

Also, keep in mind that bundling is also needed when your app depends on more than one dependency packaged with node-pre-gyp. Otherwise if:

  • node-pre-gyp is not bundled
  • npm's auto-deduping decides to not install node-pre-gyp for a given dependency
  • then node-pre-gyp binary will not be available on the PATH and the install will break

More details at #61 (comment)

@rmg
Copy link

rmg commented Jun 23, 2015

@springmeyer the empty preinstall is just to suppress the default value, which is "node-gyp rebuild".

The shared dependency case is a really good point, though. I hadn't thought of that.

@3y3
Copy link
Author

3y3 commented Jun 24, 2015

Also, keep in mind that bundling is also needed when your app depends on more than one dependency packaged with node-pre-gyp

Reasonable. How about solution from #61

"install": "node -e \"require('node-pre-gyp').install()\""

I don't say that this is a final solution, but this can be good start.
My dream is to see in readme something like this:

You can simple add node-pre-gyp to bundled dependencies or < do something other >

@springmeyer
Copy link
Contributor

@3y3 that did not work when I tried it before. But it has been too long for me to remember why exactly. Reading my old comments I can see that multiple issues might have been confused. Another was that if node-pre-gyp is downloaded dynamically and shares deps those deps might also be dedupes and not available in time.

However I think it is worth revisiting all this because I agree with you that is would be great to avoid the requirement. I don't have time to test all the scenarios right now so thanks for your efforts!

@bminer
Copy link

bminer commented Jan 7, 2016

Any other thoughts on this issue? I'm confused as to why package maintainers are forced to bundle node-pre-gyp as a dependency. This is problematic... if many native libs using node-pre-gyp live in the same project, there's a bunch of extra bloat (even if all those native libs require compatible versions of node-pre-gyp).

Maybe as @3y3 mentioned, we can give package maintainers more options?

@rmg
Copy link

rmg commented Jan 7, 2016

Before npm v3, there was no guarantee that the required dependencies would be installed before the scripts run. Bundling is the only way to guarantee it in npm < 3.x.

@bminer
Copy link

bminer commented Jan 7, 2016

I'm confused. Why doesn't this work in the package.json file (assuming node-pre-gyp is just a dependency and not a bundledDependency)?

  "scripts": {
    "preinstall": " ",
    "postinstall": "node-pre-gyp install --fallback-to-build"
  }

This ensures node-pre-gyp is run after dependencies for node-pre-gyp are installed, right?

@bminer
Copy link

bminer commented Jan 7, 2016

Is it really true that prior to npm v3, deps are not guaranteed to be downloaded before scripts are run? I find that to be... weird and awful.

Also, what about this? "preinstall": "npm install node-pre-gyp" Anyone tried that?

@3y3
Copy link
Author

3y3 commented Jan 8, 2016

Mmm... I like "preinstall": "npm install node-pre-gyp"
At current time I use

    "preinstall": "node -e 'process.exit(0)'",
    "install": "node-pre-gyp install --fallback-to-build",

in v8-debug and v8-profiler. There is no issues related with this solution for me.
But in next version I will think about "preinstall": "npm install node-pre-gyp"

@springmeyer
Copy link
Contributor

Tested using preinstall and it works great - landed new docs in #201

@reconbot
Copy link

I tried to do with for node-serialport and ended up with an endless pre-install loop when it was installed as a dependency. Anyone else have any issues?

@springmeyer
Copy link
Contributor

@reconbot not seen that, can you share the exact details of how to replicate?

@reconbot
Copy link

reconbot commented Apr 25, 2016

We published serialport@2.1.1 with the recommended settings and had to quickly push 2.1.2 reverting it.

https://github.com/voodootikigod/node-serialport/blob/2.1.1/package.json

You can still try to install it. npm i serialport@2.1.1 using node v4.4.3 (npm v3.8.6)

@springmeyer
Copy link
Contributor

springmeyer commented Apr 25, 2016

@reconbot just tested and I cannot replicate a problem. The installs run fast and to completion. There is no hang or "endless loop". I tried both:

  • npm i serialport@2.1.1
  • npm i serialport@2.1.1 -g

With both:

  • node v0.10.40 / npm 1.4.28
  • node v4.4.3 / npm 2.15.1
  • node v4.4.3 / npm v3.8.6 (upgraded)

@springmeyer
Copy link
Contributor

Can't (edited commit to make that more clear, sorry)

@springmeyer
Copy link
Contributor

@reconbot - any updates? I'm still not able to replicate and so I'm not aware of any problem.

springmeyer pushed a commit to osmcode/node-osmium that referenced this issue Apr 28, 2016
@reconbot
Copy link

reconbot commented May 1, 2016

We're about to have another release, I've removed the extra rebuild script, so lets see what happens.

@reconbot
Copy link

reconbot commented May 1, 2016

Still seeing it with

  • node v0.12.7
  • npm v3.8.3

@bminer
Copy link

bminer commented May 2, 2016

@reconbot - Does this problem happen for the latest version of Node + NPM?

@springmeyer
Copy link
Contributor

node v0.12.7
npm v3.8.3

I installed v0.12.7 with nvm on OSX, then upgraded npm with npm install npm@3.8.3 -g and when running npm i serialport@2.1.1 I see the install pause on:

validateTree              ▀ ╢██████████████████████████████████████████████████████████░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░╟

But it eventually gets past that. Then the overall install fails with:

/Users/dane/.nvm/versions/node/v0.12.7/lib
└── (empty)

npm ERR! Darwin 15.4.0
npm ERR! argv "node" "/Users/dane/.nvm/versions/node/v0.12.7/bin/npm" "i" "serialport@2.1.1" "-g"
npm ERR! node v0.12.7
npm ERR! npm  v3.8.3
npm ERR! path /Users/dane/.nvm/versions/node/v0.12.7/lib/node_modules/.staging/abbrev-c46124c0
npm ERR! code ENOENT
npm ERR! errno -2

npm ERR! enoent ENOENT, rename '/Users/dane/.nvm/versions/node/v0.12.7/lib/node_modules/.staging/abbrev-c46124c0'
npm ERR! enoent ENOENT, rename '/Users/dane/.nvm/versions/node/v0.12.7/lib/node_modules/.staging/abbrev-c46124c0'
npm ERR! enoent This is most likely not a problem with npm itself
npm ERR! enoent and is related to npm not being able to find a file.
npm ERR! enoent 

However I don't see this as related to node-pre-gyp. Seems like an npm bug to me. Both of the above problems go away if I do:

npm install npm@2.x -g
npm i serialport@2.1.1

@springmeyer
Copy link
Contributor

The ENOENT error looks like npm/npm#10995

@reconbot
Copy link

reconbot commented May 4, 2016

I've taken another run at this. Its working on node 5 with npm 3.x, and node 0.12 and npm 2.x. But I see the loop with node 0.12 and npm 3x. I'm thinking it's an npm bug.

I'm using npm install voodootikigod/node-serialport#unbundle which has only node-pre-gyp in the preinstall and doesn't overload the rebuild hook.

kidrobot:tmp$ node -v
v0.12.7
kidrobot:tmp$ npm -v
3.8.8
kidrobot:tmp$ npm install voodootikigod/node-serialport#unbundle

> serialport@3.0.1 preinstall /Users/wizard/src/tmp/node_modules/.staging/serialport-a4f0ad8f
> npm install node-pre-gyp


> serialport@3.0.1 preinstall /Users/wizard/src/tmp/node_modules/.staging/serialport-a4f0ad8f/node_modules/.staging/serialport-13fe77f0
> npm install node-pre-gyp


> serialport@3.0.1 preinstall /Users/wizard/src/tmp/node_modules/.staging/serialport-a4f0ad8f/node_modules/.staging/serialport-13fe77f0/node_modules/.staging/serialport-074522d4
> npm install node-pre-gyp


> serialport@3.0.1 preinstall /Users/wizard/src/tmp/node_modules/.staging/serialport-a4f0ad8f/node_modules/.staging/serialport-13fe77f0/node_modules/.staging/serialport-074522d4/node_modules/.staging/serialport-2138bf23
> npm install node-pre-gyp


> serialport@3.0.1 preinstall /Users/wizard/src/tmp/node_modules/.staging/serialport-a4f0ad8f/node_modules/.staging/serialport-13fe77f0/node_modules/.staging/serialport-074522d4/node_modules/.staging/serialport-2138bf23/node_modules/.staging/serialport-01c0e617
> npm install node-pre-gyp
^C

@bminer
Copy link

bminer commented May 4, 2016

Hm, weird... thanks for submitting the npm issue, @reconbot ! We'll see what happens...

@reconbot
Copy link

reconbot commented May 4, 2016

Doing some more research and testing I've found that having node-pre-gyp in our dependencies and putting node-pre-gyp install --fallback-to-build in the install hook works just fine in all node versions with the latest npm, it doesn't try to build it locally at all. Is there a reason why we're not going with that approach?

@reconbot
Copy link

reconbot commented May 4, 2016

It's also's https://github.com/mafintosh/prebuild#installing approach.

@reconbot
Copy link

reconbot commented May 5, 2016

Hey @3y3 you found a bug in the npm docs! https://docs.npmjs.com/misc/scripts#default-values conflicts with https://docs.npmjs.com/files/package.json#default-values

Actually... tests indicate that if you overide either lifecycle method preinstall or install it wont compile with node-gyp.

@bminer
Copy link

bminer commented May 5, 2016

@reconbot - The install script is run after all deps are downloaded and installed. I think @springmeyer is saying that if you simply do node-pre-gyp install --fallback-to-build in the install hook, a dep that also depends on node-pre-gyp might not have node-pre-gyp available for its hooks, due to NPM's dependency de-duping mechanism. I'm not really sure how this could be, though...

@springmeyer - For clarification, could you explain the following:

  • Suppose modA depends on modB and node-pre-gyp
  • Suppose modB depends on node-pre-gyp.
  • When modA is installed, NPM will install modB and node-pre-gyp in the order it chooses, recognizing that modB also depends on node-pre-gyp. It should "know" then to install node-pre-gyp first and then install the modB dep because modB might use node-pre-gyp for its hooks. At this point, modB can run its hooks and access the node-pre-gyp binary, right? And, finally, after modB is installed, modA hooks can run and complete the installation.

Am I missing something?

@reconbot
Copy link

reconbot commented May 5, 2016

Lets test this, this is an easy test.

I added @springmeyer's node-addon-example as a dep to serialport. It uses the preinstall method. I modified node-serialport to use the dependency method. I created a new app, installed serialport. Everything installed fine.

Next I cloned the node-addon-example and modified it to use the dependency method by removing the preinstall and prepublish hooks (the prepublish hooks were causing issues when locally adding it as a dep to node-serialport). I added the new modified version as a dep to serialport and installed it in a new app. Everything installed fine across node v0.10-6 with npm 3.x.

@springmeyer I know you did a bunch of testing looking back in the thread. Everything I tested here was done locally. (not with remote packages) Perhaps you want to take a look too?

@reconbot
Copy link

reconbot commented May 6, 2016

I can confirm that specifying either a preinstall or an install script stops the default node-gyp behavior. I've submitted a pr to fix the docs.

I'm going to release a beta of serialport without the preinstall or bundle and let people test.

@reconbot
Copy link

reconbot commented May 6, 2016

Thanks for putting up with me belaboring this closed issue for a few weeks. I think I'm finally done with exploring the options.

@springmeyer was right to be concerned about modules being available. npm 2 (which still ships with node 4 LTS) does have a race condition when multiple packages require the same package, where the modules wont always be available during the install stage. I spoke with someone from npm about it, though I haven't found other docs or discussion about it.

I got raised eyebrows from the npm cli team about installing in a preinstall step. The current documentation will work for everyone on any node and any npm but node 0.12 and npm 3 together. We should probably mention that but I think it's the most widely supported method of install we could hope for.

For node serialport I'm going to stick with the bundled dependency for a while. At least until that npm bug is fixed for npm 3. We haven't had issues with that that weren't caused during publishing and we still have a lot of node 0.12 users.

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

No branches or pull requests

5 participants