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

Update to PyPy 7.3.4 #629

Closed
wants to merge 11 commits into from
Closed

Conversation

YannickJadoul
Copy link
Member

Still in draft, but preparing for when a release is made. The Python 3.6-compatible version of PyPy was dropped, as the Python 3.7-compatibility was deemed completed (cfr. https://doc.pypy.org/en/latest/release-v7.3.4.html).

I still need to get an updated manylinux docker image.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Mar 25, 2021

The Python 3.6-compatible version of PyPy was dropped, as the Python 3.7-compatibility was deemed completed (cfr. https://doc.pypy.org/en/latest/release-v7.3.4.html).

Something to think about, btw: since this is just a patch version increase of PyPy, do we actually want to keep building pp36-pypy36_pp73 wheels with 7.3.3? I'll ask the same question when submitting a PR for PyPy's manylinux images.

EDIT: Same goes for 32-bit Windows.

@henryiii
Copy link
Contributor

The targets have changed in a patch version before (3.7 got added). I think it's fine to change on PyPy - 3.6 (due to NumPy dropping it) is rapidly getting harder to use.

Wait, are there no 32 bit binaries for Windows? They should have both, not drop 32 for 64? I know 32-bit CPython was rather popular (maybe heavily due to the fact it was the default download choice until very recently more than due to OS requirements).

@YannickJadoul
Copy link
Member Author

The targets have changed in a patch version before (3.7 got added). I think it's fine to change on PyPy - 3.6 (due to NumPy dropping it) is rapidly getting harder to use.

Right, but adding something is more backwards compatible, ofc. But yeah, fair enough; I guess it's PyPy's decision to release it as 7.3.4 (and ofc not worth breaking the ABI version for).

Wait, are there no 32 bit binaries for Windows? They should have both, not drop 32 for 64? I know 32-bit CPython was rather popular (maybe heavily due to the fact it was the default download choice until very recently more than due to OS requirements).

I had already popped onto IRC to ask, yes. Nothing that seems to secret, so I'll post it here:

<YannickJadoul> Hi all. I'm trying out PyPy 7.3.4rc1 in cibuildwheel, for the interested: https://github.com/joerick/cibuildwheel/pull/629
<YannickJadoul> Quick question: there's no win32 anymore? PyPy won't provide both 32-bit as well as 64, just like CPython still seems to do?
<mattip> hmm. Just because we can, should we? It is a small maintenance burden
<YannickJadoul> Ah, no, not saying that. Just curious, if the switch will be total
<YannickJadoul> Important to update our docs and think about releases and version numbers, etc
<mattip> unless there is a user who says otherwise: yes, the switch will be total
<YannickJadoul> OK, thanks!

I'll just ping @mattip, in case you'd like to convince him about the popularity of 32-bit :-)

@mattip
Copy link
Contributor

mattip commented Mar 25, 2021

I am willing to be convinced to release 32-bit windows, but it would have to be by a user who can explain how it is worth the (admittedly small) maintenance burden).

@YannickJadoul
Copy link
Member Author

@Czaki, I don't remember the details, but I'm seeing "PyPy patch not applied". Do you know by heart whether we can remove that patch and workaround, or should I check the PR that introduced it again?

@Czaki
Copy link
Contributor

Czaki commented Mar 25, 2021

Where did you see this? In log?

@YannickJadoul
Copy link
Member Author

Yep. The "Get some sample wheels" step of GHA:

32 wheels produced in 2 minutes:
  spam-0.1.0-cp27-cp27m-manylinux1_i686.whl
PyPy patch not applied
  spam-0.1.0-cp27-cp27m-manylinux1_x86_64.whl
  spam-0.1.0-cp27-cp27m-manylinux2010_i686.whl
  spam-0.1.0-cp27-cp27m-manylinux2010_x86_64.whl
  spam-0.1.0-cp27-cp27mu-manylinux1_i686.whl
  spam-0.1.0-cp27-cp27mu-manylinux1_x86_64.whl
...

@Czaki
Copy link
Contributor

Czaki commented Mar 25, 2021

Patch are here: https://github.com/joerick/cibuildwheel/blob/master/cibuildwheel/resources/pypy_venv_27.patch and https://github.com/joerick/cibuildwheel/blob/master/cibuildwheel/resources/pypy_venv.patch

If I good remember there was a problem that in virtualenv compiler cannot find "Python.h"

maybe current version fix it already.

@YannickJadoul
Copy link
Member Author

maybe current version fix it already.

Yeah, didn't you submit a PR/issue to PyPy? Any idea whether that got merged/resolved?

@YannickJadoul
Copy link
Member Author

OK, I stopped being lazy for a second and looked it up myself: #501 (comment)

@YannickJadoul
Copy link
Member Author

OK, our test_time_to_remove_the_pypy_venv_patch was even telling us. But I hadn't noticed yet, because of the Windows issues.

@YannickJadoul
Copy link
Member Author

So, something's wrong with the headers of pypy2.7-v7.3.4rc1-win64.zip; reported on IRC already. Apart from that, things seem to be working :-)

@mattip
Copy link
Contributor

mattip commented Mar 26, 2021

The headers are produced as part of an optional module in PyPy called cpyext that provides the C-API compatibility layer. We do not compile pypy2.7 on win64 with cpyext. Would it be hard to do if pypy and windows and python2.7: <don't check compiling a library> ? Let's see if there are any people who this is critical for them, then we can discuss whether to ship a 32-bit windows version (which does have cpyext) or to get cpyext going on win64-python2.7

I started work on this in the win64-cpyext-default branch, but then someone pointed out that tp_hash and hashes in general on python2.7 are long, which are 4-bytes even on win64. This messes with some of the RPython assumptions about sizeof(long) so it is a bit of a mess: we need to be careful not to mix long and pointers

@Czaki
Copy link
Contributor

Czaki commented Mar 26, 2021

The headers are produced as part of an optional module in PyPy called cpyext that provides the C-API compatibility layer. We do not compile pypy2.7 on win64 with cpyext. Would it be hard to do if pypy and windows and python2.7: <don't check compiling a library> ? Let's see if there are any people who this is critical for them, then we can discuss whether to ship a 32-bit windows version (which does have cpyext) or to get cpyext going on win64-python2.7

But the main task of cibuildwheel is compiling libraries. And most of the extensions need Python.h header.

@YannickJadoul
Copy link
Member Author

Would it be hard to do if pypy and windows and python2.7: <don't check compiling a library> ?

I guess we could plainly not offer PyPy Python 2.7 on Windows, then? I think cibuildwheel is only useful in case of binary extensions, right?

Oh, no, wait. There's still cffi, of course. But cibuildwheel doesn't really have a way to test what kind of extension is getting compiled :-/

This messes with some of the RPython assumptions about sizeof(long) so it is a bit of a mess: we need to be careful not to mix long and pointers

(sorry to hear about that btw; sounds like a horrible issue :-/ )

@henryiii
Copy link
Contributor

Biased for pybind11, but not having Python.h kills pybind11 extensions.

Not the best solution quite yet, but I guess needs to be mentioned: dropping Python 2 support would fix this issue...

long, which are 4-bytes even on win64

ROOT has been fighting with this for a while, still don't have a 64-bit windows build due to it. It is clearly written in the standard that long does not have to be 64-bits, but that hasn't stopped assumptions based on Linux/macOS or 32-bit Windows.

@mattip
Copy link
Contributor

mattip commented Mar 26, 2021

Biased for pybind11, but not having Python.h kills pybind11 extensions.

Right. How many projects have a user base that needs testing/releasing with PyPy on windows for python2.7? Before I spend a few days solving this, I would like to know it is going to be used.

@YannickJadoul
Copy link
Member Author

Yeah, dropping 2.7 would of course also work. Or dropping PyPy 2.7 on Windows, for now? (or probably just everywhere to be consistent)
Apart from not being able to test it, I'm mostly concerned by offering PyPy 2.7 on Windows but having users' builds fail because they expect the C API to be there. I wouldn't be surprised if that resulted in a bunch of issues blaming cibuildwheel for offering pp27-win_amd64.

@henryiii
Copy link
Contributor

Honestly, I don't know how much of a user base PyPy2 has. With NumPy requiring 3.7 now, it's likely a bit limited. And 2.7 on Windows is traditionally small due to other compiler reasons (and the fact it's easier to upgrade on Windows than having a build-in Python 2 on Linux/macOS). I do think we are going to be dropping PyPy2 when dropping CPython 2.7 in 2.0. But that's not quite here yet.

@YannickJadoul YannickJadoul changed the title Try out PyPy 7.3.4rc1 Try out PyPy 7.3.4rc2 Apr 5, 2021
@YannickJadoul
Copy link
Member Author

So, we're still failing on PyPy 2.7's missing headers on Windows. @joerick, where would you like to go with this? Still build 2.7 32-bit for now, using 7.3.3? Or just drop PyPy 2.7 on Windows, or ... ?

@mattip
Copy link
Contributor

mattip commented Apr 5, 2021

I would vote for dropping it until you hear from users.

Reasoning: If you drop PyPy2.7 for windows, you would quickly hear from any users who need it, and then PyPy could invest the effort to support cpyext for that verison. Any other work-around will encourage users to stay with an older version.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Apr 5, 2021

@mattip, sure. This seems to be working. The downside is that users also won't be able to build cffi wheels for PyPy 2.7 (on Windows), ofc. After all, this is just a decision to be made by PyPy, I'd say. (Unless you want us to support PyPy 2.7 (for cffi wheels), without C API headers; then it'd be up to @joerick to decide whether that's risking a lot of complaints about failing builds.)

README.md Outdated Show resolved Hide resolved
cibuildwheel/resources/build-platforms.toml Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor

mattip commented Apr 5, 2021

@YannickJadoul let's give it a shot. If it turns out lots of people want to package for pypy2.7 on windows, we can think harder about it.

@@ -115,8 +115,8 @@ def __init__(self, arch_str: ArchStr):
self.arch = arch_str

def update_version_windows(self, spec: Specifier) -> ConfigWinCP:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll probably take some more changes. But I'm guessing we'll have to wait for a final release of 7.3.4 to fix it.

@joerick
Copy link
Contributor

joerick commented Apr 5, 2021

I'm cool to drop 2.7 on Windows, too. We're dropping 2.7 as a whole in #596 anyway, so it probably won't be a big issue :)

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Apr 8, 2021

Waiting for pypy/manylinux#16 to pull this out of "draft"

@YannickJadoul YannickJadoul changed the title Try out PyPy 7.3.4rc2 Update to PyPy 7.3.4 Apr 8, 2021
@YannickJadoul
Copy link
Member Author

For the record:

It turns out there is a problem with the win64 release: the c-extension platform name is win32. It should be win_amd64.

This was reported by cgohlke in https://foss.heptapod.net/pypy/pypy/-/issues/3443.

I will be releasing a 7.3.5 bugfix soon. In the meantime, please do not upload win64 wheels to PyPI built with the 7.3.4 release: they will be tagged as appropriate for win32 which will cause problems for people who have locally built a 32-bit windows pypy3.7 and use "pip install".

Note we did not officially release a 32-bit windows, so the damage so far might be minimal.

Matti

See https://mail.python.org/pipermail/pypy-dev/2021-April/016150.html

So we probably ought to be skipping 7.3.4 and immediately pin 7.3.5?

@mattip
Copy link
Contributor

mattip commented Apr 21, 2021

That makes sense, although it may be a week or two until 7.3.5 is released.

@YannickJadoul
Copy link
Member Author

Thanks, @mattip! I don't think we're in a huge hurry to merge this, currently?

@henryiii
Copy link
Contributor

Not if it's broken. :)

@YannickJadoul
Copy link
Member Author

YannickJadoul commented May 12, 2021

Seems I broke GitHub by force-pushing the Windows-only changes to this branch of the closed PR. Huh. Anyway, I'll open a new draft with those leftover changes that weren't in #666

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

Successfully merging this pull request may close these issues.

None yet

6 participants