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

Make scrypt (C++ binding) optional #3

Merged
merged 2 commits into from
Dec 18, 2018
Merged

Conversation

sullivanpt
Copy link
Contributor

Hi, please consider this PR to make the scrypt binary an optional dependency.

With this change an npm install will succeed even scrypt fails to build on the local host; in this case the pure JS version (scryptsy) will be used on both the browser and on node.

The "load specific version" will still fail if the demanded version is not present; however, the failure will occur at the time of the require rather than during npm install.

var scrypt = require('scrypt.js) // pure JavaScript (will ALWAYS succeed, might return pure JS on node)
var scrypt = require('scrypt.js/js') // pure JavaScript (will ALWAYS succeed)
var scrypt = require('scrypt.js/node') // C on Node (will throw an exception if scrypt not installed)

@davidmurdoch
Copy link

@axic Any chance this PR could be merged and published to npm?

@benjamincburns
Copy link

@axic to add some detail to @davidmurdoch's request - in ganache-cli we actually use webpack specifically to replace this dependency with a non-native one in order to simplify things for our users.

This decision was made a while back because we noticed at hackathons and training events many (windows) users were spending an exorbitant amount of time troubleshooting their native build environment.

This forces us to not only pack ganache-cli, but all of its dependencies, which makes the package much heavier than it needs to be, and complicates our testing process unnecessarily.

So that's really a lot of words to say that accepting this PR would alleviate a lot of pain for us.

mikeseese pushed a commit to trufflesuite/ganache that referenced this pull request Sep 10, 2018
…162)

This pull request copies the build process from `ganache-cli` to `ganache-core` while also optimizing the resulting webpacked bundle to only include `ganache-core`'s internals plus pure-js implementations of our native transitive dependencies. All other dependencies are installed on `install`.

Additionally, ganache-core and ganache-cli will now default to using native dependencies, if successfully `install`ed, only falling back to the pure-js implementations if those dependencies can't be installed (i.e., because node-gyp on Windows is tricky).

---

This PR also purges some unnecessary `dependencies` from package.json, moves others to `devDependencies`, and updates and pins the remaining direct dependencies to their latest patch versions (at time of writing), with the exception of:

- `level-sublevel`; as version `6.6.5` needlessly regressed to a dependency on a vulnerable version of `levelup`. `6.6.5` actually looks like it was accidentally published from an older branch named `master2`. `¯\_(ツ)_/¯`
- `ethereumjs-tx`; as version [`1.3.5`](https://github.com/ethereumjs/ethereumjs-tx/releases/tag/v1.3.5) introduced bug ethereumjs/ethereumjs-tx#113 which was not fixed in [`1.3.7`](https://github.com/ethereumjs/ethereumjs-tx/releases/tag/v1.3.7) in my testing.

---

On `npm publish` our tests will automatically be run, the webpack build created, and the tests run again on the new webpacked build. We _may_ want to just trust that Travis will build and test everything for us, but having everything run on `publish` ensures that we don't ship an old or broken `./build` directory (since `./build` will *not* be created on `npm install`).

Our new `.npmignore` will ignore most of our unneeded files and directories on npm publish, but it will include `./lib` and `./build`.

Before `npm publish`ing to production we'll want to test the interaction of the new build steps, our `.npmignore`, and `./build` on a variety of OSes (mainly Windows in different configurations to ensure the non-native fallback method works appropriately), perhaps by temporarily publishing to npm under a different name.

---

With all that said and done, once axic/scrypt.js#3 is merged and published to npm we'll likely get rid of our node webpack build all together; currently, the only other consideration is `eth-block-tracker`, which is potentially node 6 incompatible, so we may still want to use its es5 build. _Note: `secp256k1` already automatically falls back to a pure-JS implementation if a native build isn't possible._
@axic axic changed the title feat: make scrypt binary optional Make scrypt (C++ binding) optional Dec 18, 2018
@axic
Copy link
Owner

axic commented Dec 18, 2018

@sullivanpt @benjamincburns sorry for the delay, I haven't gotten the notifications for some reason :(

@axic axic merged commit 816ae3c into axic:master Dec 18, 2018
@axic
Copy link
Owner

axic commented Dec 18, 2018

Released 0.3.0.

@axic
Copy link
Owner

axic commented Dec 18, 2018

Also packaging tools supposed to use the "browser": "js.js", line from package.json, but this change gives a secondary options for those which ignore it.

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.

4 participants