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

Remove python2 leftovers #2378

Merged
merged 3 commits into from
Jun 9, 2024
Merged

Conversation

twizmwazin
Copy link
Contributor

@twizmwazin twizmwazin commented Jun 1, 2024

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

Removes the python2 bits I could find.

Removes the python2 bits. No functional changes on python3. Extra changes avoided.

Test plan

Run tests on supported python3 versions.

Closing issues

#2342

@Rot127 Rot127 added the python bindings label Jun 2, 2024
@Rot127 Rot127 added this to the v6 milestone Jun 2, 2024
Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the CI and it is good IMO.

@Rot127
Copy link
Collaborator

Rot127 commented Jun 2, 2024

[ 99%] Building C object CMakeFiles/cstool.dir/cstool/getopt.c.o
[100%] Linking C executable cstool
[100%] Built target cstool
+ cd /src/capstonenext/bindings/python
+ sed -i -e s/#print/print/ capstone/__init__.py
+ export CFLAGS=
+ CFLAGS=
+ export AFL_NOOPT=1
+ AFL_NOOPT=1
+ python setup.py install
Traceback (most recent call last):
  File "setup.py", line 219, in <module>
    long_description=open('README.txt', encoding="utf8").read(),
TypeError: 'encoding' is an invalid keyword argument for this function
2024-06-02 08:12:42,865 - root - ERROR - Building fuzzers failed.
2024-06-02 08:12:42,865 - root - ERROR - Error building fuzzers for (commit: 90f34212fd79e050a3b60e6b7f5dbc525e525fa4, pr_ref: refs/pull/2378/merge).

@twizmwazin
Copy link
Contributor Author

twizmwazin commented Jun 2, 2024

Looks like the OSS Fuzz stuff is still using python 2. I can make a pull request to the OSS fuzz to switch them to using a newer python 3, but this will need to be merged first. Is it ok if we disable the fuzz pipeline temporarily here, make a pull request to OSS Fuzz, then re-enable the fuzz pipeline again once the changes are merged?

Edit: Actually nevermind I don’t think the disabling is necessary, I’ll make a PR over there today.

@Rot127
Copy link
Collaborator

Rot127 commented Jun 4, 2024

I would be surprised if the OSS Fuzz is still using Python 2. Maybe CS just uses an outdated branch or something?

@twizmwazin
Copy link
Contributor Author

twizmwazin commented Jun 4, 2024

Take a look here: https://github.com/google/oss-fuzz/tree/master/projects/capstone

In Dockerfile, it is using the generic base, not the python base, and installs python2. The branches are current, v5 and next. In build.sh, it uses python setup.py install, which will be python2.

@twizmwazin
Copy link
Contributor Author

twizmwazin commented Jun 4, 2024

Opened the PR on OSS-Fuzz: google/oss-fuzz#12028

Looks like I'll need an existing contributor to sign off on the change.

@kabeor
Copy link
Member

kabeor commented Jun 5, 2024

@aquynh, How do you think about this? Remove python 2 supporting.

@XVilka
Copy link
Contributor

XVilka commented Jun 5, 2024

@aquynh, How do you think about this? Remove python 2 supporting.

Python 2 support was already removed few months ago and discussed before. These are only remnants/leftovers.

@Rot127 Rot127 mentioned this pull request Jun 6, 2024
6 tasks
@XVilka
Copy link
Contributor

XVilka commented Jun 8, 2024

It seems that OSSFuzz PR fails because of the 5.0.1 use in the repo, so it should either should be switched to use next, or this one should be force-merged ignoring the red CIFuzz worker for now.

Copy link
Member

@kabeor kabeor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Let's try merge this first, if there is any issue, I'll revert it.

@kabeor kabeor merged commit 60d5b7e into capstone-engine:next Jun 9, 2024
13 of 14 checks passed
@kabeor
Copy link
Member

kabeor commented Jun 9, 2024

Still waitting for rerun in google/oss-fuzz#12028 .

@kabeor
Copy link
Member

kabeor commented Jun 24, 2024

Considering that the PR still needs more modifications to adapt to oss-fuzz, we must revert to let the project continue to progress. Welcome to continue to contribute.

kabeor added a commit that referenced this pull request Jun 24, 2024
kabeor added a commit that referenced this pull request Jun 24, 2024
DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Jun 27, 2024
Upstream has ended python2 support. When removing python2 compatibility
code, it was noticed that oss-fuzz was still utilizing this support.
This fixes that by moving to a supported version of python3.

See this PR: capstone-engine/capstone#2378
@XVilka
Copy link
Contributor

XVilka commented Jun 27, 2024

@kabeor @Rot127 I suppose this can be unreverted again?

@Rot127
Copy link
Collaborator

Rot127 commented Jun 28, 2024

@twizmwazin If you have a little time, it would be awesome if you could make another one. If you have one or two minutes more, please also do a grep bin/python[^3] over all scripts, update the shebang and remove the Python2 ones.

@twizmwazin twizmwazin mentioned this pull request Jun 28, 2024
2 tasks
@Rot127 Rot127 mentioned this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants