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

Python 3.10: _version_nodot, interpreter_version disagree on how to format it #308

Closed
mattip opened this issue May 23, 2020 · 26 comments
Closed

Comments

@mattip
Copy link
Contributor

mattip commented May 23, 2020

There is a problem building and using wheels for python 3.10 xref pypa/wheel#354 and pypa/pip#8312. I think the source of the problem is that wheel's bdist_wheel calls tags.interpreter_version. But tags.interpreter_version seems to be producing 310, which it will take from sysconfig.get_config_var("py_version_nodot") (if it exists), and there is no check that is conformant with the 3_10 format.

Internally, packaging uses tags._version_nodot which will always produce 3_10, this is the "correct" value used by pip and in tags.sys_tags.

This is the code that generates that value for CPython master. Note there is no '_'

@brettcannon
Copy link
Member

brettcannon commented May 23, 2020

Yeah, this is going to be a tricky one since going against py_version_nodot is not great as I'm sure folks are relying on us reading from that setting. Maybe we need to fix this upstream in Python?

@mattip
Copy link
Contributor Author

mattip commented May 23, 2020

Could try: I wonder what other things that will break. Virtualenv? setuptools?

@pradyunsg
Copy link
Member

I'm pretty sure we should fix this upstream in CPython itself.

@mattip
Copy link
Contributor Author

mattip commented May 23, 2020

CPython issue and PR submitted.

@Gelbpunkt
Copy link

Gelbpunkt commented May 23, 2020

Hi, I quickly checked the PR against my build suite. It's late and therefore I cannot investigate an in-depth error, but here's what I got when compiling orjson from source

File "/tmp/pip-build-env-gkq1mzl2/overlay/lib/python3.10/site-packages/wheel/bdist_wheel.py", line 179, in get_tag
    assert tag == supported_tags[0], "%s != %s" % (tag, supported_tags[0])
AssertionError: ('cp3_10', 'cp310', 'linux_x86_64') != ('cp310', 'cp310', 'linux_x86_64')
----------------------------------------
ERROR: Failed building wheel for maturin
Failed to build maturin
----------------------------------------

@mattip
Copy link
Contributor Author

mattip commented May 23, 2020

@Gelbpunkt when you have some time could you see if the fix in python/cpython#20333 fixes the problem?

@Gelbpunkt
Copy link

@Gelbpunkt when you have some time could you see if the fix in python/cpython#20333 fixes the problem?

I was using that one and had the issue, that's what I was referring to.

@mattip
Copy link
Contributor Author

mattip commented May 24, 2020

It turns out my cursory check of bdist_wheel.get_abi_tag was mistaken: it first checks the value of soabi = get_config_var('SOABI'), which is cpython-310-x86_64-linux-gnu on my platform. Then it uses the 310 without calling tags.interpreter_version(). It is still true that tags._version_nodot and tags.interpreter_version() disagree. But in light of the depth of the surgery required to change this for CPython, perhaps it would be easier to change tags._version_nodot here.

@brettcannon
Copy link
Member

Right, but if we start patching around it here it can still lead to skew. As you pointed out, "SOABI" and "py_version_nodot" will not match what others do if they read these directly. The key thing is we be consistent with PEP 425 and in that instance it says CPython uses "py_version_nodot" for the interpreter tag. As for SOABI, it's mentioned in the FAQ of PEP 425.

So if this doesn't change upstream then we may need to choose between doing the change ourselves and hope people all agree or revert the 3_10 formatting that's being done here.

@Gelbpunkt
Copy link

With the latest changes to the PR, my AssertionError: ('cp3_10', 'cp310', 'linux_x86_64') != ('cp310', 'cp310', 'linux_x86_64') from when it was newly opened turns into AssertionError: ('cp3_10', 'cp3_10', 'linux_x86_64') != ('cp310', 'cp3_10', 'linux_x86_64'). I assume that means it's still using the old scheme at some point

@mattip
Copy link
Contributor Author

mattip commented May 26, 2020

@Gelbpunkt it worked for me, this runs to completion with no error:

# get the commit with the fix from python/cpython#20333
cd cpython
./configure
make -j4
cd ../cython
../cpython/python -m ensurepip --user
.../cpython/python -m pip install --user wheel
../cpython/python setup.py bdist_wheel

and the produced wheel has the name Cython-3.0a4-cp3_10-cp3_10-linux_x86_64.whl

@Gelbpunkt
Copy link

Gelbpunkt commented May 26, 2020

@mattip does that also work fine when you build something like orjson from source? That's where I had this issue, gonna try cython later

Edit: same issue with lru-dict

@mattip
Copy link
Contributor Author

mattip commented May 26, 2020

@Gelbpunkt orjson mumbled something about rustc, error: the option Z is only accepted on the nightly compiler and died, not connected to this problem

@Gelbpunkt
Copy link

Gelbpunkt commented May 26, 2020

@mattip try lru-dict, that's pure C and won't require you to use rust-nightly.
I get the same in Cython though:

AssertionError: ('cp3_10', 'cp3_10', 'linux_x86_64') != ('cp310', 'cp3_10', 'linux_x86_64')

I compiled CPython from the master branch with the PR #20333 half an hour ago.

@mattip
Copy link
Contributor Author

mattip commented May 26, 2020

Are you sure you are using wheel from the HEAD of the git repo? It should not have pep425 in bdist_wheel.py, rather packaging.

@mattip
Copy link
Contributor Author

mattip commented May 26, 2020

My instructions above were wrong, it should be

../cpython/python -m pip install --user git+https://github.com/pypa/wheel.git

@Gelbpunkt
Copy link

I'm using that version of wheel (with packaging) and it gives me the error regardless. Probably because I'm doing pip wheel . to force it to build before?

@mattip
Copy link
Contributor Author

mattip commented May 26, 2020

What platform are you on? I am using linux.

@Gelbpunkt
Copy link

What platform are you on? I am using linux.

As stated in the original issue, I'm on Linux as well.

@mattip
Copy link
Contributor Author

mattip commented May 27, 2020

The code in bdist_wheel is pretty straightforward. Could you add a import pdb;pdb.set_trace() there and try to figure out what is going on? You should see a call to tags.sys_tags(), which returns a generator, you can even step in to see how it is getting the 'interpreter' part of the tag.

@Gelbpunkt
Copy link

Gelbpunkt commented May 28, 2020

@mattip I got further: Cython and other stuff builds, the issue seems to be within maturin itself, which does not even build (probably due to this), which they got from here. Removing the bdist_wheel override in the setup makes it buid as py3-none-any, that's okay for me. The resulting binary built for orjson then was orjson-3.0.2-cp310-cp310-linux_x86_64.whl, so it seems to be an issue there and not related to wheel anymore. All other stuff seems to build for me.
Thanks for your effort to make this work :)

@mattip
Copy link
Contributor Author

mattip commented Oct 13, 2020

This issue is delaying testing of python 3.10. I am not sure if it is more work to remove this clause when determining nodot, release a new version, and get that version into pip and wheel or convince CPython to accept python/cpython#20333.

jonringer added a commit to jonringer/nixpkgs that referenced this issue Oct 13, 2020
A python 3.10 wheel would produce a wheel with the
platform tag as `cp310`, which would be invalid, as
only 3_10 is accepted by the wheel package.

See: pypa/packaging#308
     pypa/wheel#354
     python/cpython#20333
@brettcannon
Copy link
Member

I'm going to see if I can't push to get the CPython PR resolved before the end of the core dev sprint.

@brettcannon
Copy link
Member

Quick report:

  1. A group of us at the core dev sprints agreed to change CPython
  2. @mattip has a couple of things to touch up on his PR for CPython
  3. I'm going to write up a quick PEP about the change
  4. Once the above are all taken care of we can merge in CPython
  5. We will have the alphas for 3.10 to see how this is taken by the community

@domdfcoding
Copy link
Contributor

Can this be closed now that #355 has been merged?

@pradyunsg
Copy link
Member

Yup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants