Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for Unicode handling in PEP 518 backend #1466

Merged
merged 3 commits into from
Aug 21, 2018
Merged

Fix for Unicode handling in PEP 518 backend #1466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83c7d77
PEP 517 hook arguments are unicode, not str (and distutils objects to…
pfmoore Aug 20, 2018
785feaf
Add a test for a Unicode metadata directory
pfmoore Aug 21, 2018
687cfab
Add a changelog entry
pfmoore Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/1466.change.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix handling of Unicode arguments in PEP 517 backend
15 changes: 14 additions & 1 deletion setuptools/build_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,19 @@ def patch(cls):
distutils.core.Distribution = orig


def _to_str(s):
"""
Convert a filename to a string (on Python 2, explicitly
a byte string, not Unicode) as distutils checks for the
exact type str.
"""
if sys.version_info[0] == 2 and not isinstance(s, str):
# Assume it's Unicode, as that's what the PEP says
# should be provided.
return s.encode(sys.getfilesystemencoding())
return s


def _run_setup(setup_script='setup.py'):
# Note that we can reuse our build directory between calls
# Correctness comes first, then optimization later
Expand Down Expand Up @@ -109,7 +122,7 @@ def get_requires_for_build_sdist(config_settings=None):


def prepare_metadata_for_build_wheel(metadata_directory, config_settings=None):
sys.argv = sys.argv[:1] + ['dist_info', '--egg-base', metadata_directory]
sys.argv = sys.argv[:1] + ['dist_info', '--egg-base', _to_str(metadata_directory)]
_run_setup()

dist_info_directory = metadata_directory
Expand Down
9 changes: 9 additions & 0 deletions setuptools/tests/test_build_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,12 @@ def test_prepare_metadata_for_build_wheel(build_backend):
dist_info = build_backend.prepare_metadata_for_build_wheel(dist_dir)

assert os.path.isfile(os.path.join(dist_dir, dist_info, 'METADATA'))


def test_prepare_metadata_for_build_wheel_with_unicode(build_backend):
dist_dir = os.path.abspath(u'pip-dist-info')
Copy link
Member

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().

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

os.makedirs(dist_dir)

dist_info = build_backend.prepare_metadata_for_build_wheel(dist_dir)

assert os.path.isfile(os.path.join(dist_dir, dist_info, 'METADATA'))