-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
python wheels using rustc #5435
Conversation
@publicarray reading back on one of your comments #4971 (comment) : Any changes to the |
Yes, but it's also only build when it's on |
Yea I suppose the idea here was that you do build the docker container locally first and verify it before pushing the changes to master. However you can create a docker build in your fork if that is easier. yea it's not a good idea to do docker builds in PRs as we don't want to modify the published image until the changes land in master. |
Is the rust env getting sourced? This is how I do it in our docker file: |
So, I've been trying to get 32 bit arms builds working using docker buildx, and came across the same issue. There is a readdir bug in qemu for 32 bit arm that makes it's way all the way into the crossenv. This link is a docker build but I think the solution might be a similar problem in this cross toolchain. Also, rustup can't tell the architecture with an 32 bit arm architecture, so it can never find the correct compiler for arm6 and 7 and sends the objects to the wrong linker for arm 5/6 The fix for this is a mixture of the solution in the link here, and a patch to rustup I had done awhile back (I'll post that when I update the patch and test these fixes in https://github.com/miigotu/python-docker-images |
That's quite an interesting catch, although, the issue is also hapening on other archs and not only |
Yeah I have the same issue with 386/686. I think it is related. That's part of the patch for rustup is to get it to recognize the bitness as 32, it detects a 32 bit arch at 64 bit currently. I'll go quick and see if I can find my bug report from last year. |
Man this is super annoying, this works locally to build armv7, but failed in github actions: from this dockerfile: https://github.com/miigotu/python-docker-images/blob/main/python/python-cryptography.dockerfile
|
# Use distrib folder as cargo download cache | ||
CARGO_HOME_PATH=/spksrc/distrib/cargo | ||
ENV += CARGO_HOME=$(CARGO_HOME_PATH) | ||
ENV += PATH=:$(CARGO_HOME_PATH)/bin/:$(PATH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed but as the CARGO_HOME
is being set to /usr/local/
therefore /usr/local/bin
is automagically being in the default PATH
.
This was reworked and now sits under /opt
where cargo
& rustup
have independant directories. Theses values have been added in the default wide environment through spksrc.tc.mk
and the /opt/cargo/bin
to spksrc.common.mk
Same dockerfile works locally for all of them smh |
First thing to notice in here, the example you're refering to is using Anyhow, having a few pending patches on my end and trying to build using spksrc my "updated" docker environment where the host is My first thing to try is, is it even cross-compiling using rust? Lets take
It works! Now let's try that using
Still working! So the theory is that I have a working Now, following your Dockerfile example, along with using host default python3, still on
Boom! working! But hold-on, why is the wheel extension being
After investigating a little further, looks like everything in the wheel file is ?OK?:
Now let's try out the exact same thing, this time using our host native built python3.10:
Again, boom! Although it works with a few deprecation warnings:
Let's investigate further:
After unziping:
So clearly a mismatch here. But it did built sort of "succesfully" using a Now, challenge really is, building using a |
I'm not building these with spksrc at all, because it isn't necessary. All pyo3 wheels built on a system with libc (non-alpine) will run on the same architecture they were built for, regardless of the host. Buildx is a crossenv. The GitHub actions runs in an x86_64 host container always (ubuntu-latest is Ubuntu:jammy right now I believe). Buildx runs each build in a qemu cross ENV either determined by the --platform argument or possibly overridden on the FROM line of the dockerfile. Venv is a totally different thing, that just isolates the python environment from the system python environment, so there are no conflicts with the system python packages or contamination of the build/install of what you are building. That could be left out entirely from the dockerfile and it would work, it just isn't best practices. So essentially, if this build worked in GitHub actions, very easily we could build every cross wheel in GitHub actions directly and then pass them into the spksrc build and build spksrc without needing rust at all. It was just trying to prove that it isn't spksrc toolchain that is the problem, there is something wonky with GitHub actions that's making it not possible to build 32 bit builds (since I can build the same action locally but it fails to find rustc on actions). I'm going to break it back into cargo fetch on amd64 and then build on the --platform=$TARGETPLATFORM stage and see if that fixes it on GitHub actions. If I get this built, I will create releases containing all of the built wheels, and you can reference them with --find-links and always avoid compile errors (and reduce storage usage and build time) on spksrc. |
I'll add the build log later, it absolutely is cross compiling the rust and c extensions. |
Once cryptography 35.0.0+ is built, you could install poetry right to the crossenv and that would be great. It will also require tomli up until python 3.11, where tomllib has been added to python stdlib https://docs.python.org/3.11/library/tomllib.html I'm not sure of flit dependencies, but that is the other major tool used for managing dependencies with pyproject.toml. I think it needs either toml or toml-w |
I gave it a shot on both Docker and a native VM to see if this isn't something in the host detection or similar... Sadly same result. It's really easy to trigger with this PR, once the build is complete (e.g.
Then build:
Interestingly enough it gets a little farther using
A call using crossenv
|
It's just crazy that I can build them with buildx locally but not on GitHub, with the same sources, commands, environment, and everything. I'm going to try another method to compile right in the actions, rather than inside a dockerfile. I can set up python in the action, set up rust toolchain (with multiple targets), and build/compile with setuptools-rust. Now whether that gets around the bug in qemu (or GitHub, I'm not 100% sure the culprit yet), I don't know. I suspect it could be because the action runner itself is in a container, and then we run another container inside that. Which I expect is why when I run the buildx command on my host machine it works, but on GitHub it doesn't. There's really no other difference between how I run here and there. |
I imagine my next attempt to look something like this: actions/setup-python (matrix with python 3.7-3.11-dev) |
@th0ma7 seems someone has already done what I was thinking, check this out: https://github.com/PyO3/setuptools-rust/pull/185/files |
You can already do something close to this by using |
Cross-post from PyO3/setuptools-rust#294 This circles back to benfogle/crossenv#65, after removing |
Good news! I got it working PyO3/setuptools-rust#294 (comment) I have to think about the various code changes I need to do to have a working fix (or pending a new |
Awesome, I hope that same thing fixes a straight build in gh actions too. Maybe this fix will bring back prebuilt wheels for arm32 and ppc64le |
@hgy59 got a working PR to generate rust-based wheels. I still have a few details to polish but overall code is looking good. The real thing in here it the updated docker image. I propose pushing it into a different PR which in turn would allow me to fully test things in this one in particular (as I can't using github-action until it's merged to master, although I tested it manually). Changes to mention related to the Docker image:
I did a lot of build testing and overall things is looking good. Although I'm sure there will be things to adjust from migrating to Debian 11 resulting in a few bumps down the road that I'm confident we can address afterwards. Thoughts? |
@publicarray examining the build errors it does use the latest Docker image. There's just a few details pending that I'm now fixing within the |
Build passed! Great work! |
This piece of code is so unmaintained that it requires an old legacy version of cmake to run. Latest changes allow using Debian 10 "Buster" of cmake and thus makes it build again.
* cross-rust.mk: Fix regular build post PR #5435 * finalize rust fix - remove unused code Co-authored-by: hgy59 <[email protected]>
Description
python wheels using rustc - Using another approach from #5399
Extras:
python310
to version 3.10.7 (possibly 3.10.8? https://peps.python.org/pep-0619/)duplicity
to version 0.8.23mysql-connector-c
which now fails to buildduplicity
, shoot for version 1.0.1 (tested onx64-7.0
,armv7-7.0
,armv5-6.1
)TODO: none left
Fixes #
Checklist
all-supported
completed successfullyType of change