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

Made eth-abi a primary dependency #127

Merged
merged 3 commits into from
Sep 17, 2018
Merged

Conversation

voith
Copy link
Contributor

@voith voith commented Sep 16, 2018

What was wrong?

eth-tester would give the following error:

In [1]: from eth_tester import EthereumTester
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-1-72c9d4f19b88> in <module>()
----> 1 from eth_tester import EthereumTester

~/Projects/eth-tester/eth_tester/__init__.py in <module>()
      2 import sys
      3
----> 4 from .main import (  # noqa: F401
      5     EthereumTester,
      6 )

~/Projects/eth-tester/eth_tester/main.py in <module>()
     29 )
     30
---> 31 from eth_tester.backends import (
     32     get_chain_backend,
     33 )

~/Projects/eth-tester/eth_tester/backends/__init__.py in <module>()
     10     MockBackend,
     11 )
---> 12 from .pyevm import (  # noqa: F401
     13     PyEVMBackend,
     14     is_pyevm_available,

~/Projects/eth-tester/eth_tester/backends/pyevm/__init__.py in <module>()
      4     is_pyevm_available,
      5 )
----> 6 from .main import (  # noqa: F401
      7     PyEVMBackend,
      8 )

~/Projects/eth-tester/eth_tester/backends/pyevm/main.py in <module>()
     10 import rlp
     11
---> 12 from eth_abi import (
     13     decode_single
     14 )

ModuleNotFoundError: No module named 'eth_abi'

This was missed in #115 (4073ee2).
This wasn't detected by tests because eth-abi was testing dependency

How was it fixed?

eth-abi was removed as a testing dependency and was made a primary dependency.

Bonus changes

  • pycryptodome or pysha3 are optional dependencies of py-evm that are needed for eth-tester to work. Install them based on the python implementation available. (Haven't tested it with pypy manually, I'm relying on tests to tell me).
  • Added python3.7 to the list of supported python versions.

PS: I have added all these in separate commits to keep unrelated changes isolated.

Cute Animal Picture

Cute animal picture

@carver
Copy link
Contributor

carver commented Sep 17, 2018

Just to explicitly state the implicit change: this treats eth-tester like an end product rather than a library. (by forcing a choice of eth-hash backend, for example)

I think this is reasonable, just wanted to make sure everyone is one the same page.

@carver carver merged commit 4f6f82b into ethereum:master Sep 17, 2018
@voith voith deleted the fix-eth-abi-setup branch September 17, 2018 17:52
@voith
Copy link
Contributor Author

voith commented Sep 17, 2018

@carver My reasoning is as follows:
I checked setup.py in py-evm. The extra-eth requirements in py-evm are:

'eth-extra': [
        "coincurve>=8.0.0,<9.0.0",
        "eth-hash[pysha3];implementation_name=='cpython'",
        "eth-hash[pycryptodome];implementation_name=='pypy'",
        "plyvel==1.0.5",
    ],

Although a user has the freedom to explicitly install any eth-hash backend, installations using eth-extra will bind the backend with the python implementation.
The change I made here is no different.

@voith
Copy link
Contributor Author

voith commented Sep 17, 2018

I encountered this in voith/eth-tester-rpc#10

@carver
Copy link
Contributor

carver commented Sep 17, 2018

That's a good point, the eth-tester[py-evm] extra should probably just be on py-evm[eth-extra] rather than on py-evm and duplicating all those packages here.

@voith
Copy link
Contributor Author

voith commented Sep 17, 2018

@carver I had thought about using py-evm[eth-extra]. But there were two extra dependencies, plyvel and coincurve for which there are no tests at the moment.
Also "eth-hash[pysha3];implementation_name=='cpython' and "eth-hash[pycryptodome];implementation_name=='pypy'" are not pinned to any range at the moment in py-evm, which worried me.

Probably, I should have fixed the eth-hash change in py-evm, but to have plylevel as dependency, it would need some tests.

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