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.

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

Spaces in platform names are problematic #652

Closed
tucked opened this issue Jan 3, 2023 · 9 comments · Fixed by #620
Closed

Spaces in platform names are problematic #652

tucked opened this issue Jan 3, 2023 · 9 comments · Fixed by #620

Comments

@tucked
Copy link
Contributor

tucked commented Jan 3, 2023

On some platforms, sysconfig.get_platform() returns a value with a space in it (e.g. isilon onefs).
This causes problems for wheel building/handling.

First, there is an issue trying to build a platform-specific wheel:

# venv/bin/pip install -I --no-cache pyyaml
Collecting pyyaml
  Downloading PyYAML-6.0.tar.gz (124 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 125.0/125.0 KB 13.3 MB/s eta 0:00:00
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: pyyaml
  Building wheel for pyyaml (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Building wheel for pyyaml (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [101 lines of output]
...
      wheel.cli.WheelError: Bad wheel filename 'PyYAML-6.0-cp38-cp38-isilon onefs_9_5_0_0_amd64.whl'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for pyyaml

I created pypa/wheel#491 to address this.

Even with that patch in place, though, pip is unable to install the built wheel because packaging does not recognize that the platform name has had its spaces replaced with underscores:

# python3 -m pip install dist/PyYAML-6.0-cp38-cp38-isilon_onefs_9_5_0_0_amd64.whl
...
ERROR: PyYAML-6.0-cp38-cp38-isilon_onefs_9_5_0_0_amd64.whl is not a supported wheel on this platform.
@brettcannon
Copy link
Member

Can you point where in the packaging code base is the falling over?

And the "fun" part of this is https://peps.python.org/pep-0425/#platform-tag doesn't specify what to do with spaces (and actually, "space" is not mentioned once in the PEP). _ makes the most sense thanks to how it escapes everything else, but it's technically undefined.

@tucked
Copy link
Contributor Author

tucked commented Jan 4, 2023

Can you point where in the packaging code base is the falling over?

If we use PyYAML-6.0-cp38-cp38-isilon_onefs_9_5_0_0_amd64.whl1, for example, we can work backwards from the error.

AFAICT, the only alternative would be to have pip turn underscores to spaces, which seems... problematic.
The only known workaround is to set UNAME_s=isilon_onefs during pip install to manually normalize the platform name.

Footnotes

  1. Remember that PyYAML-6.0-cp38-cp38-isilon_onefs_9_5_0_0_amd64.whl can only be built with https://github.com/pypa/wheel/pull/491 (or by setting UNAME_s=isilon_onefs during the build).

@brettcannon
Copy link
Member

OK, that makes sense.

In #620 (comment) , @tucked noticed we now have a test that's explicitly checking an example with spaces, but I think that's just for parsing purposes and not normalization. But to double-check, @mattip , do you see any reason not to do this since I think you may have written the test in question?

@pradyunsg you okay with this change?

@pradyunsg
Copy link
Member

Yup! Do we want to update the spec language to cover spaces -> underscore?

@mattip
Copy link
Contributor

mattip commented Jan 26, 2023

The test was only meant to check round-trip behavior. It should not use spaces.

@brettcannon
Copy link
Member

Yup! Do we want to update the spec language to cover spaces -> underscore?

I think so, but I think it's going to require bringing PEP 425 over to packaging.python.org first.

@tucked
Copy link
Contributor Author

tucked commented Jan 26, 2023

Is this not covered in PEP 427:

Each component of the filename is escaped by replacing runs of non-alphanumeric characters with an underscore _?

re.sub("[^\w\d.]+", "_", distribution, re.UNICODE)

@brettcannon
Copy link
Member

Is this not covered in PEP 427

Technically it doesn't matter because that's not the spec anymore (as the top of the PEP says). The spec is now https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode and it actually doesn't mention anything about spaces, just that the wheel tags need to be valid. And the wheel tag spec at https://packaging.python.org/en/latest/specifications/platform-compatibility-tags/ redirects to PEP 425 which also lacks an opinion.

brettcannon added a commit to brettcannon/packaging.python.org that referenced this issue Feb 11, 2023
Also update to say spaces are not allowed in wheel tags (see pypa/packaging#652 ).
@brettcannon
Copy link
Member

I opened pypa/packaging.python.org#1206 to move PEP 425 over to packaging.python.org.

pradyunsg pushed a commit to pypa/packaging.python.org that referenced this issue Mar 6, 2023
Also update to say spaces are not allowed in wheel tags (see pypa/packaging#652 )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants