Apply Pyodide-specific patches to our Emscripten toolchain installation#2800
Conversation
| try: | ||
| subprocess.run( | ||
| f"cat {shlex.quote(str(patches_dir))}/*.patch | patch -p1 --verbose", | ||
| check=True, | ||
| shell=True, | ||
| cwd=emscripten_root, | ||
| ) |
There was a problem hiding this comment.
We have some level of Windows support in pyodide-build, but not yet in cibuildwheel, so I stayed with the cat and patch commands here. We could think of a cross-platform solution later.
There was a problem hiding this comment.
We have some level of Windows support in pyodide-build
We never considered windows as a valid target so I think it is fine.
|
You may see it working here: https://github.com/pypa/cibuildwheel/actions/runs/23520265886/job/68462021117?pr=2800#step:7:665. Note that the Pyodide 0.28.0 xbuildenvs were the first to include the Emscripten patches, so this is not applicable to the Pyodide 0.27.7 xbuildenvs/builds. When we release Pyodide 0.30 in the next couple of months or so (or before that, or later), we should drop Pyodide 0.27 here and keep only Pyodide 0.29 and Pyodide 0.30. It still makes sense to keep Pyodide 0.27 for now, as it is beneficial for cibuildwheel to retain support for building wheels for two different Pyodide ABIs at a time. |
|
I'll take a look at the failing tests later today. I don't immediately see what part of my changes might be causing them, though! |
ryanking13
left a comment
There was a problem hiding this comment.
N.B: We also have an install-and-patch-emscripten command thingy somewhere IIRC... this change is not that because that would be a bigger change to incorporate and should be discussed first; it would move the Emscripten installation from cibuildwheel's control to pyodide-build and thus would make changes to that scheme difficult (say, if pyodide-build were to break at a particular version for whatever reason, it would then start impacting all Pyodide builds for downstream users of cibuildwheel).
I would like to go this way directly if possible. This PR already exposes pyodide-build internals (e.g. path to the emscripten patches) to cibuildwheel, which can silently be broken when we make changes in pyodide-build. I don't want to introduce more of that into cibuildwheel.
The pyodide-build >= 0.33 now installs emscripten and applies the patch in the background without calling any command explicitly. So I guess the thing we need to do is simply removing the emscripten installation part in cibuildwheel.
| try: | ||
| subprocess.run( | ||
| f"cat {shlex.quote(str(patches_dir))}/*.patch | patch -p1 --verbose", | ||
| check=True, | ||
| shell=True, | ||
| cwd=emscripten_root, | ||
| ) |
There was a problem hiding this comment.
We have some level of Windows support in pyodide-build
We never considered windows as a valid target so I think it is fine.
Agreed re. the coupling. If there's special magic needed for pyodide builds, let's keep as much of that as we can in pyodide-build, so it can be used without cibuildwheel.
|
The Emscripten version is defined by the Python version. pyodide-build will choose the right emscripten version (and Pyodide version) when the Python version is selected. |
Yes, correct
Okay, since both of you suggest that we should rely on pyodide-build's existing command-line machinery for the Emscripten installation, I'll update this PR to use it! We'll just have to ensure that pyodide-build doesn't start breaking :-) which it fortunately hasn't for a while now, so I'm all fine with it. |
Co-authored-by: Joe Rickerby <joerick@mac.com>
|
@joerick, assuming that https://github.com/pypa/cibuildwheel/releases/tag/v3.4.1 was the upcoming release for which this PR was held, and that it occurred seven minutes after you added the label, this should be unblocked now – please let me know if I misunderstand! Thanks! :) |
|
Yep you're right :) |
| emcc_path = ( | ||
| xbuildenv_cache_path / pyodide_version / "emsdk" / "upstream" / "emscripten" / "emcc" |
There was a problem hiding this comment.
Maybe we can make a config variable something like pyodide config get emscripten-path in the future so that this path is not hardcoded in cibuildwheel.
There was a problem hiding this comment.
Maybe we can make a config variable something like
pyodide config get emscripten-pathin the future so that this path is not hardcoded in cibuildwheel.
Agreed. We already have one that points to the xbuildenv path, so why not!
Edit: working on this rn
Yes, it does – |
|
I have opened pyodide/pyodide-build#321, and will make a release after merge. |
Thanks. Actually, you don't need to block this PR for pyodide-build change. My comment was a suggestion not a merge blocker. |
|
Oh, sounds good! Thanks for the reviews, @ryanking13 and @joerick! I'll put together a follow-up PR later :) |
Our Emscripten toolchain installation here has mostly remained vanilla-like, but pyodide-build and pyodide-recipes also apply some Pyodide-specific patches. This makes both mechanisms of building Pyodide wheels uniform by applying the patches here as well.
I have been wanting to get to this for a while now, having used a pesky combination of sparse-checking out the patches with
actions/checkoutand then applying them in thebefore-buildstage in GHA before. My motivation to do this today came from flintlib/python-flint#382, so I thought I'd just put it in here!N.B: We also have anI've gone ahead and implemented theinstall-and-patch-emscriptencommand thingy somewhere IIRC... this change is not that because that would be a bigger change to incorporate and should be discussed first; it would move the Emscripten installation from cibuildwheel's control to pyodide-build and thus would make changes to that scheme difficult (say, ifpyodide-buildwere to break at a particular version for whatever reason, it would then start impacting all Pyodide builds for downstream users ofcibuildwheel).pyodide xbuildenv install-emscriptencommand as that's what we ended up agreeing with at the end, in that it doesn't expose any pyodide-build internals to cibuildwheel.cc: @oscarbenjamin