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

Workaround to fix BPO-43112 #10678

Closed
1 task done
dvarrazzo opened this issue Nov 23, 2021 · 23 comments
Closed
1 task done

Workaround to fix BPO-43112 #10678

dvarrazzo opened this issue Nov 23, 2021 · 23 comments
Labels
type: feature request Request for a new feature

Comments

@dvarrazzo
Copy link

dvarrazzo commented Nov 23, 2021

What's the problem this feature will solve?

In this moment, because of BPO-43112, we are distributing broken musllinux_1_1 wheel packages for Python 3.9 and 3.10, which work on Docker images such as python:3.9-alpine but will not work on the alpine using the system Python.

When BPO-43112 will be solved, there will be a situation of incompatibility, resulting in newly built packages not working on the python-alpine image, for some time, and not working for current images.

Describe the solution you'd like

Pip is in the right place to save the day 🙂 because it could easily rename the broken file on install, if it detects the right conditions (tag musllinux_1_1, .so file suffix mismatching the sysconfig.get_config_vars("SOABI")) it might simply rename the wrongly named library. It has the file system permission to do so and it would work both to make broken wheels work on new images and fixed wheels to work on old images.

Alternative Solutions

A workaround bridge solution of distributing a compatibility symlink (see psycopg/psycopg#161) is hampered by the fact that symlinks in zip files are not handled by the zipfile module (https://bugs.python.org/issue18595, https://bugs.python.org/issue27318).

Additional context

Code of Conduct

@henryiii
Copy link
Contributor

I think pip should make a symlink so both names work, otherwise this could become dependent on the Python patch version reading the installed copy. This should only be needed for Python < 3.11, as 3.11 should handle this correctly, I would expect.

As mentioned, ideally audithwheel should do this, but it can't make symlinks in wheels since CPython never fixed symlinks in zip files.

@pfmoore
Copy link
Member

pfmoore commented Nov 23, 2021

I don't quite understand the issue here. PEP 656 defines the tags for MUSL wheels. So when you say "we are distributing broken musllinux_1_1 wheel packages", why is the remedy not simply "don't do that, then"? I'm not being awkward here, I'm completely OK with fixing this issue, but the messages I've seen have been a bit confused, and I don't understand why this is suddenly an issue.

The inability to have symlinks in wheels is far from a new issue, and the patch referenced in BPO-43112 (python/cpython#24502) was submitted back in February, well before the musllinux PEP was approved - so why was the PEP submitted for approval if the PR hadn't been accepted? What was the expectation for how people would build musllinux wheels given that bpo-43112 was a known issue at the time (in particular, @tiran linked it in the PEP 656 thread here).

As I say, I'm fine with getting a fix for this, but the contract behind musllinux (and the perennial manylinux standard that it's based on) is that wheel producers are responsible for ensuring the wheels are compatible with all environments implied by the tag. How did that break down in this case?

@uranusjr
Copy link
Member

uranusjr commented Nov 23, 2021

I believe the problem is that the “non-broken” wheels don’t work on any of the currently released Python versions, so this request is based on an implicit engineering decision to make new wheels from this point on to work on those existing Python versions.

The wheel tag is also not technically related to the SOABI issue, because a platform-tagged wheel does not necessarily contain binary (it’s the other way around), and wheels can be built and deployed, it’s only Python is unable to load some code installed from it.

I’m wondering though, is it really the best idea to treat this as a bugfix? I also commented the same thought on bpo-43112; the current “wrong” -gnu suffix “works”, it’s just not disinguishing the libc implementations. I am missing information on how the current scheme is not working. Why must this behavioural change be backported?

@henryiii
Copy link
Contributor

henryiii commented Nov 23, 2021

Quick visual summary: Python on musllinux_1_1, Alpine 3.12, and Alpine 3.13 both produces and expects the following file and internal .so (zip structure hidden for clarity):

psycopg_binary-3.0.4-cp310-cp310-musllinux_1_1_x86_64.whl
 - _psycopg.cpython-310-x86_64-linux-gnu.so

Hover, Alpine 3.141 produces2 and expects only this:

psycopg_binary-3.0.4-cp310-cp310-musllinux_1_1_x86_64.whl
 - _psycopg.cpython-310-x86_64-linux-musl.so

For CPython 3.11, this is easy, just produce and read the -musl suffix. For older versions, or for abi3 extensions, or for wheels like cmake that don't have Python specific components that need to work on all Python versions, this suddenly becomes much more complex. A wheel built with -musl.so inside will not work anywhere but Alpine 3.14 (maybe some patch release version too? Or maybe 3.14.0?). A wheel build with -gnu.so is the reverse, it only works on Alpine 3.14.

Footnotes

  1. Is this a patch they have added? It seems this might not be fixed in CPython yet? Wasn't following that.

  2. technically produces musllinux_1_2, since 3.14 has musl 1.2. But this can be ignored, it has no bearing on the expectation.

@dvarrazzo
Copy link
Author

@pfmoore The issue comes from Python having been broken for some time in ways that are not captured by the MUSL tag (and not adequately fixed by a new tag). Python distributed in the python:3.9-alpine docker image (and 10) is buggy (affected by BPO-43112):

$ docker run --rm python:3.9-alpine python -c 'import sysconfig; print(sysconfig.get_config_vars("SOABI"))'
['cpython-39-x86_64-linux-gnu']  <<< wrong

on this docker image gets built the manylinux, which creates libraries with -gnu in their name, which work on python:alpine, but:

  • they don't work on alpine images, because their python goes:
$ docker run -ti --rm alpine
/ # apk update && apk add python3
/ # python3 -c 'import sysconfig; print(sysconfig.get_config_vars("SOABI"))'
['cpython-39-x86_64-linux-musl']
  • they will not work on python:alpine either once the bug is fixed.
  • after the bug is fixed, packages built on manylinux based on the fixed Python image will not work on currently released Python images.

The options to provide wheel packages that work for the whole Python 3.9 and 3.10 lifespan is either to build packages twice as big (and they get chubby pretty quickly already, see pydantic/pydantic#3439) or that the bug is addressed at install time, and pip happens to be in position to do so.

@uranusjr
Copy link
Member

uranusjr commented Nov 23, 2021

Python distributed in the python:3.9-alpine docker image (and 10) is buggy

This definition of “buggy” assumes that -gnu is wrong. But this is only a string the interpreter uses internally to find modules; if it’s any other value, say -linux, it suddenly looks much less wrong, does it not?

they don't work on alpine images, because their python goes

This can arguably be categorises as an Alpine bug instead, because they distribute Python built in a way that can’t load modules built from the official Python distribution. Using the same hypothetical -linux suffix, if a hypothetical Linux distribution builds Python to use -windows, would its inability to load modules even be remotely considered as a CPython bug? I doubt it.

@tiran
Copy link
Contributor

tiran commented Nov 23, 2021

The issue comes from Python having been broken for some time in ways that are not captured by the MUSL tag (and not adequately fixed by a new tag). Python distributed in the python:3.9-alpine docker image (and 10) is buggy (affected by BPO-43112)

There is some confusing around official Python container images on Dockerhub. The official Docker images on Dockerhub are not provided by the Python core team, Python release managers or PSF. They are created by the Docker community. We have no control what goes into the containers and how they are build.

@henryiii
Copy link
Contributor

$ docker run --rm -it alpine:3.14
# apk add python3
...
# python3 -c 'import sysconfig; print(sysconfig.get_config_vars("SOABI"))'
['cpython-39-x86_64-linux-musl']

Is it currently supported to add multiple extensions in a single installed folder? I think that was the idea behind the tagging in the first place; you could have a shared directory accessible by multiple OSs; in this case, if it had worked, this could have been usable by both musl and gnu linux. Not sure how important this is (you can't mix manylinux versions, etc).

If not, then calling this "-gnu" isn't necessarily a bug, and Alpine's patching is at fault.

official Python container images on Dockerhub

These are not doing any special patching on CPython, only Alpine is, from what I understand. Ether -gnu is wrong, and therefore CPython is buggy, or it is correct, and Alpine is buggy.

@henryiii
Copy link
Contributor

@dvarrazzo
Copy link
Author

My definition of who is buggy and who is right came from reading the BPO issue, e.g. https://bugs.python.org/issue43112#msg386710

As far as I'm concerned I can let Python and Alpine people decide who is right and who is wrong.

The status quo is that, under the manylinux_1_1 tag, different incompatible images can be produced, and the risk is that, with a change of the build tools (manylinux issue), released packages will suddenly stop working (final packager issue).

As far as I am concerned, I can see that the -gnu suffix is more used than the -musl one (being python:alpine a popular base), so I would be totally fine if Alpine decided to go the -gnu way. However, as a packages creator I need:

  • to be reassured that extensions will always be -gnu on 3.9 and 3.10 aka "43112 is not a bug, it's a feature", or
  • the install tool is able to take care of the changes that may happen along the Python 3.9.x lifetime.

Without either of these reassurances, I cannot conscientiously release psycopg musllinux binary packages, and will stop doing so from the next bugfix release, in order to safeguard my users.

@henryiii
Copy link
Contributor

henryiii commented Nov 23, 2021

I'd also add that I maintain cmake and ninja, and am aware of others, like clang-format, that do not have a Python specific portion of the tag, and so will need to be able to load on CPython 3.11 and 3.10/3.9. Also ABI3 wheels need to be loadable from both. So changing this based on Python version might not skip over all issues.

Edit: Actually, since they don't make extensions, but load via general shared objects, this is invalid. Only ABI3 would be affected, sorry! (And not sure I've seen much of that in the wild)

@uranusjr
Copy link
Member

I want to also comment on why I seem to oppose a seemingly simple fix to the situation. Wheel is an open standard, and pip is not the only wheel installer. So to properly apply the solution proposed in the top post, we need to add this special clause to the wheel standard and notify all relevant parties. This is much work with longer lasting effect than just a pip change.

@pfmoore
Copy link
Member

pfmoore commented Nov 23, 2021

OK, if I'm following this, two different Alpine docker images have Python configured differently? And one, the one that isn't supported by the core Python devs, is applying a behaviour-changing patch that has been submitted to core Python but hasn't been applied there yet.

... and all of this is purely about what string is used to identify a .so file, there's no actual binary compatibility here, just an inability to agree on a common standard?

So essentially, the principle in PEP 656, that "the tag promises the wheel works on any mainstream Linux distribution that uses musl version ${MUSLMAJOR}.${MUSLMINOR}" is being violated, and the reason is that the MUSL community are distributing Python builds that make it impossible to satisfy that promise (if we consider "the official python docker image" and "the official alpine docker image" as being two "mainstream Linux distributions that use musl") 🙁

I'm getting less and less comfortable with pip patching over this mess. It's a social and communication issue, that was raised at the time of the PEP 656 discussion, and I flagged in my acceptance of the PEP that making the "play nice with others" approach work would be a key to the success of the PEP.

The status quo is that, under the manylinux_1_1 tag, different incompatible images can be produced

Yes, it looks like that. And that's (IMO) an issue for the musl community to solve. Frankly, it was pretty much the basis for approving the musllinux PEP, that the community would work together to ensure things worked.

Without either of these reassurances, I cannot conscientiously release psycopg musllinux binary packages, and will stop doing so from the next bugfix release, in order to safeguard my users.

I think that's a reasonable position to take.

@dvarrazzo
Copy link
Author

@pfmoore

I'm getting less and less comfortable with pip patching over this mess.

Honestly, me too, in the sense that, if Alpine people are the ones to dictate that they want a -musl at home, it is their game to do so. However, if they just backported -musl from a yet-to-be-accepted 43112 solution, a return to an uniform practice is something that might happen.

If Python decides that -gnu is there to stay for the 3.9 and 3.10 lifespans I'm happy to keep providing compatible wheels. If Alpine provides something not compatible in their packages it means that they will have to maintain similarly mismatched psycopg, as well as every other binary package 🤷

@uranusjr
Copy link
Member

One minor correction: neither of the two Alpine images are supported by CPython core devs, but different third-party vendors (the Alpine and Docker community, respectively).

@henryiii
Copy link
Contributor

How hard would it be to patch CPython to accept both extensions? I'm not thinking of an upstream python/cpython patch, but just a damage-reduction patch for Alpine 3.14 (and 3.15, which shipped this morning before they noticed this) to be applied to their already-patched Python 3.9 on those platforms. It would just add cpython-39-x86_64-linux-gnu or cpython-39-x86_64-linux-musl to the list of extensions it recognizes regardless of the SOABI. The packages already built for Alpine 3.14 (and now 3.15) have -musl, and official sources have -gnu. Could that be done reasonably?

@dvarrazzo
Copy link
Author

I had in mind to contact the Alpine maintainers to hear from them too: @henryiii has already done it. Thank you for that 🙂

https://gitlab.alpinelinux.org/alpine/aports/-/issues/13227

@henryiii
Copy link
Contributor

henryiii commented Nov 24, 2021

We had a call and have a potential path forward. Quick summary:

  • ALPINE: Add a patch on top of the current patch to make CPython look for -gnu on top of -musl for Alpine 3.15 and 3.14. Reverting the patch would break every Alpine wheel previously locally compiled (like NumPy) and would require rebuilding all shipped packages that depend on Python.
  • ALPINE: Revert the patch for CPython 3.10 in Alpine 3.16, due mid next year.
  • CPYTHON: Take the existing patch (bpo-43112: detect musl as a separate SOABI python/cpython#24502) targeting upstream CPython 3.11 and change search to include abi3-gnu on musl after looking for abi3-musl. The ability to install both binaries into a single folder would be a new "feature" of CPython 3.11.
  • AUDITWHEEL: Optionally this could be checked and normalized by auditwheel (like changing -musl to -gnu on 3.9) if desired. ABI3 wheels targeting <3.11 could be normalized to -gnu.

If we'd notified them a day earlier 3.15 could likely have been delayed and fixed.

How does that sound?

@uranusjr
Copy link
Member

To be clear, the first two bullets are for Alpine, the third for CPython, and the last for packaging folks, right? Not that every party needs to be be involved in every step.

@pfmoore
Copy link
Member

pfmoore commented Nov 24, 2021

More to the point, none of this needs any change to pip, is that right?

@henryiii
Copy link
Contributor

Yes to both. No change to pip required.

@pradyunsg
Copy link
Member

pradyunsg commented Nov 25, 2021

Thanks @dvarrazzo and @henryiii for driving this to a clear path for resolution!

No change to pip required.

So... I know what to do with this issue then! 😛

@pradyunsg pradyunsg removed the S: needs triage Issues/PRs that need to be triaged label Nov 25, 2021
@ncopa
Copy link

ncopa commented Nov 25, 2021

We had a call and have a potential path forward. Quick summary:

  • ALPINE: Add a patch on top of the current patch to make CPython look for -gnu on top of -musl for Alpine 3.15 and 3.14. Reverting the patch would break every Alpine wheel previously locally compiled (like NumPy) and would require rebuilding all shipped packages that depend on Python.

FYI. This is done in Alpine now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature request Request for a new feature
Projects
None yet
Development

No branches or pull requests

7 participants