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

Update dev plyvel requirement #1816

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@davesque
Copy link
Contributor

commented Aug 8, 2019

What was wrong?

See #1815 .

How was it fixed?

Updated plyvel dev requirement to 1.1.0.

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@@ -25,7 +25,7 @@
"coincurve>=10.0.0,<11.0.0",
"eth-hash[pysha3];implementation_name=='cpython'",
"eth-hash[pycryptodome];implementation_name=='pypy'",
"plyvel==1.0.5",

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Aug 8, 2019

Contributor

I wonder if we should change this into a range to not break things for those that are stuck on something lower than 1.1.0 which is known to work well?

This comment has been minimized.

Copy link
@davesque

davesque Aug 8, 2019

Author Contributor

Yeah, probably. Just made that change.

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Aug 8, 2019

Contributor

Ah wait, what I meant was to include everything from 1.0.5 up to whatever you say is working fine. Because 1.0.5 is the currently used version and I don't see a good reason to require a higher version when 1.0.5 is known to work well. Especially because there may be people who can not easily upgrade to a higher version.

This comment has been minimized.

Copy link
@davesque

davesque Aug 8, 2019

Author Contributor

As a reminder, plyvel is just the python interface library to LevelDB. If we require a more recent plyvel version, it doesn't necessarily mean we're requiring a higher LevelDB version. And it's a stronger guarantee that macOS users won't encounter the linked issue.

Also, apart from leveldblib, plyvel has no sub-dependencies. So that also makes it seem less risky to make this change.

What do you think?

This comment has been minimized.

Copy link
@davesque

davesque Aug 8, 2019

Author Contributor

@cburgdorf By the way, with my comment above, you didn't mention being concerned about the level db version, but I figured I'd mention my thoughts on that just in case you were.

This comment has been minimized.

Copy link
@davesque

davesque Aug 8, 2019

Author Contributor

Anyway, I'll just go with your suggestion. I don't actually feel that strongly about it. 😄

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Aug 9, 2019

Contributor

As a reminder, plyvel is just the python interface library to LevelDB

Ah, that's a good point. That weakens my concerns. 👍

With that in mind, I guess a range of plyvel>=1.1.0, <1.2.0 is fine.

Anyway, I'll just go with your suggestion. I don't actually feel that strongly about it.

Sorry, for not making myself clear enough. I didn't mean to suggest a range without an upper bound such as plyvel>=1.0.5".

Because I think that one can in fact become problematic. Current plyvel works under the assumption of having a leveldb of version 1.2 or higher on the system. An unbounded plyvel dependency might even install a plyvel version 2.x which may depend on a leveldb 2.x (hypothetical future versions) and with breaking changes things might fall apart then.

I'd pick one of these:

  • plyvel>=1.1.0, <2 (if we trust plyvel does semver right)
  • plyvel>=1.0.5, <2 (if we trust plyvel does semver right)
  • plyvel>=1.1.0, <1.2.0 (more conservative)
  • plyvel>=1.0.5, <1.2.0 (more conservative)

This comment has been minimized.

Copy link
@davesque

davesque Aug 9, 2019

Author Contributor

I'll go with the last suggestion since it basically solves the macOS issue and also potentially avoids other issues with plyvel being incompatible with certain LevelDB versions.

@davesque davesque force-pushed the davesque:fix-plyvel-fail branch 2 times, most recently from dcc59be to c81313a Aug 8, 2019

@davesque davesque force-pushed the davesque:fix-plyvel-fail branch from c81313a to 89a8dc4 Aug 9, 2019

@davesque davesque requested a review from carver Aug 13, 2019

@carver

carver approved these changes Aug 13, 2019

Copy link
Contributor

left a comment

Is there a reason to think that plyvel is not using semver? Anyway, no objection to the PR as-is.

@davesque davesque merged commit 95ed61f into ethereum:master Aug 13, 2019

20 checks passed

ci/circleci: py36-benchmark Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-database Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py36-transactions Your tests passed on CircleCI!
Details
ci/circleci: py36-vm Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
ci/circleci: py37-database Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-transactions Your tests passed on CircleCI!
Details
ci/circleci: py37-vm Your tests passed on CircleCI!
Details
@cburgdorf

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Is there a reason to think that plyvel is not using semver? Anyway, no objection to the PR as-is.

No idea. Previously, the dependency was pinned to a single specific version which might serve as a hint that there were issues about this in the past but it's pure speculation ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.