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

Bundling/preinstall woes #260

Closed
springmeyer opened this issue Dec 16, 2016 · 9 comments
Closed

Bundling/preinstall woes #260

springmeyer opened this issue Dec 16, 2016 · 9 comments

Comments

@springmeyer
Copy link
Contributor

springmeyer commented Dec 16, 2016

Ticket to keep track of edge cases around applications bundling node-pre-gyp (which was the original recommendation but alternatives were proposed in #162) and not bundling (which was documented as an option in #201).

More details: fsevents/fsevents#157 (comment)

@tjwebb
Copy link

tjwebb commented Jan 22, 2017

may lead to unexplainable hangs with various npm versions:

They are not unexplainable, they are a logical result of using npm/package.json in a way they are not designed to be used. You've already read my explanation: mapnik/node-mapnik#650 (comment)

@reconbot
Copy link

reconbot commented Jan 22, 2017 via email

@tjwebb
Copy link

tjwebb commented Jan 24, 2017

Hey @reconbot! Thanks for visiting us in Norfolk yesterday.

and has a direct dependency on a dependency of node-pre-gyp - likely will be de-duped and breaks node v0.10.x install when node-pre-gyp is bundled and the dep is missing

This can be solved by setting NODE_PATH=node_modules. Running a separate npm install inside of npm install feels like a really heavy-handed solution for a path problem. This creates issues newer (npm3+) node versions in order to fix issues in older (~npm2) node versions. The direction of this tradeoff seems backwards to me. If breakage is required, we should break old versions, not new ones.

Another issue here is some modules' reliance on the particular way npm2/npm3 structure the node_modules folder. Some of these issues can be avoided by being a little bit cute with the paths in binding.gyp, e.g. https://github.com/langateam/node-mapnik/blob/master/binding.gyp#L60-L61. This bullet point describes a slightly different issue, but it's in the same theme of path problems introduced by different npm versions.

@springmeyer springmeyer changed the title Bundling woes Bundling/preinstall woes Mar 14, 2017
springmeyer added a commit to springmeyer/node-addon-example that referenced this issue Mar 14, 2017
@springmeyer
Copy link
Contributor Author

Thanks everyone for the feedback. I've reverted #201 by applying #269, which changes the documentation to recommend bundling. Because this is only a documentation change the work ahead then is to:

  • Find modules using the preinstall method
  • Add back bundledDependencies and the "prepublish": "npm ls" (the latter to help avoid incorrect bundling)
  • Use this ticket to drop ideas or problems with ^^

/cc @tjwebb @reconbot

@SomeoneElseOSM
Copy link

SomeoneElseOSM commented Apr 9, 2017

What's the best way to handle "anecdotes of failure"? I've just had the familiar

npm ERR! Tell the author that this fails on your system:
npm ERR!     node-pre-gyp install --fallback-to-build

on another system but I presume that you don't want this issue clogged up with with each individual failure and stacktraces. Are there any particular bits of information that are useful to obtain?

(for completeness, in case it invalidates this as relevant to this issue - this was with the LTS version of node as recommended at kosmtik/kosmtik#232 (comment) ).

@springmeyer
Copy link
Contributor Author

@SomeoneElseOSM kosmtik/kosmtik#232 is likely resolvable by downgrading to npm to 2.x or upgrading node-mapnik to 3.6.x.

rafatower pushed a commit to CartoDB/node-mapnik that referenced this issue Jan 5, 2018
@pronebird
Copy link

I always get X is missing a bundled dependency "node-pre-gyp". Is it ok for modules shipped over GitHub and not npm?

@springmeyer
Copy link
Contributor Author

I always get X is missing a bundled dependency "node-pre-gyp". Is it ok for modules shipped over GitHub and not npm?

This is expected/normal @pronebird - generally this is okay for modules coming via github, because (if I recall correctly), in this case npm will not dedupe. This lack of deduping avoids one of the problems that requires bundling in the case of an npm install: which is that:

  • you might have a project that depends on > 1 module that uses node-pre-gyp
  • npm might dedupe node-pre-gyp (when pulling via the npm repository)
  • then you might have one of the modules fail to install because node-pre-gyp is not available on PATH
    when it is needed (due to deduping)

@springmeyer
Copy link
Contributor Author

Closing this. As long as you are running node > 4:

  • You should not need to bundle deps
  • You should not need to preinstall node-pre-gyp
  • Just list node-pre-gyp in your deps. It should be installed early on in the process such that it can be used to install the binaries for the module it is a dependency of

If anyone finds otherwise, please comment here. But node-sqlite3 has been released recently without any problems without using preinstall or bundledDependencies, so I'm considering this issue resolved.

refs #403

springmeyer added a commit to mapbox/mapbox-gl-native that referenced this issue Jul 23, 2018
This is an obsolete workaround for old npm versions that broke with bundling but also did not install deps predictably.

Refs:

 - https://github.com/mapbox/node-pre-gyp/blob/master/README.md#configuring
 - mapbox/node-pre-gyp#260
jfirebaugh pushed a commit to mapbox/mapbox-gl-native that referenced this issue Jul 23, 2018
This is an obsolete workaround for old npm versions that broke with bundling but also did not install deps predictably.

Refs:

 - https://github.com/mapbox/node-pre-gyp/blob/master/README.md#configuring
 - mapbox/node-pre-gyp#260
pronebird added a commit to mullvad/windows-security that referenced this issue Jul 24, 2018
springmeyer added a commit to Project-OSRM/osrm-backend that referenced this issue Aug 22, 2019
Bundling node-pre-gyp was only needed for node v4 and earlier.

Remove it to avoid errors like:

> npm ERR! enoent ENOENT: no such file or directory, rename '/home/circleci/project/node_modules/node_or_tools/node_modules/abbrev' -> '/home/circleci/project/node_modules/node_or_tools/node_modules/.abbrev.DELETE'

More context: mapbox/node-pre-gyp#260 (comment)
danpat pushed a commit to Project-OSRM/osrm-backend that referenced this issue Jan 27, 2021
Bundling node-pre-gyp was only needed for node v4 and earlier.

Remove it to avoid errors like:

> npm ERR! enoent ENOENT: no such file or directory, rename '/home/circleci/project/node_modules/node_or_tools/node_modules/abbrev' -> '/home/circleci/project/node_modules/node_or_tools/node_modules/.abbrev.DELETE'

More context: mapbox/node-pre-gyp#260 (comment)
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