unicorn-engine / unicorn

Unicorn CPU emulator framework (ARM, AArch64, M68K, Mips, Sparc, PowerPC, RiscV, S390x, TriCore, X86)
http://www.unicorn-engine.org
GNU General Public License v2.0
7.53k stars 1.33k forks source link

Restore python 2 compatibility #1822

Closed Arusekk closed 1 year ago

Arusekk commented 1 year ago

Not sure why the py2 wheels made it to PyPI, since importing Unicorn throws a SyntaxError every time, but here is a fix.

Another option would be to retain the type annotations and just mark the affected versions on PyPI as python3-only, so that pip2 install unicorn at least installs something old that works.

wtdcode commented 1 year ago

This is not the correct way to support Py2 as annotations is one of the most important features of Python3 which enables much faster scripting. I would rather accept restore the compatibility by introducing some code duplication.

wtdcode commented 1 year ago

And the alternative solution is: https://peps.python.org/pep-0484/#suggested-syntax-for-python-2-7-and-straddling-code

aquynh commented 1 year ago

We plan to bring Python2 support back in the next release 2.0.2, so please pull req

wtdcode commented 1 year ago

1823 To reduce the maintenance efforts, I propose restoring py2 compatibility by introducing 2 files. Check the PR if it works for you.

@gerph could you also test this?

gerph commented 1 year ago

Apologies for the delay, yes, I can test this I believe. I will test this PR, then I will test the PR 1823 as well.

I'm going to describe what I did to test this - I suspect that this might be a little longwinded, but hopefully it'll indicate where I've made mistakes if I have assumed too much :-) ... When I've got it working (or broken), I will summarise my findings and suggestions...

Testing

git remote add arusekk https://github.com/Arusekk/unicorn
git pull arusekk
git checkout patch-1
cd build
cmake .. -DCMAKE_BUILD_TYPE=Release -DUNICORN_ARCH=arm
make
cd ../bindings/python/
make bdist

At this point I get errors:

Traceback (most recent call last):
  File "/Users/charles/home/RO/unicorn/bindings/python/setup.py", line 232, in <module>
    setup(
  File "/usr/local/Cellar/python@3.9/3.9.1_6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/core.py", line 108, in setup
    _setup_distribution = dist = klass(attrs)
  File "/usr/local/lib/python3.9/site-packages/setuptools/dist.py", line 423, in __init__
    _Distribution.__init__(self, {
  File "/usr/local/Cellar/python@3.9/3.9.1_6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/dist.py", line 267, in __init__
    getattr(self.metadata, "set_" + key)(val)
  File "/usr/local/Cellar/python@3.9/3.9.1_6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/dist.py", line 1227, in set_requires
    distutils.versionpredicate.VersionPredicate(v)
  File "/usr/local/Cellar/python@3.9/3.9.1_6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/versionpredicate.py", line 114, in __init__
    raise ValueError("expected parenthesized list: %r" % paren)
ValueError: expected parenthesized list: '; python_version<"3.5"'

This appears to be due to the line in setup.py:

    requires=['ctypes', 'typing; python_version<"3.5"'],

A quick google for this suggests that it should be install_requires, so I made that change to the setup.py and re-ran the make bdist. This caused the system to rebuild all the architectures - I only need ARM 32bit, and I'm not sure how to restrict it so... I'm ignoring that problem.

This gets as far as the attempt to upload the binary with twine, like this:

removing build/bdist.macosx-10.14-x86_64/wheel
running register
running check
warning: Check: This command has been deprecated. Use `twine check` instead: https://packaging.python.org/guides/making-a-pypi-friendly-readme#validating-restructuredtext-markup

warning: Check: Not checking long description content type 'text/markdown', this command only checks 'text/x-rst'.

We need to know who you are, so please choose either:
 1. use your existing login,
 2. register as a new user,
 3. have the server generate a new password for you (and email it to you), or
 4. quit
Your selection [default 1]: 

I do not wish to upload the binary so I press ctrl-c. It would be nice to have a target that doesn't try to upload the binary to pypi, but this is how I've done it up to now and it's not a change in this patch. At this point if I ls dist I get:

charles@laputa ~/projects/RO/unicorn/bindings/python (patch-1)> ls dist/
unicorn-2.0.1.post1-py2.py3-none-macosx_10_14_x86_64.whl

So this has been built correctly, it seems.

Attempting to install fails...

charles@laputa ~/projects/RO/unicorn/bindings/python (patch-1)> 
pip install dist/unicorn-2.0.1.post1-py2.py3-none-macosx_10_14_x86_64.whl
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
Processing ./dist/unicorn-2.0.1.post1-py2.py3-none-macosx_10_14_x86_64.whl
Collecting ctypes (from unicorn==2.0.1.post1)
  ERROR: Could not find a version that satisfies the requirement ctypes (from unicorn==2.0.1.post1) (from versions: none)
ERROR: No matching distribution found for ctypes (from unicorn==2.0.1.post1)

This is probably because ctypes is a builtin and it is unnecessary to install. So I modify install_requires to remove ctypes, and re-run the make bdist.

Having done this, I can run my application to check whether the version has been picked up:

charles@laputa ~/projects/RO/unicorn/bindings/python (patch-1)> pyrodev --version full
Pyromaniac 0.46 (04 Apr 2023)
Host platform: Darwin/x86_64
System emulation: Unicorn 2.0
Python version: 2.7.16 (default, Jun 19 2019, 07:40:37) 

This means it is recognised and is able to be interrogated. A success so far...

Going further, I can use it to run the operating system and enter the desktop and run applications - this is a success for me. Not only is it working by allowing an import as a Python 2 package, but it's working when I do so.

Findings

It was necessary to make a small change to the setup.py in order to get it to build the wheel and install:

diff --git a/bindings/python/setup.py b/bindings/python/setup.py
index f108e9af..ea7b336d 100755
--- a/bindings/python/setup.py
+++ b/bindings/python/setup.py
@@ -245,7 +245,7 @@ setup(
         'Programming Language :: Python :: 2',
         'Programming Language :: Python :: 3',
     ],
-    requires=['ctypes', 'typing; python_version<"3.5"'],
+    install_requires=['typing; python_version<"3.5"'],
     cmdclass=cmdclass,
     zip_safe=False,
     include_package_data=True,
wtdcode commented 1 year ago

Closed due to #1823 and 2.1.0 will fix this.