search

capstone-engine / capstone

Capstone disassembly/disassembler framework for ARM, ARM64 (ARMv8), Alpha, BPF, Ethereum VM, HPPA, LoongArch, M68K, M680X, Mips, MOS65XX, PPC, RISC-V(rv32G/rv64G), SH, Sparc, SystemZ, TMS320C64X, TriCore, Webassembly, XCore and X86.
http://www.capstone-engine.org
7.51k stars 1.54k forks source link

Python/remove deprecated packaging #2396

Closed twizmwazin closed 2 months ago

twizmwazin commented 3 months ago

Your checklist for this pull request

Detailed description

This is a rebase of #2369, trying to fix the CI so it can be merged.

Test plan

...

Closing issues

...

Rot127 commented 3 months ago

After you are done fixing the CI (thanks again!), it would be enough to have most of the Python package jobs triggered on merge. No need to have them run on every push to a PR. I think only having the oldest and newest Python version is enough for a PR. Because if one of them fails, it is likely that something in-between failed as well.

twizmwazin commented 3 months ago

So just an update on the progress here, I've ended up making several changes to the bindings packaging and CI. Since the python and cython packages have different build processes, I have separated them into two separate directories, though since they share much of their code I have symlinked the relevant directories to avoid duplication as much as possible. Additionally, I have changed the python bindings CI to be much more of a typical python CI pipeline: set up python, checkout, pip install, run tests. This is both easier to me as well as more aligned with how I assume most users using the python bindings will install capstone, at least this is true for angr users. For a future PR I would like to refactor the python bindings tests to use pytest.

Cython bindings still have several issues I need to work through, but if anyone has any other feedback, please let me know.

Rot127 commented 3 months ago

Nice! Sounds very reasonable!

For a future PR I would like to refactor the python bindings tests to use pytest.

Spent only as little work as absolutely necessary on the testing. I currently re-write it completely: https://github.com/capstone-engine/capstone/pull/2384

Rot127 commented 2 months ago

Do you think you will manage to finish this in the next week or so? Otherwise I would move this one to the v5.0.3 milestone and ask @kabeor to release v5.0.2. Just so we have release before v6/v5.0.3 as we talked about it before.

twizmwazin commented 2 months ago

I have no prior experience with Cython so I haven't yet been able to solve much with the cython bindings. I can try to see if I can solve them by the end of this week but I cannot make any promises. If nobody is actually using them I would advocate for dropping them altogether since I highly doubt the performance overhead of cffi vs cython is significant with how capstone is used from python.

twizmwazin commented 2 months ago

Alright @Rot127, CI is green and I think this is ready

Rot127 commented 2 months ago

Awesome! I'll review it tomorrow. Could you open also open a PR on the v5 branch with these changes?

twizmwazin commented 2 months ago

One last tweak: Python 3.7 is EOL and thus I earlier removed it from testing due to it not being supported on current OS versions. This change reflects that in setup.py so that users using python 3.7 will get the last supported version instead of this version, which may not work.

XVilka commented 2 months ago

@kabeor @aquynh please merge

Rot127 commented 2 months ago

Uh, wait. I can merge this time as well :D