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 PyPy to 7.3.5 and use PyPA's manylinux images with PyPy #671

Merged
merged 15 commits into from
May 25, 2021

Conversation

YannickJadoul
Copy link
Member

This also includes the leftover Windows changes from #629 and #666.

@YannickJadoul
Copy link
Member Author

@mattip, FYI, 7.3.5rc2 seems to pass all our tests, also on Windows :-)

@mayeut
Copy link
Member

mayeut commented May 23, 2021

@YannickJadoul, with PyPy 7.3.5 released and integrated in manylinux images, I've pushed a few commits and will push a bit more on this branch.

@YannickJadoul
Copy link
Member Author

Great news, @mayeut! Let me quickly rebase this onto master (with #685).

@YannickJadoul
Copy link
Member Author

(I hope this isn't going to cause any issues for you. If so, feel free to force-push whatever it is you currently have!)

@YannickJadoul YannickJadoul changed the title Try out PyPy 7.3.5 release candidates Update PyPy to 7.3.5 and use PyPA's manylinux images with PyPy May 23, 2021
@mayeut
Copy link
Member

mayeut commented May 23, 2021

rebased onto master (with #689)

@mayeut
Copy link
Member

mayeut commented May 24, 2021

@mattip, FYI, 7.3.5 seems to pass all our tests (including the new manylinux images with x86_64/i686/aarch64)

@mattip
Copy link
Contributor

mattip commented May 24, 2021

Whew! Do the windows wheels look normal?

@mayeut
Copy link
Member

mayeut commented May 24, 2021

Do the windows wheels look normal?

@mattip, What do you mean by normal ?

They can be built with the proper tag, they can be installed and they can be used.

@mayeut mayeut marked this pull request as ready for review May 24, 2021 11:16
@mayeut
Copy link
Member

mayeut commented May 24, 2021

I think this is now ready for review.

@@ -110,7 +110,7 @@ Options
| | [`CIBW_BEFORE_ALL`](https://cibuildwheel.readthedocs.io/en/stable/options/#before-all) | Execute a shell command on the build system before any wheels are built. |
| | [`CIBW_BEFORE_BUILD`](https://cibuildwheel.readthedocs.io/en/stable/options/#before-build) | Execute a shell command preparing each wheel's build |
| | [`CIBW_REPAIR_WHEEL_COMMAND`](https://cibuildwheel.readthedocs.io/en/stable/options/#repair-wheel-command) | Execute a shell command to repair each (non-pure Python) built wheel |
| | [`CIBW_MANYLINUX_X86_64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_I686_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_PYPY_X86_64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_AARCH64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_PPC64LE_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_S390X_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) | Specify alternative manylinux docker images |
| | [`CIBW_MANYLINUX_X86_64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_I686_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_PYPY_X86_64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_AARCH64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_PPC64LE_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_S390X_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_PYPY_AARCH64_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) <br> [`CIBW_MANYLINUX_PYPY_I686_IMAGE`](https://cibuildwheel.readthedocs.io/en/stable/options/#manylinux-image) | Specify alternative manylinux docker images |
Copy link
Member

Choose a reason for hiding this comment

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

I will do a single comment here that might apply throughout the remaining of the changes.

Should we remove the CIBW_MANYLINUX_PYPY_* environment variables (adding a deprecation warning for CIBW_MANYLINUX_PYPY_X86_64_IMAGE ) ?

I went with "we should keep it" and thus "them" for consistency's sake.

The only thing remaining would thus be how to best order those options in docs (either group by CPython/PyPy or by arch ? The current layout is by age).

Copy link
Member Author

@YannickJadoul YannickJadoul May 24, 2021

Choose a reason for hiding this comment

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

I was thinking something similar, but seems you beat me :-) If manylinux now "compresses" the two into one, then shouldn't we do so as well?

The more I think about it, the more I think we should do this, if we can deprecate CIBW_MANYLINUX_PYPY_X86_64_IMAGE. Extra argument: the PyPy manylinux images also include everything from manylinux itself, as a base. So if someone needs to use these old ones, it should still be possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

About deprecating CIBW_MANYLINUX_PYPY_X86_64_IMAGE: this is slightly more tricky. Before, when we did so, we basically added it to the new setting. But now, if both CIBW_MANYLINUX_X86_64_IMAGE and CIBW_MANYLINUX_PYPY_X86_64_IMAGE are set, it's not clear what the new value of CIBW_MANYLINUX_X86_64_IMAGE should be?

Given that I'm not expecting a lot of users to actually set CIBW_MANYLINUX_PYPY_X86_64_IMAGE, would it be possible to just raise an error and break the config?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing remaining would thus be how to best order those options in docs (either group by CPython/PyPy or by arch ? The current layout is by age).

That's the other thing I noticed, yes. If we would keep them, I think maybe just lsting all PyPy things at the end in the same order as the default ones would be best, as we also e.g. do this for the Python versions.

Copy link
Member

@mayeut mayeut May 24, 2021

Choose a reason for hiding this comment

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

About deprecating CIBW_MANYLINUX_PYPY_X86_64_IMAGE: this is slightly more tricky. Before, when we did so, we basically added it to the new setting. But now, if both CIBW_MANYLINUX_X86_64_IMAGE and CIBW_MANYLINUX_PYPY_X86_64_IMAGE are set, it's not clear what the new value of CIBW_MANYLINUX_X86_64_IMAGE should be?

Given that I'm not expecting a lot of users to actually set CIBW_MANYLINUX_PYPY_X86_64_IMAGE, would it be possible to just raise an error and break the config?

You're right, I didn't fully thought this through.

One benefit of keeping a separate key for PyPy is that it allows the following config to be valid:

# Use manylinux1 for CPython because pip isn't always updated...
CIBW_MANYLINUX_X86_64_IMAGE: manylinux1
CIBW_MANYLINUX_I686_IMAGE: manylinux1
# Use the default for PyPy, do not skip it

Seeing this https://foss.heptapod.net/pypy/pypy/-/issues/3425 might also prove having a separate key for PyPy to be worth it.
While I do agree it should probably be dropped sooner rather than later, the "pip situation" does not agree with that...

@mayeut mayeut mentioned this pull request May 24, 2021
@henryiii
Copy link
Contributor

This is fine to merge, right? Then we could eventually consolidate the manylinux images, but for now, it's rather handy to have both. One idea could be to leave the pypy one blank, and then use the regular one if it's blank. (But than can be a separate discussion and PR, probably after #684 - it's also slightly non-backwards compatible, so a good idea to decide before 2.0)

@mayeut
Copy link
Member

mayeut commented May 25, 2021

Let's merge this as proposed by @henryiii, I will add an item to #657 for the remaining tasks that should be done before 2.0

@mayeut mayeut merged commit 58b2dc3 into pypa:master May 25, 2021
@mayeut mayeut mentioned this pull request May 25, 2021
12 tasks
mayeut added a commit that referenced this pull request May 26, 2021
…695)

* fix: `bin/update_pythons.py` to work properly with PyPy `win_amd64`
* refactor: use the same pattern for all python updates
* follow up to #671, for the record.
@YannickJadoul YannickJadoul deleted the pypy-7.3.5 branch May 26, 2021 13:23
@henryiii henryiii mentioned this pull request Jun 3, 2021
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

4 participants