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

Add Python requirements.txt file with pinned version numbers #5474

Closed
RecRanger opened this issue May 11, 2024 · 10 comments · Fixed by #5475
Closed

Add Python requirements.txt file with pinned version numbers #5474

RecRanger opened this issue May 11, 2024 · 10 comments · Fixed by #5475

Comments

@RecRanger
Copy link
Contributor

It is not clear what version of protobuf is required. Installing the latest version with pip install protobuf, as suggested by many of the Python scripts, does not work.

Please add a requirements.txt file in the root of the repo which pins the versions of Python dependencies.

@solardiz
Copy link
Member

Installing the latest version with pip install protobuf, as suggested by many of the Python scripts, does not work.

FWIW, I only see this suggested by one script, not many - run/protobuf/wallet_pb2.py as used by run/multibit2john.py. We also have similar dependency for Coinomi wallet, but no try/except with that suggestion.

@solardiz
Copy link
Member

@RecRanger Can you show how exactly the script doesn't work with the latest version of protobuf? Can you try and fix that? Thank you!

@RecRanger
Copy link
Contributor Author

I don't see anything wrong with continuing to use the older version of Protobuf (which is tested, validated, etc.). Upgrading the dependency seems like a pointless endeavor for no gain.

The error message explicitly says that the current Python code uses features that are deprecated in protobuf>3.20. We'd likely have to regenerate the Python protobuf files, iirc (which is a huge pain).

@solardiz
Copy link
Member

@RecRanger Can you show how exactly the script doesn't work with the latest version of protobuf?

@RecRanger I would still be interested in having such detail recorded in here, as it could be useful when we revisit the issue later. So I'd appreciate it if you show the issue you ran into, perhaps as a piece of copy-paste from the terminal. Thank you!

@RecRanger
Copy link
Contributor Author

Fair enough, here you go!

$ pip install protobuf
#Collecting protobuf
  Downloading protobuf-5.26.1-cp37-abi3-manylinux2014_x86_64.whl.metadata (592 bytes)

$ run/multibit2john.py 
Traceback (most recent call last):
  File ".../run/multibit2john.py", line 23, in <module>
    from protobuf.wallet_pb2 import *
  File ".../run/protobuf/wallet_pb2.py", line 34, in <module>
    _descriptor.EnumValueDescriptor(
  File ".../venv/lib/python3.10/site-packages/google/protobuf/descriptor.py", line 914, in __new__
    _message.Message._CheckCalledFromGeneratedFile()
TypeError: Descriptors cannot be created directly.
If this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:
 1. Downgrade the protobuf package to 3.20.x or lower.
 2. Set PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python (but this will use pure-Python parsing and will be much slower).

More information: https://developers.google.com/protocol-buffers/docs/news/2022-05-06#python-updates

I'm seeing just now that another potential work-around is to force that environment variable within the Python script.

Running this gets past the imports at least: PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python run/multibit2john.py

@solardiz
Copy link
Member

Running this gets past the imports at least: PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python run/multibit2john.py

Cool. Can you also try embedding the setting of this variable inside the script?

Does coinomi2john.py also need this? Does the same trick work?

Maybe you can patch this/these script(s) as needed to use this workaround by default, and then not pin the version of protobuf (just the dependency, without version)?

@exploide
Copy link
Contributor

I have no strong opinion whether the environment variable approach should be favored over the version constrained. Assuming this really works, it would eliminate the need of the pinned version. On the other hand, I don't know whether support for this workaround will stay for the foreseeable future or if it will eventually break.

Using the variable would also hide that the script should ideally be made compatible with latest protobuf. I guess we would at least need a comment in the code explaining why we set the variable.

@solardiz
Copy link
Member

I guess we would at least need a comment in the code explaining why we set the variable.

Yes, should have that comment.

@RecRanger
Copy link
Contributor Author

I'm still in favor of pinning dependencies, at least as ranges. The user can choose to try with the latest dependencies if they want, but there should be a known working configuration documented in this repo, in the form of a requirements.txt or pyproject.toml

@solardiz
Copy link
Member

I'm still in favor of pinning dependencies, at least as ranges.

OK. But if we can also include an easy workaround that makes the script work across a wider range of dependency versions, I think we should.

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

Successfully merging a pull request may close this issue.

3 participants