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

feat: add manylinux armv7l #2052

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add manylinux armv7l #2052

wants to merge 3 commits into from

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Oct 21, 2024

This adds manylinux armv7l using an experimental ubuntu based manylinux_2_31 image using deadsnakes python packages.
An official image based might become available at some point but not before aarch64 runner support on GHA.

The experimental image should be able to generate wheels compatible with all armv7l Raspberry Pi currently supported by piwheels.org.

While the gcc toolchain is not as recent as one can get on RHEL derivatives, debian/ubuntu is the only possible path forward for armv7l.

@joerick
Copy link
Contributor

joerick commented Oct 21, 2024

Cool! would it make sense to call the ubuntu-based image something different than manylinux_2_31 e.g. manylinux_ubuntu_2_31? I'm assuming the mainline manylinux images will still be on the RHEL lineage going forward. I wouldn't want people to think the most up-to-date manylinux image has switched back to ubuntu after the manylinux_2_24 back-and-forth.

@mayeut
Copy link
Member Author

mayeut commented Oct 21, 2024

I wouldn't want people to think the most up-to-date manylinux image has switched back to ubuntu

That's why the only platform that allows manylinux_2_31 is armv7l in this PR. This is mentioned in the docs also but maybe it should be explained a bit more in there ? mostly, that you need to use apt rather than yum/dnf.

The PoC repo builds all images as a multiplatform images but if the image gets integrated in the pypa/manylinux repo, only armv7l will be added to avoid as much as possible any confusion and relive the whole manylinux_2_24 episode
If/When it gets added, the README in pypa/manylinux will reference the various threads mentioning why we limit manylinux_2_31 to armv7l only. By that time, manylinux_2_34 will exist on other platforms so should limit the confusion w.r.t manylinux as a whole moving to ubuntu.

would it make sense to call the ubuntu-based image something different than manylinux_2_31 e.g. manylinux_ubuntu_2_31?

I won't rename the images themselves, at least before any move in pypa/manylinux and probably not at that time either.
Given the cibuildwheel summary only list the resolved image (rather than the shorthand), I don't think it's worth the change.

@henryiii
Copy link
Contributor

Agree, since it's only one arch, as long as it's communicated well, it should be fine.

@joerick
Copy link
Contributor

joerick commented Oct 22, 2024

Thanks for the reply @mayeut - entirely up to you. Good to hear that it'd only be available on armv7l if included in the mainline release set.

My feedback is just that because the manylinux naming scheme is based on the glibc version, the name implies that 'this is the manylinux image for glibc 2.31', which I don't think you're trying to communicate, I think we'd like it to say 'this is an unusual manylinux image for armv7l which is on glibc 2.31'. Adding the word 'ubuntu' somewhere might help people understand that. The other advantage would be alleviating the need to remember that specifically manylinux_2_31 is a unusual image compared to the others - that's something I'm bound to forget.

Having said all that, if you'd prefer to keep it as-is, that's okay with me. I don't think it'd be a big problem if manylinux_2_34 is out by that time, just an inconvenience.

@mayeut
Copy link
Member Author

mayeut commented Nov 2, 2024

Thanks for your feedback @joerick.

the manylinux naming scheme is based on the glibc version, the name implies that 'this is the manylinux image for glibc 2.31', which I don't think you're trying to communicate.

I don't quite understand that first part: from the manylinux repo readme, each description is roughly the same and I think it's conveying the right message:

manylinux_x_y, ..., Built wheels are also expected to be compatible with other distros using glibc x.y or later, including ...

If you do feel there are some clarification needed there, please do open a PR. I'm probably too close to the subject and thus lack perspective on how to enhance this documentation.

I think we'd like it to say 'this is an unusual manylinux image for armv7l which is on glibc 2.31'.
Adding the word 'ubuntu' somewhere might help people understand that. The other advantage would be alleviating the need to remember that specifically manylinux_2_31 is an unusual image compared to the others - that's something I'm bound to forget.

I don't really want to convey the "unusual" message here. It is (& most likely will stay) "usual" for armv7l.
I do understand your point though. I feel that communicating on the architecture armv7l is more future proof & easier to remember than an "obscure" numbering scheme. I also added a footnote in the README ("manylinux armv7l support is experimental. As there are no RHEL based image for this architecture, it's using an Ubuntu based image instead.").
Also, there are a number of projects than don't even require interacting with the package manager & where the difference doesn't matter as much (the version of the toolchain is another difference but, it can change overtime so it might not be a good idea to add this into the image name).
I have not yet added a proper README to the PoC repository that's building the image used here. I will do so & part of that will eventually land in pypa/manylinux once the image is added there.

Edit: added a README on https://github.com/mayeut/manylinux-ubuntu if this helps on the doc side.

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.

3 participants