Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Update keccak to the latest version 🚀 #225

Merged
merged 1 commit into from
Jan 29, 2020
Merged

Conversation

greenkeeper[bot]
Copy link
Contributor

@greenkeeper greenkeeper bot commented Dec 28, 2019

The dependency keccak was updated from 2.1.0 to 3.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.


Publisher: fanatid
License: MIT

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 🌴

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.336% when pulling 18eea09 on greenkeeper/keccak-3.0.0 into 9354465 on master.

@michaelsbradleyjr
Copy link

It would be really awesome if this could be merged and secp256k1 could likewise be bumped to ^4.0.0. The new major versions of both include prebuilds of the binaries (so most users won't have node-gyp run on install 🎉 ) and feature compatibility with newer Node.js versions.

@alcuadrado
Copy link
Member

alcuadrado commented Jan 23, 2020

This is really interesting @michaelsbradleyjr. Thanks for pointing it out! Merging this should be pretty easy, secp256k1 not so much, as its API changed.

Do you know how are these pre built images created? Why weren't they used before? Which versions of node do they support?

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Jan 24, 2020

@alcuadrado they're built using GitHub Actions workflows:

https://github.com/cryptocoinjs/keccak/blob/master/.github/workflows/ci.yaml
https://github.com/cryptocoinjs/secp256k1-node/blob/master/.github/workflows/ci.yaml

I believe the workflows were implemented just prior to releasing the new major versions.

They seem to support Node 10.x, 12.x, and 13.x — those are the versions I tried.

Note that the prebuilds for Linux, macOS, and Windows are included in the tarballs published to the NPM registry vs. their being downloaded during package install as is common with node-pre-gyp setups. The binaries are really small in this case, so it doesn't cause bloat, which is nice.

@alcuadrado
Copy link
Member

Thanks for your answer @michaelsbradleyjr. This is really cool.

We should make sure that we are ok with dropping support for older Node.js versions, which is being discussed here ethereum/js-team-organization#7

@holgerd77
Copy link
Member

That are indeed strong arguments to drop Node 8 in this case.

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Jan 27, 2020

cf. https://github.com/cryptocoinjs/keccak/blob/master/package.json#L45

and https://github.com/cryptocoinjs/secp256k1-node/blob/master/package.json#L50

For good measure I tried with Node.js 8.0.0 and 8.17.0 — keccak@3 is definitely not buildable / installable with Node.js 8.x.

@holgerd77
Copy link
Member

Is there any way (tool) out there for getting at least a rough idea on the Node version consumers of a library are using? Any idea anyone?

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Jan 27, 2020

No idea, but at first blush it seems like npm/yarn (or the package itself during postinstall or at runtime) would have to "phone home" re: the Node.js version in use either to the registry or to package maintainer... obvious downsides to that and I'm not actually sure to what extent if any npm/yarn send user info to the registry, e.g. when running npm install.

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Jan 27, 2020

Somewhat helpful to get an overall idea of version popularity:

https://nodejs.org/metrics/summaries/version.png

Image link taken from the Versions section (not linkable) toward the bottom of:

https://nodejs.org/metrics/

@holgerd77
Copy link
Member

@michaelsbradleyjr Thanks, phew, I would say that is still too much (having a look at the version popularity chart from the first link). 😕

@iurimatias
Copy link

@holgerd77 https://raw.githubusercontent.com/nodejs/Release/master/schedule.svg?sanitize=true Node 8 is officially not supported anymore. As it is currently, many people have issues with node-gyp dependencies (probably the most common ethereum JS issue in the ecosystem) and it also slows down installations. The pre-builds are a massive QoL improvement.

@holgerd77
Copy link
Member

@iurimatias Yeah well, then let's do it. @alcuadrado @evertonfraga @ryanio what do you think?

@evertonfraga
Copy link
Contributor

I think that dropping node-gyp is compelling enough to make the change.

I’d give a period of notice (3-4 weeks) and would release a minor version.

@holgerd77
Copy link
Member

@evertonfraga Hmm, I think we also didn't followed this on EthereumJS, but technically I would say a Node.js version bump should rather be treated as a major version bump regarding semver? This DOES introduce a (very much) breaking change, since it makes existing v8 installations unusable.

Did a quick search, there are similar opinions out there, e.g.:
https://www.gatsbyjs.org/blog/2019-06-18-dropping-support-for-node-6/

Not sure, other opinions from people reading here?

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Jan 28, 2020

I think it should be a major version bump to avoid a big surprise for those users (still on older Node.js) who would pick up a minor bump owing to the default ^ operator.

@alcuadrado
Copy link
Member

I'm more inclined not to release it as a major. Using an unsupported version of any runtime is totally risky and should be avoided. If you do, you do it at your own risk.

If we release it as a major, people how are doing their things rightly will stop getting updates unless they manually upgrade to the new major.

@holgerd77
Copy link
Member

Ok, did a local test install both with the current ^2.0.0 (-> 2.1.0) and the new 3.0.0 keccak dependency version, and can add yet another confirm here from my machine (Mac OS Mojave) that this fixes the installation problems.

2.1.0 output:

> keccak@2.1.0 install PATH/keccak
> npm run rebuild || echo "Keccak bindings compilation fail. Pure JS implementation will be used."


> keccak@2.1.0 rebuild PATH/node_modules/keccak
> node-gyp rebuild

gyp ERR! configure error
gyp ERR! stack Error: Command failed: PATH/.pyenv/shims/python2 -c import sys; print "%s.%s.%s" % sys.version_info[:3];
gyp ERR! stack pyenv: python2: command not found
gyp ERR! stack
gyp ERR! stack The `python2' command exists in these Python versions:
...

(I actually directly had the horror case with my local python environment not properly configured. 😜

3.0.0 output:

> keccak@3.0.0 install PATH/node_modules/keccak
> node-gyp-build || exit 0

So I will merge here.

Seems there is no golden way for the version number, I would then follow the advice from @alcuadrado and prepare a minor release here, both argumentation lines seems pretty valid.

Not sure if we'll get the secp2561 dependency updated as well along, will do some first tests right away.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, see comment thread for additional context.

@holgerd77 holgerd77 merged commit 8396154 into master Jan 29, 2020
@holgerd77 holgerd77 deleted the greenkeeper/keccak-3.0.0 branch January 29, 2020 10:01
@holgerd77
Copy link
Member

This has now been published in the v7.0.0 release on npm.

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

Successfully merging this pull request may close these issues.

6 participants