-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Replace distutils by setuptools to import build_ext #14108
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
Conversation
setup.py
Outdated
| import platform | ||
| import subprocess | ||
| import sys | ||
| from distutils import log as logger |
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.
as said in the warning "make sure that setuptools is always imported before distutils." do we need update this distutils on line 10 as well?
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 looked into setuptools code to see how to replace it. I found setuptools is also using distutils log: https://github.com/pypa/setuptools/blob/main/setuptools/logging.py#L4. I moved distutils import.
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.
Since distutils is going to disappear, I replaced the logger by its implementation I found here: https://github.com/pypa/distutils/blob/main/distutils/_log.py.
|
I applied this change, but warning still exists. This change can only fix the warning when run setup.py, but the real warning caused by |
|
distutils was used to compare version. I replaced it with packaging. setup.py import distutils to log. The warning should disappear. |
|
|
||
| try: | ||
| from packaging.version import Version as LooseVersion | ||
| except ImportError: |
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.
curious why we need this try catch?
|
I applied all you changes in my environment, now the warning is gone. Thank you so much for your help. @xadupre |
Description
Uses setuptools instead of distutils.
Motivation and Context
Fixes #14107.