-
Notifications
You must be signed in to change notification settings - Fork 239
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
Allow the use of arbitrary Pyodide versions #2002
base: main
Are you sure you want to change the base?
Allow the use of arbitrary Pyodide versions #2002
Conversation
The Windows test failures are unrelated. I'll try to fix them later in the day, but happy to step back if someone else does it before me, or wishes to. |
a4457a3
to
9a78845
Compare
The failing CircleCI-Linux-Python312 test should be unrelated. |
cibuildwheel/pyodide.py
Outdated
# Fetch just the "stable" versions | ||
compatible_xbuildenvs_filtered = [ | ||
version for version in compatible_xbuildenvs if not any(_ in version for _ in "abc") | ||
] | ||
# TODO: possibly remove that? Since this won't allow testing the unstable/dev versions |
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.
I'm poised to remove this check, since this would allow downstream users to test versions such as Pyodide 0.27.0a2 and see if things are working. Filtering the versions with this check restricts us within the same minor release at times (say, 0.26.2 or 0.26.3, etc.) and the Emscripten version bump/ABI break in Pyodide is never achieved until a new minor release, say, 0.27 or 0.28 (or later) is released. Testing with new Pyodide patch releases thus does not bring much merit.
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.
e551021
removes this check, feedback is welcome.
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.
Generally looks good to me. I'd like to see:
- pyodide_version override handled like all the other options
- an override for pyodide_build_version
cibuildwheel/pyodide.py
Outdated
f" {compatible_versions}. Please use the 'pyodide xbuildenv search' command to" | ||
f" find the compatible versions for {pyodide_build_version}" |
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.
Didn't you just tell them what the compatible versions are? So they probably don't need to actually do this.
Also, maybe we want a newline after are:
.
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.
We have a writeup about the compatible versions in the docs, but the instance where we tell them about this at runtime is here in case users don't read the docs is here – or did you mean something else? I might have misunderstood.
cibw_pyodide_version = os.environ.get( | ||
"CIBW_PYODIDE_VERSION", python_configuration.pyodide_version | ||
) |
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.
The other options can be passed either as a cli flag e.g., --pyodide-version
or as the corresponding environment variable in this case CIBW_PYODIDE_VERSION
. They can also come from the cibuildwheel section in pyproject.toml
. We should handle this in the same way as all the other options. Then we can just use python_configuration.pyodide_version
and it should already have the right value.
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.
Most options are not available as flags (and I wouldn't add this one), only stuff you often need on the command line, like --platform
. Just env var and config.
@@ -219,7 +290,10 @@ def build(options: Options, tmp_path: Path) -> None: | |||
|
|||
log.build_start(config.identifier) | |||
|
|||
identifier_tmp_dir = tmp_path / config.identifier | |||
# Include both the identifier and the Pyodide version in the temp directory name | |||
cibw_pyodide_version = os.environ.get("CIBW_PYODIDE_VERSION", config.pyodide_version) |
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.
Again we should get the correct value into config.pyodide_version
to begin with.
docs/options.md
Outdated
@@ -255,7 +255,7 @@ Default: `auto` | |||
|
|||
- For `linux`, you need [Docker or Podman](#container-engine) running, on Linux, macOS, or Windows. | |||
- For `macos` and `windows`, you need to be running on the respective system, with a working compiler toolchain installed - Xcode Command Line tools for macOS, and MSVC for Windows. | |||
- For `pyodide` you need to be on an x86-64 linux runner and `python3.12` must be available in `PATH`. | |||
- For `pyodide` `python3.12` must be available in `PATH` and you need to be on one of the following runners: x86-64 Linux, arm64 Linux. Intel and Silicon macOS hosts may work, too. See [the section on Pyodide](setup.md#pyodide-(WebAssembly)-builds-(experimental)) for more information. |
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.
Why don't we keep telling people to use x86-64 linux runners? Is there a good reason to use arm64 linux? We don't test it ourselves, are we sure that it works? "Mac builds may succeed but there are known bugs".
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.
We could do that, though since Emscripten has arm64 binaries now, I've started using them in Docker containers and on macOS and they seem to work as intended. Maybe it would be worth adding a pipeline to test Linux arm64 for Pyodide here as well, should I try that? I'll replace the macOS sentence with your suggestion.
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.
Added your phrasing here: 057e542. I'd like to see if there is interest in adding Linux-arm64 and macOS-arm64 Pyodide builds, so I've kept this conversation as "unresolved" for now :)
It is also possible to target a specific Pyodide version by setting the `CIBW_PYODIDE_VERSION` environment variable to the desired version. Users are responsible for setting an appropriate Pyodide version according to the `pyodide-build` version. A list is available in Pyodide's [cross-build environments metadata file](https://github.com/pyodide/pyodide/blob/main/pyodide-cross-build-environments.json) or can be | ||
referenced using the `pyodide xbuildenv search` command. | ||
|
||
This option is **not available** in the `pyproject.toml` config. |
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.
Is there a good reason that it isn't available in the pyproject.toml config?
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.
It seems like it should be.
Co-Authored-By: Hood Chatham <[email protected]>
Co-Authored-By: Hood Chatham <[email protected]>
I'll check 1) in a bit, but for 2), we can do this with the current structure and use a newer |
Also, I think I should add a test workflow (or another job through a matrix) that builds Pyodide wheels with the new |
@@ -890,6 +890,11 @@ | |||
}, | |||
"test-requires": { | |||
"$ref": "#/properties/test-requires" | |||
}, | |||
"pyodide-version": { |
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.
This file gets generated, I don't see the bin/generate_schema.py
being updated?
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.
🤔 Just tried nox -s generate_schema
and the changes went away. I'll check how to update the schema properly.
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.
Doesn't cibuildwheel/options.py also need updating? I don't see a .get("pyodide-versions")
being read there.
Wild idea (probably), but what about allowing the version to be part of the platform? So |
RE: an override for pyodide_build_version Can't this be done like overriding any other constrained package? You could set the versions to |
I would be fine with implementing this, but in principle (and to be slightly pedantic),
Yes, I had the same suggestion in #2002 (comment), but TIL that |
If Unless someone really likes the idea, let's keep going with pyodide-version. My only thought is that it's a general config setting only applicable to a specific platform. Though we do have a few of those (the manylinux settings, for example). |
pyodide-build
has been unvendored
Ah, you raise a good point! If we have an option CIBW_PYODIDE_VERSION and the user customises it, well we still have constraints in So, yes, the other approach would be to read the contents of CIBW_DEPENDENCY_VERSIONS to get the pyodide-build version. We already do something similar here, it's fairly easy using The other thing that would help here is a way to specify CIBW_DEPENDENCY_VERSIONS inline, without the extra file. Something like: CIBW_DEPENDENCY_VERSIONS_PYODIDE: "requirements: pyodide-build==0.29.1" (I don't love the |
Description
This PR updates the Pyodide build procedure (see #1456) that is enabled with
CIBW_PLATFORM: "pyodide"
(or with the--platform pyodide
CLI equivalent) post the changes in pyodide/pyodide#4882, wherepyodide/pyodide-build
was unvendored from the main Pyodide repository to accommodate faster updates and fixes.This means that the Pyodide version and
pyodide-build
are not going to be in sync going forward, and that the Pyodide xbuildenv to install must be inferred by the versions available to install bypyodide-build
through a recently addedpyodide xbuildenv search
command, which prints out this table:Tap to expand table
Alternatively, one may use
pyodide xbuildenv search --all
to return both compatible and non-compatible versions. This would, however, be better received as JSON (please see pyodide/pyodide-build#28).Additionally, in this PR, support has been added for installing arbitrary Pyodide versions, or, more specifically, arbitrary versions for "Pyodide cross-build environments (xbuildenvs)" – though, only the ones that are supported for a given
pyodide-build
version. This has been implemented through an environment variableCIBW_PYODIDE_VERSION
and an associated configuration variable in the schema (through a table implemented via pyodide/pyodide-build#26).The rationale behind this is that WebAssembly/Pyodide builds are already experimental, and it would be useful to not tie the available Pyodide version to the
cibuildwheel
version – this would be helpful for downstream projects (statsmodels/statsmodels#9343, scikit-image/scikit-image#7525, scikit-learn/scikit-learn#29791, and so on) to allow testing against Pyodide's alpha releases and/or for the case of greater reproducibility against Pyodide's older releases.cc: @hoodmane and @ryanking13 for visibility
Suggested CHANGELOG entry
Since I didn't find a way to add an entry without the pre-commit hook removing previous entries, I've added a few lines here based on the current state of this PR. Please feel free to suggest changes or modify these lines directly.