polkascan / py-substrate-interface

Python Substrate Interface
https://polkascan.github.io/py-substrate-interface/
Apache License 2.0
240 stars 114 forks source link

Packaging is now a requirement #124

Closed LefterisJP closed 3 years ago

LefterisJP commented 3 years ago

Trying to package the latest version seems to complain about the packaging module

Checking binary
Traceback (most recent call last):
  File "build/rotkehlchen/rotkehlchen-script.py", line 1, in <module>
    import rotkehlchen.__main__
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/__main__.py", line 8, in <module>
    from rotkehlchen.server import RotkehlchenServer
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/server.py", line 7, in <module>
    from rotkehlchen.api.server import APIServer, RestAPI
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/api/server.py", line 17, in <module>
    from rotkehlchen.api.rest import RestAPI, api_response, wrap_in_fail_result
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/api/rest.py", line 32, in <module>
    from rotkehlchen.accounting.ledger_actions import LedgerAction
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/accounting/ledger_actions.py", line 6, in <module>
    from rotkehlchen.assets.asset import Asset
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/assets/asset.py", line 6, in <module>
    from rotkehlchen.constants.resolver import (
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/constants/__init__.py", line 1, in <module>
    from .misc import *  # noqa: F401, F403
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/constants/misc.py", line 2, in <module>
    from rotkehlchen.typing import EmptyStr, EventType
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/typing.py", line 7, in <module>
    from rotkehlchen.chain.substrate.typing import KusamaAddress
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "rotkehlchen/chain/substrate/typing.py", line 4, in <module>
    from substrateinterface import SubstrateInterface
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "substrateinterface/__init__.py", line 18, in <module>
  File "/home/lefteris/.virtualenvs/rotkipy37/lib/python3.7/site-packages/PyInstaller/loader/pyimod03_importers.py", line 627, in exec_module
    exec(bytecode, module.__dict__)
  File "substrateinterface/contracts.py", line 19, in <module>

And indeed it's imported there and is not in the requirements

arjanz commented 3 years ago

Ok that's interesting, the CI runner on GitHub sadly didn't notified me of this missing requirement (maybe installed by default).

I'll look into it and make a new release if necessary, thanks for reporting

arjanz commented 3 years ago

Fix released in: https://pypi.org/project/substrate-interface/0.13.10/

yabirgb commented 3 years ago

Thank you! We'll upgrade to test it

yabirgb commented 3 years ago

Hello @arjanz This version allowed us to build, thank you! Two things:

  1. As lefteris said requirements for libraries shouldn't be imposed unless strictly needed and should be as wide as possible. We had to update requests to get the library working
  2. We avoided having packaging added to our binary and I've seen that you use it to check one version. I'm completely unaware if this is a common practice but I believe it could be an issue that might hit more people. Would it be possible to remove this dependency from the code?

Thank you for your fast response to this issue :)

arjanz commented 3 years ago
  1. Yes absolutely, as I also replied on @LefterisJP comment the package requirements policy should indeed be improved, much wider and not dictate too much unnecessary upgrades, I will specify a safe upper and lower bound of version per requirement.
  2. What is the reason you want to avoid packaging? Basically we use it the compare version for compatibility switches and been told it is commonly used tool to compare version in Python. I guess we can replace it with a simple string split to get the job done if it really in your way.
arjanz commented 3 years ago

@yabirgb I have specified version ranges in master for which I have tested the library from the beginning and at the other end the next major version, this should be a sane and wide enough range..

yabirgb commented 3 years ago

@arjanz Thank you very much :) It would be great to have it in pypi since our docker build needs this upgrade

arjanz commented 3 years ago

@yabirgb @LefterisJP packaging dependency has been removed and replace with custom function in https://pypi.org/project/substrate-interface/0.13.11/

LefterisJP commented 3 years ago

Hey @yabirgb so this is now considered done right? You guys tested this and it works fine for rotki now, right?

yabirgb commented 3 years ago

@LefterisJP Yes forgot to reply on this thread. Yes it works now. Thank you @arjanz !