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

Native addons support #57

Closed
guybedford opened this Issue Nov 26, 2018 · 4 comments

Comments

3 participants
@guybedford
Copy link
Collaborator

guybedford commented Nov 26, 2018

Assuming the target machine is the same architecture as the current machine (for this reason the suggestion would be to implement this behind a --binaries flag or similar), here's a suggested approach for remapping and emitting binaries.

The assumption is that the "patterns" for binary emission are generally only of a few types, and that we can build static analysis to detect those patterns and provide an adequate substitution for them through (1) detecting where the binary is based on this static analysis and (2) rewriting the locator code as we do for other fs relocations to point to this new binary location.

Detection Cases (not comprehensive, just what I can find on a quick review):

  1. Statically detect any calls to node-pre-gyp's find method. (literally detecting require('node-pre-gyp').find and all its natural simple variations. Eg like in https://unpkg.com/grpc@1.16.1/src/grpc_extension.js.
  2. Statically detect any calls to require('bindings')('binding.node') and its associated lookup algorithm.
  3. Direct requires to .node binaries being treated as emitted relocated externals

Rewriting

The rewriting just full replaces the statically known path. Eg require('node-pre-gyp').find(staticExpression) -> __dirname + '/emitted.node'. This is exactly like the asset relocator already does, but simply extended to these more complex patterns.

There may still be edge cases that are not analyzable, but this partial executor already takes into account all path functions, __dirname and __filename expressions. We can tailor the analysis to the real-world test cases, and likely get quite far here too.

@rauchg rauchg changed the title Possible approach for native binaries Native addons support Nov 26, 2018

@rixrix

This comment has been minimized.

Copy link

rixrix commented Nov 28, 2018

tried ncc on one of our APIs with Newrelic on it - failed due to "node-gyp"

see logs on Node v8.12.0

ʎ  ncc build app.js
ncc: Module directory "/Users/xir/Workspace/epic-nodejs-api/node_modules/@newrelic/native-metrics/lib" attempted to require "npm" but could not be resolved, assuming external.
ncc: Module directory "/Users/xir/Workspace/epic-nodejs-api/node_modules/@newrelic/native-metrics/lib" attempted to require "node-gyp" but could not be resolved, assuming external.
Error: Hash: 763418c0e21c6da9c96a
Version: webpack 4.26.0
Time: 5977ms
Built at: 2018-11-28 16:04:05
 Asset      Size  Chunks  Chunk Names
out.js  6.63 MiB       0  main
Entrypoint main = out.js
  [8] external "path" 42 bytes {0} [built]
 [10] external "fs" 42 bytes {0} [built]
 [25] ./config/config.js 15.6 KiB {0} [built]
 [33] external "os" 42 bytes {0} [built]
 [61] ./utils/logger.js 1.34 KiB {0} [built]
 [96] ./node_modules/express/index.js 224 bytes {0} [built]
 [97] ./node_modules/cors/lib/index.js 6.34 KiB {0} [built]
[100] ./node_modules/winston/lib/winston.js 4.12 KiB {0} [built]
[126] ./utils/logglyClient.js 2.08 KiB {0} [built]
[199] external "cluster" 42 bytes {0} [built]
[462] ./app.js 980 bytes {0} [built]
[487] ./node_modules/winston-loggly/lib/winston-loggly.js 7.42 KiB {0} [built]
[580] ./main.js 1.39 KiB {0} [built]
[581] ./node_modules/newrelic/index.js 5.54 KiB {0} [built]
[905] ./node_modules/helmet/index.js 1.17 KiB {0} [built]
    + 1008 hidden modules
@TooTallNate

This comment has been minimized.

Copy link
Member

TooTallNate commented Nov 28, 2018

node-canvas requires the .node file directly without a helper module, so we should support that use-case as well:

https://github.com/Automattic/node-canvas/blob/490ef47432af83582cf5e491a8ca8797d07c93b0/lib/bindings.js#L3

@TooTallNate

This comment has been minimized.

Copy link
Member

TooTallNate commented Nov 28, 2018

Could also use / take inspiration from https://www.npmjs.com/package/node-native-loader

This was referenced Nov 30, 2018

@guybedford

This comment has been minimized.

Copy link
Collaborator

guybedford commented Dec 3, 2018

Completed in #93 and #85. If any new scenarios come up, please post a new issue.

@guybedford guybedford closed this Dec 3, 2018

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