-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 for Unicode handling in PEP 518 backend #1466
Conversation
@jaraco What's the release schedule for setuptools? I need this change in a released version for the pip tests to succeed for the new PEP 517 implementation work. If a release is not likely to happen for a while, I can rework the pip test process to be able to handle testing against an unreleased setuptools, but there's no point if there'll be a release relatively soon :-) |
It's probably worth making at least a bugfix release soon to fix #1462. I think we'd want to bump it to a minor version release if this is included. |
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 to me. I have one minor edit to make, but I can make that following the merge.
|
||
|
||
def test_prepare_metadata_for_build_wheel_with_unicode(build_backend): | ||
dist_dir = os.path.abspath(u'pip-dist-info') |
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'd prefer to use from __future__ import unicode_literals
than using the somewhat deprecated u''
literal. If that's too complicated, let's use six.u('pip-dist-info')
or b'pip-dist-info'.decode()
.
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 think six.u'
is only necessary for Python <3.3. u'
is available and not really deprecated on all supported versions of Python, though I agree with using from __future__ import unicode_literals
, which will make our eventual transition to 3.x only easier.
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 was really quite happy when Python 3 got rid of the u''
syntax, and was sad when we brought it back, because it's an anti-pattern that's been perpetuated mostly out of convenience (and rarely because it's the best option). It's just one piece of Python 2 debt I've managed to avoid in my projects.
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 noticed that u''
was uncommon, but to be honest I was trying to make the change as minimal as possible, and I didn't want to risk breaking other tests by changing a global setting. But I'm fine with using the future import. I find it hard to be sure what is best practice with six
(every project seems to have its own variation) and I hadn't thought of b''.decode()
. But whatever works best - fighting the differences between the Python 2 and Python 3 Unicode models is something I'll be glad to see the back of once we can finally ditch Python 2.
Summary of changes
Modifies the PEP 517 backend to handle a Unicode value for the
metadata_directory
inprepare_metadata_for_build_wheel
. Distutils has an explicit check for astr
value foregg-base
, which fails on Python 2 when passed Unicode (even if the value is convertible to str), so we convert the parameter to a string using the filesystem encoding in that case.Pull Request Checklist