Skip to content

cpython: have powerpc64le use "ppc64le" to follow PEP600#168083

Merged
FRidh merged 1 commit intomasterfrom
unknown repository
Jun 19, 2022
Merged

cpython: have powerpc64le use "ppc64le" to follow PEP600#168083
FRidh merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 10, 2022

Description of changes

The PEP600 standard gives Python's naming scheme for various architectures; it follows the convention which was in use by Fedora in 2014. According to PEP600, the architecture name for Power PC is ppc64le, not powerpc64le. This is also how python3 declares its "supported wheels" under Debian on PowerPC, as checked with pip debug --verbose

  $ pip debug --verbose | grep powerpc
  $ pip debug --verbose | grep ppc | head
  cp39-cp39-manylinux_2_31_ppc64le
  cp39-cp39-manylinux_2_30_ppc64le
  cp39-cp39-manylinux_2_29_ppc64le
  cp39-cp39-manylinux_2_28_ppc64le
  cp39-cp39-manylinux_2_27_ppc64le
  cp39-cp39-manylinux_2_26_ppc64le
  cp39-cp39-manylinux_2_25_ppc64le
  cp39-cp39-manylinux_2_24_ppc64le
  cp39-cp39-manylinux_2_23_ppc64le

Let's adjust the pythonHostPlatform expression in cpython/default.nix to pass the architecture using the naming scheme Python expects.

Verified on a Raptor Computing Systems Talos II.

Without this commit, PyQt5 fails to build, failing with "unsupported wheel". With this commit, it builds successfully. Currently building qutebrowser for additional verification.

CC: @r-burns @CrystalGamma

Things done

Built PyQt5 successfully with this. Currently building qutebrowser.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
    • powerpc64le-linux
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Apr 10, 2022
@ghost ghost marked this pull request as ready for review April 10, 2022 01:42
@ghost ghost requested a review from FRidh as a code owner April 10, 2022 01:42
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 10, 2022
@tpwrules
Copy link
Copy Markdown
Contributor

I'm still not confident that Python itself expects to be called ppc64 and that having ppc64 in the wheel name isn't just an unfortunate mistake which should be adapted for elsewhere. Can you double check with upstream for guidance?

Like I said in the previous issue, I think the right solution might be to just not build manylinux wheels by default.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 10, 2022

I'm still not confident that Python itself expects to be called ppc64 and that having ppc64 in the wheel name isn't just an unfortunate mistake which should be adapted for elsewhere.

That's why I checked the behavior on Debian, which is definitely the distribution with the best support for ppc64le. Debian puts ppc64le in the wheel name. See the pip debug --verbose lines in the first comment and the commit message -- those were executed using the plain-vanilla debian bullseye packages on a powerpc64le machine. If this is an unfortunate mistake, Debian has been making the same mistake too for quite a long time now.

Can you double check with upstream for guidance?

Frankly I wouldn't know who's in charge of that in order to ask, or how to ask. Do you know?

They'll probably look at me strangely and ask what this weird "nix" stuff is all about and tell me to just run ./configure without any options or environment variables, like everybody else does.

I'm not a python developer, I just really like qutebrowser.

Like I said in the previous issue, I think the right solution might be to just not build manylinux wheels by default.

Unfortunately PyQt5 has needed a manylinux wheel since February: #163589

@tpwrules
Copy link
Copy Markdown
Contributor

tpwrules commented Apr 10, 2022

So the problem that leads to that error building PyQt5 and stops you from using qutebrowser is that sip generates a wheel name for PyQt5 that pip does not believe is correct. The ultimate problem on PowerPC seems to be that in some situations the architecture is called ppc64le and on others it's called powerpc64le.

That's why I checked the behavior on Debian, which is definitely the distribution with the best support for ppc64le. Debian puts ppc64le in the wheel name.

What do NixOS and Debian (and Fedora, if you can test it easily) print for python3 -c 'import sysconfig; print(sysconfig.get_config_var("EXT_SUFFIX"), sysconfig.get_platform())'? That's what Python itself believes its architecture to be, not what pip labels the wheels it will accept. The issue is that those two things are different on NixOS.

They'll probably look at me strangely and ask what this weird "nix" stuff is all about and tell me to just run ./configure without any options or environment variables, like everybody else does.

I understand the sentiment, but I'm pretty sure this will give you a Python which calls the architecture powerpc64le (and will say such when running the command above). The manylinux bug report agrees that ppc64le is nonstandard and not intended by the Python developers.

It turns out further in that thread that CentOS/RHEL/Fedora patches their Python to call the architecture ppc64le for whatever reason, that this made it into the manylinux wheel names (which are based on the CentOS "ABI") by accident in PEP 600 and its predecessors, and that they regret this mistake and wish to change it back to powerpc64le in the future (at least that's how I interpret it). I don't think that change has happened yet, but if it's not too much headache it might be good to test that command I gave you on the Fedora versions mentioned in that comment.

That issue is also where I would post a comment to ask for upstream's opinion, and what the planned future names are for both the architecture as returned by sysconfig.get_platform() and as defined in the wheel name, and if the PEP will be updated accordingly. Your patch proposes to change the architecture Python reports itself as to ppc64le on NixOS, which disagrees with the Python developers' intent and I think is moving in the wrong direction, although I admit it will cause it to match the wheel name and fix the issue.

Unfortunately PyQt5 has needed a manylinux wheel since February: #163589

The reason this issue originally occurred on aarch64 is that sip was updated past 5.2.0, which started to build manylinux wheels by default, with an invalid name. However, sip has an option to turn this off. Manylinux wheels are pretty useless with NixOS because a) they refer to libraries by paths only present on a NixOS system, instead of generic paths that would be on many distros, and b) you can't actually do anything with them anyway, they only exist for a few seconds between the package build completing and the wheel being installed in the next phase.

Therefore, I propose that the ultimate solution is to patch sip to not build manylinux wheels by default (or perhaps to forbid building them entirely), and instead build a wheel for the specific system on which it will be installed like it did before, thus sidestepping this problem of the architecture name entirely. It's not clear to me if this works properly under cross-compilation, or if cross-compilation even works properly now (I suspect not).

I can look into creating this patch myself (and will be sure to test it on aarch64 if you create it), but it should be easy for you to check if simply patching self.manylinux to False here fixes the build. I didn't originally patch sip this way because a) the way it names manylinux wheels was clearly broken and b) I hadn't thought this far through the implications of using manylinux wheels on NixOS. But if they are causing us this much headache and won't be useful in the future, maybe it's good to just avoid them.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 10, 2022

The ultimate problem on PowerPC seems to be that in some situations the architecture is called ppc64le and on others it's called powerpc64le.

Yes, unfortunately that problem exists because so many software packages and linux distributions cook up their own naming scheme for architectures instead of simply using gcc/autoconf tuples. You'll see powerpc64le because that's the name used by gcc/autoconf tuples. gcc/binutils/autoconf are (still) the "first arrival" on any new architecture, so they have the most complete taxonomy by far. It's also a very very stable taxonomy, except for one unfortunate case (32-bit x86 where we have the i386/i486/i586/i686 mess).

Unfortunately Python does not follow the gcc/autoconf taxonomy.

What do NixOS and Debian (and Fedora, if you can test it easily) print for python3 -c 'import sysconfig; print(sysconfig.get_config_var("EXT_SUFFIX"), sysconfig.get_platform())'?

I don't use NixOS, but I can answer for nixpkgs instead.

sysconfig.get_platform()

My nixpkgs install just spits back pythonHostPlatform from cpython/default.nix. It will say back to me whatever I put there. Before this commit nixpkgs chose pythonHostPlatform to be powerpc64le, so it printed linux-powerpc64le. After this commit, which changes pythonHostPlatform to ppc64le, I get:

$ nix-shell -p python3 --run "python3 -c 'import sysconfig; print(sysconfig.get_platform())'"
linux-ppc64le

Debian (bullseye, python3_3.9.2-3)

$ /usr/bin/python3 -c 'import sysconfig; print(sysconfig.get_platform())'
linux-ppc64le

I understand the sentiment, but I'm pretty sure this will give you a Python which calls the architecture powerpc64le (and will say such when running the command above)

Nope.

EXT_SUFFIX

I'm not sure what the relevance of this is; EXT_SUFFIX uses the gcc/autoconf taxonomy, not the Python taxonomy.

EXT_SUFFIX is defined as .${SOABI}${SHLIB_SUFFIX}, and SOABI is defined to be autoconf's PLATFORM_TRIPLET (which is actually a 5-tuple, not a 3-tuple), which is the gnu autoconf tuple for the platform. That is, has always been, and always will be powerpc64le-linux-gnuabi.

Anyways:

$ nix-shell -p python3 --run "python3 -c 'import sysconfig; print(sysconfig.get_config_var(\"EXT_SUFFIX\"))'"
.cpython-39-powerpc64le-linux-gnu.so

Debian:

$ /usr/bin/python3 -c 'import sysconfig; print(sysconfig.get_config_var("EXT_SUFFIX"))'
.cpython-39-powerpc64le-linux-gnu.so

Again, a match.

It would be great if Python used autoconf tuples everywhere, but it clearly doesn't. This is definitely not the only situation where EXT_SUFFIX is different from sysconfig.get_platform(). EXT_SUFFIX gives you the gcc/autoconf tuple, while sysconfig.get_platform() uses Python's naming scheme.

Future Hypotheticals

I can look into creating this patch myself (and will be sure to test it on aarch64 if you create it), but it should be easy for you to check if simply patching self.manylinux to False here fixes the build.

I'm testing this now, but the build will need to run overnight. If this works, I'm perfectly happy with it being merged instead of my PR. However if it doesn't work, and this PR is the only known fix that gets PyQt (and everything downstream of it) building again, I don't think it's appropriate to blockade it and leave powerpc (only) broken while we wait some indeterminate amount of time for some better solution to materialize.

@tpwrules
Copy link
Copy Markdown
Contributor

tpwrules commented Apr 10, 2022

Thank you for taking the time to look into and explain this further. It looks like I misunderstood how sysconfig.get_platform() and EXT_SUFFIX relate. I thought that the former was simply a rearrangement of the latter.

I'm not sure what the relevance of this is; EXT_SUFFIX uses the gcc/autoconf taxonomy, not the Python taxonomy. [...] That is, has always been, and always will be powerpc64le-linux-gnuabi.

This is not correct, Fedora did in fact have a patch to change it to ppc64le-linux-gnu. This is also what pip allegedly uses (or at least used) to validate the wheel name versus the system architecture, so that's why pip rejected wheels with powerpc64 in them and this whole issue wasn't noticed initially. It appears though with Fedora 31 and onward, this patch has been removed and its Python taught to recognize both names.

Since Debian's Python gives sysconfig.get_platform() as linux-ppc64le, and I can't find evidence in their patches that they've patched it to be this way, then it appears Nixpkgs is currently wrong and your patch is a correct solution. I retract my previous comment that your patch is the wrong approach.

However, it would be nice if you could still wait to find out if my idea to disable manylinux builds works on your system. If they do, then we know we can safely do it if this issue pops up a third time somehow...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Logically I would expect this to be ppcle?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes indeed, how embarrassing. I've pushed the fix.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 10, 2022

I can look into creating this patch myself (and will be sure to test it on aarch64 if you create it), but it should be easy for you to check if simply patching self.manylinux to False here fixes the build.

I'm testing this now, but the build will need to run overnight. If this works, I'm perfectly happy with it being merged instead of my PR.

It looks like this worked! I've written it up as #168115; if you'd prefer that one instead it's all the same to me. #168115 is marked as draft right now because I need to do one last rebuild with it to make sure nothing broke; assuming it doesn't I'll un-draft it in the morning.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 10, 2022

This is not correct, Fedora did in fact have a patch to change it to ppc64le-linux-gnu.
...
It appears though with Fedora 31 and onward, this patch has been removed and its Python taught to recognize both names.

Yikes, changing things like this back and forth must have been unpleasant for Fedora users.

it appears Nixpkgs is currently wrong and your patch is a correct solution

I don't think there's really a right or wrong here. Like all other distributions nixpkgs gets to pick its "Python name" for each platform.

I was under the mistaken impression that something in sip or PyQt5 was hardwired to use ppc64le, and that we would either need to use that name in nixpkgs or else go digging around inside of sip or PyQt5 to undo the hardwiring.

However, it would be nice if you could still wait to find out if my idea to disable manylinux builds works on your system.

Thank you for pointing this out! It does work!

In this case it really doesn't matter to me which name -- ppc64le or powerpc64le nixpkgs declares to be the "Python name" of this platform as long as we can merge #168115 or something similar to it. I guess whatever hardwiring exists in the downstream packages only applies to the manylinux case, so if we just don't offer those wheels the downstream packages will go along with whatever naming scheme we want.

This is great!

Thanks for pointing this out, I never would've guessed it would work.

Assuming the build for #168115 completes without problems (it should) I'll un-draft that PR in the morning and mark this one as draft to indicate that your approach is preferred.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 10, 2022

Draftified in favor of @tpwrules suggestion implemented in #168115

@ghost ghost marked this pull request as draft April 10, 2022 11:15
@tpwrules
Copy link
Copy Markdown
Contributor

I really think this is the better fix now. It seems weird that the distro gets to pick what sysconfig.get_platform() reports. We already know because this issue exists that at least pip keys off it to validate wheel names, and theoretically there could be some other software that parses it to load ppc64-specific stuff. If this PR is merged, then both sysconfig.get_platform() and EXT_SUFFIX should match their values for all other distros that we know of on ppc64le.

I suppose now we can wait for the Nixpkgs Python maintainers to weigh in and see which approach they think is better. But I believe this fixes a genuine bug in Nixpkgs as opposed to simply working around the problem like #168115.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 11, 2022

I suppose now we can wait for the Nixpkgs Python maintainers to weigh in and see which approach they think is better. But I believe this fixes a genuine bug in Nixpkgs as opposed to simply working around the problem like #168115.

Okay, sounds good; unmarked as draft.

@ghost ghost marked this pull request as ready for review April 11, 2022 02:10
@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 11, 2022

Squashed to a single commit, no other changes

The PEP600 standard gives Python's naming scheme for various
architectures; it follows the convention which was in use by Fedora in
2014.  According to PEP600, the architecture name for Power PC is
`ppc64le`, not `powerpc64le`.  This is also how python3 declares its
"supported wheels" under Debian on PowerPC, as checked with `pip debug
--verbose`

  $ pip debug --verbose | grep powerpc
  $ pip debug --verbose | grep ppc | head
  cp39-cp39-manylinux_2_31_ppc64le
  cp39-cp39-manylinux_2_30_ppc64le
  cp39-cp39-manylinux_2_29_ppc64le
  cp39-cp39-manylinux_2_28_ppc64le
  cp39-cp39-manylinux_2_27_ppc64le
  cp39-cp39-manylinux_2_26_ppc64le
  cp39-cp39-manylinux_2_25_ppc64le
  cp39-cp39-manylinux_2_24_ppc64le
  cp39-cp39-manylinux_2_23_ppc64le

Let's adjust the `pythonHostPlatform` expression in
cpython/default.nix to pass the architecture using the naming scheme
Python expects.

Verified on a Raptor Computing Systems Talos II.  Without this commit,
PyQt5 fails to build, failing with "unsupported wheel".  With this
commit, it builds successfully.
Copy link
Copy Markdown
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

@jonringer @FRidh do you agree with this?

@Artturin Artturin added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 11, 2022
@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 12, 2022

Ping

@SuperSandro2000
Copy link
Copy Markdown
Member

@FRidh @jonringer any comment on this? Otherwise I would go ahead and merge it since it can't cause regressions for our main platforms.

@r-burns
Copy link
Copy Markdown
Contributor

r-burns commented Jun 16, 2022

Should we also change _PYTHON_SYSCONFIGDATA_NAME? It looks like that still contains "powerpc64le".

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 18, 2022

Should we also change _PYTHON_SYSCONFIGDATA_NAME? It looks like that still contains "powerpc64le".

Unfotunately Python uses its own taxonomy ("ppc64le") in some places and the autoconf/multiarch taxonomy ("powerpc64le") in other places. It looks like _PYTHON_SYSCONFIGDATA_NAME is one of those places where the autoconf/multiarch taxonomy is used.

Wheel names definitely use the python taxonomy, however.

I think this whole two-taxonomies situation is the reason why the build script for python has both _PYTHON_HOST_PLATFORM and _PYTHON_SYSCONFIGDATA_NAME.

@FRidh FRidh merged commit b21933f into NixOS:master Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants