Skip to content

Upgrade to ring 0.16#49

Closed
djc wants to merge 1 commit intoZenGo-X:masterfrom
djc:ring-0.16
Closed

Upgrade to ring 0.16#49
djc wants to merge 1 commit intoZenGo-X:masterfrom
djc:ring-0.16

Conversation

@djc
Copy link
Contributor

@djc djc commented Aug 15, 2019

This requires upgrading secp256k1 as well, because otherwise they have conflicting dependendencies no the cc crate.

@omershlo
Copy link
Contributor

@djc great! looks like there's a minor cargo format error.

cc : @oleiba

@djc
Copy link
Contributor Author

djc commented Aug 15, 2019

Fixed the formatting problems.

This requires upgrading secp256k1 as well, because otherwise they have
conflicting dependendencies no the cc crate.
@djc
Copy link
Contributor Author

djc commented Aug 15, 2019

I saw this failure, too, but as far as I can tell it is intermittent. It doesn't seem like this would be related to this PR, since this test doesn't even seem to be using ring.

@omershlo
Copy link
Contributor

its because we have a probabilistic test. It fails from time to time.

@omershlo
Copy link
Contributor

We actually try to get rid of ring library all-together. Too many other libraries are using it with different versions and it creates a lot of compatibility problems. Since we expect Curv to be integrated into other systems it happened to us more than once that one of the other libraries used ring from a different - not latest - version.
I am inclined to closed this pr and replace with an issue of removing ring.
WDYT ?

@omershlo
Copy link
Contributor

@oleiba if you have inputs to add

@djc
Copy link
Contributor Author

djc commented Aug 15, 2019

We have a similar issue: we want to use ring for other things as well, but we prefer to stay on the latest version. I don't know what timeline you have in mind for replacing ring, but in the meantime (and since I worked on this PR anyway) it would seem nicer if you rely on the latest one.

@omershlo
Copy link
Contributor

Make sense.

@omershlo
Copy link
Contributor

All of our stack is aligned on this specific version of ring:

error: failed to select a version for `ring`.
    ... required by package `curv v0.1.0 (https://github.com/djc/curv?branch=ring-0.16#037f1b32)`
    ... which is depended on by `multi-party-ecdsa v0.1.0 (/Users/omershlo/Downloads/from_github/multi-party-ecdsa)`
versions that meet the requirements `^0.16` are: 0.16.7, 0.16.6, 0.16.5, 0.16.4, 0.16.3, 0.16.2, 0.16.1, 0.16.0

the package `ring` links to the native library `ring-asm`, but it conflicts with a previous package which links to `ring-asm` as well:
package `ring v0.13.5`
    ... which is depended on by `cookie v0.11.1`
    ... which is depended on by `rocket_http v0.4.2`
    ... which is depended on by `rocket v0.4.2`
    ... which is depended on by `multi-party-ecdsa v0.1.0 (/Users/omershlo/Downloads/from_github/multi-party-ecdsa)`

failed to select a version for `ring` which could resolve this conflict

We use ring for hash function so it is not very critical for us.

@kigawas kigawas mentioned this pull request Aug 16, 2019
@kigawas
Copy link
Contributor

kigawas commented Aug 16, 2019

https://crates.io/crates/merkle

merkle also depends on ring, removing ring is not enough
I suggest using dependabot in order to automatically bump dependencies

#52

@djc
Copy link
Contributor Author

djc commented Aug 16, 2019

Dependabot is a great way to stay up to date, I use it for many of my projects.

@omershlo
Copy link
Contributor

I removed ring in this pr: #42

@omershlo
Copy link
Contributor

Can I close this PR? (ring is no longer a dependency for this library)

@djc
Copy link
Contributor Author

djc commented Aug 19, 2019

I'll close it.

@djc djc closed this Aug 19, 2019
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.

3 participants