Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 40 commits
2ae389d
0e3b3f1
394459f
dff7bf2
16057bc
f167c50
31a6be9
cbca40b
9003067
d8a8d5e
cf5dd3e
55459cb
4fe86e0
b6830ee
aaf32e5
bb6e0d6
735d5bb
b8ac6c0
7796311
97e22c8
d30eb6a
ce2a3f0
13fbf66
14ec071
aae64bb
6956121
e5d8443
95c3681
09af46f
66999cb
1af1e5d
ad3a203
d344548
b3143b4
738ed1f
41e466a
2a66ddd
dff72ca
9a78845
e551021
1fe8a04
8a8177c
fffb705
0df3c45
057e542
29b5eff
321e0de
92babdb
d73f71f
0f52a4b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 caseCIBW_PYODIDE_VERSION
. They can also come from the cibuildwheel section inpyproject.toml
. We should handle this in the same way as all the other options. Then we can just usepython_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.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.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.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 :)
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.