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. |
jaraco
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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_directoryinprepare_metadata_for_build_wheel. Distutils has an explicit check for astrvalue 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