-
Notifications
You must be signed in to change notification settings - Fork 101
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
Handle wheel.bdist_wheel
deprecation warning
#463
Conversation
setuptools_rust/setuptools_ext.py
Outdated
try: # old version of setuptools | ||
from wheel.bdist_wheel import bdist_wheel | ||
except ImportError: | ||
bdist_wheel = None |
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.
This is a bit ugly (multiple degrees of nested), but similar to other code in the same file...
Alternatively we could also shove this conditional import into a separated file to hide the mess.
2d6a1c8
to
7bcef2a
Compare
setuptools_rust/build.py
Outdated
try: | ||
from wheel.bdist_wheel import bdist_wheel as CommandBdistWheel | ||
except ImportError: # wheel installation might be deferred in PEP 517 | ||
from setuptools import Command as CommandBdistWheel | ||
from setuptools.command.bdist_wheel import bdist_wheel as CommandBdistWheel | ||
except ImportError: # old version of setuptools | ||
from setuptools import Command as CommandBdistWheel # type: ignore[assignment] |
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.
You should instead try all 3 imports...
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.
No problems, I can do that. I felt safe to skip the wheel
import in this particular case because it is used for type checking only (adding the import from wheel would inevitably require some type: ignore
s comments and at that point the contribution for type checking would be questionable).
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.
Implemented in 810ae21. Can be reversed if deemed not necessary.
2e1d65b
to
810ae21
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.
Thanks as always, looks good to me!
(Needs a CHANGELOG entry; I will push one and add later if you don't beat me to it 👍) |
... by trying to import first from setuptools.
I also changed some of the test/CI related files to remove the (no-longer-)dependency on
wheel
(which also impliessetuptools>=70.1
in some cases).Finally it was necessary to add a temporary workaround for python-cffi/cffi#117 (i.e. "pin"
setuptools<74
for some scenarios). This workaround can be removed once python-cffi/cffi#117 is tackled.Closes #461