Skip to content

Add ppc64le support #165

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

Closed
wants to merge 7 commits into from
Closed

Conversation

clnperez
Copy link
Contributor

@clnperez clnperez commented Mar 7, 2023

Adding support for ppc64le. As with s390x support (added 7a0c31d), this only needed additions for the build triples and a new build target defined. I'd like to get this added to use with the envoy builds. (See envoyproxy/envoy#25090).

Signed-off-by: Christy Norman <[email protected]>
@indygreg
Copy link
Collaborator

indygreg commented Mar 8, 2023 via email

@ninehills
Copy link

ninehills commented Mar 8, 2023

I am also working on the porting of the envoy to the ppc64le architecture. Next, I will test this PR.~

I find some bug in this pr, and fix it in:

clnperez#1

Then trigger the github action:

https://github.com/ninehills/python-build-standalone/actions/runs/4361856285

@ninehills
Copy link

Local build successfully. @clnperez you can merge my pr.

@clnperez
Copy link
Contributor Author

clnperez commented Mar 8, 2023

thanks @ninehills!

@clnperez
Copy link
Contributor Author

clnperez commented Mar 8, 2023

@indygreg happy to hear that! i merged @ninehills's changes into this PR. Would you rather me squash and re-push? Or will you just do a squash & merge?

@ninehills
Copy link

I found that some of the validation and publishing logic still needs to be modified. I will continue testing.

@ninehills
Copy link

github workflow run successfully: https://github.com/ninehills/python-build-standalone/actions/runs/4371696198

pr already pushed.

@clnperez clnperez changed the title WIP: Add ppc64le support Add ppc64le support Mar 10, 2023
@clnperez
Copy link
Contributor Author

removing from WIP status, as @ninehills has tested and the workflow ran successfully on my fork. PTAL @indygreg and lmk how you want me to handle the multiple commits. thanks!

@clnperez
Copy link
Contributor Author

@indygreg can you take a look at this one?

Copy link
Collaborator

@indygreg indygreg left a comment

Choose a reason for hiding this comment

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

This PR looks great!

I think the only needed change is the platform name is being advertised as linux-powerpc64le. But I think it should be linux-ppc64le. (I'm not 100% sure about this.)

CPython's configure picks up the platform name from the detected target triple. From build logs we see: checking host system type... powerpc64le-unknown-linux-gnu. So it goes with powerpc64le.

In order to change / fix this, we'll need to patch configure.ac to set an appropriate _PYTHON_HOST_PLATFORM value when the target triple is powerpc64le-unknown-linux-*. The existing patch-apple-cross.patch does this. I'd write a similar patch or rename/extend this existing patch to cover ppc64le. Make sure PYTHON_ARCH in build-cpython.sh matches the new platform name.

If you'd like some help with this, just let me know and I can do it. I'm also open to contrasting opinions that the platform name of powerpc64le is actually correct.

This change looks great otherwise! Thanks for your hard work.

@@ -677,6 +677,9 @@ mipsel-unknown-linux-gnu)
mips64el-unknown-linux-gnuabi64)
PYTHON_ARCH="mips64el-linux-gnuabi64"
;;
ppc64le-unknown-linux-gnu)
PYTHON_ARCH="powerpc64le-linux-gnu"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it should be as well, and i see NixOS/nixpkgs#168083 referencing PEP600 and coming to the same conclusion (setting pythonHostPlatform and therefore _PYTHON_HOST_PLATFORM to ppc64le : https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/python/cpython/default.nix#L209) . But... the cpython configure.ac that @ninehills linked does have powerpc64le- instead

I see that you're using PYTHON_ARCH to create a symlink. If we change this will this impact that at all or be fine? I was trying to poke around and see where things land as far as directory structure on my dev VM running ppc64le -- but it's currently unavailable so I just have to bother you with questions.

Choose a reason for hiding this comment

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

I changed PYTHON_ARCH from powerpc64le-linux-gnu to ppc64le-linux-gnu, then built failed:

Github action: powerpc64le-linux-gnu

It seems that CPython has hard-coded this value. Unless we patch the CPython source code, it's best to keep the value of PYTHON_ARCH as powerpc64le-linux-gnu.

build error message:

Traceback (most recent call last):
  File "/home/runner/work/python-build-standalone/python-build-standalone/cpython-unix/build.py", line 1088, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/runner/work/python-build-standalone/python-build-standalone/cpython-unix/build.py", line 1071, in main
    build_cpython(
  File "/home/runner/work/python-build-standalone/python-build-standalone/cpython-unix/build.py", line 803, in build_cpython
    fh.write(build_env.get_output_archive("python"))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/python-build-standalone/python-build-standalone/pythonbuild/buildenv.py", line 116, in get_output_archive
    data = normalize_tar_archive(data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/python-build-standalone/python-build-standalone/pythonbuild/utils.py", line 338, in normalize_tar_archive
    filedata = tf.extractfile(ti)
               ^^^^^^^^^^^^^^^^^^
File "/ opt /hostedtoolcache/Python/3.11.2/x64/lib/python3.11/tarfile.py", line 2143, in extractfile
    return self.extractfile(self._find_link_target(tarinfo))
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/ opt /hostedtoolcache/Python/3.11.2/x64/lib/python3.11/tarfile.py", line 2452, in _find_link_target
    raise KeyError("linkname %r not found" % linkname)
KeyError: "linkname 'python/install/lib/python3.9/config-3.9d-ppc64le-linux-gnu/libpython3.9d.a' not found"
make: *** [Makefile:278: /home/runner/work/python-build-standalone/python-build-standalone/build/cpython-3.9.16-ppc64le-unknown-linux-gnu-debug.tar] Error 1
Error: Process completed with exit code 2.

@ninehills
Copy link

ninehills commented Mar 20, 2023

It seems that CPython can only accept powerpc64le-linux-gnu during compilation (possibly referencing the architecture naming of gcc), but the standard architecture name we built should be ppc64le according to the standard.

https://github.com/python/cpython/blob/5e6661bce968173fa45b74fa2111098645ff609c/configure.ac#L1011

Other discussions:
pypa/manylinux#687

@clnperez
Copy link
Contributor Author

@ninehills how did you package & test this with envoy when you built it? i can cross-compile just fine, but am having no luck figuring out what was built and how it is turned into a consumable release archive.

@ninehills
Copy link

@ninehills how did you package & test this with envoy when you built it? i can cross-compile just fine, but am having no luck figuring out what was built and how it is turned into a consumable release archive.

https://github.com/ninehills/python-build-standalone/actions/runs/4524288535

Now build is success.

@clnperez
Copy link
Contributor Author

clnperez commented Mar 29, 2023

@indygreg can you weigh in?

i can see from the github workflow that the archives are built -- but when I run just the power cross-build (./build-linux.py --target-triple ppc64le-unknown-linux-gnu) -- i don't get one. so i tried ./build-linux.py --target-triple ppc64le-unknown-linux-gnu --python cpython-3.11 --optimizations noopt and then I get:
compressing Python archive to /home/christy/go/src/github.com/indygreg/python-build-standalone/dist/cpython-3.11.1-ppc64le-unknown-linux-gnu-noopt-20230309T0934.tar.zst

It doesn't seem that the type of build via build-flags would be the deciding factor on whether or not something shows up in /dist.

@indygreg
Copy link
Collaborator

indygreg commented Apr 8, 2023

It seems like ppc64le is what's in PEP 599 / 600 and was the historically accurate target name. But powerpc64le seems to be what people want to transition to. And CPython's configure.ac appears to insist on powerpc64le.

So I think it makes sense to keep go with powerpc64le since it will be future compatible. But I wouldn't be surprised if we run into issues installing binary wheels when using this platform name since ppc64le was historically used. 🤷‍♂️

@clnperez
Copy link
Contributor Author

thanks @indygreg! it is a messy space for sure 🤞🏻

@clnperez
Copy link
Contributor Author

oh -- do i need to squash these commits? anything else to merge?

@indygreg
Copy link
Collaborator

I have this cherry picked into my local repo. Will push this to main once CI passes.

Thanks for your hard work and patience!

@indygreg indygreg closed this in e3ae98b Apr 24, 2023
@clnperez
Copy link
Contributor Author

Thanks @indygreg. Does this project have a release schedule we should wait for, or can we just request a new one be made so we can add this to the envoy builds?

@indygreg
Copy link
Collaborator

There's no formal release schedule. I try to cut releases after new Python versions, important security vulns, or other major changes. We're several weeks overdue for a release. Please create a new issue to track the next release. Hopefully I'll find time in the next 1-2 weeks.

@clnperez
Copy link
Contributor Author

adding to rules_python via bazel-contrib/rules_python#1234. thanks for the release @indygreg 🎉

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