-
Notifications
You must be signed in to change notification settings - Fork 692
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
Stop using distutils #2564
Stop using distutils #2564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Thanks for the pr, care to keep 2.7 compat? |
Based on https://docs.python.org/2.7/library/sysconfig.html , it is 2.7 compatible? The CI fail looks unrelated:
|
You changed the search paths for includes, i guess sysconfig and distutils.sysconfig does not return the expected paths on python 2.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're picking the wrong path for the second call, but I think there's something wrong (different) with the include paths that are built and the actual system. I printed the difference between the two methods here https://github.com/unbit/uwsgi/actions/runs/6398069839/job/17367389012?pr=2567#step:8:13 and you can see that the newer call returns a /usr/local/ path for the includes.
I clearly latched onto platform-specific files in the docs, without considering it was for header files, nice catch! |
distutils is due to be removed in Python 3.12, but even so we don't need to use it, since the sysconfig module can handle our use-cases just fine for all supported Python versions.
2783e8b
to
8041fc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like debian has the following patch https://salsa.debian.org/cpython-team/python2/-/blob/2.7.18-12/debian/patches/distutils-install-layout.diff which is breaking this. You could specify PYTHONUSERBASE=1
to bypass the bug, but this looks like it will break other users if you did that to pass the tests.
You could handle it with the following suggestion (I've tested it with #2567), which you can pull into your branch by creating PR s-t-e-v-e-n-k/uwsgi@no-more-distutils...terencehonles:uwsgi:pr-2564-patch and merging it (which will update this PR).
However, this may break on Python 3.12 (which currently doesn't build) because if that patch still applies to Python 3, distutils is no longer included, and the path is still wrong, the include path will not be set correctly.
@@ -1,10 +1,10 @@ | |||
from distutils import sysconfig | |||
import sysconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import sysconfig | |
import os | |
import sysconfig | |
def get_includes(): | |
try: | |
from distutils import sysconfig as legacy | |
except ImportError: | |
legacy = None | |
yield sysconfig.get_path('include') | |
yield sysconfig.get_path('platinclude') | |
if legacy: | |
yield legacy.get_python_inc() | |
yield legacy.get_python_inc(plat_specific=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sysconfig.get_path
also allows us to specify which scheme we want with the second argument, rather than what you've done here. However, I'm loathe to check if 'deb_system' is a suitable scheme, but this only appears to rear it's head on Python 2, rather than every version of Python 3 that also ships distutils.
barriere% python3
Python 3.9.2 (default, Feb 28 2021, 17:03:44)
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_path('include')
'/usr/include/python3.9'
>>>
barriere% python2
Python 2.7.18 (default, Jul 14 2021, 08:11:37)
[GCC 10.2.1 20210110] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_path('include')
'/usr/local/include/python2.7'
>>> sysconfig.get_path('include', 'deb_system')
'/usr/include/python2.7'
'-I' + sysconfig.get_path('include'), | ||
'-I' + sysconfig.get_path('platinclude') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'-I' + sysconfig.get_path('include'), | |
'-I' + sysconfig.get_path('platinclude') | |
'-I' + i for i in filter(os.path.exists, get_includes()), |
Merged a modified version in #2570 |
distutils is due to be removed in Python 3.12, but even so we don't need to use it, since the sysconfig module can handle our use-cases just fine for all supported Python versions.