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

Fix Python 3.8 to 3.11 #2122

Closed
wants to merge 1 commit into from
Closed

Fix Python 3.8 to 3.11 #2122

wants to merge 1 commit into from

Conversation

Arusekk
Copy link
Contributor

@Arusekk Arusekk commented Mar 3, 2025

Current logic requires old setuptools on py3.8 up to py3.11. So if new setuptools is installed, importing unicorn always fails with ImportError on these Python versions.

It now tries to use old setuptools on py3.8, but falls back gracefully to not using anything.
So it works on py3.9+ fully, and on py3.8 it also works but with more limited path discovery (I think it is enough, since no one complained yet).

Fixes: 0c34496a395eac567dc24eb6574550938a52b126 ("Modify canonicals import")
Ref: Gallopsled/pwntools#2556

Previous logic required old setuptools on py3.8 up to py3.11.
So if new setuptools was installed, importing unicorn always failed with
ImportError on these Python versions.

It now tries to use old setuptools on py3.8, but falls back gracefully
to not using anything.
So it works on py3.9+ fully, and on py3.8 it also works but with more
limited path discovery (I think it is enough, since no one complained
yet).

Fixes: 0c34496 ("Modify canonicals import")
Ref: unicorn-engine@0c34496
Ref: Gallopsled/pwntools#2556
@wtdcode
Copy link
Member

wtdcode commented Mar 3, 2025

Thanks for your patch but this actually surprises me as our wheels are tested for all these versions. How to reproduce it? So that we can add this to our CI too. Is it related to outdated (or newer?) setuptools? Is it possible that we should have more precise dependency on setuptools?

FYI, our CI testing wheels on all python versions: https://github.com/unicorn-engine/unicorn/actions/runs/13309809343

cc @Antelox

@Arusekk
Copy link
Contributor Author

Arusekk commented Mar 3, 2025

What I did:

# set up UV with virtualenv
uv pip install unicorn  # selects the wheel but does not install setuptools at all
python -c 'import unicorn'

UV is an alternative implementation of pip, which does not install even pip into the venv.

The reason this worked in CI is because precisely in python3.12 the stdlib venv module stopped installing setuptools. This was probably the cause 3.12 was chosen by the PR author as the split point instead of 3.9.
So it should not normally happen, in virtual environments or otherwise; this bug is low-impact. But this means that setuptools (with deprecated pkg_resources module) is currently a de-facto requirement for unicorn with py3.11 and below, so this bug will surface on systems where unicorn is a distro package. This PR lifts that restriction.

@Arusekk
Copy link
Contributor Author

Arusekk commented Mar 3, 2025

I just saw that Unicorn CI uses virtualenv rather than venv, but the same applies: pypa/virtualenv#2558

@wtdcode
Copy link
Member

wtdcode commented Mar 3, 2025

What I did:

# set up UV with virtualenv
uv pip install unicorn  # selects the wheel but does not install setuptools at all
python -c 'import unicorn'

UV is an alternative implementation of pip, which does not install even pip into the venv.

The reason this worked in CI is because precisely in python3.12 the stdlib venv module stopped installing setuptools. This was probably the cause 3.12 was chosen by the PR author as the split point instead of 3.9. So it should not normally happen, in virtual environments or otherwise; this bug is low-impact. But this means that setuptools (with deprecated pkg_resources module) is currently a de-facto requirement for unicorn with py3.11 and below, so this bug will surface on systems where unicorn is a distro package. This PR lifts that restriction.

Thanks for good explanation. I think our excellent contributor @Antelox is looking into it.

@wtdcode
Copy link
Member

wtdcode commented Mar 3, 2025

Before that I need to fix CI firstly, btw.

@Antelox
Copy link
Contributor

Antelox commented Mar 3, 2025

Hi @Arusekk please take a look at the PR I created right now. I' sort of took over from this one to resolve an old issue as well. Thanks for this!

@wtdcode
Copy link
Member

wtdcode commented Mar 4, 2025

Closing in favor of #2123

@wtdcode wtdcode closed this Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants