john icon indicating copy to clipboard operation
john copied to clipboard

Add Python `requirements.txt` file with pinned version numbers

Open RecRanger opened this issue 1 year ago • 10 comments
trafficstars

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.

RecRanger avatar May 11 '24 20:05 RecRanger

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 avatar May 13 '24 16:05 solardiz

@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!

solardiz avatar May 13 '24 19:05 solardiz

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).

RecRanger avatar May 13 '24 21:05 RecRanger

@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!

solardiz avatar May 15 '24 18:05 solardiz

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

RecRanger avatar May 15 '24 23:05 RecRanger

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)?

solardiz avatar May 16 '24 13:05 solardiz

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.

exploide avatar May 16 '24 15:05 exploide

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

Yes, should have that comment.

solardiz avatar May 16 '24 16:05 solardiz

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

RecRanger avatar May 16 '24 17:05 RecRanger

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.

solardiz avatar May 16 '24 22:05 solardiz